:sparkles: Accept a Secret or a ConfigMap for BMC CA and Trusted CA by dtantsur · Pull Request #526 · metal3-io/ironic-standalone-operator

dtantsur · 2026-01-30T15:43:46Z

Unfortunately, various CA providers are inconsistent in what they create (cert-manager creates secrets, OpenShift creates config maps). This change deprecates BMCCAName and TrustedCAName in favour of BMCCA and TrustedCA that contain a Kind.

While here, also address the existing quirk of only supporting the first key of a ConfigMap: now a Key can also be provided.

Generated-By: Claude Code (commertical license)
Closes: #431

metal3-io-bot · 2026-01-30T15:43:49Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

metal3-io-bot · 2026-01-30T15:43:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign elfosardo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Unfortunately, various CA providers are inconsistent in what they create (cert-manager creates secrets, OpenShift creates config maps). This change deprecates BMCCAName and TrustedCAName in favour of BMCCA and TrustedCA that contain a Kind. While here, also address the existing quirk of only supporting the first key of a ConfigMap: now a Key can also be provided. Generated-By: Claude Code (commertical license) Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>

dtantsur · 2026-03-02T11:09:05Z

/retest

Copilot

Pull request overview

This PR addresses issue #431 by extending the Ironic operator to support both ConfigMaps and Secrets as sources for the BMC CA and Trusted CA certificates. Previously, only Secrets were supported for BMC CA and only ConfigMaps for Trusted CA (mirroring the behavior of the deprecated BMCCAName/TrustedCAName fields). The new BMCCA and TrustedCA structured fields accept either kind, with an additional Key selector for TrustedCA. The deprecated BMCCAName/TrustedCAName fields are retained for backward compatibility.

Changes:

New ResourceReference and ResourceReferenceWithKey types added to the API, plus new BMCCA and TrustedCA fields on the TLS struct, with backward-compatible deprecation of the old fields
Controller, local-mode parser, and container-building logic updated to fetch and mount either a Secret or ConfigMap for each CA, and to select the correct key from a TrustedCA resource
New validateCASettings validation function and associated golangci suppression for deprecated field references

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`api/v1alpha1/ironic_types.go`	Adds new `BMCCA` and `TrustedCA` fields with deprecation notices on old fields
`api/v1alpha1/common.go`	Defines new `ResourceReference` and `ResourceReferenceWithKey` types
`api/v1alpha1/zz_generated.deepcopy.go`	Auto-generated deep-copy methods for new types
`config/crd/bases/ironic.metal3.io_ironics.yaml`	CRD schema updated with new fields and deprecation notices
`docs/api.md`	API documentation updated for new fields
`pkg/ironic/utils.go`	Adds `GetBMCCA`, `GetTrustedCA`, `volumeForSecretOrConfigMap` helpers; extends `Resources` struct
`pkg/ironic/containers.go`	Updates `buildTrustedCAEnvVars` to handle both Secret and ConfigMap; updates volume-mounting for both CA types
`pkg/ironic/validation.go`	Adds `validateCASettings` to validate new fields and their consistency with deprecated ones
`pkg/ironic/version.go`	Extends `checkVersion` to check for either `BMCCASecret` or `BMCCAConfigMap`
`pkg/ironic/local.go`	Updates YAML parsing to load both Secret and ConfigMap for each CA
`internal/controller/ironic_controller.go`	Updates reconciler to fetch Secret or ConfigMap based on resolved CA reference
`pkg/ironic/containers_test.go`	Adds new tests for `buildTrustedCAEnvVars` with key selection logic
`.golangci.yaml`	Adds staticcheck suppression for deliberate use of deprecated fields

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T11:46:56Z

pkg/ironic/containers.go

+				cctx.Logger.Info("ignoring duplicate key in Trusted CA "+resourceKind,
+					"key", key, resourceKind, namespace+"/"+resourceName)


Same issue as line 229: resourceKind is used as the structured log key, making the field name dynamic ("ConfigMap" or "Secret"). This should be replaced with a stable string literal key (e.g. "resource") to maintain consistent structured log output.

Copilot · 2026-03-02T11:46:56Z

pkg/ironic/containers_test.go

+			// Note: Warning verification would require a custom logger implementation
+			// For now, we've verified the functionality works correctly
+			_ = logOutput
+			_ = tc.expectWarning
+			_ = tc.warningContains


