pkg/controller/istiocsr/test_utils.go (1)

312-322: Helper OK

createCertificateConfigMap aligns with test expectations (key "ca-cert.pem"). Consider parameterizing the key to reuse for negative-key tests.

pkg/controller/istiocsr/deployments_test.go (2)

450-451: Error message: missing resource name and double space

The expected string shows “failed to update resource with watch label…”. Likely the reconciler omitted object identification, leading to unclear logs and brittle tests. Include kind/name in the message (as done in Line 921 scenario) and update this expectation accordingly.

---

781-809: Negative CA ConfigMap coverage is comprehensive

Great assertions for not found, missing key, invalid PEM, watch-label update failure, non-CA, and missing CertSign. Consider adding one assertion to inspect the generated Deployment for the presence/absence of the CA volume/mount to strengthen behavioral validation.

Also applies to: 811-842, 844-875, 877-922, 924-954, 956-986

config/crd/bases/operator.openshift.io_istiocsrs.yaml (2)

1031-1050: Consider making the IstioCACertificate field immutable after creation.

The istioCACertificate field allows users to configure a CA certificate from a ConfigMap. Once set, changing this field could lead to certificate validation issues and potential service disruptions. Consider adding an immutability validation rule similar to other critical fields in this CRD.

Add an x-kubernetes-validations rule to prevent changes after creation:

                      istioCACertificate:
                        description: |-
                          istioCACertificate when provided, the operator will use the CA certificate from the specified ConfigMap. If empty, the operator will
                          automatically extract the CA certificate from the Secret containing the istiod certificate obtained from cert-manager.
                        properties:
                          key:
                            description: key name holding the required data.
                            type: string
                          name:
                            description: name of the ConfigMap.
                            type: string
                          namespace:
                            description: namespace in which the ConfigMap exists.
                              If empty, ConfigMap will be looked up in the IstioCSR
                              namespace.
                            type: string
                        required:
                        - key
                        - name
                        type: object
+                       x-kubernetes-validations:
+                       - message: istioCACertificate is immutable once set
+                         rule: self == oldSelf

---

1033-1034: Improve field description clarity.

The description could be clearer about the behavior when this field is not provided.

-                        description: |-
-                          istioCACertificate when provided, the operator will use the CA certificate from the specified ConfigMap. If empty, the operator will
-                          automatically extract the CA certificate from the Secret containing the istiod certificate obtained from cert-manager.
+                        description: |-
+                          istioCACertificate references a ConfigMap containing the CA certificate to be used by istio-csr.
+                          When provided, the operator will use the CA certificate from the specified ConfigMap.
+                          When not provided, the operator will automatically extract the CA certificate from the issuer's Secret.

pkg/controller/istiocsr/deployments.go (4)

267-282: Consider improving error messages for better troubleshooting.

The error wrapping could be more specific about which CA provisioning path failed.

 func (r *Reconciler) updateVolumes(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
 	// Use user-configured CA certificate if provided
 	if istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate != nil {
 		if err := r.handleUserProvidedCA(deployment, istiocsr, resourceLabels); err != nil {
-			return FromError(err, "failed to validate and mount CA certificate ConfigMap")
+			return FromError(err, "failed to handle user-provided CA certificate from ConfigMap")
 		}
 		return nil
 	}
 
 	// Fall back to issuer-based CA certificate if CA certificate is not configured
 	// Handle issuer-based CA configuration
 	if err := r.handleIssuerBasedCA(deployment, istiocsr, resourceLabels); err != nil {
-		return err
+		return FromError(err, "failed to handle issuer-based CA certificate")
 	}
 
 	return nil
 }

---

284-291: Remove unnecessary nil check.

The nil check for caCertConfig is redundant since this function is only called when IstioCACertificate is not nil.

 func (r *Reconciler) handleUserProvidedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
 	caCertConfig := istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate
-	if caCertConfig == nil {
-		return NewIrrecoverableError(fmt.Errorf("CA certificate configuration is nil"), "CA certificate validation failed")
-	}
 
 	// Determine the namespace - use specified namespace or default to IstioCSR namespace

---

529-542: Consider optimizing label updates to avoid unnecessary API calls.

The function always calls UpdateWithRetry even if the label already exists with the correct value.

 func (r *Reconciler) updateWatchLabel(obj client.Object, istiocsr *v1alpha1.IstioCSR) error {
 	labels := obj.GetLabels()
 	if labels == nil {
 		labels = make(map[string]string)
 	}
+	expectedValue := fmt.Sprintf(istiocsrResourceWatchLabelValueFmt, istiocsr.GetNamespace(), istiocsr.GetName())
+	if labels[istiocsrResourceWatchLabelName] == expectedValue {
+		// Label already set correctly, no update needed
+		return nil
+	}
-	labels[istiocsrResourceWatchLabelName] = fmt.Sprintf(istiocsrResourceWatchLabelValueFmt, istiocsr.GetNamespace(), istiocsr.GetName())
+	labels[istiocsrResourceWatchLabelName] = expectedValue
 	obj.SetLabels(labels)
 
 	if err := r.UpdateWithRetry(r.ctx, obj); err != nil {
 		return fmt.Errorf("failed to update %s resource with watch label: %w", obj.GetName(), err)
 	}
 	return nil
 }

---

