Only use a Key for TrustedCA, not for BMCCA

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>

Author: dtantsur

.golangci.yaml

@@ -134,6 +134,12 @@ linters:
     - linters:
       - tagliatelle
       text: CA
+    - linters:
+      - staticcheck
+      text: 'SA1019: tls\.(TrustedCAName|BMCCAName) is deprecated: .*'
+      paths:
+      - pkg/ironic/validation.go
+      - pkg/ironic/utils.go
     paths:
     - zz_generated.*\.go$
     - .*conversion.*\.go$

api/v1alpha1/common.go

@@ -18,3 +18,24 @@ var (
 	IronicServiceLabel = IronicLabelPrefix + "/ironic"
 	IronicVersionLabel = IronicLabelPrefix + "/version"
 )
+
+// ResourceReference references a ConfigMap or Secret resource.
+type ResourceReference struct {
+	// Name of the resource.
+	Name string `json:"name"`
+
+	// Kind of the resource (ConfigMap or Secret).
+	// +kubebuilder:validation:Enum=ConfigMap;Secret
+	Kind string `json:"kind"`
+}
+
+// ResourceReferenceWithKey references a ConfigMap or Secret resource and
+// targets a specific key from it.
+type ResourceReferenceWithKey struct {
+	ResourceReference `json:",inline"`
+
+	// Key within the resource to use. If not specified and the resource contains multiple keys,
+	// the first key will be used and a warning will be logged for other keys.
+	// +optional
+	Key string `json:"key,omitempty"`
+}

api/v1alpha1/ironic_types.go

@@ -60,21 +60,6 @@ type Inspection struct {
 	VLANInterfaces []string `json:"vlanInterfaces,omitempty"`
 }
 
