Fix CodeRabbit critical and major issues

- Fix CSR approval to use /approval subresource instead of /status
- Fix certificate URI construction to use ODataID instead of UUID
- Fix certificate type detection to use URI path instead of Type field
- Document CertificateSecretRef namespace in API comments
- Fix CSR name length to respect 253 char Kubernetes limit
- Fix certificate install race by persisting status before BMC call
- Fix signer allowlist to include configured signer name
- Simplify checkCertificateExpiration (remove unused error return)

Author: stefanhipfel

api/v1alpha1/bmc_types.go

@@ -288,6 +288,7 @@ type BMCStatus struct {
 	CertificateSigningRequestRef *string `json:"certificateSigningRequestRef,omitempty"`
 
 	// CertificateSecretRef references the Secret containing the installed certificate.
+	// The Secret is created in the metal-operator controller's namespace.
 	// +optional
 	CertificateSecretRef *v1.LocalObjectReference `json:"certificateSecretRef,omitempty"`
 }

bmc/bmc.go

@@ -326,6 +326,16 @@ const (
 	CertificateTypeCA CertificateType = "PEM"
 )
 
+// CertificatePurpose defines the purpose/usage of a certificate.
+type CertificatePurpose string
+
+const (
+	// CertificatePurposeHTTPS indicates an HTTPS/TLS server certificate
+	CertificatePurposeHTTPS CertificatePurpose = "HTTPS"
+	// CertificatePurposeCA indicates a CA certificate
+	CertificatePurposeCA CertificatePurpose = "CA"
+)
+
 // CSRRequest contains parameters for generating a Certificate Signing Request.
 type CSRRequest struct {
 	// CommonName is the common name for the certificate (typically hostname or IP)

bmc/redfish.go

@@ -1179,13 +1179,15 @@ func (r *RedfishBaseBMC) GenerateCSR(ctx context.Context, req CSRRequest) (*CSRR
 
 	// Get manager to find certificate collection
 	managers, err := r.client.GetService().Managers()
-	if err != nil || len(managers) == 0 {
+	if err != nil {
 		return nil, fmt.Errorf("failed to get managers: %w", err)
 	}
+	if len(managers) == 0 {
+		return nil, fmt.Errorf("no managers found")
+	}
 	manager := managers[0]
 
-	// Construct certificate collection URI
-	certCollectionURI := fmt.Sprintf("/redfish/v1/Managers/%s/NetworkProtocol/HTTPS/Certificates", manager.UUID)
+	certCollectionURI := manager.ODataID + "/NetworkProtocol/HTTPS/Certificates"
 
 	// Set default key algorithm if not specified
 	keyAlgorithm := req.KeyPairAlgorithm
@@ -1233,13 +1235,15 @@ func (r *RedfishBaseBMC) InstallCertificate(ctx context.Context, certificatePEM
 
 	// Get manager to find certificate collection
 	managers, err := r.client.GetService().Managers()
-	if err != nil || len(managers) == 0 {
+	if err != nil {
 		return fmt.Errorf("failed to get managers: %w", err)
 	}
+	if len(managers) == 0 {
+		return fmt.Errorf("no managers found")
+	}
 	manager := managers[0]
 
-	// Certificate collection URI
-	certCollectionURI := fmt.Sprintf("/redfish/v1/Managers/%s/NetworkProtocol/HTTPS/Certificates", manager.UUID)
+	certCollectionURI := manager.ODataID + "/NetworkProtocol/HTTPS/Certificates"
 
 	// Prepare certificate installation payload
 	payload := map[string]any{
@@ -1269,13 +1273,15 @@ func (r *RedfishBaseBMC) InstallCertificate(ctx context.Context, certificatePEM
 func (r *RedfishBaseBMC) GetCertificates(ctx context.Context) ([]CertificateInfo, error) {
 	// Get manager to find certificate collection
 	managers, err := r.client.GetService().Managers()
-	if err != nil || len(managers) == 0 {
+	if err != nil {
 		return nil, fmt.Errorf("failed to get managers: %w", err)
 	}
+	if len(managers) == 0 {
+		return nil, fmt.Errorf("no managers found")
+	}
 	manager := managers[0]
 
-	// Certificate collection URI
-	certCollectionURI := fmt.Sprintf("/redfish/v1/Managers/%s/NetworkProtocol/HTTPS/Certificates", manager.UUID)
+	certCollectionURI := manager.ODataID + "/NetworkProtocol/HTTPS/Certificates"
 
 	resp, err := r.client.Get(certCollectionURI)
 	if err != nil {

config/crd/bases/metal.ironcore.dev_bmcs.yaml

@@ -228,8 +228,9 @@ spec:
                     type: string
                 type: object
               certificateSecretRef:
-                description: CertificateSecretRef references the Secret containing
-                  the installed certificate.
+                description: |-
+                  CertificateSecretRef references the Secret containing the installed certificate.
+                  The Secret is created in the metal-operator controller's namespace.
                 properties:
                   name:
                     default: ""

internal/controller/bmc_controller.go

@@ -254,9 +254,7 @@ func (r *BMCReconciler) reconcile(ctx context.Context, bmcObj *metalv1alpha1.BMC
 			log.Error(condErr, "Failed to update certificate condition")
 		}
 	}
-	if err := r.checkCertificateExpiration(ctx, bmcObj); err != nil {
-		log.Error(err, "Failed to check certificate expiration")
-	}
+	r.checkCertificateExpiration(ctx, bmcObj)
 
 	log.V(1).Info("Reconciled BMC")
 	return ctrl.Result{}, nil
@@ -760,9 +758,21 @@ func (r *BMCReconciler) initiateCertificateRequest(ctx context.Context, bmcObj *
 		signerName = DefaultCertificateSignerName
 	}
 
+	// Generate safe CSR name (max 253 chars for Kubernetes metadata.name)
+	// Format: bmc-<bmcName>-<shortUID>
+	// Ensure bmcName is truncated if needed to stay under limit
+	uidStr := string(bmcObj.UID)
+	shortUID := uidStr[:8]               // First 8 chars of UUID
+	maxBMCNameLen := 253 - 4 - 1 - 8 - 1 // 253 - "bmc-" - "-" - shortUID - null = 239
+	bmcName := bmcObj.Name
+	if len(bmcName) > maxBMCNameLen {
+		bmcName = bmcName[:maxBMCNameLen]
+	}
+	csrName := fmt.Sprintf("bmc-%s-%s", bmcName, shortUID)
+
 	k8sCSR := &certificatesv1.CertificateSigningRequest{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: fmt.Sprintf("bmc-%s-%s", bmcObj.Name, string(bmcObj.UID[:8])),
+			Name: csrName,
 			Labels: map[string]string{
 				"metal.ironcore.dev/bmc": bmcObj.Name,
 			},
@@ -987,13 +997,7 @@ func (r *BMCReconciler) installCertificate(ctx context.Context, bmcObj *metalv1a
 		return fmt.Errorf("certificate missing ServerAuth extended key usage")
 	}
 
-	if err := bmcClient.InstallCertificate(ctx, string(k8sCSR.Status.Certificate), bmc.CertificateTypeHTTPS); err != nil {
-		_ = r.updateConditions(ctx, bmcObj, true, bmcCertificateInstalledConditionType,
-			corev1.ConditionFalse, bmcCertificateInstallFailedReason,
-			fmt.Sprintf("Failed to install certificate: %v", err))
-		return err
-	}
-
+	// Create/update secret first
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("bmc-%s-cert", bmcObj.Name),
@@ -1017,18 +1021,28 @@ func (r *BMCReconciler) installCertificate(ctx context.Context, bmcObj *metalv1a
 	}
 	log.Info("Certificate secret created or updated", "operation", opResult)
 
+	// Persist secret reference to status before calling BMC
 	bmcBase := bmcObj.DeepCopy()
 	bmcObj.Status.CertificateSecretRef = &corev1.LocalObjectReference{Name: secret.Name}
 	bmcObj.Status.CertificateSigningRequestRef = nil
 
-	if err := r.updateCertificateInfo(ctx, bmcObj, bmcClient); err != nil {
-		log.Error(err, "Failed to update certificate info")
+	if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil {
+		return fmt.Errorf("failed to persist certificate secret reference: %w", err)
 	}
 
-	if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil {
+	// Now install certificate on BMC
+	// If this fails, on retry we'll have secretRef set and can skip secret creation
+	if err := bmcClient.InstallCertificate(ctx, string(k8sCSR.Status.Certificate), bmc.CertificateTypeHTTPS); err != nil {
+		_ = r.updateConditions(ctx, bmcObj, true, bmcCertificateInstalledConditionType,
+			corev1.ConditionFalse, bmcCertificateInstallFailedReason,
+			fmt.Sprintf("Failed to install certificate: %v", err))
 		return err
 	}
 
+	if err := r.updateCertificateInfo(ctx, bmcObj, bmcClient); err != nil {
+		log.Error(err, "Failed to update certificate info")
+	}
+
 	log.Info("Certificate installed successfully")
 	_ = r.updateConditions(ctx, bmcObj, true, bmcCertificateInstalledConditionType,
 		corev1.ConditionTrue, bmcCertificateInstalledReason,
@@ -1037,11 +1051,11 @@ func (r *BMCReconciler) installCertificate(ctx context.Context, bmcObj *metalv1a
 	return nil
 }
 
-func (r *BMCReconciler) checkCertificateExpiration(ctx context.Context, bmcObj *metalv1alpha1.BMC) error {
+func (r *BMCReconciler) checkCertificateExpiration(ctx context.Context, bmcObj *metalv1alpha1.BMC) {
 	log := ctrl.LoggerFrom(ctx)
 
 	if bmcObj.Status.CertificateInfo == nil {
-		return nil
+		return
 	}
 
 	renewalThreshold := r.DefaultCertificateRenewalThreshold
@@ -1058,29 +1072,21 @@ func (r *BMCReconciler) checkCertificateExpiration(ctx context.Context, bmcObj *
 			_ = r.updateConditions(ctx, bmcObj, true, bmcCertificateExpiringConditionType,
 				corev1.ConditionTrue, bmcCertificateExpiringSoonReason,
 				fmt.Sprintf("Certificate expires in %s", timeUntilExpiry))
-
-			if bmcObj.Spec.CertificateManagementPolicy != nil &&
-				*bmcObj.Spec.CertificateManagementPolicy == metalv1alpha1.CertificateManagementPolicyAutomatic {
-				bmcBase := bmcObj.DeepCopy()
-				bmcObj.Status.CertificateSecretRef = nil
-				if err := r.Status().Patch(ctx, bmcObj, client.MergeFrom(bmcBase)); err != nil {
-					return err
-				}
-			}
 		} else {
 			_ = r.updateConditions(ctx, bmcObj, true, bmcCertificateExpiringConditionType,
 				corev1.ConditionFalse, bmcCertificateValidReason,
 				fmt.Sprintf("Certificate valid until %s", expiryTime))
 		}
 	}
-
-	return nil
 }
 
 func (r *BMCReconciler) approveCSR(ctx context.Context, csr *certificatesv1.CertificateSigningRequest) error {
 	allowedSigners := []string{
 		DefaultCertificateSignerName,
 	}
+	if r.DefaultCertificateSignerName != "" && r.DefaultCertificateSignerName != DefaultCertificateSignerName {
+		allowedSigners = append(allowedSigners, r.DefaultCertificateSignerName)
+	}
 
 	if !slices.Contains(allowedSigners, csr.Spec.SignerName) {
 		return fmt.Errorf("controller not authorized to approve signer: %s (only allowed: %v)",
@@ -1096,7 +1102,7 @@ func (r *BMCReconciler) approveCSR(ctx context.Context, csr *certificatesv1.Cert
 		LastTransitionTime: metav1.Now(),
 	})
 
-	if err := r.Status().Update(ctx, csr); err != nil {
+	if err := r.SubResource("approval").Update(ctx, csr); err != nil {
 		return err
 	}
 
@@ -1178,9 +1184,9 @@ func (r *BMCReconciler) updateCertificateInfo(ctx context.Context, bmcObj *metal
 		return err
 	}
 
-	// Find HTTPS certificate
+	// Find HTTPS certificate (determined by URI path, not CertificateType field)
 	for _, cert := range certs {
-		if cert.Type == bmc.CertificateTypeHTTPS {
+		if strings.Contains(cert.URI, "/HTTPS/Certificates") {
 			notBefore, _ := time.Parse(time.RFC3339, cert.ValidNotBefore)
 			notAfter, _ := time.Parse(time.RFC3339, cert.ValidNotAfter)