feat: add external-dns/dnsenpoint generation for the gateway #364
Conversation
WalkthroughThis PR adds DNS endpoint reconciliation for gateway resources, enabling dynamic creation and management of DNS records via the external-dns operator. It introduces configuration for private and public DNS endpoints, a new reconciliation workflow, supporting helpers, scheme registration, and test infrastructure updates. Changes
Sequence DiagramsequenceDiagram
participant Gateway as Gateway Resource
participant Reconciler as Gateway Reconciler
participant ConfigMgr as Configuration Manager
participant DNSMgr as DNS Endpoint Manager
participant DNSEndpoint as DNSEndpoint CRD
participant ExtDNS as External-DNS Operator
Reconciler->>Reconciler: isDNSEndpointAvailable?
alt DNS Endpoint Available
Reconciler->>ConfigMgr: getDNSConfig(private)
ConfigMgr-->>Reconciler: config
Reconciler->>DNSMgr: createDNSEndpoint(private)
DNSMgr->>DNSMgr: expandDNSPattern({stack})
DNSMgr->>DNSEndpoint: Create/Update Endpoint
DNSEndpoint->>ExtDNS: Trigger DNS reconciliation
ExtDNS->>ExtDNS: Manage DNS records
Reconciler->>ConfigMgr: getDNSConfig(public)
ConfigMgr-->>Reconciler: config
Reconciler->>DNSMgr: createDNSEndpoint(public)
DNSMgr->>DNSEndpoint: Create/Update Endpoint
DNSEndpoint->>ExtDNS: Trigger DNS reconciliation
else DNS Endpoint Disabled
Reconciler->>DNSMgr: reconcileDNSEndpoints
DNSMgr->>DNSEndpoint: Delete Endpoint (if exists)
end
Reconciler-->>Gateway: Reconciliation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
🧹 Nitpick comments (4)
docs/09-Configuration reference/01-Settings.md (1)
62-73: DNS endpoint settings docs align with implementationThe new
gateway.dns.private.*/gateway.dns.public.*rows and the “Configure DNS Endpoints” section accurately reflect the configuration consumed bygetDNSConfigandreconcileDNSEndpoints(keys,{stack}placeholder, defaultCNAME, provider-specific/annotations behavior). This should be sufficient for operators to configure both private and public DNSEndpoints.One small improvement would be to explicitly call out that
dns-nameandtargetsare mandatory whenever*.enabledis set totrue, to mirror the validation logic in code and make misconfigurations easier to spot.Also applies to: 278-406
internal/resources/gateways/init.go (1)
27-28: Gateway reconciler wiring for DNSEndpoints looks correct
- Importing
externaldnsv1alpha1and adding the RBAC rule forexternaldns.k8s.io/dnsendpointsmatches the new use ofDNSEndpointin reconciliation.- Calling
reconcileDNSEndpoints(ctx, gateway)after service/ingress creation ensures DNS is reconciled against the current gateway state.- Registering
WithOwn[*v1beta1.Gateway](&externaldnsv1alpha1.DNSEndpoint{})will cause DNSEndpoint changes to requeue the owning gateway, which is desirable.Be aware that any configuration error in DNS settings will cause
Reconcileto return an error (viareconcileDNSEndpoints), so DNS misconfigurations will block the gateway reconcile loop until fixed; if you ever want DNS issues to be non-blocking, you’d need to surface them via a Condition instead.Also applies to: 40-40, 89-92, 110-110
internal/resources/gateways/dnsendpoints.go (2)
86-142: DNSEndpoint creation is correct; stabilize provider-specific orderingThe overall DNSEndpoint creation logic is solid:
- One
Endpointper DNS pattern with{stack}expansion,- Shared targets and record type,
- Optional provider-specific properties and annotations,
- OwnerReference set via
core.WithController.One subtle improvement: iteration over
config.ProviderSpecis over a map, so the order ofProviderSpecificentries ind.Spec.Endpointsis non-deterministic between reconciles. That can cause spurious “changes” and unnecessary updates when comparing specs.You can make the spec stable by sorting the keys before building
ProviderSpecific:- if len(config.ProviderSpec) > 0 { - providerSpecific := endpoint.ProviderSpecific{} - for name, value := range config.ProviderSpec { - providerSpecific = append(providerSpecific, endpoint.ProviderSpecificProperty{ - Name: name, - Value: value, - }) - } - ep.ProviderSpecific = providerSpecific - } + if len(config.ProviderSpec) > 0 { + providerSpecific := endpoint.ProviderSpecific{} + keys := make([]string, 0, len(config.ProviderSpec)) + for name := range config.ProviderSpec { + keys = append(keys, name) + } + sort.Strings(keys) + for _, name := range keys { + providerSpecific = append(providerSpecific, endpoint.ProviderSpecificProperty{ + Name: name, + Value: config.ProviderSpec[name], + }) + } + ep.ProviderSpecific = providerSpecific + }Optionally, you might also normalize
RecordTypetostrings.ToUpperfor consistency with common DNS tooling.
144-181: DNS reconcile flow matches settings semantics; errors are currently blocking
reconcileDNSEndpointscleanly implements the intended behavior:
- For each of
privateandpublic, it:
- Loads config via
getDNSConfig,- Creates/updates DNSEndpoints when config is present,
- Deletes
{gateway-name}-private/{gateway-name}-publicwhen config is absent/disabled.This is easy to reason about and aligns with the documented naming.
Note that any validation error from
getDNSConfig(e.g., missingdns-name/targets) will bubble up and fail the entire gateway reconcile. If you prefer misconfigured DNS to not block gateway deployments, you could instead:
- Capture these as Conditions on the Gateway, and
- Swallow the error (or convert it to a
Pending-style error) while still progressing the rest of the reconcile.That design choice is outside the core correctness here but worth considering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (8)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/rbac/role.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/operator/templates/gen/rbac.authorization.k8s.io_v1_clusterrole_formance-manager-role.yamlis excluded by!**/gen/**,!**/*.yaml,!**/gen/**tools/kubectl-stacks/go.modis excluded by!**/*.modtools/kubectl-stacks/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
cmd/main.go(2 hunks)docs/09-Configuration reference/01-Settings.md(2 hunks)internal/resources/gateways/dnsendpoints.go(1 hunks)internal/resources/gateways/init.go(4 hunks)internal/resources/stacks/init.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/resources/gateways/init.go (1)
internal/core/reconciler.go (1)
WithOwn(70-74)
internal/resources/gateways/dnsendpoints.go (4)
internal/core/context.go (1)
Context(10-16)internal/resources/settings/helpers.go (4)
GetBoolOrDefault(235-244)GetStringOrEmpty(57-59)GetMapOrEmpty(376-386)GetStringOrDefault(46-55)api/formance.com/v1beta1/shared.go (1)
Dependent(280-283)internal/core/utils.go (1)
WithController(70-79)
🔇 Additional comments (2)
internal/resources/stacks/init.go (1)
44-44: RBAC fordnsendpointsis consistent and sufficientThe additional RBAC rule for
externaldns.k8s.iodnsendpointswith full CRUD verbs matches how DNSEndpoints are managed by the operator and looks correct.cmd/main.go (1)
35-36: ExternalDNS scheme registration is correctly wiredImporting
sigs.k8s.io/external-dns/apis/v1alpha1and callingexternaldnsv1alpha1.AddToScheme(scheme)is the right way to makeDNSEndpointusable by the manager and reconcilers.Please verify that the external-dns module/version in
go.modmatches this import path and that CI builds the manager binary successfully.Also applies to: 48-51
| type DNSConfig struct { | ||
| Enabled bool | ||
| DNSPatterns []string | ||
| Targets []string | ||
| ProviderSpec map[string]string | ||
| Annotations map[string]string | ||
| RecordType string | ||
| } |
There was a problem hiding this comment.
Config loading is sound; consider filtering empty entries and trimming unused field
getDNSConfig correctly:
- Gates on
*.enabled, - Enforces presence of
dns-nameandtargetswhen enabled, - Splits comma-separated values,
- Loads maps and defaults
record-typetoCNAME.
Two minor points:
- After splitting
dns-name/targets, empty items (for example, from trailing commas or",,") are kept as-is. That can yield invalid/empty DNS names or targets in the generatedDNSEndpoint. It would be safer to trim and either drop or reject empty entries. - The
Enabledfield onDNSConfigis never read;nilvs non-nilalready encodes enabled/disabled. You can drop it to avoid confusion.
Example refinement:
- dnsNames := strings.Split(dnsNameStr, ",")
- for i := range dnsNames {
- dnsNames[i] = strings.TrimSpace(dnsNames[i])
- }
+ rawNames := strings.Split(dnsNameStr, ",")
+ dnsNames := make([]string, 0, len(rawNames))
+ for _, n := range rawNames {
+ n = strings.TrimSpace(n)
+ if n != "" {
+ dnsNames = append(dnsNames, n)
+ }
+ }
+ if len(dnsNames) == 0 {
+ return nil, fmt.Errorf("gateway.dns.%s.dns-name must contain at least one non-empty entry", dnsType)
+ }(and similarly for targets), plus optionally removing Enabled from DNSConfig if not needed.
Also applies to: 27-84
🤖 Prompt for AI Agents
internal/resources/gateways/dnsendpoints.go around lines 18-25 (and apply same
change for parsing logic in lines ~27-84): the DNSConfig struct includes an
Enabled bool that is never read and the parsing of comma-separated dns-name and
targets preserves empty/whitespace entries; remove the Enabled field from
DNSConfig to avoid confusion, and in the parsing code trim whitespace from each
split item and filter out empty strings (or return an error if you prefer strict
validation) for both dns-name and targets so the resulting DNS names and targets
are non-empty and valid before creating DNSEndpoint objects.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/resources/gateways/dnsendpoints.go (3)
18-25: Drop redundantEnabledfield fromDNSConfig
getDNSConfigalready returnsnilwhengateway.dns.<type>.enabledisfalse, and the rest of the code only checks forconfig != nilvsnil. TheEnabledfield is never read and duplicates that state, which can confuse future readers.You can simplify
DNSConfigby removingEnabledand its initialization.
37-57: Filter out empty dns-names/targets after trimming to avoid invalid endpointsAfter
GetStringSlice, you trim each entry but don’t drop empty ones. Inputs like", ,my-host"or"my-target,,"will pass thelen(...) == 0checks, but after trimming will still contain empty strings. Those propagate intoEndpoint.DNSName/Targets, potentially producing invalid DNSEndpoint specs.Consider normalizing both slices by trimming and filtering, and revalidating that at least one non-empty value remains. For example:
- dnsNames, err := settings.GetStringSlice(ctx, stack, "gateway", "dns", dnsType, "dns-names") + dnsNames, err := settings.GetStringSlice(ctx, stack, "gateway", "dns", dnsType, "dns-names") if err != nil { return nil, err } - if len(dnsNames) == 0 { - return nil, fmt.Errorf("gateway.dns.%s.dns-names is required when gateway.dns.%s.enabled is true", dnsType, dnsType) - } - for i := range dnsNames { - dnsNames[i] = strings.TrimSpace(dnsNames[i]) - } + cleanNames := make([]string, 0, len(dnsNames)) + for _, n := range dnsNames { + n = strings.TrimSpace(n) + if n != "" { + cleanNames = append(cleanNames, n) + } + } + if len(cleanNames) == 0 { + return nil, fmt.Errorf("gateway.dns.%s.dns-names must contain at least one non-empty entry when gateway.dns.%s.enabled is true", dnsType, dnsType) + } + dnsNames = cleanNames @@ - targets, err := settings.GetStringSlice(ctx, stack, "gateway", "dns", dnsType, "targets") + targets, err := settings.GetStringSlice(ctx, stack, "gateway", "dns", dnsType, "targets") if err != nil { return nil, err } - if len(targets) == 0 { - return nil, fmt.Errorf("gateway.dns.%s.targets is required when gateway.dns.%s.enabled is true", dnsType, dnsType) - } - for i := range targets { - targets[i] = strings.TrimSpace(targets[i]) - } + cleanTargets := make([]string, 0, len(targets)) + for _, t := range targets { + t = strings.TrimSpace(t) + if t != "" { + cleanTargets = append(cleanTargets, t) + } + } + if len(cleanTargets) == 0 { + return nil, fmt.Errorf("gateway.dns.%s.targets must contain at least one non-empty entry when gateway.dns.%s.enabled is true", dnsType, dnsType) + } + targets = cleanTargets(and then use
dnsNames/targetsas you already do when constructingDNSConfig).This prevents empty DNS names/targets from ever reaching the DNSEndpoint spec.
181-194: Make deletion fully idempotent by ignoring NotFound onDeleteas wellYou already ignore
NotFoundon theGet, but there is still a small race where another actor (or GC) deletes the DNSEndpoint betweenGetandDelete, causingDeleteto return aNotFounderror and fail reconciliation unnecessarily.Align this with existing patterns in the codebase by wrapping the
Deletecall withclient.IgnoreNotFound:- return ctx.GetClient().Delete(ctx, dnsEndpoint) + return client.IgnoreNotFound(ctx.GetClient().Delete(ctx, dnsEndpoint))This makes
deleteDNSEndpointfully idempotent.
🧹 Nitpick comments (2)
docs/09-Configuration reference/01-Settings.md (1)
279-407: Clarify behavior when disabling DNS endpoint settingsThe docs clearly explain how endpoints are created, but they don’t mention what happens when
gateway.dns.private.enabled/gateway.dns.public.enabledare later set back tofalse. Since the reconciler deletes the corresponding DNSEndpoint when disabled, consider adding a short note here that turning these flags off will remove the{gateway-name}-private/{gateway-name}-publicDNSEndpoint resources from the stack namespace, for symmetry and to set expectations.internal/resources/gateways/dnsendpoints.go (1)
102-115: BuildProviderSpecificin a deterministic order
providerSpecificis constructed by iterating over a map, which yields an arbitrary key order. That can cause theProviderSpecificslice order to fluctuate between reconciles, leading to noisy patches even when the underlying settings haven’t changed.Consider collecting the keys from
config.ProviderSpec, sorting them, and then appending properties in that order to keep the generated spec stable across runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/09-Configuration reference/01-Settings.md(2 hunks)internal/resources/gateways/dnsendpoints.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/resources/gateways/dnsendpoints.go (4)
internal/core/context.go (1)
Context(10-16)internal/resources/settings/helpers.go (4)
GetBoolOrDefault(235-244)GetStringSlice(61-70)GetMapOrEmpty(376-386)GetStringOrDefault(46-55)api/formance.com/v1beta1/shared.go (1)
Dependent(280-283)internal/core/utils.go (1)
WithController(70-79)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/resources/settings/helpers.go (1)
77-79: Consider explicit nil return for clarity.At line 78,
erris guaranteed to benil(since any non-nil error would have caused an early return at line 75). Whilereturn nil, erris consistent withGetStringSliceat line 67, explicitly returningnil, nilwould be clearer.Apply this diff for improved clarity:
if value == nil { - return nil, err + return nil, nil }Note: Consider updating line 67 in
GetStringSlicesimilarly for consistency across the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/resources/gateways/dnsendpoints.go(1 hunks)internal/resources/settings/helpers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/resources/gateways/dnsendpoints.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/resources/settings/helpers.go (1)
internal/core/context.go (1)
Context(10-16)
🔇 Additional comments (1)
internal/resources/settings/helpers.go (1)
72-89: GetTrimmedStringSlice function is properly used in the codebase.Verification confirms the function is actively called in
internal/resources/gateways/dnsendpoints.goat lines 37 and 45, where it retrieves DNS names and targets for the gateway DNS endpoint management feature. The implementation is correct and follows the established patterns of existing helper functions.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
internal/tests/crds/externaldns.k8s.io_DNSEnpoint.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
internal/tests/internal/bootstrap.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/tests/internal/bootstrap.go (1)
api/formance.com/v1beta1/groupversion_info.go (1)
AddToScheme(44-44)
🔇 Additional comments (1)
internal/tests/internal/bootstrap.go (1)
75-75: LGTM! Scheme assignment required for external-dns types recognition.Setting the
Schemefield ensures the test environment recognizes external-dns API types registered in theinit()function. This is necessary for tests that interact with DNSEndpoint resources.
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | ||
| "sigs.k8s.io/controller-runtime/pkg/metrics/server" | ||
| "sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
| externaldnsv1alpha1 "sigs.k8s.io/external-dns/apis/v1alpha1" |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! External-DNS scheme registration follows standard pattern.
The import and scheme registration in the init() function correctly integrates external-dns v1alpha1 API types into the test environment. The panic on registration failure is acceptable for init functions.
Since external-dns dependencies and versions may have changed since my knowledge cutoff, please verify the library version for security advisories:
Also applies to: 47-52
🏁 Script executed:
#!/bin/bash
# Check for security advisories for external-dns
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "sigs.k8s.io/external-dns") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 49
Now checking if the CRD directory referenced in line 69 exists with necessary external-dns CRD files:
🏁 Script executed:
#!/bin/bash
# Check if CRD directory exists and list contents
if [ -d "internal/tests/crds" ]; then
echo "=== CRD directory exists ==="
ls -la internal/tests/crds/
echo ""
echo "=== External-DNS CRD files ==="
find internal/tests/crds -name "*external*dns*" -o -name "*externaldns*" 2>/dev/null
else
echo "CRD directory not found"
fiLength of output: 23
Checking the actual bootstrap.go file to understand the context of the CRD directory reference:
🏁 Script executed:
#!/bin/bash
# View bootstrap.go to understand CRD directory usage
cat internal/tests/internal/bootstrap.go | head -100Length of output: 2699
Checking if external-dns CRD files exist in the repository and verifying the config directory:
🏁 Script executed:
#!/bin/bash
# Search for external-dns CRD files and check config/crd/bases directory
echo "=== Checking config/crd/bases directory ==="
if [ -d "config/crd/bases" ]; then
echo "Directory exists, contents:"
ls -la config/crd/bases/
else
echo "Directory does not exist"
fi
echo ""
echo "=== Searching for external-dns CRD files ==="
find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "external.*dns" 2>/dev/null | head -20
echo ""
echo "=== Checking entire repo for 'externaldnscluster' or 'externaldns' types ==="
grep -r "kind.*External" --include="*.yaml" --include="*.yml" . 2>/dev/null | head -10Length of output: 192
CRD directory path missing - tests will fail on startup
The changes add internal/tests/crds to the CRDDirectoryPaths array (line 69-70), but this directory does not exist. With ErrorIfCRDPathMissing: true on line 68, the test environment will fail to initialize.
Either create the directory with external-dns CRD files, or remove the path from the CRDDirectoryPaths array if external-dns resources aren't needed in integration tests.
Note: config/crd/bases referenced in line 69 also doesn't exist in the repository.
🤖 Prompt for AI Agents
In internal/tests/internal/bootstrap.go around lines 27 and especially lines
~68-70, the CRDDirectoryPaths array includes non-existent paths
("internal/tests/crds" and "config/crd/bases") while ErrorIfCRDPathMissing is
set to true, causing testenv startup failures; fix by either (A) creating the
referenced directories and adding the required external-dns CRD YAMLs into
internal/tests/crds (and any CRDs under config/crd/bases), or (B) remove those
non-existent paths from the CRDDirectoryPaths array (or set
ErrorIfCRDPathMissing to false) so the test environment no longer errors on
missing CRD dirs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/resources/gateways/init.go (1)
119-152: Consider refactoring for consistency and efficiency.The runtime CRD detection logic works correctly, but could be improved:
Inconsistent error handling: Line 128 uses
panic(err)while line 134 returns errors. Consider returning the error consistently or document why panic is necessary here.Inefficient CRD lookup: Lines 131-135 list all CRDs in the cluster to find one. For better performance, use
client.Get()with a specific CRD name:crd := &apiextensionsv1.CustomResourceDefinition{} err := client.Get(ctx, types.NamespacedName{ Name: "dnsendpoints.externaldns.k8s.io", }, crd) if err == nil { // CRD exists isDNSEndpointAvailable = true builder.Owns(&externaldnsv1alpha1.DNSEndpoint{}) } else if !apierrors.IsNotFound(err) { return err }
- Add logging: Consider logging whether DNS endpoint reconciliation is enabled/disabled to aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
config/rbac/role.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modhelm/operator/templates/gen/rbac.authorization.k8s.io_v1_clusterrole_formance-manager-role.yamlis excluded by!**/gen/**,!**/*.yaml,!**/gen/**
📒 Files selected for processing (2)
cmd/main.go(3 hunks)internal/resources/gateways/init.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/main.go
🔇 Additional comments (4)
internal/resources/gateways/init.go (4)
26-31: LGTM! Imports align with the DNS endpoint reconciliation feature.The new imports are necessary for runtime CRD detection and DNS endpoint ownership registration.
44-45: LGTM! RBAC permissions are appropriately scoped.The permissions grant full lifecycle management for DNS endpoints and read-only access for CRD detection.
94-99: LGTM! Conditional reconciliation logic is correct.The DNS endpoint reconciliation is properly guarded and errors are propagated. Note that
isDNSEndpointAvailableis determined once at startup, so installing the DNSEndpoint CRD after the operator starts would require a restart to activate this feature.
108-108: LGTM! Package-level flag is appropriate for this use case.The flag is set once during initialization and safely read during reconciliation.
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
This reverts commit b705bad.
sylr
force-pushed
the
feat/gateway-dnsendpoints
branch
from
32952be to
191968b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/resources/gateways/dnsendpoints.go (2)
18-25: Remove unusedEnabledfield from DNSConfig.The
Enabledfield is set at line 69 but never read. The caller (reconcileDNSEndpoints) determines enablement by checking whethergetDNSConfigreturnsnil, making this field redundant.Apply this diff:
type DNSConfig struct { - Enabled bool DNSPatterns []string Targets []string ProviderSpec map[string]string Annotations map[string]string RecordType string }And remove the assignment at line 69:
return &DNSConfig{ - Enabled: enabled, DNSPatterns: dnsNames, Targets: targets, ProviderSpec: providerSpec, Annotations: annotations, RecordType: recordType, }, nil
175-189: Make DNSEndpoint deletion fully idempotent (still not addressed).Despite the past review comment indicating this was addressed in commit 1b80262, line 188 still lacks
client.IgnoreNotFoundwrapping. The race condition remains: if another actor deletes the DNSEndpoint between theGet(line 180) andDelete(line 188), theDeletewill fail withNotFound, unnecessarily failing reconciliation.Apply this diff to make deletion fully idempotent:
- return ctx.GetClient().Delete(ctx, dnsEndpoint) + return client.IgnoreNotFound(ctx.GetClient().Delete(ctx, dnsEndpoint))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (10)
.github/actions/default/action.ymlis excluded by!**/*.ymlconfig/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/rbac/role.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/operator/templates/gen/rbac.authorization.k8s.io_v1_clusterrole_formance-manager-role.yamlis excluded by!**/gen/**,!**/*.yaml,!**/gen/**internal/tests/crds/externaldns.k8s.io_DNSEnpoint.yamlis excluded by!**/*.yamltools/kubectl-stacks/go.modis excluded by!**/*.modtools/kubectl-stacks/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
cmd/main.go(3 hunks)docs/09-Configuration reference/01-Settings.md(2 hunks)internal/resources/gateways/dnsendpoints.go(1 hunks)internal/resources/gateways/init.go(4 hunks)internal/resources/settings/helpers.go(1 hunks)internal/resources/stacks/init.go(1 hunks)internal/tests/internal/bootstrap.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/resources/stacks/init.go
- internal/tests/internal/bootstrap.go
- cmd/main.go
- docs/09-Configuration reference/01-Settings.md
🧰 Additional context used
🧬 Code graph analysis (3)
internal/resources/settings/helpers.go (1)
internal/core/context.go (1)
Context(10-16)
internal/resources/gateways/init.go (3)
internal/core/reconciler.go (1)
WithRaw(76-80)internal/core/context.go (1)
Context(10-16)internal/tests/internal/bootstrap.go (2)
GetScheme(43-45)List(228-230)
internal/resources/gateways/dnsendpoints.go (4)
internal/core/context.go (1)
Context(10-16)internal/resources/settings/helpers.go (5)
GetBoolOrDefault(254-263)GetTrimmedStringSlice(72-89)GetMapOrEmpty(395-405)GetStringOrDefault(46-55)Get(22-40)api/formance.com/v1beta1/shared.go (1)
Dependent(280-283)internal/core/utils.go (1)
WithController(70-79)
🔇 Additional comments (1)
internal/resources/settings/helpers.go (1)
72-89: LGTM! Clean implementation of trimmed string slice helper.The function correctly:
- Propagates errors from GetString
- Returns nil when the value is nil (consistent with GetStringSlice)
- Filters empty strings after trimming
- Preallocates capacity for efficiency
This addresses the need for comma-separated configuration parsing with proper whitespace handling.
| WithRaw[*v1beta1.Gateway](func(ctx Context, builder *builder.Builder) error { | ||
|
|
||
| // manager provided client is not ready at this stage | ||
| cfg := ctrl.GetConfigOrDie() | ||
|
|
||
| client, err := client.New(cfg, client.Options{ | ||
| Scheme: ctx.GetScheme(), | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| crds := apiextensionsv1.CustomResourceDefinitionList{} | ||
| err = client.List(ctx, &crds) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| v := &externaldnsv1alpha1.DNSEndpoint{} | ||
| kinds, _, err := ctx.GetScheme().ObjectKinds(v) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, item := range crds.Items { | ||
| if item.GroupVersionKind() == kinds[0] { | ||
| isDNSEndpointAvailable = true | ||
| builder.Owns(v) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| }), |
There was a problem hiding this comment.
Critical: CRD detection logic compares wrong GVK.
Line 144 compares item.GroupVersionKind() (the GVK of the CRD object itself: apiextensions.k8s.io/v1/CustomResourceDefinition) with kinds[0] (the GVK of DNSEndpoint: externaldns.k8s.io/v1alpha1/DNSEndpoint). These will never match, so isDNSEndpointAvailable will always remain false and DNS endpoint reconciliation will never run.
To properly detect the DNSEndpoint CRD, check the CRD's spec fields:
for _, item := range crds.Items {
- if item.GroupVersionKind() == kinds[0] {
+ if item.Spec.Group == kinds[0].Group && item.Spec.Names.Kind == kinds[0].Kind {
isDNSEndpointAvailable = true
builder.Owns(v)
break
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WithRaw[*v1beta1.Gateway](func(ctx Context, builder *builder.Builder) error { | |
| // manager provided client is not ready at this stage | |
| cfg := ctrl.GetConfigOrDie() | |
| client, err := client.New(cfg, client.Options{ | |
| Scheme: ctx.GetScheme(), | |
| }) | |
| if err != nil { | |
| panic(err) | |
| } | |
| crds := apiextensionsv1.CustomResourceDefinitionList{} | |
| err = client.List(ctx, &crds) | |
| if err != nil { | |
| return err | |
| } | |
| v := &externaldnsv1alpha1.DNSEndpoint{} | |
| kinds, _, err := ctx.GetScheme().ObjectKinds(v) | |
| if err != nil { | |
| return err | |
| } | |
| for _, item := range crds.Items { | |
| if item.GroupVersionKind() == kinds[0] { | |
| isDNSEndpointAvailable = true | |
| builder.Owns(v) | |
| break | |
| } | |
| } | |
| return nil | |
| }), | |
| WithRaw[*v1beta1.Gateway](func(ctx Context, builder *builder.Builder) error { | |
| // manager provided client is not ready at this stage | |
| cfg := ctrl.GetConfigOrDie() | |
| client, err := client.New(cfg, client.Options{ | |
| Scheme: ctx.GetScheme(), | |
| }) | |
| if err != nil { | |
| panic(err) | |
| } | |
| crds := apiextensionsv1.CustomResourceDefinitionList{} | |
| err = client.List(ctx, &crds) | |
| if err != nil { | |
| return err | |
| } | |
| v := &externaldnsv1alpha1.DNSEndpoint{} | |
| kinds, _, err := ctx.GetScheme().ObjectKinds(v) | |
| if err != nil { | |
| return err | |
| } | |
| for _, item := range crds.Items { | |
| if item.Spec.Group == kinds[0].Group && item.Spec.Names.Kind == kinds[0].Kind { | |
| isDNSEndpointAvailable = true | |
| builder.Owns(v) | |
| break | |
| } | |
| } | |
| return nil | |
| }), |
🤖 Prompt for AI Agents
internal/resources/gateways/init.go around lines 119-152: the current logic
wrongly compares the CRD object's GVK to the DNSEndpoint GVK (they will never
match); instead inspect each CRD's spec (e.g. item.Spec.Group,
item.Spec.Names.Kind and item.Spec.Versions) to detect the externaldns.k8s.io
DNSEndpoint CRD (match group == "externaldns.k8s.io" and kind == "DNSEndpoint"
and/or a version "v1alpha1"); when a match is found set isDNSEndpointAvailable =
true and call builder.Owns(&externaldnsv1alpha1.DNSEndpoint{}) then break;
ensure you handle missing Spec defensively and compare strings rather than GVKs.
Signed-off-by: Sylvain Rabot <sylvain@formance.com>
sylr
force-pushed
the
feat/gateway-dnsendpoints
branch
from
191968b to
c4e74e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/09-Configuration reference/01-Settings.md (1)
279-407: Minor inconsistency: public DNS endpoint example doesn't demonstrate annotations setting.While the table documents
gateway.dns.public.annotationswith an empty example (line 74), the public DNS endpoint YAML example (lines 345-385) doesn't include a corresponding Settings block for annotations, unlike the private example. Consider adding a similar annotations example for the public configuration to maintain documentation consistency and completeness.Optional enhancement—add public DNS annotations example (similar to lines 330-338):
--- apiVersion: formance.com/v1beta1 kind: Settings metadata: name: formance-dev-dns-public-annotations spec: key: gateway.dns.public.annotations stacks: - 'formance-dev' value: "your-annotation-key=your-annotation-value"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
docs/09-Configuration reference/01-Settings.md(2 hunks)
🔇 Additional comments (2)
docs/09-Configuration reference/01-Settings.md (2)
63-74: Well-documented new DNS endpoint settings with clear examples and practical guidance.The table entries comprehensively document all six DNS settings for both private and public endpoints. The examples are practical and realistic, and the detailed explanation section clearly covers placeholders, multiple endpoints, provider-specific configurations, and annotations. The API version reference and external-dns operator requirement callouts are appropriately placed and helpful.
281-281: Verify external-dns DNSEndpoint CRD API version alignment with implementation.The documentation references
externaldns.k8s.io/v1alpha1as the DNSEndpoint CRD API version, which aligns with the standard external-dns implementation. Ensure the Go code registering the scheme uses the same API version to avoid runtime mismatches.
- Use core.NewMissingSettingsError for validation errors - Remove duplicate RBAC marker from stacks/init.go - Add logging to DNS endpoint create/delete operations - Simplify deleteDNSEndpoint function - Fix typo in CRD test file (DNSEnpoint -> DNSEndpoint)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/resources/gateways/dnsendpoints.go (2)
101-110: Consider sorting provider-specific properties for deterministic ordering.Go map iteration order is non-deterministic. This could cause the
ProviderSpecificslice to have different ordering on each reconciliation, potentially triggering unnecessary updates even when the values haven't changed.♻️ Suggested fix
+import ( + "slices" + ... +) // Add provider-specific settings if present if len(config.ProviderSpec) > 0 { providerSpecific := endpoint.ProviderSpecific{} - for name, value := range config.ProviderSpec { + keys := make([]string, 0, len(config.ProviderSpec)) + for name := range config.ProviderSpec { + keys = append(keys, name) + } + slices.Sort(keys) + for _, name := range keys { providerSpecific = append(providerSpecific, endpoint.ProviderSpecificProperty{ Name: name, - Value: value, + Value: config.ProviderSpec[name], }) } ep.ProviderSpecific = providerSpecific }
140-177: LGTM!Clean reconciliation logic that properly handles both creation and cleanup of DNS endpoints for private and public configurations.
Optional: The private/public handling is repetitive. Consider extracting to a helper if more DNS types are added in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
internal/tests/crds/externaldns.k8s.io_DNSEndpoint.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
internal/resources/gateways/dnsendpoints.go
🔇 Additional comments (5)
internal/resources/gateways/dnsendpoints.go (5)
19-26: LGTM!Clean configuration struct with appropriate fields for DNS endpoint configuration.
28-77: LGTM!Good validation logic for required fields. The
nil, nilreturn when disabled is a clean pattern to signal "no configuration needed."
79-81: LGTM!Simple and effective pattern expansion.
115-138: LGTM!Good use of
core.CreateOrUpdatefor idempotent operations and proper owner reference setup for garbage collection.
179-188: LGTM!Correct idempotent deletion pattern using
client.IgnoreNotFound.
No description provided.