Add DNS record creation support for servers#546
Conversation
…figMap - Introduced DNSRecordTemplateConfigMap field in BMCReconciler to specify the ConfigMap for DNS record templates. - Implemented logic to create DNS records for servers during discovery if the ConfigMap is configured. - Added a new ConfigMap for DNS record templates with example structure. - Enhanced unit tests to verify DNS record creation functionality.
stefanhipfel
changed the title
github-actions
bot
added
api-change
documentation
|
Btw. it would be also good if we update the documentation of the |
afritzler
changed the title
- Introduced LoadTemplate function to load DNS record templates from files. - Updated BMCReconciler to use DNSRecordTemplate instead of DNSRecordTemplatePath. - Enhanced documentation to include usage of DNS record templates. - Added tests for DNS record template loading.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
actor Operator
participant Manager
participant DNSLoader as dns.LoadTemplate
participant Reconciler
participant K8sAPI as Kubernetes API
Operator->>Manager: start with --dns-record-template-path=path
Manager->>DNSLoader: LoadTemplate(path)
DNSLoader-->>Manager: template string / error
Manager->>Reconciler: initialize with DNSRecordTemplate
Reconciler->>K8sAPI: watch / reconcile BMC resources
Note right of Reconciler: On BMC reconcile\n(render & apply per BMC)
Reconciler->>Reconciler: render template (DNSRecordTemplateData)
Reconciler->>K8sAPI: CreateOrPatch DNSRecord (unstructured, set ownerRef)
K8sAPI-->>Reconciler: created/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (15)📓 Common learnings📚 Learning: 2026-01-15T14:28:36.694ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-09T15:31:43.862ZApplied to files:
📚 Learning: 2026-01-14T19:27:09.972ZApplied to files:
🧬 Code graph analysis (1)internal/controller/bmc_controller.go (2)
⏰ 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). (3)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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: 6
🤖 Fix all issues with AI agents
In `@docs/concepts/bmcs.md`:
- Line 81: Fix the typo in the "Create DNS Record" bullet: remove the duplicated
word "the" in the phrase "via the the managers `dns-record-template-path` flag"
so it reads "via the managers `dns-record-template-path` flag" (reference the
"Create DNS Record" bullet and the `dns-record-template-path` flag).
- Around line 99-106: The docs incorrectly state that "Spec" and "Status" are
injected as top-level template fields; in reality DNSRecordTemplateData embeds
metalv1alpha1.BMCSpec and metalv1alpha1.BMCStatus so their fields (e.g.,
Hostname, IP) are available at the top level in Go templates. Update the docs
section describing the injected template data to either list the actual injected
fields (for example Hostname from BMCSpec and IP from BMCStatus) or explicitly
explain that DNSRecordTemplateData embeds metalv1alpha1.BMCSpec and
metalv1alpha1.BMCStatus so template authors can reference embedded fields
directly (e.g., {{ .Hostname }} and {{ .IP }}) without using {{ .Spec.* }} or {{
.Status.* }}.
In `@internal/controller/bmc_controller_test.go`:
- Around line 327-336: The test deletes a Server before ensuring the reconciler
created it and also skips waiting for deletions; modify the cleanup to first
wait for the server to exist using the same pattern as other tests (e.g.,
Eventually(Object(server)) or Eventually(Get(...)).Should(Succeed())) to ensure
creation has occurred, then delete bmc, bmcSecret, server, and dnsRecord, and
after each Delete use Eventually(Get(...)).Should(Satisfy(apierrors.IsNotFound))
(or equivalent Eventually checks) to wait for actual removal; update references
to server, bmc, bmcSecret, and dnsRecord and reuse the existing
Eventually(Object(...)) / Get(...) helpers used elsewhere in this file.
In `@internal/controller/bmc_controller.go`:
- Around line 325-336: The CreateOrPatch mutation passed to
controllerutil.CreateOrPatch (in the block that operates on dnsRecord) never
sets any fields and thus never applies updates; change the anonymous mutation
function to copy the desired state into the existing object (e.g., set
dnsRecord.Spec, dnsRecord.Labels and any other mutable fields from the
desired/template DNS record) so updates are applied on patch, or if records are
intended to be immutable replace the CreateOrPatch call with a Create and handle
already-exists errors (IgnoreAlreadyExists) for clarity.
- Line 338: The log message uses the wrong identifier: replace the use of
server.Name in the log.Info call that reports DNS record creation/patch (the
line logging "Created or patched DNS record", "Record", server.Name,
"Operation", opResult) with the DNS record name sourced from bmcObj.Name (the
variable used when creating the DNS record earlier). Update the log invocation
to reference bmcObj.Name so the logged "Record" value matches the actual DNS
record created/patched.
In `@test/data/dns_record_template.yaml`:
- Around line 1-16: The test template currently renders a ConfigMap which breaks
createDNSRecordForServer() expecting a DNSRecord; update the YAML to use
apiVersion: dns.ironcore.dev/v1alpha1 and kind: DNSRecord and move the resource
payload from the data section into the DNSRecord spec (e.g., spec.hostname,
spec.ip, spec.recordType, spec.record, spec.zoneName, spec.ttl) while keeping
metadata (name, namespace, labels) the same so the rendered template unmarshals
as a DNSRecord for createDNSRecordForServer().
🧹 Nitpick comments (6)
api/v1alpha1/bmc_types.go (1)
64-67: LGTM - Hostname field follows Kubernetes API conventions.The new optional
Hostnamefield correctly uses:
- PascalCase for the Go struct field
- camelCase for the JSON serialization
- Pointer type for optional semantics
- Proper kubebuilder markers
Consider adding hostname format validation (e.g., RFC 1123 subdomain) via kubebuilder markers if strict DNS-compliant hostnames are required:
// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`test/data/dns_record_template.yaml (1)
6-9: Quote label values to prevent YAML parsing errors.When labels contain special characters (e.g., colons, quotes, or values starting with
@,*), the rendered YAML may be invalid. The static analysis error on line 8 is expected for template files, but the underlying concern is valid for rendered output.♻️ Suggested fix
labels: {{- range $key, $value := .Labels }} - {{ $key }}: {{ $value }} + "{{ $key }}": "{{ $value }}" {{- end }}config/manager/dns-record-template.yaml (1)
39-42: Quote label keys and values in the production template.Same concern as the test template—label keys or values with special characters could produce invalid YAML when rendered.
♻️ Suggested fix
labels: {{- range $key, $value := .Labels }} - {{ $key }}: {{ $value }} + "{{ $key }}": "{{ $value }}" {{- end }}internal/controller/bmc_controller.go (3)
70-72: Fix misleading comment.The comment states "path to the file" but the field actually holds the template content (loaded at startup), not the file path.
📝 Suggested fix
- // DNSRecordTemplatePath is the path to the file containing the DNSRecord template. + // DNSRecordTemplate holds the DNS record template content for creating DNS records. DNSRecordTemplate string
268-275: Remove unnecessaryIsNotFoundcheck.
createDNSRecordForServerusesCreateOrPatch, which creates the resource if it doesn't exist. It won't returnNotFounderrors, making this check dead code that could mask future issues if the function's behavior changes.♻️ Suggested fix
// Create DNS record for the server if template path is configured if r.ManagerNamespace != "" && r.DNSRecordTemplate != "" { - if err := r.createDNSRecordForServer(ctx, log, bmcObj, server); err != nil && !apierrors.IsNotFound(err) { + if err := r.createDNSRecordForServer(ctx, log, bmcObj, server); err != nil { log.Error(err, "failed to create DNS record for server", "Server", server.Name) errs = append(errs, fmt.Errorf("failed to create DNS record for server %s: %w", server.Name, err)) } }
303-307: Parse template once at startup instead of on every reconciliation.Parsing the template on every call to
createDNSRecordForServeris inefficient. Since the template content doesn't change after startup, parse it once and store the parsed*template.Templatein the reconciler.♻️ Suggested approach
- Change
DNSRecordTemplate stringtoDNSRecordTemplate *template.TemplateinBMCReconciler- Parse the template in
main.goafter loading:if dnsRecordTemplatePath != "" { templateContent, err := dns.LoadTemplate(dnsRecordTemplatePath) if err != nil { setupLog.Error(err, "unable to load DNS record template") os.Exit(1) } dnsRecordTemplate, err = template.New("dnsRecord").Parse(templateContent) if err != nil { setupLog.Error(err, "unable to parse DNS record template") os.Exit(1) } }
- Use the pre-parsed template directly in
createDNSRecordForServerThis also provides early validation of the template syntax at startup.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/chart/templates/crd/metal.ironcore.dev_bmcs.yamlis excluded by!**/dist/**
📒 Files selected for processing (13)
REUSE.tomlapi/v1alpha1/bmc_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/manager/main.goconfig/crd/bases/metal.ironcore.dev_bmcs.yamlconfig/manager/dns-record-template.yamldocs/api-reference/api.mddocs/concepts/bmcs.mdinternal/cmd/dns/dns.gointernal/controller/bmc_controller.gointernal/controller/bmc_controller_test.gointernal/controller/suite_test.gotest/data/dns_record_template.yaml
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/cmd/dns/dns.goapi/v1alpha1/bmc_types.gocmd/manager/main.gointernal/controller/suite_test.goapi/v1alpha1/zz_generated.deepcopy.gointernal/controller/bmc_controller_test.gointernal/controller/bmc_controller.go
**/api/**/*_types.go
📄 CodeRabbit inference engine (AGENTS.md)
**/api/**/*_types.go: API type fields must follow Kubernetes naming conventions: camelCase in JSON, PascalCase in Go structs
Usemetav1.Time,metav1.Condition, andresource.Quantityfor standard Kubernetes types in API definitions
Spec fields must be declarative and user-facing; Status fields must be controller-owned
Conditions in Status must follow Kubernetes conventions with stable Type, correct Status, Reason, and Message fields
No mutable fields in Spec that belong in Status; maintain clear separation between desired state and observed state
Do not make breaking changes to existing API fields without versioning; ensure additive-only changes for existing API versions
Clearly mark and document deprecated fields in API types
Files:
api/v1alpha1/bmc_types.go
**/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement defaulting and validation via webhooks (preferred) or OpenAPI schema annotations
Files:
api/v1alpha1/bmc_types.goapi/v1alpha1/zz_generated.deepcopy.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Tests must be deterministic and not rely on timing assumptions
Files:
internal/controller/suite_test.gointernal/controller/bmc_controller_test.go
🧠 Learnings (16)
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
cmd/manager/main.gointernal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Use `metav1.Time`, `metav1.Condition`, and `resource.Quantity` for standard Kubernetes types in API definitions
Applied to files:
cmd/manager/main.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Write unit tests for reconcile logic and pure functions
Applied to files:
internal/controller/suite_test.gointernal/controller/bmc_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Use Envtest-based tests for controller behavior and API interactions
Applied to files:
internal/controller/suite_test.gointernal/controller/bmc_controller_test.go
📚 Learning: 2026-01-14T19:27:09.972Z
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:09.972Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Applied to files:
docs/api-reference/api.mdinternal/controller/bmc_controller_test.gointernal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_test.go : Use fake clients appropriately in tests
Applied to files:
internal/controller/bmc_controller_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Use structured logging with controller-runtime logger; avoid excessive log noise in hot paths
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Follow Kubernetes API and controller patterns rather than inventing new abstractions
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use retries via requeue or error return in controllers, not explicit loops
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Avoid silent failures; ensure logging includes sufficient context (namespaced name, resource identifiers)
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Errors must be propagated correctly to trigger retries; distinguish between transient and permanent errors where possible
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status must reflect observed state, not desired state; conditions must be updated consistently and deterministically
Applied to files:
internal/controller/bmc_controller.go
🧬 Code graph analysis (4)
cmd/manager/main.go (1)
internal/cmd/dns/dns.go (1)
LoadTemplate(12-23)
internal/controller/suite_test.go (2)
internal/cmd/dns/dns.go (1)
LoadTemplate(12-23)internal/controller/bmc_controller.go (1)
BMCReconciler(58-73)
internal/controller/bmc_controller_test.go (3)
internal/controller/suite_test.go (1)
SetupTest(105-332)api/v1alpha1/bmcsecret_types.go (2)
BMCSecret(24-57)BMCSecretUsernameKeyName(13-13)internal/bmcutils/bmcutils.go (1)
GetServerNameFromBMCandIndex(219-221)
internal/controller/bmc_controller.go (1)
api/v1alpha1/bmc_types.go (3)
BMCSpec(25-68)BMCStatus(169-222)BMC(252-258)
🪛 YAMLlint (1.37.1)
test/data/dns_record_template.yaml
[error] 8-8: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (16)
api/v1alpha1/zz_generated.deepcopy.go (1)
798-802: Auto-generated deepcopy is correct.The generated deepcopy for
Hostname *stringfollows the standard controller-gen pattern for pointer fields and is consistent with other pointer field handling in this file.docs/api-reference/api.md (1)
550-550: Documentation accurately reflects the API.The hostname field documentation correctly shows the field type and description, consistent with the
BMCSpecdefinition inbmc_types.go.REUSE.toml (1)
26-27: LGTM - REUSE annotations extended for YAML test data.Adding
test/data/*.yamlto the annotations path ensures the new DNS record template test fixtures are properly covered by SPDX license annotations, consistent with the existing JSON test data coverage.config/crd/bases/metal.ironcore.dev_bmcs.yaml (1)
167-169: CRD schema correctly reflects the API type definition.The
hostnamefield is properly defined as an optional string field, consistent with theBMCSpec.Hostnamefield in the Go API types.internal/cmd/dns/dns.go (1)
11-22: LGTM!The function is clean, focused, and follows Go idioms with proper error handling and descriptive error messages.
config/manager/dns-record-template.yaml (1)
43-47: LGTM!The spec section is well-structured with sensible defaults for
recordTypeandttl. The template variables forhostnameandipare appropriately used.internal/controller/bmc_controller.go (2)
283-290: LGTM!The struct design with embedded
BMCSpecandBMCStatusprovides flexible template access while explicitly controlling exposed fields through theLabelsmap.
320-323: LGTM!Using
controllerutil.SetControllerReferencefollows controller-runtime best practices. Based on learnings, this is the correct approach for owner references.cmd/manager/main.go (3)
151-152: LGTM!The flag definition follows the existing patterns in the codebase and has a clear description.
172-181: LGTM!Loading the template at startup with proper error handling follows the pattern requested in previous reviews. Failing fast on startup if the template cannot be loaded is appropriate.
348-348: LGTM!The loaded template is correctly passed to the
BMCReconciler.internal/controller/bmc_controller_test.go (2)
23-23: LGTM!The change from discarding (
_) to capturing (ns) the return value ofSetupTestis correct, as the namespace is now needed for the DNS record assertion on line 310.
313-326: LGTM!The assertions correctly verify:
- Owner reference pointing to the BMC with controller=true
- DNS record data fields (hostname, ip, recordType, ttl)
The test validates the core DNS record creation functionality as configured via the template. Based on learnings, this follows the Envtest-based testing approach for controller behavior.
internal/controller/suite_test.go (3)
20-20: LGTM!The import for
internal/cmd/dnsis correctly added to support thedns.LoadTemplatefunction usage.
167-169: LGTM!The DNS template loading follows the established test setup patterns:
- Relative path correctly resolves from
internal/controller/totest/data/- Error handling is consistent with other setup assertions
- Template is loaded in
BeforeEach, ensuring each test spec gets a fresh manager with the template configured
170-179: Consider: DNS template is now always enabled for all tests.The
DNSRecordTemplateis configured for all tests usingSetupTest. This means every test in the suite will have DNS record creation enabled, which could affect tests that aren't explicitly testing DNS functionality.However, looking at the test file, the DNS record is only created when
bmc.Spec.Hostnameis set (based on the new test case on line 301), so existing tests without a hostname should remain unaffected. This is acceptable.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…rity and functionality
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/bmc_controller.go`:
- Around line 293-301: The function createDNSRecordForServer has an unused
server parameter; remove the parameter and rename the function to
createDNSRecordForBMC (or keep name but drop the unused arg) and update all call
sites to match the new signature; specifically change the method signature on
BMCReconciler (remove the server *metalv1alpha1.Server param), update any
references to createDNSRecordForServer to the new name, and ensure you delete
any unused local/server references inside the function body or replace them with
server-specific template fields only if you intend to actually generate
per-server DNS records.
♻️ Duplicate comments (2)
internal/controller/bmc_controller.go (1)
320-326: CreateOrPatch mutation doesn't preserve template-rendered data on updates.When the DNS record already exists,
CreateOrPatchfetches and overwritesdnsRecordwith the existing object's data before running the mutation function. Since the mutation only sets the controller reference, the template-rendered spec/data is lost on subsequent reconciliations.If DNS records should be updatable, store the desired state before
CreateOrPatchand restore it in the mutation. If they should be immutable after creation, useCreatewithIgnoreAlreadyExistsfor clarity.🔧 Fix for updatable DNS records
+ // Store desired state before CreateOrPatch overwrites dnsRecord + desiredObject := dnsRecord.DeepCopy() + // Create or patch the DNS record opResult, err := controllerutil.CreateOrPatch(ctx, r.Client, dnsRecord, func() error { + // Restore template-rendered data + for key, value := range desiredObject.Object { + if key != "metadata" { + dnsRecord.Object[key] = value + } + } return controllerutil.SetControllerReference(bmcObj, dnsRecord, r.Scheme) })docs/concepts/bmcs.md (1)
81-81: Grammar: missing possessive apostrophe."the managers" should be "the manager's" (possessive form).
-- **Create DNS Record**: Automatically create DNS record using a provided Go YAML template via the managers `dns-record-template-path` flag. +- **Create DNS Record**: Automatically create DNS record using a provided Go YAML template via the manager's `dns-record-template-path` flag.
🧹 Nitpick comments (3)
internal/controller/bmc_controller.go (2)
70-72: Comment doesn't match field name.The comment says "DNSRecordTemplatePath" but the field is named
DNSRecordTemplate. Since this holds the template content (not a path), the comment should be updated for accuracy.- // DNSRecordTemplatePath is the path to the file containing the DNSRecord template. + // DNSRecordTemplate is the template content for creating DNSRecord resources. DNSRecordTemplate string
303-312: Consider parsing template once at startup instead of per-reconciliation.The template is parsed on every call to
createDNSRecordForServer. Since the template content doesn't change during runtime, parsing once at startup (or lazily on first use with caching) would be more efficient.♻️ Suggested approach
Store the parsed template in the reconciler struct:
type BMCReconciler struct { // ... existing fields DNSRecordTemplate string parsedDNSRecordTemplate *template.Template // Add cached parsed template }Parse once during setup or lazily:
func (r *BMCReconciler) getParsedDNSTemplate() (*template.Template, error) { if r.parsedDNSRecordTemplate == nil { tmpl, err := template.New("dnsRecord").Parse(r.DNSRecordTemplate) if err != nil { return nil, err } r.parsedDNSRecordTemplate = tmpl } return r.parsedDNSRecordTemplate, nil }Based on learnings, template loading was suggested to move to startup in
main.go.docs/concepts/bmcs.md (1)
99-106: Clarify that Spec and Status fields are directly accessible (not nested).The documentation says "Spec" and "Status" are injected, but since these are embedded structs in
DNSRecordTemplateData, their fields are accessible at the top level (e.g.,{{ .Hostname }}not{{ .Spec.Hostname }}). This aligns with the template example on lines 66-67, but the bullet list could be clearer.📝 Suggested clarification
6. **Create BMC DNS Record**: If a DNS record template path is provided via the managers `dns-record-template-path` flag, the controller will create an instance using this template. The following BMC data is injected to the Go YAML template: - Namespace - Name -- Spec (the full `Spec` object is injected into the template context, allowing direct access to its properties) -- Status (the full `Status` object is injected into the template context, allowing direct access to its properties) +- All fields from BMCSpec (e.g., `Hostname`, `Protocol`) - accessible directly as `{{ .Hostname }}` +- All fields from BMCStatus (e.g., `IP`, `MACAddress`) - accessible directly as `{{ .IP }}` - Labels
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/concepts/bmcs.mdinternal/controller/bmc_controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/controller/bmc_controller.go
🧠 Learnings (18)
📓 Common learnings
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
📚 Learning: 2026-01-15T14:28:36.694Z
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
Applied to files:
docs/concepts/bmcs.mdinternal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : No mutable fields in Spec that belong in Status; maintain clear separation between desired state and observed state
Applied to files:
docs/concepts/bmcs.md
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Spec fields must be declarative and user-facing; Status fields must be controller-owned
Applied to files:
docs/concepts/bmcs.md
📚 Learning: 2026-01-14T19:27:09.972Z
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:09.972Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Applied to files:
docs/concepts/bmcs.mdinternal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/api/**/*_types.go : Conditions in Status must follow Kubernetes conventions with stable Type, correct Status, Reason, and Message fields
Applied to files:
docs/concepts/bmcs.md
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status updates must be performed via `Status().Update()` or `Status().Patch()` and separated from spec mutations
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Avoid silent failures; ensure logging includes sufficient context (namespaced name, resource identifiers)
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Use structured logging with controller-runtime logger; avoid excessive log noise in hot paths
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Follow Kubernetes API and controller patterns rather than inventing new abstractions
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use retries via requeue or error return in controllers, not explicit loops
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Errors must be propagated correctly to trigger retries; distinguish between transient and permanent errors where possible
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status must reflect observed state, not desired state; conditions must be updated consistently and deterministically
Applied to files:
internal/controller/bmc_controller.go
🧬 Code graph analysis (1)
internal/controller/bmc_controller.go (3)
api/v1alpha1/bmc_types.go (3)
BMCSpec(25-68)BMCStatus(169-222)BMC(252-258)bmc/bmc.go (2)
BMC(36-123)Server(182-189)internal/api/registry/server.go (1)
Server(17-26)
🔇 Additional comments (4)
internal/controller/bmc_controller.go (2)
268-275: LGTM on conditional invocation.The guards for
ManagerNamespaceandDNSRecordTemplateappropriately make this an opt-in feature. Error handling correctly logs and accumulates errors without blocking other servers.
283-290: LGTM on template data structure.The embedded
BMCSpecandBMCStatusstructs allow template authors to reference their fields directly (e.g.,{{ .Hostname }},{{ .IP }}), which aligns with the documented template examples.docs/concepts/bmcs.md (2)
29-29: LGTM on hostname field example.The example correctly demonstrates the new
hostnamefield in the BMC spec.
53-72: LGTM on template example.The template example correctly demonstrates direct field access (
{{ .Hostname }},{{ .IP }}) which works becauseBMCSpecandBMCStatusare embedded in the template data structure.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/controller/bmc_controller.go`:
- Around line 70-72: The struct comment for DNSRecordTemplate is incorrect: it
says "path to the file" but the field DNSRecordTemplate actually holds the
template content (used by createDNSRecordForServer). Update the comment to
reflect that DNSRecordTemplate contains the DNSRecord template content (not a
file path) and ensure any related docs or other comments in the same struct
align with that semantic.
- Around line 268-275: The DNS creation block inside the server iteration is
incorrect because createDNSRecordForServer only uses bmcObj and not per-server
data; move the entire if r.ManagerNamespace != "" && r.DNSRecordTemplate != "" {
... } block (including the call to r.createDNSRecordForServer and its error
handling) out of the per-server loop so it runs once per BMC reconciliation
(using bmcObj.Name in logs/errors rather than server.Name), and keep existing
checks for apierrors.IsNotFound when handling the returned error.
♻️ Duplicate comments (1)
internal/controller/bmc_controller.go (1)
292-293: Function name is misleading after refactor.The function name
createDNSRecordForServerimplies server-specific DNS records, but the server parameter was removed and the function creates records based solely on BMC data. Rename for clarity.📝 Suggested rename
-// createDNSRecordForServer creates a DNS record resource from a YAML template loaded from ConfigMap -func (r *BMCReconciler) createDNSRecordForServer(ctx context.Context, log logr.Logger, bmcObj *metalv1alpha1.BMC) error { +// createDNSRecordForBMC creates a DNS record resource from a YAML template +func (r *BMCReconciler) createDNSRecordForBMC(ctx context.Context, log logr.Logger, bmcObj *metalv1alpha1.BMC) error {
🧹 Nitpick comments (1)
internal/controller/bmc_controller.go (1)
303-307: Consider caching the parsed template.The template is parsed on every reconciliation call. Since
r.DNSRecordTemplatedoesn't change during runtime, parsing once at startup (or lazily on first use with sync.Once) would be more efficient.♻️ Example: Parse template once
Add a field to BMCReconciler:
type BMCReconciler struct { // ... DNSRecordTemplate string dnsRecordTemplateParsed *template.Template templateOnce sync.Once }Then in the method:
r.templateOnce.Do(func() { r.dnsRecordTemplateParsed, _ = template.New("dnsRecord").Parse(r.DNSRecordTemplate) }) if r.dnsRecordTemplateParsed == nil { return fmt.Errorf("DNS record template not parsed") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/bmc_controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/controller/bmc_controller.go
🧠 Learnings (15)
📓 Common learnings
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
📚 Learning: 2026-01-15T14:28:36.694Z
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status updates must be performed via `Status().Update()` or `Status().Patch()` and separated from spec mutations
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Avoid silent failures; ensure logging includes sufficient context (namespaced name, resource identifiers)
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Use structured logging with controller-runtime logger; avoid excessive log noise in hot paths
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Follow Kubernetes API and controller patterns rather than inventing new abstractions
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use retries via requeue or error return in controllers, not explicit loops
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Errors must be propagated correctly to trigger retries; distinguish between transient and permanent errors where possible
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status must reflect observed state, not desired state; conditions must be updated consistently and deterministically
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-14T19:27:09.972Z
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:09.972Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Applied to files:
internal/controller/bmc_controller.go
🧬 Code graph analysis (1)
internal/controller/bmc_controller.go (1)
api/v1alpha1/bmc_types.go (3)
BMCSpec(25-68)BMCStatus(169-222)BMC(252-258)
⏰ 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). (4)
- GitHub Check: Run linter
- GitHub Check: Run test chart
- GitHub Check: Run tests
- GitHub Check: Run test e2e
🔇 Additional comments (2)
internal/controller/bmc_controller.go (2)
7-18: LGTM on imports.The new imports are appropriate for the DNS record templating feature:
bytesfor buffered rendering,text/templatefor YAML templating, andunstructured/yamlfor dynamic Kubernetes resource handling.
320-326: CreateOrPatch mutation now correctly sets owner reference.The mutation function properly sets the controller reference, ensuring garbage collection works when the BMC is deleted. This addresses the previous review feedback.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| opResult, err := controllerutil.CreateOrPatch(ctx, r.Client, dnsRecord, func() error { | ||
| return controllerutil.SetControllerReference(bmcObj, dnsRecord, r.Scheme) | ||
| }) |
There was a problem hiding this comment.
I think this doesn't work for patching an existing record, due to CreateOrPatch() comparing the return value of the mutateFn with object passed, which produces no diff, if the owner isn't changed.
There was a problem hiding this comment.
ok I think SSA should be the easier way to solve it
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/bmc_controller.go`:
- Around line 282-334: The template rendering currently silently substitutes
missing fields with "<no value>"; update the template creation in
createDNSRecord by calling template.New("dnsRecord").Option("missingkey=error")
before Parse (so the template will error on missing keys), handle and propagate
that parse error (same fmt.Errorf pattern) and ensure the Execute error path
already present will now surface missing-key issues for the DNSRecord template.
♻️ Duplicate comments (1)
internal/controller/bmc_controller.go (1)
70-72: Fix DNSRecordTemplate comment to match actual semantics.The field stores template content, not a file path (Line 70). This doc mismatch is confusing.
✏️ Proposed fix
- // DNSRecordTemplatePath is the path to the file containing the DNSRecord template. + // DNSRecordTemplate is the DNS record YAML template content. DNSRecordTemplate string
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/bmc_controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/controller/bmc_controller.go
🧠 Learnings (15)
📓 Common learnings
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
📚 Learning: 2026-01-15T14:28:36.694Z
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status updates must be performed via `Status().Update()` or `Status().Patch()` and separated from spec mutations
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Avoid silent failures; ensure logging includes sufficient context (namespaced name, resource identifiers)
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Use structured logging with controller-runtime logger; avoid excessive log noise in hot paths
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Follow Kubernetes API and controller patterns rather than inventing new abstractions
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use retries via requeue or error return in controllers, not explicit loops
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Errors must be propagated correctly to trigger retries; distinguish between transient and permanent errors where possible
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status must reflect observed state, not desired state; conditions must be updated consistently and deterministically
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-14T19:27:09.972Z
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:09.972Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Applied to files:
internal/controller/bmc_controller.go
🧬 Code graph analysis (1)
internal/controller/bmc_controller.go (2)
api/v1alpha1/bmc_types.go (3)
BMCSpec(25-68)BMCStatus(169-222)BMC(252-258)bmc/bmc.go (1)
BMC(36-123)
⏰ 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). (4)
- GitHub Check: Run tests
- GitHub Check: Run test chart
- GitHub Check: Run test e2e
- GitHub Check: Run linter
🔇 Additional comments (2)
internal/controller/bmc_controller.go (2)
6-19: No review comment for this hunk.
176-181: LGTM: DNS record creation is correctly gated by configuration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/bmc_controller.go`:
- Around line 176-181: Update the comment and simplify error handling around DNS
record creation: change the comment above the block referencing
r.ManagerNamespace and r.DNSRecordTemplate to say that a DNS record will be
created when the template is configured (not "template path"), and remove the
unnecessary apierrors.IsNotFound(err) check from the if that calls
r.createDNSRecord(ctx, log, bmcObj) so any error returned is propagated (leave
the existing fmt.Errorf wrapping intact); refer to r.ManagerNamespace,
r.DNSRecordTemplate, and createDNSRecord to locate the code to edit.
♻️ Duplicate comments (1)
internal/controller/bmc_controller.go (1)
70-72: Comment doesn't match field semantics.The comment says "path to the file" but the field
DNSRecordTemplatestores the template content directly (used at line 302 inParse(r.DNSRecordTemplate)), not a file path.📝 Suggested fix
- // DNSRecordTemplatePath is the path to the file containing the DNSRecord template. + // DNSRecordTemplate is the DNS record YAML template content. DNSRecordTemplate string
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/bmc_controller.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified
Files:
internal/controller/bmc_controller.go
🧠 Learnings (15)
📓 Common learnings
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
📚 Learning: 2026-01-15T14:28:36.694Z
Learnt from: afritzler
Repo: ironcore-dev/metal-operator PR: 546
File: test/data/dns_record_template.yaml:1-16
Timestamp: 2026-01-15T14:28:36.694Z
Learning: In unit tests for the metal-operator BMC controller, when testing DNS record creation functionality without real DNS controller CRDs installed, a ConfigMap is used as a fallback/mock object in the test template (test/data/dns_record_template.yaml) instead of an actual DNSRecord resource.
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Flag non-idempotent reconcile logic, Spec mutations during reconciliation, and custom patterns duplicating standard Kubernetes behavior
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status updates must be performed via `Status().Update()` or `Status().Patch()` and separated from spec mutations
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Avoid silent failures; ensure logging includes sufficient context (namespaced name, resource identifiers)
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use `client.Client`, `controllerutil.CreateOrUpdate`, and OwnerReferences from controller-runtime rather than direct API calls
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Use structured logging with controller-runtime logger; avoid excessive log noise in hot paths
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Follow Kubernetes API and controller patterns rather than inventing new abstractions
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Use retries via requeue or error return in controllers, not explicit loops
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*.go : Include relevant identifiers in logs; emit events for meaningful user-facing state changes
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Errors must be propagated correctly to trigger retries; distinguish between transient and permanent errors where possible
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Prevent hot loops caused by status-only changes triggering reconciliation
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Reconciliation must be based on current cluster state, not assumptions; handle NotFound errors correctly and gracefully manage partial failures
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller.go : Status must reflect observed state, not desired state; conditions must be updated consistently and deterministically
Applied to files:
internal/controller/bmc_controller.go
📚 Learning: 2026-01-14T19:27:09.972Z
Learnt from: Nuckal777
Repo: ironcore-dev/metal-operator PR: 471
File: internal/controller/bmcsettingsset_controller.go:233-236
Timestamp: 2026-01-14T19:27:09.972Z
Learning: In the ironcore-dev/metal-operator repository, BMCSettings.Spec.BMCRef is marked with +required kubebuilder marker, ensuring API-level validation that prevents nil values; defensive nil checks are not needed when iterating BMCSettings from List operations.
Applied to files:
internal/controller/bmc_controller.go
🧬 Code graph analysis (1)
internal/controller/bmc_controller.go (1)
api/v1alpha1/bmc_types.go (3)
BMCSpec(25-68)BMCStatus(169-222)BMC(252-258)
⏰ 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). (3)
- GitHub Check: Run test e2e
- GitHub Check: Run test chart
- GitHub Check: Run tests
🔇 Additional comments (3)
internal/controller/bmc_controller.go (3)
7-18: LGTM!The new imports are appropriate for the DNS record templating functionality:
bytesfor buffering template output,text/templatefor parsing,unstructuredfor dynamic Kubernetes objects, andyamlfor unmarshaling.
282-289: LGTM!The
DNSRecordTemplateDatastruct with embeddedBMCSpecandBMCStatusprovides a clean template interface, allowing direct field access (e.g.,{{.IP}},{{.Hostname}}) rather than nested paths. The field names between spec and status don't collide based on the type definitions.
291-336: Implementation is correct;fieldOwneris properly defined and accessible.The SSA-based approach with
client.ApplyandForceOwnershipis the correct pattern for declarative resource management. The implementation includes solid defensive practices:
Option("missingkey=error")for fail-fast on template issues- GVK and name validation before applying
- Proper controller reference for garbage collection
fieldOwnercorrectly scoped in the controller packageThe template is parsed on every reconciliation, which is acceptable for current performance needs.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
4 participants
Proposed Changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.