Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] update GCP Cloud SQL database guides #41681

Merged
merged 1 commit into from
May 24, 2024

Conversation

GavinFrazar
Copy link
Contributor

This is a docs only PR that updates our GCP SQL database guides:

  1. make a new partial for creating the db service's service account
  2. explain that using a project level permission bind will grant broader permissions than necessary
  3. eliminate the "gather cloud sql information" step and put these details in the "configure teleport" step instead.
  4. eliminate the screenshots for gathering the instance ID and public IP, as it seems quite obvious where to find them, and IMO there were too many screenshots in this guide which made it disorienting to navigate
  5. add a common GCP troubleshooting partial, for now just troubleshooting for GCP credential loading error
  6. broke up the "Set up teleport database service" step, which was trying to do too many things in one step. The expanded steps are install teleport > configure teleport > configure GCP credentials for teleport > start teleport
  7. replaced the yaml config files with teleport db configure equivalent, in a partial just for Cloud SQL databases.
  8. added a warning about the potential risk of using service account keys in prod environments

This PR also updates the auth token generation partial (which is used by many other db guides) to:

  1. use --format=text so it only outputs the token itself. When someone is following our guides, they do not need to see the enormous amount of text hints we print normally from that command (I'm going to reduce that text spam in a code PR later btw).
  2. improve (imo) the phrasing to clarify that we're talking about joining the Teleport cluster and they should save the token in a file on the "server" (instead of "node").

@GavinFrazar GavinFrazar added documentation database-access Database access related issues and PRs backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels May 17, 2024
@GavinFrazar GavinFrazar force-pushed the gavinfrazar/update-cloudsql-guides branch from 877defd to e63d8f0 Compare May 17, 2024 04:12
Copy link

🤖 Vercel preview here: https://docs-kjr7savfi-goteleport.vercel.app/docs/ver/preview

Copy link

🤖 Vercel preview here: https://docs-4xori4813-goteleport.vercel.app/docs/ver/preview

Copy link

🤖 Vercel preview here: https://docs-9b8a0yrsp-goteleport.vercel.app/docs/ver/preview

Copy link

🤖 Vercel preview here: https://docs-3omd515rm-goteleport.vercel.app/docs/ver/preview

Copy link

🤖 Vercel preview here: https://docs-pu5jjxozx-goteleport.vercel.app/docs/ver/preview

Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for reviewing this.

Could you double check on this part?

### (Optional) Allow only SSL connections

I feel the original doc is wrong. createEphemeral should be used when client certs are required, not when SSL is required.
Screenshot 2024-05-22 at 11 09 33 AM

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from zmb3 May 22, 2024 15:10
@GavinFrazar
Copy link
Contributor Author

Thanks again for reviewing this.

Could you double check on this part?

### (Optional) Allow only SSL connections

I feel the original doc is wrong. createEphemeral should be used when client certs are required, not when SSL is required. Screenshot 2024-05-22 at 11 09 33 AM

I don't even think "cloudsq.sslCerts.createEphemeral" permission exists anymore. Top search results for it are our documentation and an angry user's issue specifically about this permission requirement 😅 .

I looked at the "Cloud SQL Admin" role in our gcp dev account and it doesnt have that permission:
image

However the "Cloud SQL Admin" role is required (custom role doesnt work) when client cert is required.

It also looks like our code is a little out of date:

return dbi.Settings.IpConfiguration.RequireSsl, nil

We use RequireSsl which is either true/false:

	// RequireSsl: Use `ssl_mode` instead. Whether SSL/TLS connections over IP are
	// enforced. If set to false, then allow both non-SSL/non-TLS and SSL/TLS
	// connections. For SSL/TLS connections, the client certificate won't be
	// verified. If set to true, then only allow connections encrypted with SSL/TLS
	// and with valid client certificates. If you want to enforce SSL/TLS without
	// enforcing the requirement for valid client certificates, then use the
	// `ssl_mode` flag instead of the legacy `require_ssl` flag.
	RequireSsl bool `json:"requireSsl,omitempty"`

We should probably update that to use ssl_mode.
However, from both this description and actually trying it, it works without creating a client certificate as long as it's not the "require client cert" setting.

By the way, our code will swallow the error if the database agent doesn't have permissions to check the ssl settings of the db, so this:

		if trace.IsAccessDenied(err) {
			return false, trace.Wrap(err, `Could not get GCP database instance settings:

  %v

Make sure Teleport db service has "Cloud SQL Admin" GCP IAM role,
or "cloudsql.instances.get" IAM permission.`, err)
		}

will not be seen in logs or by user. Instead, MySQL will fail because they connect to the wrong port (3306) instead of (3307).

I'll open a ticket for these issues

Copy link

🤖 Vercel preview here: https://docs-5xb5par2y-goteleport.vercel.app/docs/ver/preview

Copy link

🤖 Vercel preview here: https://docs-nwdu40k1t-goteleport.vercel.app/docs/ver/preview

Merged via the queue into master with commit dd80713 May 24, 2024
38 checks passed
@GavinFrazar GavinFrazar deleted the gavinfrazar/update-cloudsql-guides branch May 24, 2024 00:38
@public-teleport-github-review-bot

@GavinFrazar See the table below for backport results.

Branch Result
branch/v14 Create PR
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v14 backport/branch/v15 database-access Database access related issues and PRs documentation gcp no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants