Replace Bitnami Usage #42
Conversation
WalkthroughMigrate external-dns from Bitnami to Kubernetes SIGs, update Helm chart source/version and chart_values to provider.name + extraArgs, remove Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TF as Terraform
participant Local as local.enabled
participant Module as module.external_dns
participant AWS as data.aws_partition
note over TF,Local: plan/apply begins
TF->>Local: evaluate local.enabled
alt enabled == true
TF->>Module: create module (count = 1)
Module->>AWS: read partition (current[0].partition)
Module->>Module: render Helm values (provider.name + extraArgs)
Module-->>TF: export metadata (module.external_dns[0].metadata)
else enabled == false
TF-->>Module: skip creation (count = 0)
TF-->>TF: output metadata = null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Important Description is necessary and should not be empty.Kindly provide details with what was changed, why it was changed. |
|
/terratest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
test/fixtures/stacks/catalog/usecase/disabled.yaml (2)
28-36: Example block aligns with upstream values schema—consider echoing a minimal concrete configThe example shifting to provider.name + extraArgs is correct for upstream. Optional: for consistency with basic.yaml, consider providing a minimal concrete chart_values example even in the disabled use case (will not apply when enabled=false) so test fixtures mirror the intended schema everywhere.
39-39: Nit: keep fixture comments aligned across use casesdns_components: [] remains unchanged; no action needed. If you later add entries in basic.yaml to exercise multi-zone logic, mirror a commented example here for parity between fixtures.
src/resources/values.yaml (1)
1-5: Defaulting AWS flag via extraArgs is fine—document/guard list override behaviorPlacing --aws-evaluate-target-health=false in extraArgs at the base values works, but note Helm’s list semantics: if users set chart_values.extraArgs, they will override (not merge) this default and silently drop the flag. Two options:
- Document this explicitly in README.yaml (values section) and in the catalog example.
- Or, in src/main.tf, render values as two separate values documents (base defaults and user overrides) where you programmatically concatenate lists. If helpful, I can propose a Terraform snippet to merge lists before yamlencoding.
I can open a follow-up PR that merges extraArgs from defaults + user input to avoid accidental drops.
src/main.tf (1)
50-74: Consider tying iam_role_enabled to component enablementWhen the component is disabled, keeping
iam_role_enabled = trueis noisy and can surface evaluation gotchas in downstream expressions. Aligning it withlocal.enabledkeeps behavior predictable and reduces risk.- iam_role_enabled = true + iam_role_enabled = local.enabledsrc/README.md (2)
156-158: RBAC description is vague; clarify user intent“Service Account for pods.” doesn’t explain behavior. Suggest clarifying that this toggles creation of RBAC resources (ClusterRole/Binding) required by the ServiceAccount.
-| <a name="input_rbac_enabled"></a> [rbac_enabled](#input_rbac_enabled) | Service Account for pods. | `bool` | `true` | no | +| <a name="input_rbac_enabled"></a> [rbac_enabled](#input_rbac_enabled) | Whether to create RBAC resources (ClusterRole/ClusterRoleBinding) for the service account used by ExternalDNS. | `bool` | `true` | no |
51-52: Link points to Artifact Hub: consider adding a direct link to the chart values schemaFor migrations, a direct link to the chart’s values.yaml (in GitHub) helps users map keys quickly when moving from Bitnami.
Propose adding a second link: “Values reference (GitHub)” right after the Artifact Hub link.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
README.md(1 hunks)README.yaml(4 hunks)src/README.md(5 hunks)src/main.tf(2 hunks)src/resources/values.yaml(1 hunks)src/variables.tf(1 hunks)test/fixtures/stacks/catalog/usecase/basic.yaml(2 hunks)test/fixtures/stacks/catalog/usecase/disabled.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (11)
test/fixtures/stacks/catalog/usecase/disabled.yaml (1)
13-15: Verify external-dns chart version and Kubernetes compatibility
- Confirmed that version 1.18.0 of the external-dns Helm chart is published in the kubernetes-sigs repository, and that the current latest release is 1.19.0 .
- If you intend to stick with 1.18.0, please check its
kubeVersionconstraint in the Chart.yaml to ensure it supports your target Kubernetes versions. Otherwise, consider bumping to 1.19.0 for the latest fixes and features.test/fixtures/stacks/catalog/usecase/basic.yaml (2)
28-36: Docs link and schema look correct for upstream; niceThe Artifact Hub path and the provider.name + extraArgs example align with upstream chart conventions.
13-15: external-dns Helm chart v1.18.0 is available and supports Kubernetes ≥1.21
- The
external-dns-helm-chart-1.18.0release is published in thehttps://kubernetes-sigs.github.io/external-dns/repository. (github.com)- According to the ExternalDNS compatibility matrix, versions ≥0.18.x (i.e. the chart’s AppVersion v0.18.0) only support Kubernetes 1.21 and above. Specifically:
• Kubernetes 1.21 ✅
• Kubernetes ≥1.22 and ≤1.32 ✅
• Kubernetes ≥1.33 ✅
• Kubernetes ≤1.20 ❌ (github.com)Please ensure your CI clusters are running one of the supported Kubernetes versions (≥1.21) before pinning chart v1.18.0.
src/main.tf (2)
98-107: Provider block shape is correct as a mappingThe upstream external-dns chart v1.18.0 defines
provideras a map with anamekey (and additional subkeys such aswebhook), not as a simple string. Your Terraformyamlencodealready produces:provider: name: aws # …other provider settings…which exactly matches the chart’s
values.yamlschema. No change is required here.
89-96: Manual Verification of External-DNS Metrics Keys RequiredI attempted to fetch the upstream
values.yamlfor kubernetes-sigs/external-dns v1.18.0 but couldn’t locate any of themetrics,serviceMonitor, orprometheusblocks in the expected paths. To ensure we’re using the correct hierarchy for enabling the ServiceMonitor, please manually inspect the chart’svalues.yamlat your targeted version and confirm which of the following is valid:
serviceMonitor.enabledmetrics.serviceMonitor.enabledprometheus.serviceMonitor.enabledOnce you’ve identified the correct path, update the snippet in
src/main.tf(lines 89–96) so that theyamlencodematches the upstream schema.src/README.md (2)
51-60: Provider configuration is already correctThe upstream Helm chart for ExternalDNS recommends using a nested
provider.namefield (with additional settings underprovider.<name>.<key>) rather than the legacy flatprovider: awssyntax — exactly as shown in your example snippet. (github.com, kubernetes-sigs.github.io)No changes are needed to the README example.
Likely an incorrect or invalid review comment.
36-38: Chart version “1.18.0” upstream availability confirmed
- Verified that version 1.18.0 is listed in the external-dns Helm repository index (https://kubernetes-sigs.github.io/external-dns/index.yaml).
- No changes to the pinned chart_version in src/README.md (lines 36–38) are needed.
README.yaml (4)
6-8: Great doc update to upstream repoSwitching references to kubernetes-sigs/external-dns is consistent and clear. Nice.
68-75: References updated correctlyArtifact Hub and GitHub links now point to the upstream project. Looks good.
31-33: chart_version “1.18.0” confirmed in upstream repoThe pinned Helm chart version 1.18.0 was found in the external-dns index (see index.yaml lines showing version: 1.18.0 and corresponding download URL), so no update is needed and examples remain aligned.
51-55: No changes required for theprovidershape
Upstream theexternal-dnsHelm chart explicitly definesprovider: # @schema type: [object, string]; name: aws …meaning it accepts either an object (with
nameand nested settings) or a simple string. The commented example in this README uses the object form, which exactly matches the chart’s default values.yaml. You may optionally useprovider: awsfor simpler cases, but the existing snippet is correct as written.
milldr
added
major
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main.tf (1)
61-61: Fix unsafe indexing: data.aws_partition when disabled will cause plan failureAccessing
data.aws_partition.current[0].partitionwill panic with an index error whenlocal.enabled = false(becausedata.aws_partition.currenthascount = 0). Terraform still evaluates this expression even if the enclosing module hascount = 0. This was raised earlier; repeating here since it remains.Apply one of the safe patterns below (Option A mirrors prior behavior and avoids evaluation issues):
- resources = formatlist("arn:${data.aws_partition.current[0].partition}:route53:::hostedzone/%s", local.zone_ids) + resources = formatlist("arn:${join("", data.aws_partition.current.*.partition)}:route53:::hostedzone/%s", local.zone_ids)Or, short-circuit when disabled (also safe):
- resources = formatlist("arn:${data.aws_partition.current[0].partition}:route53:::hostedzone/%s", local.zone_ids) + resources = local.enabled ? formatlist("arn:${join("", data.aws_partition.current.*.partition)}:route53:::hostedzone/%s", local.zone_ids) : []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main.tf(3 hunks)src/outputs.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
src/outputs.tf (1)
2-2: LGTM: Output safely gated behind local.enabledUsing a conditional to avoid evaluating
module.external_dns[0]when disabled prevents "index out of range" errors and matches the module’s count-based gating. Looks good.src/main.tf (1)
27-27: LGTM: Module creation gated via count
count = local.enabled ? 1 : 0is consistent with the output’s conditional indexing and prevents unnecessary evaluation of Helm arguments during disablement.
|
/terratest |
|
/terratest |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/component_test.go (4)
69-76: Swap expected/actual in assertions to improve failure messagesassert.Equal takes expected before actual. Current order flips them, which muddles diffs on failure.
Apply this minimal diff for the changed lines:
- assert.Equal(s.T(), metadata.AppVersion, "0.18.0") + assert.Equal(s.T(), "0.18.0", metadata.AppVersion) ... - assert.Equal(s.T(), metadata.Version, "1.18.0") + assert.Equal(s.T(), "1.18.0", metadata.Version)If you want to align the whole block for consistency, I can follow up with a broader patch.
69-76: Reduce test flakiness: replace fixed 2-minute sleep with polling/backoffExternalDNS + Route53 propagation is eventually consistent; a fixed sleep can be flaky or unnecessarily slow.
Proposal: poll Route53 until the record appears (with timeout), instead of time.Sleep.
I can add a small helper (using the AWS SDK or a Terratest list call) to wait up to N minutes, checking every few seconds. Say the word and I’ll push a concrete implementation.
35-38: Nit: fix variable name typo for clarityclusrerId → clusterID. Improves readability and follows Go naming conventions.
- clusrerId := atmos.Output(s.T(), clusterOptions, "eks_cluster_id") - cluster := awsHelper.GetEksCluster(s.T(), context.Background(), awsRegion, clusrerId) + clusterID := atmos.Output(s.T(), clusterOptions, "eks_cluster_id") + cluster := awsHelper.GetEksCluster(s.T(), context.Background(), awsRegion, clusterID)
69-76: Centralize hard-coded version literals in testsThe assertions in test/component_test.go (lines 69–76) currently embed literal versions (
"0.18.0"and"1.18.0") that don’t appear elsewhere in fixtures, source, or docs. Pinning these values in the test means you’ll need to update the file on every chart/app version bump.• File: test/component_test.go
Lines: 69–76Introduce file-level constants and update the assertions to use them:
package test +const ( + expectedAppVersion = "0.18.0" + expectedChartVersion = "1.18.0" +) @@ -69,7 +73,7 @@ func (s *ComponentTestSuite) TestMetadata() { - assert.Equal(s.T(), metadata.AppVersion, "0.18.0") + assert.Equal(s.T(), expectedAppVersion, metadata.AppVersion) assert.Equal(s.T(), metadata.Chart, "external-dns") assert.NotNil(s.T(), metadata.FirstDeployed) assert.NotNil(s.T(), metadata.LastDeployed) @@ -75,7 +79,7 @@ func (s *ComponentTestSuite) TestMetadata() { assert.NotNil(s.T(), metadata.Values) - assert.Equal(s.T(), metadata.Version, "1.18.0") + assert.Equal(s.T(), expectedChartVersion, metadata.Version) }Optional: if these version values are already surfaced via your Atmos stack inputs or another central source, consider reading them at test-time instead, so they automatically stay in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/component_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (7)
test/component_test.go (7)
7-7: Import reordering looks goodImporting testing within the main block is correct and consistent with Go conventions.
9-9: Whitespace-only changeNo functional impact.
12-13: Import aliasing and order are appropriateawsHelper and helm are both used; aliases/readability are clear.
21-21: Runtime schema import is correctUsed for GroupVersionResource; matches usage below.
100-100: Formatting-only changeMap alignment change is cosmetic; no behavioral impact.
106-106: Formatting-only changeWhitespace alignment in the spec block; no functional difference.
21-21: Chart CRD configuration not found in this repository
It appears there is no Helm chart present here (noChart.yamlorcrds/directory under any chart folder), so CRD installation and “CRD source” settings cannot be verified in this repo. Please instead confirm in your Helm chart repository that:
- The
DNSEndpointCRD (fromexternaldns.k8s.io/v1alpha1) is installed under the chart’scrds/directory.- The chart’s default values (or the CLI flags) enable the CRD source (e.g.
--source=crdorvalues.sourcesincludes"crd").- If using an
installCRDstoggle (for helm v3.7+), it’s set appropriately.Likely an incorrect or invalid review comment.
|
/terratest |
1 similar comment
|
/terratest |
|
These changes were released in v2.0.0. |
what
why
ref
Summary by CodeRabbit
Documentation
Chores
Refactor
Tests