GoogleCloudPlatform / GoogleCloudPlatform/cloud-sql-proxy-operator
preStop hook can't reach the localhost-only admin server since #745; add first-class exec-based shutdown
- Lenguaje dominante
- Go
- Estrellas
- 120
- Forks
- 18
- Métricas de merge de PR
- Sin PR fusionados en 30 d
Descripción
Since #745, the injected `preStop` hook cannot reach the proxy's admin server, and there is no first-class way to use the exec-based `shutdown` command that the proxy gained in v2.20.0. This issue covers both, since the second is the fix for the first.
## The preStop hook no longer reaches the admin server
`addHealthCheck()` unconditionally injects an HTTP `preStop` hook ([`podspec_updates.go:896-904`](https://github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/blob/v1.8.2/internal/workload/podspec_updates.go#L896-L904)):
```go
c.Lifecycle = &corev1.Lifecycle{
PreStop: &corev1.LifecycleHandler{
HTTPGet: &corev1.HTTPGetAction{
Port: intstr.IntOrString{IntVal: adminPort},
Path: "/quitquitquit",
},
},
}
```
#745 removed `Host: "localhost"` from this handler to fix #739 (k8s 1.34 blocks `host` on lifecycle handlers under baseline/restricted PSA). That fix was necessary, but it leaves the hook pointing somewhere the admin server isn't listening:
- An `httpGet` handler with no `Host` [defaults to the pod IP](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#lifecycle), so kubelet dials `podIP:9091`.
- The proxy's admin server binds localhost only — `net.JoinHostPort("localhost", cmd.conf.AdminPort)` ([`cmd/root.go:1238`](https://github.com/GoogleCloudPlatform/cloud-sql-proxy/blob/main/cmd/root.go#L1238)). There is no flag to change the bind address; `--admin-port` sets only the port, and `--http-address` applies to the health-check server (which the operator already sets to `0.0.0.0` at [`podspec_updates.go:873`](https://github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/blob/v1.8.2/internal/workload/podspec_updates.go#L873)).
So the hook should be getting connection-refused and producing a `FailedPreStopHook` event on every pod termination. This affects v1.7.5 (the first release containing #745) through v1.8.2.
The impact is muted — `CSQL_PROXY_EXIT_ZERO_ON_SIGTERM=true` means the container still exits 0, so the "exited in an error state" symptom from #425 stays fixed — but the graceful drain that #425 built the hook for no longer happens, and there's a warning event on every stop.
Caveat: this is a code-level analysis of both repos, not something I've reproduced on a live cluster. Happy to be corrected if the hook is in fact reaching the admin server through some path I've missed. The existing tests assert only the generated PodSpec, so they wouldn't catch this either way.
## Why exec is the fix
Both constraints are now fixed points: PSA forbids `host` on the handler, and the admin server won't bind anything but localhost. That leaves no working HTTP form. An exec handler runs inside the container's own network namespace, so `127.0.0.1` resolves correctly and no `host` field is involved.
The proxy added exactly this in v2.20.0 (GoogleCloudPlatform/cloud-sql-proxy#2514):
```yaml
lifecycle:
preStop:
exec:
command: ["/cloud-sql-proxy", "shutdown", "--admin-port", ""]
```
The command is safe for the operator to generate: the published image is distroless with `ENTRYPOINT ["/cloud-sql-proxy"]`, and the operator already resolves the admin port itself (`adminPort` at [`podspec_updates.go:891`](https://github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/blob/v1.8.2/internal/workload/podspec_updates.go#L891), from `adminServer.port` or `DefaultAdminPort`).
Today the only way to get this is `authProxyContainer.container`, which fully replaces the container ([`podspec_updates.go:591-597`](https://github.com/GoogleCloudPlatform/cloud-sql-proxy-operator/blob/v1.8.2/internal/workload/podspec_updates.go#L591-L597) returns before any reconcile logic runs). That means hand-maintaining image, env, args, probes and security context forever, and forfeiting automatic proxy-image upgrades — the same complaint as #507.
## Proposal
Add an opt-in shutdown config under `authProxyContainer`. Two shapes seem reasonable and they compose, so I'd like input on which to build rather than guessing.
**A. First-class `Exec` mode.** The operator generates the command from the admin port it already resolves:
```yaml
spec:
authProxyContainer:
shutdown:
mode: Exec # HTTP | Exec, default HTTP
```
Nothing to keep in sync — change `adminServer.port` and the hook follows. Fixes the broken hook for anyone who opts in, with a one-line spec change.
**B. User-supplied command.** A general escape hatch for the `preStop` command:
```yaml
spec:
authProxyContainer:
shutdown:
exec:
command: ["/cloud-sql-proxy", "shutdown", "--admin-port", "9091"]
```
More flexible and useful for custom images or wrapper entrypoints, but the admin port has to be repeated by hand and can silently drift from `adminServer.port`.
These aren't exclusive — `mode: Exec` with an optional `command` override covers both, defaulting to the generated command when omitted.
Open questions worth settling in this thread:
- **Should `Exec` eventually become the default?** The bundled default image is 2.25.2, well past 2.20.0, and the HTTP form appears broken for everyone — so defaulting to HTTP means shipping a known-broken hook to anyone who doesn't opt in. Against that: it's a behavior change on upgrade, and users pinning a proxy older than 2.20.0 would get a hook whose subcommand doesn't exist. Opt-in first and flip the default in a later minor seems like the conservative path.
- **Version detection.** There's no version-awareness anywhere in the operator today, and `authProxyContainer.image` is a free-form string, so auto-selecting exec by parsing the tag would be new and fragile machinery (`latest` and digest-pinned images can't be resolved at all). An explicit field avoids that entirely — the user asserts their image supports it.
- **Where the field belongs.** `authProxyContainer.shutdown` as sketched, or folded into `adminServer` alongside `port`/`enableAPIs`, since it's an admin-server-driven behavior?
Happy to send a PR for whichever shape you prefer, tests included. If the analysis of the broken hook holds up, that part may be worth splitting into its own fix — let me know how you'd like it structured.
Guía de contribución
Línea de trabajo
Start in internal/workload/podspec_updates.go at addHealthCheck() (896-904), then trace authProxyContainer handling at 591-597 and adminPort resolution at 891. Run the existing generated-PodSpec tests described in the issue to confirm the current HTTP lifecycle output. Done means the chosen shutdown configuration is represented in the API, generates the intended lifecycle handler, and covers the selected compatibility and default behavior in tests.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- go, google-cloud, kubernetes
- Área
- devops, infrastructure
- Tipo de issue
- Nueva funcionalidad
- Dificultad
- 5/5
- Tiempo estimado
- Más de una semana
- Estado de actividad
- Activo
- Claridad
- Bastante claro
- Aptitud para principiantes
- 48/100