GoogleCloudPlatform / GoogleCloudPlatform/cloud-sql-proxy-operator

preStop hook can't reach the localhost-only admin server since #745; add first-class exec-based shutdown

オープン
#797 コメント 2 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Go
スター
120
フォーク
18
PR マージ指標
30日以内にマージされた PR はありません

説明

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.

コントリビューションガイド

コントリビューションガイドを開く

調査の方向性

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.

索引モデルが issue の本文から書いたものです。

評価

技術スタック
go, google-cloud, kubernetes
領域
devops, infrastructure
issue の種類
機能追加
難易度
5/5
見積もり時間
1週間以上
活発さ
活発
明瞭さ
おおむね明確
初心者へのやさしさ
48/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。