Resolve node.attr.zone via the Node label instead of cloud IMDS - #900
Draft
goat-ssh wants to merge 1 commit into
Draft
Resolve node.attr.zone via the Node label instead of cloud IMDS#900goat-ssh wants to merge 1 commit into
goat-ssh wants to merge 1 commit into
Conversation
CrateDB's node.attr.zone (used for zone-aware shard allocation) was queried per-provider from each cloud's instance metadata service at 169.254.169.254. That endpoint needs pod egress to survive on all four providers, and it's a classic SSRF/pivot target — closing it off at the NetworkPolicy layer (crate/cloud#3850, crate/cloud#3851) breaks zone detection on every cluster. Read the pod's own Node object via the in-cluster API server instead: the topology.kubernetes.io/zone label is already populated consistently by AWS, Azure, GCP and STACKIT's cloud-controller-managers. One code path for all four providers, and no pod egress to IMDS required. Requires a ClusterRole/ClusterRoleBinding granting the cluster's default ServiceAccount read-only access to its own Node object; the binding is per-cluster and explicitly cleaned up on delete, since a namespaced CrateDB CR can't own a cluster-scoped object via owner_references. Every provider's crate pod restarts once to pick up the new command and NODE_NAME env var.
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete issues (misleading comment in get_statefulset_crate_env and unconditional ClusterRoleBinding deletion behavior) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR switches CrateDB zone-aware shard allocation (node.attr.zone) from cloud instance metadata services (IMDS at 169.254.169.254) to reading the pod’s Kubernetes Node label (topology.kubernetes.io/zone) via the in-cluster API server, unifying behavior across AWS/Azure/GCP/STACKIT and removing the need for pod egress to IMDS.
Changes:
- Replace per-cloud IMDS zone lookup with a single in-cluster Kubernetes Node API lookup for zone-aware providers.
- Add
NODE_NAMEenv var (Downward API:spec.nodeName) for zone-aware providers to drive the Node lookup. - Add RBAC creation for a shared
ClusterRoleand per-clusterClusterRoleBinding, plus explicit cleanup logic on cluster deletion; update/extend tests and changelog accordingly.
File summaries
| File | Description |
|---|---|
crate/operator/create.py |
Implements Node-label-based zone resolution, injects NODE_NAME, and creates ClusterRole/ClusterRoleBinding for Node reads. |
crate/operator/handlers/handle_delete_cratedb.py |
Adds explicit deletion of the per-cluster ClusterRoleBinding on CrateDB deletion. |
crate/operator/constants.py |
Introduces a constant for the shared ClusterRole name. |
tests/test_create.py |
Updates zone-attr tests to assert the new Kubernetes API-based resolution and OpenShift behavior. |
tests/test_stackit.py |
Removes STACKIT IMDS-specific zone tests and adds coverage for NODE_NAME env injection. |
CHANGES.rst |
Documents the new zone-resolution mechanism and networking implications. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+99
to
+101
| binding_name = f"crate-node-zone-reader-{namespace}-{name}" | ||
|
|
||
| async with GlobalApiClient() as api_client: |
Comment on lines
+721
to
+723
| # Referenced as ``${NODE_NAME}`` by the ``-Cnode.attr.zone`` Node lookup | ||
| # above, on every provider that resolves zone awareness that way. | ||
| if config.CLOUD_PROVIDER in ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
CrateDB's node.attr.zone (used for zone-aware shard allocation) was queried per-provider from each cloud's instance metadata service at 169.254.169.254. That endpoint needs pod egress to survive on all four providers, and it's a classic SSRF/pivot target — closing it off at the NetworkPolicy layer (crate/cloud#3850, crate/cloud#3851) breaks zone detection on every cluster.
Read the pod's own Node object via the in-cluster API server instead: the topology.kubernetes.io/zone label is already populated consistently by AWS, Azure, GCP and STACKIT's cloud-controller-managers. One code path for all four providers, and no pod egress to IMDS required.
Requires a ClusterRole/ClusterRoleBinding granting the cluster's default ServiceAccount read-only access to its own Node object; the binding is per-cluster and explicitly cleaned up on delete, since a namespaced CrateDB CR can't own a cluster-scoped object via owner_references.
Every provider's crate pod restarts once to pick up the new command and NODE_NAME env var.
Checklist
CHANGES.rst