Repository navigation
Conversation
Adds a read-only tool that plans the connection from a GKE workload to a Cloud SQL instance (engine auto-detected). It inspects Workload Identity, node pool metadata mode, VPC/Shared VPC, VPC-native networking, private nodes and PSC, then returns a summary, network path, Auth Proxy sidecar manifests, state-tailored setup steps and troubleshooting tips. All GKE logic lives in the tool package and reuses the shared cloudsqlconnect helpers unchanged; the GCE tool and shared package are not modified. Part of googleapis#2345
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the cloud-sql-connect-gke tool, which plans and configures connections from Google Kubernetes Engine (GKE) workloads to Cloud SQL instances (PostgreSQL, MySQL, or SQL Server). It includes comprehensive validation, connection recommendations, manifest generation, and step-by-step setup instructions. The review feedback highlights two key areas for improvement: updating the GKE client initialization to use a sync.RWMutex instead of sync.Once to prevent permanently caching transient initialization errors, and correcting the gcpLocationRe regular expression to support multi-segment GCP regions like us-gov-west1.
| var ( | ||
| containerOnce sync.Once | ||
| containerService *container.Service | ||
| containerErr error | ||
| ) |
There was a problem hiding this comment.
Using sync.Once to initialize the process-wide GKE client caches any transient initialization errors (such as temporary network issues or credential loading failures) permanently. In a long-running server, this means the GKE service will remain broken until the server is restarted. Use a sync.RWMutex to safely initialize the client without caching transient errors.
var (
containerMu sync.RWMutex
containerService *container.Service
)There was a problem hiding this comment.
Fixed in ecbda56. Replaced sync.Once with a sync.RWMutex and a double-checked init. Only a successfully built client is cached now, so a transient ADC or network failure goes back to that caller and the next request retries. It still uses context.Background() so a cancelled first request cannot poison the cache, and caller tokens still bypass it.
| func getContainerService(ctx context.Context, accessToken string) (*container.Service, error) { | ||
| if accessToken != "" { | ||
| ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: accessToken}) | ||
| svc, err := container.NewService(ctx, option.WithTokenSource(ts)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build token-scoped GKE client: %w", err) | ||
| } | ||
| return svc, nil | ||
| } | ||
| containerOnce.Do(func() { | ||
| containerService, containerErr = container.NewService(context.Background(), option.WithScopes(container.CloudPlatformScope)) | ||
| }) | ||
| return containerService, containerErr | ||
| } |
There was a problem hiding this comment.
Update getContainerService to use the sync.RWMutex double-checked locking pattern. This ensures that transient initialization errors are not cached permanently, allowing subsequent requests to retry GKE client creation.
func getContainerService(ctx context.Context, accessToken string) (*container.Service, error) {
if accessToken != "" {
ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: accessToken})
svc, err := container.NewService(ctx, option.WithTokenSource(ts))
if err != nil {
return nil, fmt.Errorf("failed to build token-scoped GKE client: %w", err)
}
return svc, nil
}
containerMu.RLock()
svc := containerService
containerMu.RUnlock()
if svc != nil {
return svc, nil
}
containerMu.Lock()
defer containerMu.Unlock()
if containerService != nil {
return containerService, nil
}
var err error
containerService, err = container.NewService(context.Background(), option.WithScopes(container.CloudPlatformScope))
if err != nil {
return nil, fmt.Errorf("failed to build GKE client: %w", err)
}
return containerService, nil
}There was a problem hiding this comment.
Fixed in ecbda56. getContainerService now follows the suggested pattern: RLock fast path, then Lock with a re-check, then build. On error it returns failed to build GKE client: %w without caching anything. The token-scoped path (useClientOAuth) is unchanged.
| var ( | ||
| projectIDRe = regexp.MustCompile(`^[a-z][-a-z0-9]{4,28}[a-z0-9]$`) | ||
| gkeClusterNameRe = regexp.MustCompile(`^[a-z]([-a-z0-9]{0,38}[a-z0-9])?$`) | ||
| gcpLocationRe = regexp.MustCompile(`^[a-z]+-[a-z]+[0-9]+(-[a-z])?$`) |
There was a problem hiding this comment.
The current gcpLocationRe regex does not match valid GCP regions with multiple hyphens before the number, such as us-gov-west1 or us-gov-east1. Update the regex to support multi-segment geographies/regions.
| gcpLocationRe = regexp.MustCompile(`^[a-z]+-[a-z]+[0-9]+(-[a-z])?$`) | |
| gcpLocationRe = regexp.MustCompile("^[a-z]+(-[a-z]+)*[0-9]+(-[a-z])?$") |
There was a problem hiding this comment.
Fixed in ecbda56. gcpLocationRe is now ^[a-z]+(-[a-z]+)*[0-9]+(-[a-z])?$, so multi-segment regions like us-gov-west1 and their zones are accepted. Added test cases for those plus northamerica-northeast1, and negative cases (us-central, us-central1-ab, -us1) to make sure it is still strict.
… cloud-sql-connect-gke - Replace sync.Once with an RWMutex double-checked init so a transient ADC/network failure is returned to that caller and retried on the next request instead of being cached for the life of the server. - Broaden the cluster_location regex to accept multi-segment regions such as us-gov-west1 (and their zones); add test cases.
|
Addressed the automated review in ecbda56:
Unit tests, |
This branch has not been deployed
2 participants
Description
Adds
cloud-sql-connect-gke, the second compute destination from the plan in #2345 (GCE shipped in #3740). Like the GCE tool, it is one tool for all engines: PostgreSQL, MySQL, and SQL Server are auto-detected viaParseDatabaseTypeStrict. It is read-only: it inspects the instance and the cluster, then returns a plan the agent can walk the user through.GKE → Cloud SQL usually breaks because of cluster facts you can't see from inside a pod. The tool reads those facts from the GKE API and shapes everything it returns around them:
GKE_METADATAnode-pools updatestep. Pods would otherwise silently fall back to the node SA.initContainers+restartPolicy: Always).validation.valid=false. The summary starts with "Blocked:" and a remediation step.Guiding the agent
GKE setup has a lot of steps, so the output is built to be easy for an agent to follow:
summary: a one-paragraph verdict to relay first. It is either the blocker, or the chosen method + network path + pending cluster changes. It also tells the agent to ask the user before mutating IAM, the cluster, or the instance.setupSteps: numbered with no gaps, and limited to work that is still needed. Every command passes explicit--project/--location/-nflags. No step uses pipes, heredocs, or$(...), so each line can be shown and run as is.serviceAccountYaml,sidecarYaml,deploymentYaml,secretYaml. The Secret is created withkubectl create secretso passwords stay out of files.troubleshooting: symptom / cause / fix for the failures users actually hit. Tips are filtered by method and sidecar type (for example startup races and Jobs that never finish only show up for classic sidecars).Design decisions
--auto-iam-authnby default: it breaks built-in database users. IAM database auth is reported as a validation check, and the troubleshooting tips explain when to add the flag.cluster_projectparameter: GKE often runs in a different project from the database.roles/cloudsql.clientis granted in the instance project, and sqladmin is enabled in both.cluster_locationoptional: if omitted, the tool does onelocations/-list and matches the name client-side. On a miss, the error lists the clusters it found and any unreachable locations.Follows the review feedback from #3740
sync.OnceGKE client is initialized withcontext.Background(), and its token is scoped to the caller's OAuth token when the source setsuseClientOAuth(ADC otherwise).internal/util/cloudsqlconnect. The tool includesvar _ tools.Tool = Tool{},GetSourceName,ValidateSource, andToConfig.kind: source/---/kind: toolform;switchinstead of if/else chains.Scope: additive only
This PR adds new files only (21 files, +3452/−0). The single line outside the new tool is the standard registration in
cmd/internal/imports.go.internal/util/cloudsqlconnectare unchanged. The tool reuses the shared helpers as they are (ValidateInstanceConnectionName,ExtractSQLInfo,ParseDatabaseTypeStrict,BuildConnectionStrings,GenerateCodeSnippet, and the shared result types).gke.go,manifests.go,setup.go,validation.go,recommendations.go,inputvalidate.go). The file layout mirrors the shared package.summary,troubleshooting,serviceAccountYaml,deploymentYaml) come from tool-local types that embedcloudsqlconnect.ConnectResultandEnvironmentConfig. The JSON keys stay compatible with the GCE tool's output.GenerateGKESetupSteps,GenerateGKESidecarYAML, ...) are not used by any tool and are left untouched. Consolidating them can be a separate follow-up if maintainers want it.Testing
--auto-iam-authn)tests/cloudsql{pg,mysql,mssql}/*_connect_gke_test.go). They are skipped unlessCLOUDSQL_TEST_GKE_CLUSTER_NAMEis set;_LOCATIONand_PROJECTare optional.PR Checklist
!if this involve a breaking change🛠️ Part of #2345