The test declares a logOutput strings.Builder and three test-case fields (expectWarning, warningContains) that are intentionally silenced with blank identifiers on lines 765-767. Since the logger is created as logr.New(logr.Discard().GetSink()), logOutput is never written to, and the warning assertions are never made. This means the warning-logging behavior (the "specified key not found" and "ignoring duplicate key" cases) is effectively untested. Either implement a capturing logger sink to actually verify warnings, or remove the dead logOutput, expectWarning, and warningContains fields from the struct to avoid misleading future readers.

Copilot · 2026-03-02T11:46:56Z

pkg/ironic/validation.go

+func validateCASettings(tls *metal3api.TLS) error {
+	// Validate BMCCA
+	if tls.BMCCA != nil {
+		if tls.BMCCA.Name == "" {
+			return errors.New("tls.bmcCA.name is required when tls.bmcCA is set")
+		}
+		// Both old and new fields are set - validate they're consistent
+		if tls.BMCCAName != "" && (tls.BMCCA.Kind != metal3api.ResourceKindSecret || tls.BMCCA.Name != tls.BMCCAName) {
+			return errors.New("tls.bmcCA and tls.bmcCAName are both set but inconsistent; use tls.bmcCA only")
+		}
+	}
+
+	// Validate TrustedCA
+	if tls.TrustedCA != nil {
+		if tls.TrustedCA.Name == "" {
+			return errors.New("tls.trustedCA.name is required when tls.trustedCA is set")
+		}
+		// Both old and new fields are set - validate they're consistent
+		if tls.TrustedCAName != "" && (tls.TrustedCA.Kind != metal3api.ResourceKindConfigMap || tls.TrustedCA.Name != tls.TrustedCAName) {
+			return errors.New("tls.trustedCA and tls.trustedCAName are both set but inconsistent; use tls.trustedCA only")
+		}
+	}
+
+	return nil
+}


The new validateCASettings function is called by ValidateIronic, but there are no test cases in validation_test.go exercising its logic: empty name, inconsistent BMCCA/BMCCAName, inconsistent TrustedCA/TrustedCAName, or TrustedCA pointing to a Secret while TrustedCAName is also set. Given that the rest of ValidateIronic is thoroughly covered in this file, the absence of any test cases for these new validation paths is a gap.

Copilot · 2026-03-02T11:46:57Z

pkg/ironic/containers.go

+	switch {
+	case resources.TrustedCAConfigMap != nil:
+		keys = make(map[string]struct{}, len(resources.TrustedCAConfigMap.Data))
+		for key := range resources.TrustedCAConfigMap.Data {
+			keys[key] = struct{}{}
+		}
+		resourceName = resources.TrustedCAConfigMap.Name
+		namespace = resources.TrustedCAConfigMap.Namespace
+		resourceKind = metal3api.ResourceKindConfigMap
+	case resources.TrustedCASecret != nil:
+		keys = make(map[string]struct{}, len(resources.TrustedCASecret.Data))
+		for key := range resources.TrustedCASecret.Data {
+			keys[key] = struct{}{}
+		}
+		resourceName = resources.TrustedCASecret.Name
+		namespace = resources.TrustedCASecret.Namespace
+		resourceKind = metal3api.ResourceKindSecret
+	default:
+		return nil
+	}


There is a priority inconsistency between buildTrustedCAEnvVars and volumeForSecretOrConfigMap for the TrustedCA case: buildTrustedCAEnvVars prefers the ConfigMap first (checking resources.TrustedCAConfigMap != nil before resources.TrustedCASecret), whereas volumeForSecretOrConfigMap (called in buildIronicVolumesAndMounts) prioritizes the Secret. If both TrustedCASecret and TrustedCAConfigMap were set simultaneously, the volume would be backed by the Secret while the env var path would point to a key in the ConfigMap — resulting in a misconfigured container. While the controller only ever sets one of them at a time (making this a non-issue in normal operation), the inconsistency is a latent bug that could cause subtle failures in edge cases (e.g., unit tests or the local mode). Aligning the priority in both functions (preferring Secret, or preferring ConfigMap, consistently) would eliminate this hazard.

Copilot · 2026-03-02T11:46:57Z

pkg/ironic/containers_test.go

