Autodelegate - #48
Open
azgabur wants to merge 7 commits into
Open
Conversation
azgabur
commented
Sep 25, 2025
averevki
reviewed
Sep 25, 2025
azgabur
force-pushed
the
autodelegate
branch
2 times, most recently
from
October 3, 2025 10:25
3e1bf44 to
0bc8aff
Compare
Member
Author
|
Instead of using aws cli with a horrible way to append records to record sets, rather use https://github.com/barnybug/cli53 |
This reverts commit b579375. Signed-off-by: Alex Zgabur <azgabur@redhat.com>
Signed-off-by: Alex Zgabur <azgabur@redhat.com>
Member
Author
The tool was not that intuitive as I first anticipated, it was done via aws-cli (thanks claude) |
azgabur
requested review from
averevki and
trepel
and removed request for
trepel
February 9, 2026 21:14
averevki
reviewed
Feb 10, 2026
averevki
left a comment
averevki
left a comment
Member
There was a problem hiding this comment.
Looks very good! I would wait this chart integration into the pipelines to try it out myself. Or I can try it out now, if you can show me how it would work locally if I want to autodelegate 2 primary clusters. Would it be enough to just set correct values for coredns tools chart for each cluster, and just run it on both primaries?
Comment on lines
+4
to
+5
| name: coredns-config | ||
| namespace: {{ .Values.tools.namespace }} |
Member
There was a problem hiding this comment.
Why is this secret created in the tools namespace, would it require a copy between the namespaces from the testsuite?
Member
Author
There was a problem hiding this comment.
In tools charts I do not have access to value of "kuadrant" namespace. I could hardcode it, but this secret is just for reference which zones were deployed. The same information could be found in Corefile.
I imagine testsuite reading this secret and creating appropriate dns provider secret in "kuadrant" namespace at runtime
Do you think having a task in deploy pipeline which will create default dns provider secret for coredns is wanted?
Member
Author
Yes exactly, in tools-values.yaml you choose zones you wish to deploy and autodelegate and it will handle multiple clusters having the same zone, even when deleting so you can re-create one cluster and the other cluster should not be affected. Addition to pipelines is coming 👍 |
azgabur
force-pushed
the
autodelegate
branch
from
February 11, 2026 10:30
6be6776 to
9940ba9
Compare
Signed-off-by: Alex Zgabur <azgabur@redhat.com>
azgabur
force-pushed
the
autodelegate
branch
from
February 12, 2026 01:53
9940ba9 to
415e079
Compare
averevki
previously approved these changes
Feb 16, 2026
📝 WalkthroughWalkthroughThis pull request refactors CoreDNS configuration from a nested Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant K8s as Kubernetes Cluster
participant DNS as CoreDNS Service
participant AWS as AWS Route53
participant Job as Post-Install Job
Admin->>K8s: Deploy Helm chart with autoDelegate=true
K8s->>K8s: Create ConfigMap (create.sh, delete.sh)
K8s->>K8s: Create ServiceAccount & RBAC
K8s->>K8s: Create CoreDNS Deployment
K8s->>Job: Trigger post-install Job
Job->>DNS: Poll: GET coredns LoadBalancer IP/hostname
loop Until LB Ready
DNS-->>Job: Pending (no IP)
Job->>Job: Wait
end
DNS-->>Job: IP/hostname available
loop For each zone in config
Job->>AWS: GET current NS records for zone
AWS-->>Job: Existing NS record set
Job->>Job: Merge/create new NS entry
Job->>AWS: UPSERT NS record batch
AWS-->>Job: Change ID
Job->>Job: Wait for propagation
Job->>AWS: Create A record (nameserver.zone → IP)
AWS-->>Job: Success
end
Job->>K8s: Complete
Note over DNS,AWS: CoreDNS now accessible via delegated domain
sequenceDiagram
actor Admin
participant K8s as Kubernetes Cluster
participant Job as Pre-Delete Job
participant AWS as AWS Route53
Admin->>K8s: Delete Helm release
K8s->>Job: Trigger pre-delete Job
loop For each zone in config
Job->>AWS: GET current NS records for zone
AWS-->>Job: Existing NS record set
Job->>Job: Filter out delegated nameserver
alt Remaining NS records exist
Job->>AWS: UPSERT (remaining NS values)
AWS-->>Job: Success
else No NS records remain
Job->>AWS: DELETE NS record
AWS-->>Job: Success
end
Job->>AWS: DELETE A record (nameserver.zone)
AWS-->>Job: Success
end
Job->>K8s: Complete
K8s->>K8s: Proceed with release deletion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/kuadrant-instances/templates/coredns/03-service.yaml (1)
12-21:⚠️ Potential issue | 🟠 MajorMixed TCP/UDP on a single LoadBalancer Service requires cloud-provider support.
Consolidating UDP/53 and TCP/53 into one LoadBalancer Service depends on the underlying cloud LB implementation:
- AWS in-tree CLB (the default without any
service.beta.kubernetes.io/aws-load-balancer-*annotation) does not support mixed protocols. The Kubernetes AWS cloud-controller explicitly validates and rejects mixed-protocol Services. To use mixed protocols on EKS, you must switch to AWS Load Balancer Controller (which provisions NLBs with mixed-protocol support) via configuration.- GCP LoadBalancer does not support mixed protocols natively. Whilst the MixedProtocolLBService feature gate reached GA in Kubernetes 1.26, the GCP cloud-provider does not provision mixed-protocol support. Full support requires GKE 1.34.1-gke.2190000 or later with the specific
loadBalancerClass: networking.gke.io/l4-regional-external. Earlier GKE versions may silently fail or provide unreliable provisioning.Document the minimum Kubernetes/managed-service versions required for mixed-protocol LoadBalancer Services in the chart README, and consider adding an optional
.Values.coredns.service.annotationsso users can configure cloud-provider-specific settings (e.g., AWS NLB) without forking the chart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kuadrant-instances/templates/coredns/03-service.yaml` around lines 12 - 21, The Service currently exposes both UDP and TCP on port 53 under a single Service (ports: "udp-53" and "tcp-53" with type: LoadBalancer), which requires cloud-provider support; update the chart by (1) documenting required Kubernetes/managed-cluster versions and provider-specific notes in the chart README (explain AWS in-tree vs ALB/NLB, GKE version/loadBalancerClass requirements and any feature gates), and (2) add an optional .Values.coredns.service.annotations map and merge it into the Service metadata.annotations in the coredns template so users can set provider-specific annotations (e.g., AWS NLB annotations or GKE loadBalancerClass) or choose to split protocols via separate Services if their provider does not support mixed-protocol LBs.
🧹 Nitpick comments (3)
charts/kuadrant-instances/templates/coredns/autodelegate/04-delete-job.yaml (1)
17-17: Avoid the:latestimage tag for hook Jobs.Using
quay.io/kuadrant/testsuite-pipelines-tools:latestmakes the pre-delete cleanup non-reproducible and risks sudden breakage if the upstream image changes incompatibly. Consider pinning to a digest or explicit tag, ideally exposed through a value (e.g..Values.coredns.autoDelegate.image) so it can be overridden. Same concern applies to02-create-job.yamlline 17.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kuadrant-instances/templates/coredns/autodelegate/04-delete-job.yaml` at line 17, Replace the hardcoded quay.io/kuadrant/testsuite-pipelines-tools:latest image in the auto-delegate hook Jobs with a configurable, pinned image reference: add a new chart value (e.g. .Values.coredns.autoDelegate.image) and use that value in both templates (04-delete-job.yaml and 02-create-job.yaml) so callers can specify a tagged or digest-pinned image (sha256) instead of :latest; update the templates to use the value and consider adding a corresponding .Values.coredns.autoDelegate.imagePullPolicy to keep behavior explicit.example-additionalManifests.yaml (1)
132-142: Example usesstringDatawhile the other example secrets usedata.Minor consistency nit: every other secret in this example file uses
data:with placeholder values, whereas this newcoredns-autodelegatesecret usesstringData:. Both are valid, but mixing styles in the same example can confuse users copying the template. Consider aligning with the rest of the file (or converting the others ifstringDatais the preferred style going forward).Also worth noting for users: the secret only contains
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/ZONE_ID— noAWS_REGION. That is correct for Route53 (a global service), but a short comment here would spare users from wondering whether region needs to be added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-additionalManifests.yaml` around lines 132 - 142, The coredns-autodelegate Secret in toolsManifests uses stringData while other example Secrets use data; change the coredns-autodelegate entry to use data with base64-placeholder values (or convert the other Secrets to stringData if you prefer that style) so examples are consistent, and add a short inline comment near the coredns-autodelegate Secret noting that AWS_REGION is intentionally omitted for Route53 (region not required). Reference: toolsManifests -> coredns-autodelegate, fields stringData/data, and keys AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, ZONE_ID.charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml (1)
13-18: Shell hardening suggestions.A few small robustness tweaks that would benefit both scripts:
set -xeechoes every command, including theawsinvocations — fine for debugging but any secret substitution into args (not the case today, but easy to introduce) would end up in Job logs. Considerset -eo pipefailand leaving-xoff by default (toggle via an env var).set -ewill abort on the first failing zone in therange, leaving subsequent zones unprocessed. For the delete path in particular, you may want to continue-on-error per zone and exit non-zero at the end, so one broken zone doesn't block cleanup of the rest.- The
kubectl wait ... || exit 1on line 16 is redundant underset -e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml` around lines 13 - 18, Switch strictness: replace unsafe "set -xe" with "set -eo pipefail" and add an env-var toggle (e.g., DEBUG or VERBOSE) to enable "-x" only when debugging; in the script areas referencing NS_NAME, TTL, COREDNS_IP and COREDNS_HOSTNAME keep the quieter default. Change per-zone error handling in the deletion range loop so failures for one zone do not abort the whole loop: capture per-zone exit status, continue processing remaining zones, and after the loop exit with non-zero if any zone failed. Remove the redundant "|| exit 1" on the "kubectl wait ... service coredns" invocation (since set -e will handle failures) and ensure kubectl waits still respect the configured timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kuadrant-instances/templates/coredns/01-deployment.yaml`:
- Around line 10-14: The RollingUpdate settings allow the single CoreDNS pod to
be taken down (replicas: 1 with rollingUpdate.maxUnavailable: 1); update the
Deployment spec so rollingUpdate.maxUnavailable is 0 and add
rollingUpdate.maxSurge: 1 to maintain availability during updates (or make
replicas configurable for HA), changing the fields under strategy ->
rollingUpdate (maxUnavailable and add maxSurge) and ensure replicas is
documented/configurable rather than fixed at 1.
In `@charts/kuadrant-instances/templates/coredns/06-remoteServiceAccount.yaml`:
- Line 1: The templates are evaluating .Values.tools.enabled as a string,
causing truthy behavior; update either values.yaml to set tools.enabled: false
(boolean, no quotes) or change each template conditional (e.g., the coredns
template's condition using the symbol '{{ if and .Values.coredns.enable
.Values.tools.enabled }}') to an explicit comparison like '{{ if and
.Values.coredns.enable (eq .Values.tools.enabled "true") }}'; apply the chosen
fix consistently across all seven affected templates that reference
.Values.tools.enabled.
In `@charts/kuadrant-instances/templates/coredns/autodelegate/00-rbac.yaml`:
- Around line 22-28: Remove the unused RBAC permission for Kubernetes secrets by
deleting the resources: - secrets block (and its verbs: - get / - create) from
the Role/ClusterRole in the coredns autodelegate RBAC manifest; specifically
remove the rule that lists apiGroups: [""] with resources: ["secrets"] and verbs
including "get" and "create" so the Role/ClusterRole only grants the remaining
necessary permissions (e.g., service-related verbs), preserving YAML structure
and indentation for the surrounding rules.
In
`@charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml`:
- Around line 71-93: The script silently skips cleanup when both COREDNS_IP and
COREDNS_HOSTNAME are empty; update the pre-delete logic in delete.sh (the block
that sets COREDNS_IP/COREDNS_HOSTNAME and computes EXCLUDE_VALUE) to first wait
for the Service LB ingress (e.g., run a kubectl wait for
.status.loadBalancer.ingress with a short timeout against the service in {{
.Values.coredns.namespace }}) and then re-evaluate COREDNS_IP/COREDNS_HOSTNAME;
if after waiting both are still empty, fail fast (exit 1) with a clear error so
deletion does not silently skip cleanup. Ensure references to COREDNS_IP,
COREDNS_HOSTNAME, EXCLUDE_VALUE, NS_NAME and ZONE_ID remain unchanged so the
later A/NS deletion logic can proceed.
- Around line 82-90: The DELETE change-batch currently uses the live Service IP
($COREDNS_IP) which can differ from the value stored in the zone; modify the
block that builds CHANGE_BATCH (where CURRENT_A, TTL, CHANGE_BATCH and the aws
route53 change-resource-record-sets call are used) to extract the actual record
value from CURRENT_A (e.g. the ResourceRecords[0].Value) and use that extracted
IP for the --arg ip in the jq -n payload so the DELETE matches the exact
existing record.
- Around line 108-125: Add a short retry loop around the aws route53
change-resource-record-sets call to handle the TOCTOU race: on error responses
like "InvalidChangeBatch" or other stale-version failures, re-run the
list-resource-record-sets command to refresh CURRENT_RECORDS (or CURRENT_NS),
recompute REMAINING_VALUES/REMAINING_COUNT, rebuild the CHANGE_BATCH (using the
same logic that chooses DELETE vs UPSERT), and retry the aws call up to a small
max attempts with a brief sleep between tries; implement this around both
branches that construct CHANGE_BATCH and call aws route53
change-resource-record-sets so REMAINING_COUNT, CURRENT_RECORDS,
REMAINING_VALUES and CHANGE_BATCH are re-evaluated before each retry.
In `@charts/kuadrant-instances/templates/coredns/autodelegate/02-create-job.yaml`:
- Around line 7-8: Add a Helm hook delete policy so completed post-install Jobs
are removed: update the annotations block in the post-install Job (the template
that creates the post-install-autodelegate Job) to include
"helm.sh/hook-delete-policy": "hook-succeeded" (so successful runs are deleted).
For the pre-delete/autodelegate-cleanup Job referenced in 04-delete-job.yaml,
ensure it keeps "helm.sh/hook-delete-policy": "before-hook-creation" (or omit
deletion on failure) so the Job is retained for debugging.
In `@charts/kuadrant-instances/templates/coredns/autodelegate/04-delete-job.yaml`:
- Line 10: The pre-delete Job sets backoffLimit: 10 which can block helm
uninstall on persistent failures; update the Job spec (referencing backoffLimit
and restartPolicy in the pre-delete Job manifest) to reduce backoffLimit to a
lower value (e.g., 3 to match the create job) and add an activeDeadlineSeconds
(e.g., 300) to bound total runtime so uninstall fails fast and surfaces a clear
error instead of retrying for a long time.
In
`@charts/kuadrant-instances/templates/coredns/autodelegate/05-coredns-config-secret.yaml`:
- Around line 1-12: The secret template renders coredns-credentials in namespace
"kuadrant" but is incorrectly gated on .Values.tools.enabled; either remove the
tools gate so the template condition uses only .Values.coredns.enable and
.Values.coredns.autoDelegate, or keep the current condition but add the
annotation "helm.sh/resource-policy": "keep" to metadata.annotations (matching
other kuadrant secrets) so the secret is not removed on upgrades; update the
template around the if statement and metadata.annotations for the Secret named
coredns-credentials accordingly.
- Line 11: The ZONES line applies trimSuffix to the whole joined string causing
only the last zone to lose its trailing dot; update the template to trim the
trailing dot for each element of .Values.coredns.zones before joining.
Specifically, iterate over .Values.coredns.zones (or build a new $cleaned
slice), call trimSuffix "." on each zone value, then join the cleaned slice with
"," and quote the result so ZONES contains consistently trimmed entries
(reference the ZONES line, .Values.coredns.zones, trimSuffix and join).
In `@README.md`:
- Line 39: Update the product name spelling in the README by replacing the
incorrect "Coredns" instance with the canonical "CoreDNS" so all user-facing
documentation uses the standard capitalization; locate the occurrence of the
string "Coredns" in the README and change it to "CoreDNS".
In `@tools-install.sh`:
- Line 8: The script currently forces additional_flags='--values
additionalManifests.yaml' which makes tools-install.sh fail if that file is
missing and can wrongly deploy secrets; change the logic so additional_flags is
empty by default and only set to '--values additionalManifests.yaml' if the file
exists (check [-f additionalManifests.yaml]) or if an opt-in env/CLI flag is
provided, and additionally guard any deployment of toolsManifests secrets (e.g.,
coredns-autodelegate) behind the appropriate feature flag (CoreDNS autodelegate)
or config check before applying them; update all uses of additional_flags in
tools-install.sh to rely on this conditional value.
In `@values.yaml`:
- Line 99: Replace the unpinned CoreDNS image tag "latest" with a specific,
immutable version in values.yaml (update the image field/value for the CoreDNS
chart entry) so the chart uses a fixed tag (e.g., set image to
quay.io/kuadrant/coredns-kuadrant:<semver-or-digest>), and ensure
imagePullPolicy is appropriate (If using a digest or specific tag, change
imagePullPolicy from Always to IfNotPresent or document why Always is required);
also update any README or chart values comments to require updating this tag for
upgrades.
---
Outside diff comments:
In `@charts/kuadrant-instances/templates/coredns/03-service.yaml`:
- Around line 12-21: The Service currently exposes both UDP and TCP on port 53
under a single Service (ports: "udp-53" and "tcp-53" with type: LoadBalancer),
which requires cloud-provider support; update the chart by (1) documenting
required Kubernetes/managed-cluster versions and provider-specific notes in the
chart README (explain AWS in-tree vs ALB/NLB, GKE version/loadBalancerClass
requirements and any feature gates), and (2) add an optional
.Values.coredns.service.annotations map and merge it into the Service
metadata.annotations in the coredns template so users can set provider-specific
annotations (e.g., AWS NLB annotations or GKE loadBalancerClass) or choose to
split protocols via separate Services if their provider does not support
mixed-protocol LBs.
---
Nitpick comments:
In
`@charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml`:
- Around line 13-18: Switch strictness: replace unsafe "set -xe" with "set -eo
pipefail" and add an env-var toggle (e.g., DEBUG or VERBOSE) to enable "-x" only
when debugging; in the script areas referencing NS_NAME, TTL, COREDNS_IP and
COREDNS_HOSTNAME keep the quieter default. Change per-zone error handling in the
deletion range loop so failures for one zone do not abort the whole loop:
capture per-zone exit status, continue processing remaining zones, and after the
loop exit with non-zero if any zone failed. Remove the redundant "|| exit 1" on
the "kubectl wait ... service coredns" invocation (since set -e will handle
failures) and ensure kubectl waits still respect the configured timeout.
In `@charts/kuadrant-instances/templates/coredns/autodelegate/04-delete-job.yaml`:
- Line 17: Replace the hardcoded
quay.io/kuadrant/testsuite-pipelines-tools:latest image in the auto-delegate
hook Jobs with a configurable, pinned image reference: add a new chart value
(e.g. .Values.coredns.autoDelegate.image) and use that value in both templates
(04-delete-job.yaml and 02-create-job.yaml) so callers can specify a tagged or
digest-pinned image (sha256) instead of :latest; update the templates to use the
value and consider adding a corresponding
.Values.coredns.autoDelegate.imagePullPolicy to keep behavior explicit.
In `@example-additionalManifests.yaml`:
- Around line 132-142: The coredns-autodelegate Secret in toolsManifests uses
stringData while other example Secrets use data; change the coredns-autodelegate
entry to use data with base64-placeholder values (or convert the other Secrets
to stringData if you prefer that style) so examples are consistent, and add a
short inline comment near the coredns-autodelegate Secret noting that AWS_REGION
is intentionally omitted for Route53 (region not required). Reference:
toolsManifests -> coredns-autodelegate, fields stringData/data, and keys
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, ZONE_ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 684177ca-8769-4d80-a3cf-62a8f1723356
📒 Files selected for processing (18)
README.mdcharts/kuadrant-instances/templates/coredns/00-namespace.yamlcharts/kuadrant-instances/templates/coredns/00-rbac.yamlcharts/kuadrant-instances/templates/coredns/01-deployment.yamlcharts/kuadrant-instances/templates/coredns/03-service.yamlcharts/kuadrant-instances/templates/coredns/04-corefile.yamlcharts/kuadrant-instances/templates/coredns/05-serviceMonitor.yamlcharts/kuadrant-instances/templates/coredns/06-remoteServiceAccount.yamlcharts/kuadrant-instances/templates/coredns/autodelegate/00-rbac.yamlcharts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yamlcharts/kuadrant-instances/templates/coredns/autodelegate/02-create-job.yamlcharts/kuadrant-instances/templates/coredns/autodelegate/04-delete-job.yamlcharts/kuadrant-instances/templates/coredns/autodelegate/05-coredns-config-secret.yamlexample-additionalManifests.yamlinstall.shtools-install.shvalues-tools.yamlvalues.yaml
💤 Files with no reviewable changes (1)
- values-tools.yaml
Comment on lines
+22
to
+28
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - secrets | ||
| verbs: | ||
| - get | ||
| - create |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no kubectl secret operations in the autodelegate scripts
rg -nP --type=yaml -C2 '\bsecret(s)?\b' charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml
rg -nP -C2 'kubectl\s+(get|create|apply).*secret' charts/kuadrant-instances/templates/coredns/autodelegate/Repository: Kuadrant/helm-charts-olm
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Attempt to locate and examine the coredns autodelegate scripts
find . -path "*/charts/kuadrant-instances/templates/coredns/autodelegate/*" -type f 2>/dev/null | head -20Repository: Kuadrant/helm-charts-olm
Length of output: 468
🏁 Script executed:
#!/bin/bash
# Read the ConfigMap script file to see what the create.sh and delete.sh do
cat -n ./charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yamlRepository: Kuadrant/helm-charts-olm
Length of output: 6796
🏁 Script executed:
#!/bin/bash
# Check the job definitions to see how they access credentials
cat -n ./charts/kuadrant-instances/templates/coredns/autodelegate/02-create-job.yaml
echo "---"
cat -n ./charts/kuadrant-instances/templates/coredns/autodelegate/04-delete-job.yamlRepository: Kuadrant/helm-charts-olm
Length of output: 2264
🏁 Script executed:
#!/bin/bash
# Check the secret definition and full RBAC
cat -n ./charts/kuadrant-instances/templates/coredns/autodelegate/05-coredns-config-secret.yaml
echo "---"
cat -n ./charts/kuadrant-instances/templates/coredns/autodelegate/00-rbac.yamlRepository: Kuadrant/helm-charts-olm
Length of output: 1773
🏁 Script executed:
#!/bin/bash
# Search for where coredns-autodelegate secret is created
rg -r 'coredns-autodelegate' --type=yaml ./charts/kuadrant-instances/templates/coredns/
echo "---"
# Also check if there's any kubectl secret operation in the entire autodelegate directory
rg -i 'kubectl.*secret|secret.*kubectl' --type=yaml ./charts/kuadrant-instances/templates/coredns/autodelegate/Repository: Kuadrant/helm-charts-olm
Length of output: 71
🏁 Script executed:
#!/bin/bash
# Broader search for coredns-autodelegate in the entire repository
rg 'coredns-autodelegate' ./charts/ --type=yaml
echo "---"
# Also check if any secret creation happens in shell scripts
rg -i 'kubectl.*create.*secret|kubectl.*apply.*secret' ./charts/kuadrant-instances/templates/coredns/ --type=yamlRepository: Kuadrant/helm-charts-olm
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check if coredns-autodelegate secret is created by any Job or external process
rg 'coredns-autodelegate' ./charts/ --type=yaml -B5 -A5
echo "---"
# Also check if there are any other references or comments about secret creation
rg -i 'secret.*create|create.*secret' ./charts/kuadrant-instances/templates/coredns/autodelegate/ --type=yamlRepository: Kuadrant/helm-charts-olm
Length of output: 2478
Remove unused secrets: [get, create] RBAC rule.
The create.sh and delete.sh scripts perform no kubectl secret operations—they only use kubectl get and kubectl wait on services. AWS credentials are injected via envFrom referencing the coredns-autodelegate secret, which is provided externally, not created by the Job. The secrets rule therefore violates the principle of least privilege and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kuadrant-instances/templates/coredns/autodelegate/00-rbac.yaml` around
lines 22 - 28, Remove the unused RBAC permission for Kubernetes secrets by
deleting the resources: - secrets block (and its verbs: - get / - create) from
the Role/ClusterRole in the coredns autodelegate RBAC manifest; specifically
remove the rule that lists apiGroups: [""] with resources: ["secrets"] and verbs
including "get" and "create" so the Role/ClusterRole only grants the remaining
necessary permissions (e.g., service-related verbs), preserving YAML structure
and indentation for the surrounding rules.
Comment on lines
+71
to
+93
| COREDNS_IP="$(kubectl get -n {{ .Values.coredns.namespace }} service coredns '-o=jsonpath={.status.loadBalancer.ingress[0].ip}')" | ||
| COREDNS_HOSTNAME="$(kubectl get -n {{ .Values.coredns.namespace }} service coredns '-o=jsonpath={.status.loadBalancer.ingress[0].hostname}')" | ||
|
|
||
| {{- range .Values.coredns.zones }} | ||
| ZONE="{{ . }}" | ||
|
|
||
| # Delete A record if using IP | ||
| if [ -n "$COREDNS_IP" ]; then | ||
| EXCLUDE_VALUE="${NS_NAME}.${ZONE}" | ||
| CURRENT_A=$(aws route53 list-resource-record-sets --hosted-zone-id "${ZONE_ID}" \ | ||
| --query "ResourceRecordSets[?Name=='${NS_NAME}.${ZONE}' && Type=='A']" --output json) | ||
| if [ "$CURRENT_A" != "[]" ]; then | ||
| TTL=$(echo "$CURRENT_A" | jq -r '.[0].TTL') | ||
| CHANGE_BATCH=$(jq -n \ | ||
| --arg name "${NS_NAME}.${ZONE}" \ | ||
| --arg ip "$COREDNS_IP" \ | ||
| --argjson ttl "$TTL" \ | ||
| '{Changes: [{Action: "DELETE", ResourceRecordSet: {Name: $name, Type: "A", TTL: $ttl, ResourceRecords: [{Value: $ip}]}}]}') | ||
| aws route53 change-resource-record-sets --hosted-zone-id "${ZONE_ID}" --change-batch "$CHANGE_BATCH" | ||
| fi | ||
| else | ||
| EXCLUDE_VALUE="${COREDNS_HOSTNAME}." | ||
| fi |
There was a problem hiding this comment.
delete.sh silently skips cleanup if the CoreDNS Service no longer has an LB ingress.
Both COREDNS_IP and COREDNS_HOSTNAME are best-effort kubectl get calls (no wait, no error handling). If the Service has already been deleted, is still pending, or was never ready, both variables are empty, and the else branch sets EXCLUDE_VALUE="." — which will not match any NS record value, so no NS cleanup happens and the stale delegation stays in Route53. Given this is a pre-delete hook the Service should still exist, but defensively:
kubectl waitfor.status.loadBalancer.ingresswith a short timeout, or- abort (
exit 1) when bothCOREDNS_IPandCOREDNS_HOSTNAMEare empty so the failure is visible rather than silent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml`
around lines 71 - 93, The script silently skips cleanup when both COREDNS_IP and
COREDNS_HOSTNAME are empty; update the pre-delete logic in delete.sh (the block
that sets COREDNS_IP/COREDNS_HOSTNAME and computes EXCLUDE_VALUE) to first wait
for the Service LB ingress (e.g., run a kubectl wait for
.status.loadBalancer.ingress with a short timeout against the service in {{
.Values.coredns.namespace }}) and then re-evaluate COREDNS_IP/COREDNS_HOSTNAME;
if after waiting both are still empty, fail fast (exit 1) with a clear error so
deletion does not silently skip cleanup. Ensure references to COREDNS_IP,
COREDNS_HOSTNAME, EXCLUDE_VALUE, NS_NAME and ZONE_ID remain unchanged so the
later A/NS deletion logic can proceed.
Comment on lines
+82
to
+90
| if [ "$CURRENT_A" != "[]" ]; then | ||
| TTL=$(echo "$CURRENT_A" | jq -r '.[0].TTL') | ||
| CHANGE_BATCH=$(jq -n \ | ||
| --arg name "${NS_NAME}.${ZONE}" \ | ||
| --arg ip "$COREDNS_IP" \ | ||
| --argjson ttl "$TTL" \ | ||
| '{Changes: [{Action: "DELETE", ResourceRecordSet: {Name: $name, Type: "A", TTL: $ttl, ResourceRecords: [{Value: $ip}]}}]}') | ||
| aws route53 change-resource-record-sets --hosted-zone-id "${ZONE_ID}" --change-batch "$CHANGE_BATCH" | ||
| fi |
There was a problem hiding this comment.
DELETE of the A record reuses the current service IP; this will fail if the IP changed.
Route53's DELETE action requires the ResourceRecords.Value in the change batch to match exactly what's stored in the zone. Here you read TTL from CURRENT_A but substitute $COREDNS_IP (from the current Service) for the value:
--arg ip "$COREDNS_IP" \If the LoadBalancer IP has changed since the record was created (e.g. cluster recreated without a clean uninstall, LB reassigned), the DELETE call will error with InvalidChangeBatch and leave a stale A record behind. Derive the value from CURRENT_A instead:
🛠️ Proposed fix
if [ "$CURRENT_A" != "[]" ]; then
TTL=$(echo "$CURRENT_A" | jq -r '.[0].TTL')
+ CURRENT_A_RECORDS=$(echo "$CURRENT_A" | jq '.[0].ResourceRecords')
CHANGE_BATCH=$(jq -n \
--arg name "${NS_NAME}.${ZONE}" \
- --arg ip "$COREDNS_IP" \
--argjson ttl "$TTL" \
- '{Changes: [{Action: "DELETE", ResourceRecordSet: {Name: $name, Type: "A", TTL: $ttl, ResourceRecords: [{Value: $ip}]}}]}')
+ --argjson records "$CURRENT_A_RECORDS" \
+ '{Changes: [{Action: "DELETE", ResourceRecordSet: {Name: $name, Type: "A", TTL: $ttl, ResourceRecords: $records}}]}')
aws route53 change-resource-record-sets --hosted-zone-id "${ZONE_ID}" --change-batch "$CHANGE_BATCH"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@charts/kuadrant-instances/templates/coredns/autodelegate/01-configMap-script.yaml`
around lines 82 - 90, The DELETE change-batch currently uses the live Service IP
($COREDNS_IP) which can differ from the value stored in the zone; modify the
block that builds CHANGE_BATCH (where CURRENT_A, TTL, CHANGE_BATCH and the aws
route53 change-resource-record-sets call are used) to extract the actual record
value from CURRENT_A (e.g. the ResourceRecords[0].Value) and use that extracted
IP for the --arg ip in the jq -n payload so the DELETE matches the exact
existing record.
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
2 participants
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.
Closes #43
Reverts #39 due to problems described in #39 (comment)
The cloud problem needs to be solved different way #49
Changes watch namespace to empty string to not have hardcoded "kuadrant" so now coredns will watch all namespaces
Added need for secrets in tools-instances due to autodelegate script needing AWS IAM user
Added autodelegation script which will add .Values.tools.coredns.zones to AWS
coredns-configsecret is created intoolsnamespace which contains deployed zone, useful for testsuite loadingNext work before merge: Kuadrant/testsuite-pipelines#130
Summary by CodeRabbit
Release Notes
New Features
Refactor