-// ResourceReference references a ConfigMap or Secret resource.
-type ResourceReference struct {
-	// Name of the resource.
-	Name string `json:"name"`
-
-	// Kind of the resource (ConfigMap or Secret).
-	// +kubebuilder:validation:Enum=ConfigMap;Secret
-	Kind string `json:"kind"`
-
-	// Key within the resource to use. If not specified and the resource contains multiple keys,
-	// the first key will be used and a warning will be logged for other keys.
-	// +optional
-	Key string `json:"key,omitempty"`
-}
-
 type DHCP struct {
 	// DNSAddress is the IP address of the DNS server to pass to hosts via DHCP.
 	// Must not be set together with ServeDNS.
@@ -232,10 +217,8 @@ type TLS struct {
 	// TrustedCA is a reference to a ConfigMap or Secret containing the CA certificate(s)
 	// to use when validating TLS connections to image servers and other services.
 	// The resource should contain one or more CA certificates in PEM format.
-	// If the resource contains multiple keys, only the first key will be used and
-	// a warning will be logged.
 	// +optional
-	TrustedCA *ResourceReference `json:"trustedCA,omitempty"`
+	TrustedCA *ResourceReferenceWithKey `json:"trustedCA,omitempty"`
 
 	// TrustedCAName is a reference to the configmap with the CA certificate(s)
 	// to use when validating TLS connections to image servers and other services.
@@ -261,36 +244,6 @@ type TLS struct {
 	InsecureRPC *bool `json:"insecureRPC,omitempty"`
 }
 
-// GetBMCCA returns the effective BMC CA resource reference.
-// It prefers the new BMCCA field over the deprecated BMCCAName field.
-func (t *TLS) GetBMCCA() *ResourceReference {
-	if t.BMCCA != nil {
-		return t.BMCCA
-	}
-	if t.BMCCAName != "" {
-		return &ResourceReference{
-			Name: t.BMCCAName,
-			Kind: ResourceKindSecret,
-		}
-	}
-	return nil
-}
-
-// GetTrustedCA returns the effective Trusted CA resource reference.
-// It prefers the new TrustedCA field over the deprecated TrustedCAName field.
-func (t *TLS) GetTrustedCA() *ResourceReference {
-	if t.TrustedCA != nil {
-		return t.TrustedCA
-	}
-	if t.TrustedCAName != "" {
-		return &ResourceReference{
-			Name: t.TrustedCAName,
-			Kind: ResourceKindConfigMap,
-		}
-	}
-	return nil
-}
-
 type Images struct {
 	// DeployRamdiskBranch is the branch of IPA to download. The main branch is used by default.
 	// Not used if deployRamdisk.disableDownloader is true.

api/v1alpha1/zz_generated.deepcopy.go

@@ -3431,11 +3431,6 @@ spec:
                       to use when validating TLS connections to BMCs.
                       Supported in Ironic 32.0 or newer.
                     properties:
-                      key:
-                        description: |-
-                          Key within the resource to use. If not specified and the resource contains multiple keys,
-                          the first key will be used and a warning will be logged for other keys.
-                        type: string
                       kind:
                         description: Kind of the resource (ConfigMap or Secret).
                         enum:
@@ -3480,8 +3475,6 @@ spec:
                       TrustedCA is a reference to a ConfigMap or Secret containing the CA certificate(s)
                       to use when validating TLS connections to image servers and other services.
                       The resource should contain one or more CA certificates in PEM format.
-                      If the resource contains multiple keys, only the first key will be used and
-                      a warning will be logged.
                     properties:
                       key:
                         description: |-

config/crd/bases/ironic.metal3.io_ironics.yaml

@@ -6941,9 +6941,7 @@ HighAvailability feature gate to be set.<br/>
         <td>
           TrustedCA is a reference to a ConfigMap or Secret containing the CA certificate(s)
 to use when validating TLS connections to image servers and other services.
-The resource should contain one or more CA certificates in PEM format.
-If the resource contains multiple keys, only the first key will be used and
-a warning will be logged.<br/>
+The resource should contain one or more CA certificates in PEM format.<br/>
         </td>
         <td>false</td>
       </tr><tr>
@@ -6997,14 +6995,6 @@ Supported in Ironic 32.0 or newer.
           Name of the resource.<br/>
         </td>
         <td>true</td>
-      </tr><tr>
-        <td><b>key</b></td>
-        <td>string</td>
-        <td>
-          Key within the resource to use. If not specified and the resource contains multiple keys,
-the first key will be used and a warning will be logged for other keys.<br/>
-        </td>
-        <td>false</td>
       </tr></tbody>
 </table>
 
@@ -7017,8 +7007,6 @@ the first key will be used and a warning will be logged for other keys.<br/>
 TrustedCA is a reference to a ConfigMap or Secret containing the CA certificate(s)
 to use when validating TLS connections to image servers and other services.
 The resource should contain one or more CA certificates in PEM format.
-If the resource contains multiple keys, only the first key will be used and
-a warning will be logged.
 
 <table>
     <thead>

docs/api.md

@@ -168,7 +168,7 @@ func (r *IronicReconciler) handleIronic(cctx ironic.ControllerContext, ironicCon
 
 	var bmcCASecret *corev1.Secret
 	var bmcCAConfigMap *corev1.ConfigMap
-	if bmcCARef := ironicConf.Spec.TLS.GetBMCCA(); bmcCARef != nil {
+	if bmcCARef := ironic.GetBMCCA(&ironicConf.Spec.TLS); bmcCARef != nil {
 		switch bmcCARef.Kind {
 		case metal3api.ResourceKindSecret:
 			bmcCASecret, requeue, err = r.getAndUpdateSecret(cctx, ironicConf, bmcCARef.Name)
@@ -185,7 +185,7 @@ func (r *IronicReconciler) handleIronic(cctx ironic.ControllerContext, ironicCon
 
 	var trustedCASecret *corev1.Secret
 	var trustedCAConfigMap *corev1.ConfigMap
-	if trustedCARef := ironicConf.Spec.TLS.GetTrustedCA(); trustedCARef != nil {
+	if trustedCARef := ironic.GetTrustedCA(&ironicConf.Spec.TLS); trustedCARef != nil {
 		switch trustedCARef.Kind {
 		case metal3api.ResourceKindSecret:
 			trustedCASecret, requeue, err = r.getAndUpdateSecret(cctx, ironicConf, trustedCARef.Name)

internal/controller/ironic_controller.go

@@ -211,16 +211,19 @@ func buildTrustedCAEnvVars(cctx ControllerContext, resources Resources) []corev1
 	}
 
 	// Get the TrustedCA reference to check if a specific key was requested
-	trustedCARef := resources.Ironic.Spec.TLS.GetTrustedCA()
+	var requestedKey string
+	if resources.Ironic.Spec.TLS.TrustedCA != nil {
+		requestedKey = resources.Ironic.Spec.TLS.TrustedCA.Key
+	}
 	var selectedKey string
 
-	if trustedCARef != nil && trustedCARef.Key != "" {
+	if requestedKey != "" {
 		// User specified a key, use it if it exists
-		if _, exists := keys[trustedCARef.Key]; exists {
-			selectedKey = trustedCARef.Key
+		if _, exists := keys[requestedKey]; exists {
+			selectedKey = requestedKey
 		} else {
 			cctx.Logger.Info("specified key not found in Trusted CA "+resourceKind+", using first available key",
-				"requestedKey", trustedCARef.Key, resourceKind, namespace+"/"+resourceName)
+				"requestedKey", requestedKey, resourceKind, namespace+"/"+resourceName)
 			// Fall through to select first key
 		}
 	}
@@ -459,8 +462,7 @@ func buildIronicVolumesAndMounts(resources Resources) (volumes []corev1.Volume,
 		}
 	}
 
-	if maybeVolume := volumeForSecretOrConfigMap(bmcCAVolumeName, resources.BMCCASecret, resources.BMCCAConfigMap,
-		resources.Ironic.Spec.TLS.GetBMCCA()); maybeVolume != nil {
+	if maybeVolume := volumeForSecretOrConfigMap(bmcCAVolumeName, resources.BMCCASecret, resources.BMCCAConfigMap); maybeVolume != nil {
 		volumes = append(volumes, *maybeVolume)
 		mounts = append(mounts,
 			corev1.VolumeMount{
@@ -471,8 +473,7 @@ func buildIronicVolumesAndMounts(resources Resources) (volumes []corev1.Volume,
 		)
 	}
 
-	if maybeVolume := volumeForSecretOrConfigMap(trustedCAVolumeName, resources.TrustedCASecret, resources.TrustedCAConfigMap,
-		resources.Ironic.Spec.TLS.GetTrustedCA()); maybeVolume != nil {
+	if maybeVolume := volumeForSecretOrConfigMap(trustedCAVolumeName, resources.TrustedCASecret, resources.TrustedCAConfigMap); maybeVolume != nil {
 		volumes = append(volumes, *maybeVolume)
 		mounts = append(mounts,
 			corev1.VolumeMount{

pkg/ironic/containers.go

@@ -529,7 +529,7 @@ func TestPrometheusExporterEnvVars(t *testing.T) {
 func TestBuildTrustedCAEnvVars(t *testing.T) {
 	testCases := []struct {
 		name            string
-		trustedCARef    *metal3api.ResourceReference
+		trustedCARef    *metal3api.ResourceReferenceWithKey
 		configMapData   map[string]string
 		secretData      map[string][]byte
 		expectedKey     string
@@ -538,10 +538,12 @@ func TestBuildTrustedCAEnvVars(t *testing.T) {
 	}{
 		{
 			name: "ConfigMap with specific key",
-			trustedCARef: &metal3api.ResourceReference{
-				Name: "trusted-ca",
-				Kind: "ConfigMap",
-				Key:  "custom-ca.crt",
+			trustedCARef: &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "trusted-ca",
+					Kind: "ConfigMap",
+				},
+				Key: "custom-ca.crt",
 			},
 			configMapData: map[string]string{
 				"custom-ca.crt": "cert1",
@@ -552,10 +554,12 @@ func TestBuildTrustedCAEnvVars(t *testing.T) {
 		},
 		{
 			name: "Secret with specific key",
-			trustedCARef: &metal3api.ResourceReference{
-				Name: "trusted-ca-secret",
-				Kind: "Secret",
-				Key:  "tls.crt",
+			trustedCARef: &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "trusted-ca-secret",
+					Kind: "Secret",
+				},
+				Key: "tls.crt",
 			},
 			secretData: map[string][]byte{
 				"tls.crt": []byte("cert1"),
@@ -566,10 +570,12 @@ func TestBuildTrustedCAEnvVars(t *testing.T) {
 		},
 		{
 			name: "Key specified but doesn't exist - ConfigMap",
-			trustedCARef: &metal3api.ResourceReference{
-				Name: "trusted-ca",
-				Kind: "ConfigMap",
-				Key:  "nonexistent.crt",
+			trustedCARef: &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "trusted-ca",
+					Kind: "ConfigMap",
+				},
+				Key: "nonexistent.crt",
 			},
 			configMapData: map[string]string{
 				"actual-ca.crt": "cert1",
@@ -580,10 +586,12 @@ func TestBuildTrustedCAEnvVars(t *testing.T) {
 		},
 		{
 			name: "Key specified but doesn't exist - Secret",
-			trustedCARef: &metal3api.ResourceReference{
-				Name: "trusted-ca-secret",
-				Kind: "Secret",
-				Key:  "missing.crt",
+			trustedCARef: &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "trusted-ca-secret",
+					Kind: "Secret",
+				},
+				Key: "missing.crt",
 			},
 			secretData: map[string][]byte{
 				"available.crt": []byte("cert1"),
@@ -594,9 +602,11 @@ func TestBuildTrustedCAEnvVars(t *testing.T) {
 		},
 		{
 			name: "Multiple keys without Key specified - ConfigMap",
-			trustedCARef: &metal3api.ResourceReference{
-				Name: "trusted-ca",
-				Kind: "ConfigMap",
+			trustedCARef: &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "trusted-ca",
+					Kind: "ConfigMap",
+				},
 			},
 			configMapData: map[string]string{
 				"ca1.crt": "cert1",
@@ -712,10 +722,12 @@ func TestBuildTrustedCAEnvVarsKeySelection(t *testing.T) {
 				Logger: logr.Discard(),
 			}
 
-			trustedCARef := &metal3api.ResourceReference{
-				Name: "test-ca",
-				Kind: "ConfigMap",
-				Key:  tc.specifiedKey,
+			trustedCARef := &metal3api.ResourceReferenceWithKey{
+				ResourceReference: metal3api.ResourceReference{
+					Name: "test-ca",
+					Kind: "ConfigMap",
+				},
+				Key: tc.specifiedKey,
 			}
 
 			data := make(map[string]string)

pkg/ironic/containers_test.go

@@ -75,8 +75,8 @@ func ParseLocalIronic(inputFile string, scheme *runtime.Scheme) (*Resources, err
 	}
 
 	// Get effective CA references
-	bmcCARef := resources.Ironic.Spec.TLS.GetBMCCA()
-	trustedCARef := resources.Ironic.Spec.TLS.GetTrustedCA()
+	bmcCARef := GetBMCCA(&resources.Ironic.Spec.TLS)
+	trustedCARef := GetTrustedCA(&resources.Ironic.Spec.TLS)
 
 	for _, secretObj := range secrets {
 		// Determine secret type based on name patterns or labels