+					Kind: "ConfigMap",
+				},
+				Key: "custom-ca.crt",
+			},
+			configMapData: map[string]string{
+				"custom-ca.crt": "cert1",
+				"other-ca.crt":  "cert2",
+			},
+			expectedKey:   "custom-ca.crt",
+			expectWarning: false,
+		},
+		{
+			name: "Secret with specific key",
+			trustedCARef: &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "trusted-ca-secret",
+					Kind: "Secret",


The test cases use string literals "ConfigMap" and "Secret" for the Kind field rather than the defined constants metal3api.ResourceKindConfigMap and metal3api.ResourceKindSecret. Using the constants would be more consistent with the rest of the codebase (e.g., all production code uses the constants), and would protect against silent breakage if the constant values were ever changed.

Copilot · 2026-03-02T11:46:57Z

pkg/ironic/containers.go

+			cctx.Logger.Info("specified key not found in Trusted CA "+resourceKind+", using first available key",
+				"requestedKey", requestedKey, resourceKind, namespace+"/"+resourceName)


On lines 229-230 and 243-244, the variable resourceKind is used as a structured log key. Since resourceKind is dynamically set to either "ConfigMap" or "Secret" at runtime, the log key changes depending on the branch. Structured loggers expect consistently named fields; using a dynamic variable here would produce log entries with different field names (ConfigMap: namespace/name vs Secret: namespace/name), making log parsing and querying non-deterministic. The key should instead be a string literal such as "resource", with namespace+"/"+resourceName as its value.

tuminoid · 2026-03-02T11:50:26Z

pkg/ironic/containers.go

+	}
+
+	// If no key was specified or the specified key doesn't exist, select the first key
+	if selectedKey == "" {


Iterating over go maps gives you random key. If it changes between loops, we might reconcilate for no reason?

Sort the keys to get same alphabetical one?

tuminoid · 2026-03-02T11:51:47Z

pkg/ironic/containers.go

 		} else {
-			cctx.Logger.Info("ignoring duplicate key in Trusted CA ConfigMap",
-				"key", key, "configmap", fmt.Sprintf("%s/%s", configMap.Namespace, configMap.Name))
+			cctx.Logger.Info("specified key not found in Trusted CA "+resourceKind+", using first available key",


Shouldn't this be validation error, if specifically requsted key isn't present?

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2026

metal3-io-bot requested review from lentzi90 and tuminoid January 30, 2026 15:43

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 30, 2026

dtantsur force-pushed the ca-configmap branch from 82a766b to ef7da9a Compare January 30, 2026 17:11

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2026

dtantsur force-pushed the ca-configmap branch from 0b36e8f to 8eb848b Compare February 27, 2026 16:48

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2026

dtantsur force-pushed the ca-configmap branch from 8eb848b to 79832f9 Compare February 27, 2026 17:09

dtantsur changed the title ~~Accept a Secret or a ConfigMap for BMC CA and Trusted CA~~ ✨ Accept a Secret or a ConfigMap for BMC CA and Trusted CA Feb 27, 2026

dtantsur marked this pull request as ready for review February 27, 2026 17:10

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2026

metal3-io-bot requested review from Rozzii and elfosardo February 27, 2026 17:10

tuminoid requested a review from Copilot March 2, 2026 11:37

Copilot started reviewing on behalf of tuminoid March 2, 2026 11:37 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

tuminoid reviewed Mar 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Accept a Secret or a ConfigMap for BMC CA and Trusted CA#526

✨ Accept a Secret or a ConfigMap for BMC CA and Trusted CA#526
dtantsur wants to merge 1 commit intometal3-io:mainfrom
dtantsur:ca-configmap

dtantsur commented Jan 30, 2026 •

edited

Loading

Uh oh!

metal3-io-bot commented Jan 30, 2026

Uh oh!

metal3-io-bot commented Jan 30, 2026

Uh oh!

dtantsur commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

tuminoid Mar 2, 2026

Uh oh!

tuminoid Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		cctx.Logger.Info("ignoring duplicate key in Trusted CA "+resourceKind,
		"key", key, resourceKind, namespace+"/"+resourceName)

		cctx.Logger.Info("specified key not found in Trusted CA "+resourceKind+", using first available key",
		"requestedKey", requestedKey, resourceKind, namespace+"/"+resourceName)

Conversation

dtantsur commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metal3-io-bot commented Jan 30, 2026

Uh oh!

metal3-io-bot commented Jan 30, 2026

Uh oh!

dtantsur commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

tuminoid Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

tuminoid Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dtantsur commented Jan 30, 2026 •

edited

Loading