307-310: Enhance error message to include available keys.

When the specified key is not found in the ConfigMap, it would be helpful to list the available keys to aid debugging.

 	// Validate that the specified key exists in the ConfigMap
 	if _, exists := sourceConfigMap.Data[caCertConfig.Key]; !exists {
-		return NewIrrecoverableError(fmt.Errorf("key %q not found in ConfigMap %s/%s", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name), "CA certificate ConfigMap %s/%s does not contain required key %q", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, caCertConfig.Key)
+		availableKeys := make([]string, 0, len(sourceConfigMap.Data))
+		for k := range sourceConfigMap.Data {
+			availableKeys = append(availableKeys, k)
+		}
+		return NewIrrecoverableError(fmt.Errorf("key %q not found in ConfigMap %s/%s, available keys: %v", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, availableKeys), "CA certificate ConfigMap %s/%s does not contain required key %q, available keys: %v", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, caCertConfig.Key, availableKeys)
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

pkg/controller/istiocsr/test_utils.go (2)

5-9: Missing import for serial number generation (math/big)

Required for setting x509.Certificate.SerialNumber.

 import (
   "context"
   "crypto/rand"
   "crypto/rsa"
   "crypto/x509"
   "crypto/x509/pkix"
   "encoding/pem"
+  "math/big"
   "fmt"
   "testing"
   "time"
 )

---

274-310: Set a non-nil, random SerialNumber before CreateCertificate

Without SerialNumber, x509.CreateCertificate errors and tests panic.

 func GenerateCertificate(commonName string, organization []string, tweak CertificateTweak) string {
   // Generate RSA private key
   privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
   if err != nil {
     panic(fmt.Sprintf("Failed to generate private key: %v", err))
   }

   template := &x509.Certificate{
     Subject: pkix.Name{
       CommonName:   commonName,
       Organization: organization,
     },
     NotBefore:             time.Now(),
     NotAfter:              time.Now().Add(24 * time.Hour),
     KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
     ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
     BasicConstraintsValid: true,
     IsCA:                  false,
   }

   // Apply tweaks to modify the template
   tweak(template)

+  // Assign a random serial number
+  serialLimit := new(big.Int).Lsh(big.NewInt(1), 128)
+  serial, err := rand.Int(rand.Reader, serialLimit)
+  if err != nil {
+    panic(fmt.Sprintf("Failed to generate serial number: %v", err))
+  }
+  template.SerialNumber = serial
+
   certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey)
   if err != nil {
     panic(fmt.Sprintf("Failed to create certificate: %v", err))
   }

pkg/controller/istiocsr/constants.go (1)

86-96: Polish exported doc comments (capitalize ConfigMap, tighten phrasing)

Minor comment nits for consistency with Go doc and k8s terminology.

-// IstioCSRCAConfigMapName is the exported name of the configmap which is mounted in istiocsr container, containing the
+// IstioCSRCAConfigMapName is the exported name of the ConfigMap mounted in the istiocsr container, containing the
 // CA certificate configured in the secret referenced in the issuer.
 IstioCSRCAConfigMapName = istiocsrCAConfigMapName

-// IstioCSRCAKeyName is the exported key name holding the CA certificate in the issuer secret or the controller
-// CA configmap.
+// IstioCSRCAKeyName is the exported key name holding the CA certificate in the issuer secret or the controller
+// CA ConfigMap.
 IstioCSRCAKeyName = istiocsrCAKeyName

-// IstioCSRResourceWatchLabelName is the exported label name for identifying the resources of interest for the
-// controller but does not create or manage the resource.
+// IstioCSRResourceWatchLabelName is the exported label name for identifying resources of interest for the
+// controller that it does not create or manage.
 IstioCSRResourceWatchLabelName = istiocsrResourceWatchLabelName

pkg/controller/istiocsr/test_utils.go (1)

243-252: Include CRLSign for CA certificates (test robustness)

For CA examples, add CRLSign alongside CertSign to better emulate typical CA usage.

 func testCACertificateConfigMap() *corev1.ConfigMap {
   caPEM := GenerateCertificate("Test CA", []string{"cert-manager-operator"},
     func(cert *x509.Certificate) {
       cert.IsCA = true
-      cert.KeyUsage |= x509.KeyUsageCertSign
+      cert.KeyUsage |= x509.KeyUsageCertSign | x509.KeyUsageCRLSign
     },
   )
   return createCertificateConfigMap("ca-cert-test", testIstioCSRNamespace, caPEM)
 }

test/e2e/istio_csr_test.go (3)

413-433: Drop commented debug block

Dead commented code adds noise; rely on existing wait helpers or GinkgoWriter if needed.

-			// Add some debugging before calling waitForIstioCSRReady
-			// Eventually(func() error {
-			// 	istiocsrClient := loader.DynamicClient.Resource(istiocsrSchema).Namespace(ns.Name)
-			// 	customResource, err := istiocsrClient.Get(ctx, "default", metav1.GetOptions{})
-			// 	if err != nil {
-			// 		return fmt.Errorf("IstioCSR not found: %v", err))
-			// 	}
-			//
-			// 	status, found, err := unstructured.NestedMap(customResource.Object, "status")
-			// 	if err != nil {
-			// 		return fmt.Errorf("failed to extract status: %v", err))
-			// 	}
-			// 	if !found {
-			// 		return fmt.Errorf("status not found in IstioCSR"))
-			// 	}
-			//
-			// 	// Print status for debugging
-			// 	fmt.Printf("DEBUG: IstioCSR status: %+v\n", status)
-			// 	return nil
-			// }, "30s", "5s").Should(Succeed(), "Should be able to get IstioCSR status for debugging")

---

459-468: Strengthen assertion: also verify watch-label value

Confirms correct value format, not just presence.

-			Eventually(func() bool {
+			Eventually(func() bool {
 				sourceConfigMap, err := clientset.CoreV1().ConfigMaps(caCertConfigMap.Namespace).Get(ctx, caCertConfigMap.Name, metav1.GetOptions{})
 				if err != nil {
 					return false
 				}
-				_, hasWatchLabel := sourceConfigMap.Labels[testutils.IstioCSRResourceWatchLabelName]
-				return hasWatchLabel
+				val, ok := sourceConfigMap.Labels[testutils.IstioCSRResourceWatchLabelName]
+				expected := fmt.Sprintf("%s_%s", ns.Name, "default")
+				return ok && val == expected
 			}, "1m", "5s").Should(BeTrue(), "Source ConfigMap should have watch label")

---

498-573: Mirror label-value check for custom-namespace scenario

Same rationale as above; ensure value correctness when source is outside IstioCSR namespace.

-			Eventually(func() bool {
+			Eventually(func() bool {
 				sourceConfigMap, err := clientset.CoreV1().ConfigMaps(customNamespace.Name).Get(ctx, caCertConfigMap.Name, metav1.GetOptions{})
 				if err != nil {
 					return false
 				}
-				_, hasWatchLabel := sourceConfigMap.Labels[testutils.IstioCSRResourceWatchLabelName]
-				return hasWatchLabel
+				val, ok := sourceConfigMap.Labels[testutils.IstioCSRResourceWatchLabelName]
+				expected := fmt.Sprintf("%s_%s", ns.Name, "default")
+				return ok && val == expected
 			}, "1m", "5s").Should(BeTrue(), "Source ConfigMap in custom namespace should have watch label")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

test/e2e/istio_csr_test.go (1)

355-355: Remove focused spec: FContext will break CI

FContext found at Line 355 — change to Context.

- FContext("with CA Certificate ConfigMap", func() {
+ Context("with CA Certificate ConfigMap", func() {

Run to ensure no other focused specs remain:

#!/bin/bash
find . -type f -name '*.go' -not -path './vendor/*' -not -path './.git/*' -print0 | xargs -0 -r grep -nHE 'FDescribe\(|FContext\(|FIt\(|FWhen\(|FSpecify\(' || true

test/e2e/istio_csr_test.go (4)

13-13: Consider removing or using the debug log import.

The log package is imported but only used for debugging output. Consider either removing it if not needed in production tests or using a proper test logger.

If debug logging is needed, consider using Ginkgo's built-in GinkgoWriter instead:

-import (
-	"log"
-)

// Then replace log.Printf with:
// fmt.Fprintf(GinkgoWriter, "IstioCSR status: %+v\n", istioCSRStatus)

Also applies to: 22-22

---

435-435: Replace debug log statements with proper test logging.

Debug log statements using log.Printf should use Ginkgo's built-in logging for better test output management.

-log.Printf("IstioCSR status: %+v", istioCSRStatus)
+fmt.Fprintf(GinkgoWriter, "IstioCSR status: %+v\n", istioCSRStatus)

Also applies to: 542-542

---

404-407: Remove commented-out code.

Commented verification code should be removed or uncommented if needed.

-			// By("Verifying ConfigMap exists before creating IstioCSR")
-			// _, err = clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, configMapRefName, metav1.GetOptions{})
-			// Expect(err).ShouldNot(HaveOccurred(), "ConfigMap should exist before creating IstioCSR")
-

---

413-433: Remove or activate commented debugging code.

Large block of commented debugging code should be removed or properly integrated if debugging is needed.

If this debugging is needed, consider extracting it to a helper function that can be conditionally enabled:

func debugIstioCSRStatus(ctx context.Context, loader *TestLoader, ns string) {
    if os.Getenv("DEBUG_E2E") != "" {
        // debugging code here
    }
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

test/e2e/istio_csr_test.go (1)

355-355: Remove focused spec (FContext) — will break CI.

Switch to Context and ensure no other focused nodes remain.

Apply:

-	FContext("with CA Certificate ConfigMap", func() {
+	Context("with CA Certificate ConfigMap", func() {

#!/bin/bash
set -euo pipefail
echo "Searching for focused Ginkgo nodes..."
PAT='\b(FDescribe|FContext|FIt|FWhen|FSpecify)\b'
rg -nP "$PAT" -g '!**/vendor/**' -C1 || true

bundle/manifests/operator.openshift.io_istiocsrs.yaml (1)

1031-1051: Add immutability + basic length validation for istioCACertificate.

Prevents mid‑lifecycle drift and empty keys.

Apply:

                       istioCACertificate:
                         description: |-
                           istioCACertificate when provided, the operator will use the CA certificate from the specified ConfigMap. If empty, the operator will
                           automatically extract the CA certificate from the Secret containing the istiod certificate obtained from cert-manager.
                         properties:
                           key:
                             description: key name holding the required data.
                             type: string
+                            minLength: 1
                           name:
                             description: name of the ConfigMap.
                             type: string
+                            minLength: 1
                           namespace:
                             description: namespace in which the ConfigMap exists.
                               If empty, ConfigMap will be looked up in the IstioCSR
                               namespace.
                             type: string
                         required:
                         - key
                         - name
                         type: object
+                        x-kubernetes-validations:
+                        - message: istioCACertificate is immutable once set
+                          rule: self == oldSelf

pkg/controller/istiocsr/constants.go (2)

72-75: Fix comment to match actual label value format (underscore, not slash).

Avoid confusing consumers/tools.

Apply:

-	// istiocsrResourceWatchLabelName is the value format assigned to istiocsrResourceWatchLabelName label, which
-	// will be of the form <istiocsr_namespace>/<istiocsr_instance-Name>
+	// istiocsrResourceWatchLabelValueFmt is the value format assigned to IstiocsrResourceWatchLabelName label,
+	// of the form <istiocsr_namespace>_<istiocsr_instance-name>.

---

76-83: Polish constant comments (typo/casing).

Improves clarity; no behavior change.

Apply:

-	// istiocsrCAConfigMapName is the name o the configmap which is mounted in istiocsr container, containing the
-	// CA certificate configured in the secret referenced in the issuer.
+	// istiocsrCAConfigMapName is the name of the ConfigMap mounted in the istiocsr container,
+	// containing the CA certificate configured in the secret referenced by the issuer.

test/e2e/istio_csr_test.go (2)

13-13: Prefer GinkgoWriter over log.Printf for test output.

Keeps logs scoped to the spec.

Apply:

-	"log"
+	// prefer GinkgoWriter for test logs

And replace calls:

-			log.Printf("IstioCSR status: %+v", istioCSRStatus)
+			fmt.Fprintf(GinkgoWriter, "IstioCSR status: %+v\n", istioCSRStatus)

Do the same at Line 542.

---

470-496: Failure-path test relies on pollTillIstioCSRAvailable error semantics.

If poll returns transient nil before failing, Consistently may pass falsely. Consider asserting Deployment never becomes Available.

Possible alternative:

• Check DeploymentAvailable condition stays false/absent for the duration.

pkg/controller/istiocsr/deployments.go (2)

539-539: Fix double space in error message format

The error message format string contains "%s resource" which will result in a double space when the object name is empty.

Apply this diff to fix the formatting:

-	return fmt.Errorf("failed to update %s resource with watch label: %w", obj.GetName(), err)
+	return fmt.Errorf("failed to update resource %s with watch label: %w", obj.GetName(), err)

---

335-361: Consider adding validation for issuer-based CA certificates

While user-provided CA certificates undergo thorough PEM validation via validatePEMData, the issuer-based CA path doesn't perform the same validation on the certificate data from the secret.

Apply this diff to add validation:

 func (r *Reconciler) handleIssuerBasedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
 	var (
 		issuerConfig certmanagerv1.IssuerConfig
 	)
 
 	obj, err := r.getIssuer(istiocsr)
 	if err != nil {
 		return FromClientError(err, "failed to fetch issuer")
 	}
 
 	issuerRefKind := strings.ToLower(istiocsr.Spec.IstioCSRConfig.CertManager.IssuerRef.Kind)
 	switch issuerRefKind {
 	case clusterIssuerKind:
 		issuerConfig = obj.(*certmanagerv1.ClusterIssuer).Spec.IssuerConfig
 	case issuerKind:
 		issuerConfig = obj.(*certmanagerv1.Issuer).Spec.IssuerConfig
 	}
 
 	if issuerConfig.CA != nil && issuerConfig.CA.SecretName != "" {
 		if err := r.createCAConfigMapFromIssuerSecret(istiocsr, issuerConfig, resourceLabels); err != nil {
 			return FromClientError(err, "failed to create CA ConfigMap")
 		}
 		updateVolumeWithIssuerCA(deployment)
 	}
 
 	return nil
 }

And in createCAConfigMapFromIssuerSecret:

 func (r *Reconciler) createCAConfigMapFromIssuerSecret(istiocsr *v1alpha1.IstioCSR, issuerConfig certmanagerv1.IssuerConfig, resourceLabels map[string]string) error {
 	if issuerConfig.CA == nil || issuerConfig.CA.SecretName == "" {
 		return nil
 	}
 
 	secretKey := types.NamespacedName{
 		Name:      issuerConfig.CA.SecretName,
 		Namespace: istiocsr.Spec.IstioCSRConfig.Istio.Namespace,
 	}
 	secret := &corev1.Secret{}
 	if err := r.Get(r.ctx, secretKey, secret); err != nil {
 		return fmt.Errorf("failed to fetch secret in issuer: %w", err)
 	}
 	if err := r.updateWatchLabel(secret, istiocsr); err != nil {
 		return err
 	}
 
 	certData := string(secret.Data[IstiocsrCAKeyName])
+	// Validate the certificate data from the issuer secret
+	if err := r.validatePEMData(certData); err != nil {
+		return fmt.Errorf("invalid CA certificate in issuer secret %s/%s: %w", secretKey.Namespace, secretKey.Name, err)
+	}
 	return r.createOrUpdateCAConfigMap(istiocsr, certData, resourceLabels)
 }

pkg/controller/istiocsr/deployments_test.go (1)

450-450: Fix double-space in error message when object name is empty

pkg/controller/istiocsr/deployments.go:539 uses "failed to update %s resource with watch label" — when obj.GetName() is empty this emits two spaces (seen in pkg/controller/istiocsr/deployments_test.go:450). Update the formatting to omit the extra space or conditionally include the name.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

test/e2e/istio_csr_test.go (1)

1-576: Sanity check: focused Ginkgo nodes should be absent.

Past reviews flagged FContext; looks resolved here. Run this to verify repo-wide.

#!/bin/bash
set -euo pipefail
echo "Scanning for focused Ginkgo nodes..."
PATTERN='\b(FDescribe|FContext|FIt|FWhen|FSpecify)\b'
if command -v rg >/dev/null 2>&1; then
  rg -nP "$PATTERN" -g '!**/vendor/**' -S || true
else
  grep -RInE "$PATTERN" --exclude-dir vendor --include '*.go' . || true
fi

pkg/controller/istiocsr/constants.go (3)

68-75: Fix exported-const doc and label value format in comment.

Start the doc with the exported name and avoid “/” in examples since label values cannot contain slashes.

-// istiocsrResourceWatchLabelName is the label name for identifying the resources of interest for the
-// controller but does not create or manage the resource.
+// IstiocsrResourceWatchLabelName is the label used to mark resources of interest
+// that the controller watches but does not manage.
 
-// istiocsrResourceWatchLabelName is the value format assigned to istiocsrResourceWatchLabelName label, which
-// will be of the form <istiocsr_namespace>/<istiocsr_instance-Name>
+// istiocsrResourceWatchLabelValueFmt is the value format assigned to
+// IstiocsrResourceWatchLabelName; it is "<istiocsr_namespace>_<istiocsr_instance_name>".

---

76-79: Fix typos and exported-const doc.

Capitalize ConfigMap and start with the exported name.

-// istiocsrCAConfigMapName is the name o the configmap which is mounted in istiocsr container, containing the
-// CA certificate configured in the secret referenced in the issuer.
+// IstiocsrCAConfigMapName is the name of the ConfigMap mounted in the istiocsr
+// container that contains the CA certificate copied from the issuer or user ConfigMap.

---

80-83: Align comment with exported name and proper noun.

-// istiocsrCAKeyName is the key name holding the CA certificate in the issuer secret or the controller
-// CA configmap.
+// IstiocsrCAKeyName is the key holding the CA certificate in the issuer Secret
+// or the controller-owned CA ConfigMap.

pkg/controller/istiocsr/deployments.go (6)

142-177: Root CA arg is always set; deployment may fail if no CA gets mounted.

If neither user-provided CA nor issuer-based CA resolves, --root-ca-file points to a non-existent path. Either gate the arg or hard-fail when no CA is configured.

Option A (fail fast in issuer path when no CA configured):

 func (r *Reconciler) handleIssuerBasedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
@@
- if issuerConfig.CA != nil && issuerConfig.CA.SecretName != "" {
+ if issuerConfig.CA != nil && issuerConfig.CA.SecretName != "" {
     if err := r.createCAConfigMapFromIssuerSecret(istiocsr, issuerConfig, resourceLabels); err != nil {
       return FromClientError(err, "failed to create CA ConfigMap")
     }
     updateVolumeWithIssuerCA(deployment)
- }
-
- return nil
+   return nil
+ }
+ return NewIrrecoverableError(fmt.Errorf("no CA configured"),
+   "neither spec.istioCSRConfig.certManager.istioCACertificate nor issuer CA.secretName is configured")
 }

Option B (conditionally add arg only when CA is mounted): compute args first, then append --root-ca-file after updateVolumes determines CA presence. That would require moving arg construction or setting an env var/annotation to signal presence.

---

266-282: Short-circuit returns skip issuer path errors.

If user CA is nil, you fall back; if both are empty, you still return nil. Prefer surfacing an irrecoverable error so status reflects misconfiguration (ties to prior comment).

-  if err := r.handleIssuerBasedCA(deployment, istiocsr, resourceLabels); err != nil {
-    return err
-  }
-
-  return nil
+  return r.handleIssuerBasedCA(deployment, istiocsr, resourceLabels)

---

284-333: Validate ConfigMapReference fields early and validate PEM chains.

• Guard against empty name/key for clearer errors.
• You already validate PEM; keep it, but also accept PEM bundles in validatePEMData (see separate comment).

 func (r *Reconciler) handleUserProvidedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
   caCertConfig := istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate
   if caCertConfig == nil {
     return NewIrrecoverableError(fmt.Errorf("CA certificate configuration is nil"), "CA certificate validation failed")
   }
+  if caCertConfig.Name == "" || caCertConfig.Key == "" {
+    return NewIrrecoverableError(fmt.Errorf("invalid CA certificate reference"),
+      "ConfigMap name and key must be non-empty")
+  }
@@
   if _, exists := sourceConfigMap.Data[caCertConfig.Key]; !exists {
     return NewIrrecoverableError(fmt.Errorf("key %q not found in ConfigMap %s/%s", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name), "CA certificate ConfigMap %s/%s does not contain required key %q", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, caCertConfig.Key)
   }
 
   // Validate that the key contains PEM-formatted content
   pemData := sourceConfigMap.Data[caCertConfig.Key]
   if err := r.validatePEMData(pemData); err != nil {
     return NewIrrecoverableError(err, "invalid PEM data in CA certificate ConfigMap %s/%s key %q", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, caCertConfig.Key)
   }

---

363-406: Don’t clobber existing volumes/mounts; merge instead.

Replacing the entire lists risks dropping unrelated mounts from the base manifest.

-  for i, container := range deployment.Spec.Template.Spec.Containers {
+  for i, container := range deployment.Spec.Template.Spec.Containers {
     if container.Name == istiocsrContainerName {
-      deployment.Spec.Template.Spec.Containers[i].VolumeMounts = volumeMounts
+      // ensure mount present without removing others
+      mounts := deployment.Spec.Template.Spec.Containers[i].VolumeMounts
+      present := false
+      for _, m := range mounts {
+        if m.Name == caVolumeName {
+          present = true
+          break
+        }
+      }
+      if !present {
+        mounts = append(mounts, volumeMounts...)
+      }
+      deployment.Spec.Template.Spec.Containers[i].VolumeMounts = mounts
     }
   }
-  deployment.Spec.Template.Spec.Volumes = volumes
+  // ensure volume present without removing others
+  existing := deployment.Spec.Template.Spec.Volumes
+  vPresent := false
+  for _, v := range existing {
+    if v.Name == caVolumeName {
+      vPresent = true
+      break
+    }
+  }
+  if !vPresent {
+    existing = append(existing, volumes...)
+  }
+  deployment.Spec.Template.Spec.Volumes = existing

---

429-448: Validate issuer Secret data and PEM before copying.

Avoid writing empty/invalid data into the ConfigMap.

   if err := r.updateWatchLabel(secret, istiocsr); err != nil {
     return err
   }
 
-  certData := string(secret.Data[IstiocsrCAKeyName])
-  return r.createOrUpdateCAConfigMap(istiocsr, certData, resourceLabels)
+  data, ok := secret.Data[IstiocsrCAKeyName]
+  if !ok || len(data) == 0 {
+    return NewIrrecoverableError(fmt.Errorf("issuer secret missing %q", IstiocsrCAKeyName),
+      "issuer Secret %s/%s does not contain a CA certificate", secret.Namespace, secret.Name)
+  }
+  certData := string(data)
+  if err := r.validatePEMData(certData); err != nil {
+    return NewIrrecoverableError(err, "invalid PEM data in issuer Secret %s/%s key %q", secret.Namespace, secret.Name, IstiocsrCAKeyName)
+  }
+  return r.createOrUpdateCAConfigMap(istiocsr, certData, resourceLabels)

---

493-527: Support PEM bundles (multiple certs) in validation.

Many CA bundles contain multiple certs; iterate over all PEM blocks and require each is a CA certificate.

-func (r *Reconciler) validatePEMData(pemData string) error {
+func (r *Reconciler) validatePEMData(pemData string) error {
   if pemData == "" {
     return fmt.Errorf("PEM data is empty")
   }
 
-  // Parse the first certificate from PEM data
-  block, _ := pem.Decode([]byte(pemData))
-  if block == nil {
-    return fmt.Errorf("no valid PEM data found")
-  }
-
-  if block.Type != "CERTIFICATE" {
-    return fmt.Errorf("PEM block is not a certificate, found: %s", block.Type)
-  }
-
-  cert, err := x509.ParseCertificate(block.Bytes)
-  if err != nil {
-    return fmt.Errorf("failed to parse certificate: %w", err)
-  }
-
-  if !cert.IsCA {
-    return fmt.Errorf("certificate is not a CA certificate")
-  }
-
-  if !cert.BasicConstraintsValid {
-    return fmt.Errorf("certificate does not have valid Basic Constraints extension")
-  }
-
-  // Check Key Usage for certificate signing
-  if cert.KeyUsage&x509.KeyUsageCertSign == 0 {
-    return fmt.Errorf("certificate does not have Certificate Sign key usage")
-  }
+  rest := []byte(pemData)
+  found := false
+  for {
+    block, r := pem.Decode(rest)
+    if block == nil {
+      break
+    }
+    rest = r
+    if block.Type != "CERTIFICATE" {
+      return fmt.Errorf("PEM block is not a certificate, found: %s", block.Type)
+    }
+    cert, err := x509.ParseCertificate(block.Bytes)
+    if err != nil {
+      return fmt.Errorf("failed to parse certificate: %w", err)
+    }
+    found = true
+    if !cert.IsCA || !cert.BasicConstraintsValid || (cert.KeyUsage&x509.KeyUsageCertSign) == 0 {
+      return fmt.Errorf("PEM contains a non-CA certificate or missing cert-sign key usage")
+    }
+  }
+  if !found {
+    return fmt.Errorf("no valid PEM certificate blocks found")
+  }
   return nil
 }

test/e2e/istio_csr_test.go (2)

13-13: Prefer GinkgoWriter over log.Printf for test output.

This integrates with Ginkgo’s reporting and parallel runs.

-   "log"
+   . "github.com/onsi/ginkgo/v2"
@@
-   log.Printf("IstioCSR status: %+v", istioCSRStatus)
+   GinkgoWriter.Printf("IstioCSR status: %+v\n", istioCSRStatus)
@@
-   log.Printf("IstioCSR status: %+v", istioCSRStatus)
+   GinkgoWriter.Printf("IstioCSR status: %+v\n", istioCSRStatus)

---

355-574: Decouple e2e from controller internals.

Importing pkg/controller/istiocsr to fetch constants tightens coupling; consider duplicating string constants in e2e or exposing them via an API/test package to avoid breaking tests on controller refactors.

If you keep the import, please confirm no circular deps and that the package remains stable across branches.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

test/e2e/istio_csr_test.go (2)

481-485: Randomize custom namespace to avoid AlreadyExists flakes.

Use a randomized testing namespace to prevent cross-run collisions if cleanup is skipped.

-      customNamespace, err := loader.CreateTestingNS("custom-ca-ns", false)
+      customNamespace, err := loader.CreateTestingNS("custom-ca-ns", true)

---

22-22: Avoid e2e importing controller internals for constants.

Pull shared constants into a dedicated public package (e.g., pkg/constants) to decouple tests from controller internals. Low priority but improves layering.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)

14-16: Fix template/YAML break: move the conditional off the name: line.

Inline {{ if ... }} after name: makes the YAML invalid (yamllint error at Line 14). Emit the conditional on separate lines so the template renders valid keys.

Apply this diff:

       istioCACertificate:
-        name: {{.ConfigMapName}}{{- if .CustomNamespace}}
-        namespace: {{.CustomNamespace}}{{- end}}
-        key: {{.ConfigMapKey}}
+        name: {{ .ConfigMapName }}
+{{- if .CustomNamespace }}
+        namespace: {{ .CustomNamespace }}
+{{- end }}
+        key: {{ .ConfigMapKey }}

test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)

14-16: Minor template hygiene: add spaces and consider quoting.

Spaces inside delimiters improve readability; quoting values is optional but harmless.

-        name: {{ .ConfigMapName }}
+        name: "{{ .ConfigMapName }}"
 {{- if .CustomNamespace }}
-        namespace: {{ .CustomNamespace }}
+        namespace: "{{ .CustomNamespace }}"
 {{- end }}
-        key: {{ .ConfigMapKey }}
+        key: "{{ .ConfigMapKey }}"

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)

13-16: YAML syntax error: move inline conditional to separate lines.

The inline {{- if .CustomNamespace}} on line 14 renders invalid YAML. This issue was previously raised and remains unresolved.

Apply this diff to fix the syntax error and quote template values:

       istioCACertificate:
-        name: {{.ConfigMapName}}{{- if .CustomNamespace}}
-        namespace: {{.CustomNamespace}}{{- end}}
-        key: {{.ConfigMapKey}}
+        name: "{{ .ConfigMapName }}"
+{{- if .CustomNamespace }}
+        namespace: "{{ .CustomNamespace }}"
+{{- end }}
+        key: "{{ .ConfigMapKey }}"

pkg/controller/istiocsr/test_utils.go (2)

5-9: Missing required import for certificate serial numbers.

The crypto/x509 package requires math/big for generating certificate serial numbers, which is used in the GenerateCertificate function below.

Add the import:

 import (
   "context"
   "crypto/rand"
   "crypto/rsa"
   "crypto/x509"
   "crypto/x509/pkix"
   "encoding/pem"
+  "math/big"
   "fmt"
   "testing"
   "time"

---

274-310: Critical: missing SerialNumber causes panic.

The certificate template lacks a SerialNumber, which causes x509.CreateCertificate to panic at line 298. A cryptographically-random serial number must be set before calling CreateCertificate.

Apply this diff:

   template := &x509.Certificate{
     Subject: pkix.Name{
       CommonName:   commonName,
       Organization: organization,
     },
     NotBefore:             time.Now(),
     NotAfter:              time.Now().Add(24 * time.Hour),
     KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
     ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
     BasicConstraintsValid: true,
     IsCA:                  false,
   }
   
   // Apply tweaks to modify the template
   tweak(template)
   
+  // Set a random serial number
+  serialLimit := new(big.Int).Lsh(big.NewInt(1), 128)
+  serial, err := rand.Int(rand.Reader, serialLimit)
+  if err != nil {
+    panic(fmt.Sprintf("Failed to generate serial number: %v", err))
+  }
+  template.SerialNumber = serial
+  
   certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey)

pkg/controller/istiocsr/deployments.go (1)

313-321: Move watch label update before validation.

Adding the watch label after validation (line 318) means validation failures won't trigger reconciliation when the source ConfigMap changes. Move the watch label update before validation so that subsequent ConfigMap changes will trigger a reconcile even after a validation failure.

Based on past review feedback, apply this reordering:

   // Validate that the key contains PEM-formatted content
   pemData := sourceConfigMap.Data[caCertConfig.Key]
-  if err := r.validatePEMData(pemData); err != nil {
-    return NewIrrecoverableError(err, "invalid PEM data in CA certificate ConfigMap %s/%s key %q", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, caCertConfig.Key)
-  }
   
   // Add watch label to the source ConfigMap for tracking
+  // This is done before validation so that validation failures will trigger reconciliation
+  // when the source ConfigMap is corrected
   if err := r.updateWatchLabel(sourceConfigMap, istiocsr); err != nil {
     return FromClientError(err, "failed to update watch label on CA certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name)
   }
+
+  // Validate that the key contains PEM-formatted content
+  if err := r.validatePEMData(pemData); err != nil {
+    return NewIrrecoverableError(err, "invalid PEM data in CA certificate ConfigMap %s/%s key %q", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name, caCertConfig.Key)
+  }

pkg/controller/istiocsr/deployments.go (2)

284-288: Remove redundant nil check.

The nil check at line 286 is redundant because handleUserProvidedCA is only called from line 269 when istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate != nil.

Apply this diff:

 func (r *Reconciler) handleUserProvidedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error {
   caCertConfig := istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate
-  if caCertConfig == nil {
-    return NewIrrecoverableError(fmt.Errorf("CA certificate configuration is nil"), "CA certificate validation failed")
-  }
-
   // Determine the namespace - use specified namespace or default to IstioCSR namespace
   namespace := caCertConfig.Namespace

---

513-524: Reorder certificate validation checks.

Check BasicConstraintsValid before IsCA for more accurate error reporting. A past review also suggested removing the KeyUsage check as cert-manager will verify it, though retaining it provides defense-in-depth.

Apply this diff:

+  if !cert.BasicConstraintsValid {
+    return fmt.Errorf("certificate does not have valid Basic Constraints extension")
+  }
+
   if !cert.IsCA {
     return fmt.Errorf("certificate is not a CA certificate")
   }
   
-  if !cert.BasicConstraintsValid {
-    return fmt.Errorf("certificate does not have valid Basic Constraints extension")
-  }
-  
   // Check Key Usage for certificate signing
   if cert.KeyUsage&x509.KeyUsageCertSign == 0 {
     return fmt.Errorf("certificate does not have Certificate Sign key usage")
   }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/controller/istiocsr/test_utils.go (2)

3-11: Add missing math/big import for certificate serials.

Required to set x509.Certificate.SerialNumber. This was flagged earlier; appears regressed.

Apply:

 import (
   "context"
   "crypto/rand"
   "crypto/rsa"
   "crypto/x509"
   "crypto/x509/pkix"
   "encoding/pem"
+  "math/big"
   "fmt"
   "testing"
   "time"
 )

---

313-327: Set a random SerialNumber before CreateCertificate.

x509.CreateCertificate returns an error when SerialNumber is nil.

Apply:

   // Apply tweaks to modify the template
   tweak(template)

+  // Set a random serial number (128-bit)
+  serialLimit := new(big.Int).Lsh(big.NewInt(1), 128)
+  serial, err := rand.Int(rand.Reader, serialLimit)
+  if err != nil {
+    panic(fmt.Sprintf("Failed to generate serial number: %v", err))
+  }
+  template.SerialNumber = serial

pkg/controller/istiocsr/test_utils.go (2)

313-315: Defensive: allow nil tweak.

Prevents a panic if callers pass nil.

Apply:

-  // Apply tweaks to modify the template
-  tweak(template)
+  // Apply tweaks to modify the template
+  if tweak != nil {
+    tweak(template)
+  }

---

1-41: Do not ship test utilities in production package.

This file (non-_test.go) imports testing and exposes test-only helpers, adding dead code to the operator binary and coupling e2e to controller internals. Move helpers to a test-only package.

Recommended:

• Rename to pkg/controller/istiocsr/test_utils_test.go (or add //go:build test).
• Move GenerateCertificate and CertificateTweak to test/library (so e2e can import from test/library instead of controller package).
• Keep constants (IstiocsrCAConfigMapName/IstiocsrCAKeyName) in production constants.go and import those in e2e.

If you want, I can draft a follow-up PR plan to relocate these without breaking e2e.

pkg/controller/istiocsr/controller_test.go (1)

47-53: Reduce duplication in test stubs.

The GetCalls branches for Issuer/Secret and ExistsCalls for ConfigMap repeat across cases. Extract small helpers (e.g., setupIssuerAndSecret(m), setupDeploymentAndCAConfigMap(m)) to keep tests terse.

Also applies to: 133-139, 229-235, 61-64, 157-160

test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)

14-18: Quote templated values; keep conditional on its own lines.

Improves robustness with linters/rendering.

Apply:

-        name: {{.ConfigMapName}}
-{{- if .CustomNamespace}}
-        namespace: {{.CustomNamespace}}
-{{- end}}
-        key: {{.ConfigMapKey}}
+        name: "{{ .ConfigMapName }}"
+{{- if .CustomNamespace }}
+        namespace: "{{ .CustomNamespace }}"
+{{- end }}
+        key: "{{ .ConfigMapKey }}"

test/e2e/istio_csr_test.go (2)

17-17: Decouple e2e from controller’s test helpers.

e2e imports pkg/controller/istiocsr for GenerateCertificate and constants. Prefer:

• Keep constants in production constants.go (import from istiocsr).
• Move GenerateCertificate into test/library and import from there in e2e.

This avoids shipping test helpers in prod packages and removes tight coupling with controller internals.

Also applies to: 624-879

---

519-622: Minor: factor CA ConfigMap verifiers.

The three helpers are great; consider moving them to test/library for reuse by other e2e contexts.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting