GoogleCloudPlatform / GoogleCloudPlatform/cloud-sql-proxy-operator

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

Đang mở
#797 2 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Go
Star
120
Fork
18
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

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.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
go, google-cloud, kubernetes
Lĩnh vực
devops, infrastructure
Loại issue
Tính năng
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
48/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.