CM-679: Implements user-configurable CA certificate support for Istio CSR #312
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CM-679 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "cert-manager-1.18" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds opt-in user-provided CA ConfigMap handling, a separate issuer-based CA flow, PEM validation, generic resource watch-label updater, exported constants and ConfigMap watch predicates, refactored CA ConfigMap creation/mounting, and corresponding unit and e2e tests and templates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🔇 Additional comments (8)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 3
🧹 Nitpick comments (9)
pkg/controller/istiocsr/test_utils.go (1)
312-322: Helper OKcreateCertificateConfigMap 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 spaceThe 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 comprehensiveGreat 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
istioCACertificatefield 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
caCertConfigis redundant since this function is only called whenIstioCACertificateis 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
UpdateWithRetryeven 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) }
📜 Review details
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
📒 Files selected for processing (9)
api/operator/v1alpha1/istiocsr_types.go(1 hunks)api/operator/v1alpha1/zz_generated.deepcopy.go(3 hunks)config/crd/bases/operator.openshift.io_istiocsrs.yaml(1 hunks)pkg/controller/istiocsr/deployments.go(7 hunks)pkg/controller/istiocsr/deployments_test.go(2 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig.go(2 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/configmapreference.go(1 hunks)pkg/operator/applyconfigurations/utils.go(1 hunks)
🔇 Additional comments (11)
api/operator/v1alpha1/istiocsr_types.go (1)
149-155: Verify IstioCACertificate live-update behavior
- CRD: istioCACertificate is present in config/crd/bases/operator.openshift.io_istiocsrs.yaml (around lines 1031–1035).
- Controller: field is referenced in pkg/controller/istiocsr/deployments.go (updateVolumes, handleUserProvidedCA) and exercised in pkg/controller/istiocsr/deployments_test.go.
- Action required: confirm whether live updates are intended. If yes, confirm the controller implementation performs all of the following: re-labels/watches the source ConfigMap, atomically copies/updates the mounted ConfigMap (no stale data left on volumes), and triggers a Deployment rollout that doesn't leave stale volumes mounted. If live updates are not intended, make the field create-time only via XValidation (like issuerRef).
- Meta: CI bot flagged Jira target version mismatch for master — set the issue target to 4.21.* to unblock automation.
pkg/operator/applyconfigurations/utils.go (1)
33-35: Mapping for ConfigMapReference apply-configuration looks correctCase entry matches the new nested type; no issues spotted.
pkg/controller/istiocsr/test_utils.go (1)
42-42: Nice: tweakable certificate template for testsThe functional option keeps test cases concise.
api/operator/v1alpha1/zz_generated.deepcopy.go (3)
60-64: Deep-copy for IstioCACertificate pointer added correctlyShallow copy is fine since fields are value types. Regenerated file—no manual edits.
236-250: ConfigMapReference DeepCopy methods presentGeneration looks consistent; nothing further.
337-343: IstioCSRConfig now deep-copies CertManagerPrevents aliasing; good catch.
pkg/controller/istiocsr/deployments_test.go (1)
712-746: Positive CA ConfigMap scenarios are well coveredHappy path tests for same-namespace and custom-namespace CMs look good and exercise label update + mount flows.
Also applies to: 748-779
pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig.go (1)
1-37: LGTM! Well-structured apply configuration.The implementation correctly adds support for the
IstioCACertificatefield with proper builder pattern methods. The generated code follows the established conventions for apply configurations.pkg/operator/applyconfigurations/operator/v1alpha1/configmapreference.go (1)
1-42: LGTM! Clean builder implementation for ConfigMapReference.The generated apply configuration correctly implements the builder pattern for the
ConfigMapReferencetype with all required fields (Name, Namespace, Key).pkg/controller/istiocsr/deployments.go (2)
493-527: Comprehensive PEM and CA certificate validation.Excellent implementation of PEM validation with thorough checks for CA certificate requirements including BasicConstraints and KeyUsage verification.
513-515: Keep IsCA validation — the signing certificate must be a CA.
Istio supports root+intermediate chains, but the certificate that actually signs workload certs (root or intermediate signer) must have CA:TRUE, so rejecting non-CA certificates here is correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
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 CreateCertificateWithout 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)) }
🧹 Nitpick comments (5)
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 = istiocsrResourceWatchLabelNamepkg/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 blockDead 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 valueConfirms 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 scenarioSame 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")
📜 Review details
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
📒 Files selected for processing (5)
pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)test/e2e/istio_csr_test.go(4 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml(1 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml(1 hunks)
🔇 Additional comments (3)
pkg/controller/istiocsr/constants.go (1)
86-96: Expose constants for tests — LGTMExporting these constants unblocks e2e usage without leaking impl details. Names and values align with internal counterparts.
test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml (1)
13-16: Manifest looks correct for ConfigMap-based CA referenceFields name/key align with new API; namespace implicitly defaults to IstioCSR namespace.
test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml (1)
12-15: Covers custom-namespace CA source — LGTMTemplate placeholders make this reusable across tests.
chiragkyal
force-pushed
the
istio-ca-config
branch
2 times, most recently
from
5e4c7c3 to
2ec4c4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/istio_csr_test.go (1)
355-355: Remove focused spec: FContext will break CIFContext 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
🧹 Nitpick comments (4)
test/e2e/istio_csr_test.go (4)
13-13: Consider removing or using the debug log import.The
logpackage 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
GinkgoWriterinstead:-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.Printfshould 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 } }
📜 Review details
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
📒 Files selected for processing (7)
pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/controller.go(2 hunks)pkg/controller/istiocsr/deployments.go(9 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)test/e2e/istio_csr_test.go(3 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml(1 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/istiocsr/constants.go
- test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml
- test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml
- pkg/controller/istiocsr/test_utils.go
🔇 Additional comments (16)
pkg/controller/istiocsr/controller.go (3)
193-193: LGTM! Constant renaming for public visibility.The change from the internal constant to the exported
IstiocsrResourceWatchLabelNameis consistent with making this label name publicly accessible for external use.
199-199: LGTM! Consistent usage of the exported constant.The error message correctly uses the newly exported constant name, maintaining consistency throughout the codebase.
230-230: LGTM! Predicate correctly updated to use exported constant.The watch predicate appropriately uses the exported
IstiocsrResourceWatchLabelNameconstant for filtering resources that the controller should watch.test/e2e/istio_csr_test.go (3)
387-468: Well-structured test for CA certificate from same namespace.The test properly validates:
- CA certificate creation and validation
- IstioCSR resource creation and readiness
- CA data copying to the IstioCSR namespace
- Watch label application
Good test coverage!
470-496: Good negative test case for invalid CA certificate.The test correctly validates that IstioCSR fails to become ready when provided with a non-CA certificate, ensuring proper certificate validation is in place.
498-573: Comprehensive test for cross-namespace CA certificate usage.The test thoroughly validates:
- Custom namespace creation and cleanup
- CA certificate retrieval from a different namespace
- Proper copying of CA data across namespaces
- Watch label application in the source namespace
Excellent coverage of the cross-namespace scenario!
pkg/controller/istiocsr/deployments.go (10)
4-5: LGTM! Necessary imports for certificate validation.The addition of
crypto/x509andencoding/pempackages is appropriate for PEM data parsing and CA certificate validation.
158-158: LGTM! Consistent usage of exported constant.The argument correctly uses the exported
IstiocsrCAKeyNameconstant for the root CA file path.
267-282: Well-structured dual CA provisioning logic.The implementation correctly prioritizes user-provided CA certificates over issuer-based ones, providing a clear fallback mechanism. The error handling properly distinguishes between the two paths.
284-332: Robust user-provided CA handling with comprehensive validation.The function properly:
- Validates ConfigMap existence and key presence
- Performs PEM and CA certificate validation
- Handles namespace resolution (defaults to IstioCSR namespace)
- Applies watch labels for tracking
- Creates/updates the CA ConfigMap copy
Excellent error handling with appropriate use of
NewIrrecoverableErrorfor configuration issues.
493-527: Thorough PEM and CA certificate validation.The validation function comprehensively checks:
- Non-empty PEM data
- Valid PEM block structure
- Certificate type verification
- CA certificate requirements (IsCA, BasicConstraintsValid)
- Certificate signing key usage
This ensures only valid CA certificates are accepted.
385-386: LGTM! Consistent usage of exported constants.The ConfigMap volume source correctly uses the exported constants
IstiocsrCAConfigMapNameandIstiocsrCAKeyName.Also applies to: 389-390
529-542: Generic watch label implementation with proper persistence.The refactored function now accepts any
client.Object, making it reusable for different resource types. The implementation correctly:
- Initializes labels if nil
- Applies the watch label with proper formatting
- Persists changes using
UpdateWithRetryGood abstraction!
450-491: Well-structured CA ConfigMap creation/update logic.The function properly handles both creation and update scenarios with:
- Existence checking
- Change detection using
hasObjectChanged- Appropriate event recording
- Consistent error handling
The implementation ensures the CA ConfigMap is always in the desired state.
335-361: Consistent issuer-based CA handling.The function maintains compatibility with the existing issuer-based CA provisioning while properly integrating with the new unified CA ConfigMap approach.
446-447: LGTM! Consistent usage of exported constants throughout.All references to the CA ConfigMap name and key consistently use the exported constants
IstiocsrCAConfigMapNameandIstiocsrCAKeyName.Also applies to: 453-454, 469-469
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
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
🧹 Nitpick comments (8)
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 == oldSelfpkg/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 logsAnd 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 formatThe 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 certificatesWhile 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 emptypkg/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.
📜 Review details
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
📒 Files selected for processing (15)
api/operator/v1alpha1/istiocsr_types.go(1 hunks)api/operator/v1alpha1/zz_generated.deepcopy.go(3 hunks)bundle/manifests/operator.openshift.io_istiocsrs.yaml(1 hunks)config/crd/bases/operator.openshift.io_istiocsrs.yaml(1 hunks)pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/controller.go(2 hunks)pkg/controller/istiocsr/deployments.go(9 hunks)pkg/controller/istiocsr/deployments_test.go(2 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig.go(2 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/configmapreference.go(1 hunks)pkg/operator/applyconfigurations/utils.go(1 hunks)test/e2e/istio_csr_test.go(3 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml(1 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/operator/applyconfigurations/operator/v1alpha1/configmapreference.go
- test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml
- api/operator/v1alpha1/zz_generated.deepcopy.go
- pkg/controller/istiocsr/test_utils.go
- pkg/operator/applyconfigurations/utils.go
- pkg/controller/istiocsr/controller.go
- api/operator/v1alpha1/istiocsr_types.go
- config/crd/bases/operator.openshift.io_istiocsrs.yaml
🔇 Additional comments (18)
pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig.go (2)
30-36: Fluent setter is consistent.Nil value clears the field; OK for apply configs.
12-14: API surface addition verified — apply-config and wiring present.
- ConfigMapReferenceApplyConfiguration: pkg/operator/applyconfigurations/operator/v1alpha1/configmapreference.go
- CertManagerConfigApplyConfiguration references it (WithIstioCACertificate): pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig.go
- ForKind factory wired: pkg/operator/applyconfigurations/utils.go
- API type/field exist and deepcopy updated: api/operator/v1alpha1/istiocsr_types.go
test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml (1)
1-20: Good coverage for cross-namespace reference.Template placeholders and required fields look correct.
test/e2e/istio_csr_test.go (2)
387-468: Happy path e2e looks sound; asserts check copy + label.Good use of Eventually/intervals and constant reuse.
Please confirm controller sets the watch label key to testutils.IstiocsrResourceWatchLabelName in all copy paths (same/custom namespace).
498-573: Custom-namespace flow is well covered.Data equality and label presence checks are appropriate.
pkg/controller/istiocsr/deployments_test.go (8)
713-746: LGTM! Clean implementation of CA certificate ConfigMap testingThe test case properly validates the successful reconciliation path when a CA certificate ConfigMap is provided. The mock setup correctly simulates the creation flow and the update scenario validates the expected behavior.
748-779: LGTM! Proper handling of CA certificate in custom namespaceThe test correctly validates that CA certificates can be fetched from a custom namespace when specified in the configuration.
781-809: LGTM! Appropriate error handling for missing CA certificateThe test properly validates that a clear error message is returned when the CA certificate ConfigMap is not found.
811-842: LGTM! Validates missing key scenario correctlyThe test ensures that missing keys in the CA certificate ConfigMap are properly detected and reported with a descriptive error message.
844-875: LGTM! Comprehensive PEM validation testingThe test properly validates that invalid PEM data is rejected with an appropriate error message.
878-922: LGTM! Proper error handling for watch label update failuresThe test correctly validates that failures when updating watch labels on CA certificate ConfigMaps are properly handled and reported.
925-954: LGTM! Validates non-CA certificate rejectionThe test ensures that certificates without the CA flag are properly rejected with a clear error message.
957-986: LGTM! Validates certificate signing key usage requirementThe test properly ensures that CA certificates without the KeyUsageCertSign flag are rejected with an appropriate error message.
pkg/controller/istiocsr/deployments.go (5)
267-282: LGTM! Clean dual CA provisioning designThe implementation provides a clear priority system where user-provided CA certificates take precedence over issuer-based ones. The separation of concerns between
handleUserProvidedCAandhandleIssuerBasedCAmakes the code maintainable.
284-332: LGTM! Comprehensive user-provided CA handlingThe function properly validates the CA certificate ConfigMap, including namespace resolution, key existence, PEM validation, and watch label application. The error messages are descriptive and helpful for debugging.
493-527: LGTM! Thorough PEM and CA certificate validationThe validation ensures:
- Valid PEM format
- Certificate type is "CERTIFICATE"
- Certificate has CA flag set
- Valid Basic Constraints extension
- Certificate Sign key usage
This provides robust validation for CA certificates.
529-542: LGTM! Generic resource labeling implementationThe refactored function now accepts any
client.Object, making it reusable for both Secrets and ConfigMaps. This is a good abstraction that reduces code duplication.
450-491: LGTM! Well-structured CA ConfigMap managementThe
createOrUpdateCAConfigMapfunction properly handles both creation and update scenarios, with appropriate event recording and logging. The consolidation of CA ConfigMap management logic reduces code duplication.
chiragkyal
force-pushed
the
istio-ca-config
branch
from
2ec4c4b to
b278e61
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
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
🧹 Nitpick comments (11)
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-filepoints 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-fileafterupdateVolumesdetermines 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/istiocsrto 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.
📜 Review details
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
📒 Files selected for processing (7)
pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/controller.go(2 hunks)pkg/controller/istiocsr/deployments.go(9 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)test/e2e/istio_csr_test.go(3 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml(1 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/testdata/istio/istiocsr_with_ca_certificate.yaml
- test/e2e/testdata/istio/istiocsr_with_ca_certificate_custom_namespace.yaml
- pkg/controller/istiocsr/test_utils.go
- pkg/controller/istiocsr/controller.go
chiragkyal
force-pushed
the
istio-ca-config
branch
from
8ccc7f1 to
6820b96
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
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.
📜 Review details
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
📒 Files selected for processing (2)
test/e2e/istio_csr_test.go(3 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml
[error] 14-14: syntax error: expected , but found '{'
(syntax)
|
/retest |
chiragkyal
force-pushed
the
istio-ca-config
branch
from
6820b96 to
d166849
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)
14-16: Fix template/YAML break: move the conditional off thename:line.Inline
{{ if ... }}aftername: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 }}
🧹 Nitpick comments (1)
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 }}"
📜 Review details
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
📒 Files selected for processing (2)
test/e2e/istio_csr_test.go(3 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/istio_csr_test.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml
[error] 14-14: syntax error: expected , but found '{'
(syntax)
🔇 Additional comments (1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)
1-13: LGTM on structure and field names.Spec shape and indentation look consistent with the CRD expectations.
Also applies to: 17-20
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml
Outdated
Show resolved
Hide resolved
|
/retest |
| caCertConfig := istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate | ||
| if caCertConfig == nil { | ||
| return NewIrrecoverableError(fmt.Errorf("CA certificate configuration is nil"), "CA certificate validation failed") | ||
| } |
There was a problem hiding this comment.
This should not be failed, IstioCACertificate is not mandatory field.
There was a problem hiding this comment.
Removed the if condition check from here, because handleUserProvidedCA function will be called only when caCertConfig is not-nil
|
|
||
| // 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) |
There was a problem hiding this comment.
| 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) | |
| return NewIrrecoverableError(fmt.Errorf("key %q not found in ConfigMap %s/%s", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name), "invalid spec.istioCSRConfig.certManager.istioCACertificate") |
There was a problem hiding this comment.
Updated the message to
return NewIrrecoverableError(fmt.Errorf("key %q not found in ConfigMap %s/%s", caCertConfig.Key, sourceConfigMapKey.Namespace, sourceConfigMapKey.Name), "invalid CA certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name)I think we can avoid keeping the field names (spec.istioCSRConfig.certManager.istioCACertificate) directly into the message
| 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") | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: I don't think we need to check the key usage, which will be verified by the cert-manager. CA is part of the BasicContraints, hence first BasicConstraints check and then IsCA check would be more apt.
| 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") | |
| } | |
| 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") | |
| } |
There was a problem hiding this comment.
nit: I don't think we need to check the key usage, which will be verified by the cert-manager.
True, however I think early validation provides much better UX than runtime failures. And the validation is lightweight and doesn't impact performance and also it's consistent with the other CA validations in this function. So if there are no major concerns, we can keep it as is.
There was a problem hiding this comment.
CA is part of the BasicContraints, hence first BasicConstraints check and then IsCA check would be more apt.
Sure!
|
/cherrypick cert-manager-1.18 |
|
@chiragkyal: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-robot
added
the
needs-rebase
|
/cherrypick cert-manager-1.18 |
|
@bharath-b-rh: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
chiragkyal
force-pushed
the
istio-ca-config
branch
from
d166849 to
83d83dc
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
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) + }
🧹 Nitpick comments (2)
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") }
📜 Review details
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
📒 Files selected for processing (7)
pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/controller.go(2 hunks)pkg/controller/istiocsr/deployments.go(9 hunks)pkg/controller/istiocsr/deployments_test.go(2 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)test/e2e/istio_csr_test.go(3 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/istiocsr/constants.go
- pkg/controller/istiocsr/controller.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml
[error] 14-14: syntax error: expected , but found '{'
(syntax)
🔇 Additional comments (3)
pkg/controller/istiocsr/deployments_test.go (1)
712-986: Comprehensive test coverage for CA ConfigMap scenarios.The new test cases thoroughly validate:
- CA ConfigMap in same and custom namespaces
- Missing ConfigMap and key scenarios
- Invalid PEM data and non-CA certificates
- Certificate validation (CA flag, CertSign key usage)
- Watch label update failures
The test structure and mocking patterns are well-implemented.
test/e2e/istio_csr_test.go (1)
354-551: Well-structured e2e tests for CA ConfigMap provisioning.The new test context validates:
- CA ConfigMap data copying from same and custom namespaces
- Watch label addition to source ConfigMaps
- Non-CA certificate rejection
- Proper cleanup with AfterEach and defer (correctly placed in It blocks)
The tests use Eventually for async operations and validate both ConfigMap data and labels, ensuring the CA provisioning workflow behaves correctly.
pkg/controller/istiocsr/deployments.go (1)
267-282: Clean refactor for CA provisioning paths.The split between user-provided CA (handleUserProvidedCA) and issuer-based CA (handleIssuerBasedCA) is clear and maintainable. The validation flow properly checks ConfigMap existence, key presence, and PEM format before creating the CA ConfigMap copy.
Also applies to: 284-361
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controller/istiocsr/deployments.go (2)
236-245: Wrong value reported in issuerRef.kind error.The error uses the constant
issuerKindinstead of the user-providedissuerRefKind, misleading users.- return NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerKind) + return NewIrrecoverableError(invalidIssuerRefConfigError, "spec.istioCSRConfig.certManager.issuerRef.kind can be anyof `%s` or `%s`, configured: %s", clusterIssuerKind, issuerKind, issuerRefKind)
474-494: Issuer-secret path lacks key presence and PEM validation.If
ca.crtis missing/empty, we silently write an empty CM; consumers will fail unpredictably. Validate presence (and, optionally, PEM) before copying.if err := r.updateWatchLabel(secret, istiocsr); err != nil { return err } - - certData := string(secret.Data[IstiocsrCAKeyName]) - return r.createOrUpdateCAConfigMap(istiocsr, certData, resourceLabels) + raw, ok := secret.Data[IstiocsrCAKeyName] + if !ok || len(raw) == 0 { + return NewIrrecoverableError( + fmt.Errorf("key %q not found or empty in Secret %s/%s", IstiocsrCAKeyName, secretKey.Namespace, secretKey.Name), + "invalid issuer CA secret", + ) + } + certData := string(raw) + // Optional but recommended: early PEM/x509 validation for better UX (consistent with user-provided path). + if err := r.validatePEMData(certData); err != nil { + return NewIrrecoverableError(err, "invalid PEM data in issuer Secret %s/%s key %q", secretKey.Namespace, secretKey.Name, IstiocsrCAKeyName) + } + return r.createOrUpdateCAConfigMap(istiocsr, certData, resourceLabels)
♻️ Duplicate comments (3)
pkg/controller/istiocsr/deployments_test.go (1)
451-451: Fix double space in expected error.Remove the extra space between “update” and “resource”.
- wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to create CA ConfigMap: failed to update resource with watch label: no access`, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to create CA ConfigMap: failed to update resource with watch label: no access`,pkg/controller/istiocsr/deployments.go (2)
297-351: Watch-label before validations and rationale comment look good.Doing the watch-label update first ensures fix-ups trigger reconciliation; the replication rationale is clear.
390-451: Non-destructive CA volume/mount update implemented.Now updates existing entries or appends; addresses prior concern about overwrite.
🧹 Nitpick comments (3)
pkg/controller/istiocsr/constants.go (2)
76-79: Fix typos and clarify scope in CA ConfigMap doc comment.Tighten wording and reflect both user-provided and issuer-secret flows.
- // 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 istio-csr container, containing the + // CA certificate (copied from either a user-provided ConfigMap or the issuer Secret).
80-83: Normalize “ConfigMap/Secret” terminology in comment.Use Kubernetes terms consistently.
- // IstiocsrCAKeyName is the key name holding the CA certificate in the issuer secret or the controller - // CA configmap. + // IstiocsrCAKeyName is the data key name for the CA certificate in the issuer Secret and the + // operator-managed CA ConfigMap.pkg/controller/istiocsr/deployments.go (1)
575-587: Include NamespacedName in watch-label update errors (optional).Improves debuggability and avoids empty-name messages.
- if err := r.UpdateWithRetry(r.ctx, obj); err != nil { - return fmt.Errorf("failed to update %s resource with watch label: %w", obj.GetName(), err) + if err := r.UpdateWithRetry(r.ctx, obj); err != nil { + key := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + return fmt.Errorf("failed to update %s resource with watch label: %w", key.String(), err) }
📜 Review details
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
📒 Files selected for processing (3)
pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/deployments.go(9 hunks)pkg/controller/istiocsr/deployments_test.go(4 hunks)
🔇 Additional comments (3)
pkg/controller/istiocsr/deployments_test.go (2)
713-987: Good coverage for user-provided CA validation paths.New scenarios (missing CM/key, invalid PEM, non-CA, missing CertSign, watch-label failure) are thorough and aligned with behavior.
Once issuer-secret validation is added (see deployments.go comment), please add a test for “issuer secret missing/empty ca.crt” to ensure we surface a clear error.
1096-1211: Volume/mount update behavior verified.Tests correctly assert additive/update semantics for the CA volume and mount.
pkg/controller/istiocsr/deployments.go (1)
266-282: Correct branching between user-provided and issuer-based CA.Opt-in path first, fallback to issuer-based CA; error wrapping is appropriate.
chiragkyal
force-pushed
the
istio-ca-config
branch
from
0cea3bb to
ac704f9
Compare
openshift-merge-robot
removed
the
needs-rebase
chiragkyal
force-pushed
the
istio-ca-config
branch
from
ac704f9 to
88d506e
Compare
chiragkyal
changed the title
IstioCACertificate field to refer configurable CA certificate ConfigMap
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/library/kube_client.go (1)
49-63: Consider adding input validation for parameters.The function signature and REST request construction follow standard Kubernetes client-go patterns. However, for better error messages, consider validating that
podName,namespace, andcontainerNameare non-empty before constructing the request.Example validation:
func ExecInPod(clientset kubernetes.Interface, config *rest.Config, podName, namespace, containerName string, command []string) (string, error) { + if podName == "" || namespace == "" || containerName == "" { + return "", fmt.Errorf("podName, namespace, and containerName must not be empty") + } + req := clientset.CoreV1().RESTClient().Post().
📜 Review details
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
⛔ Files ignored due to path filters (71)
go.sumis excluded by!**/*.sumvendor/github.com/gorilla/websocket/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/tls_handshake.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/tls_handshake_116.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/gorilla/websocket/x_net_proxy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/socks/client.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/internal/socks/socks.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/proxy/dial.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/proxy/direct.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/proxy/per_host.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/proxy/proxy.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/net/proxy/socks5.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/upgrade.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/dial.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/transport.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apimachinery/third_party/forked/golang/netutil/addr.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/errorstream.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/fallback.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/reader.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/remotecommand.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/resize.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/spdy.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/v1.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/v2.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/v3.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/v4.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/v5.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/remotecommand/websocket.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/transport/spdy/spdy.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/transport/websocket/roundtripper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/util/exec/exec.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
go.mod(2 hunks)test/e2e/istio_csr_test.go(9 hunks)test/library/kube_client.go(2 hunks)
🔇 Additional comments (4)
test/library/kube_client.go (2)
7-20: LGTM!The new imports are necessary and appropriately used for the SPDY-based pod command execution functionality.
76-84: LGTM!The error handling properly includes stderr output when available, which aids debugging. Error wrapping with
%wallows proper error inspection by callers.go.mod (1)
130-130: Indirect dependencies have no known vulnerabilities.
All new indirect dependencies are either patched (gorilla/websocket v1.5.3 > v1.4.1) or have no advisories (moby/spdystream v0.5.0, mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f).test/e2e/istio_csr_test.go (1)
478-672: Great end-to-end coverage of the CA ConfigMap flows.The helpers plus the four specs exercise copy, labeling, mount, and update paths (including the negative CA validation) thoroughly. Nice job locking the regression surface down.
|
@chiragkyal: This pull request references CM-679 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "cert-manager-1.18" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
|
@chiragkyal: This pull request references CM-679 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "cert-manager-1.18" instead. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
chiragkyal
force-pushed
the
istio-ca-config
branch
from
30a420a to
1e4ba46
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
chiragkyal
force-pushed
the
istio-ca-config
branch
from
1e4ba46 to
5684141
Compare
Signed-off-by: chiragkyal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controller/istiocsr/controller.go (1)
189-200: Fix logr.Error usage; printf verbs in message won’t interpolate.Use structured fields; don’t embed %s in the message.
- r.log.Error(fmt.Errorf("invalid label format"), "%s label value(%s) not in expected format on %s resource", IstiocsrResourceWatchLabelName, value, obj.GetName()) + r.log.Error(fmt.Errorf("invalid label format"), + "watch label value not in expected format", + "labelKey", IstiocsrResourceWatchLabelName, + "value", value, + "resource", obj.GetName())
🧹 Nitpick comments (5)
test/e2e/istio_csr_test.go (3)
576-582: Avoid Consistently around a polling helper (risk of long/blocking evaluations).pollTillIstioCSRAvailable likely polls internally; wrapping it with Consistently can prolong runs and flake. Prefer a single-shot readiness check or assert on Deployment/Status conditions with Eventually/Consistently on a non-blocking function.
Example pattern:
- Check Ready/Degraded conditions directly with a single GET and assert they remain not-ready/degraded.
- Or use a fast isDeploymentAvailable() (single GET) inside Consistently, instead of a polling helper.
404-416: Also assert watch label value format to catch misconfigurations.You only check presence. Validate the value encodes “_” and matches the expected pair to ensure routing works.
Minimal tweak inside this helper:
- Fetch value := cm.Labels[testutils.IstiocsrResourceWatchLabelName]
- SplitN by “_”, assert len==2, parts[0]==namespace, parts[1]==configMapName.
537-541: Use GinkgoWriter for test logs.Replace log.Printf with fmt.Fprintf(GinkgoWriter, ...) so logs appear only on failures and integrate with Ginkgo output.
Also applies to: 619-622
pkg/controller/istiocsr/controller.go (2)
233-241: Use WatchesMetadata for ConfigMaps to reduce cache/memory overhead.You only need labels/metadata in mapFunc. Align with Secrets to avoid caching full ConfigMap objects.
- controllerConfigMapWatchPredicates := builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}, controllerConfigMapPredicates) + controllerConfigMapWatchPredicates := builder.WithPredicates( + predicate.ResourceVersionChangedPredicate{}, + controllerConfigMapPredicates, + ) - .Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerConfigMapWatchPredicates). + .WatchesMetadata(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerConfigMapWatchPredicates).Also applies to: 245-246, 258-259
144-146: Custom cache informer for all ConfigMaps may be heavy.Registering a cluster-wide ConfigMap informer (without label/namespace scoping) increases memory and watch traffic. If feasible, fetch source ConfigMaps via APIReader (no-cache) or narrow scope (e.g., per-namespace or label-based) to reduce overhead. Keep the metadata watch in the controller for eventing.
If you want, I can draft a follow-up that:
- switches source ConfigMap reads to mgr.GetAPIReader(),
- retains metadata watches for event triggers,
- drops the broad ConfigMap informer from the custom cache.
📜 Review details
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
📒 Files selected for processing (2)
pkg/controller/istiocsr/controller.go(3 hunks)test/e2e/istio_csr_test.go(9 hunks)
🔇 Additional comments (1)
test/e2e/istio_csr_test.go (1)
438-507: Mount verification looks solid.Good coverage: volume presence, ConfigMap name, mount path, and read-only.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)
14-18: Fix yamllint error by indenting template directives and quote values.Indent the
if/endlines to the same level as sibling keys and quote templated values. This resolves the “could not find expected ':'” and improves safety/readability.- name: {{.ConfigMapName}} -{{- if .CustomNamespace}} - namespace: {{.CustomNamespace}} -{{- end}} - key: {{.ConfigMapKey}} + name: "{{ .ConfigMapName }}" + {{- if .CustomNamespace }} + namespace: "{{ .CustomNamespace }}" + {{- end }} + key: "{{ .ConfigMapKey }}"
🧹 Nitpick comments (6)
test/e2e/istio_csr_test.go (6)
379-391: Optional: prefer DeferCleanup in BeforeEach over paired AfterEachUsing AfterEach works, but registering cleanups right after each CreateFromFile with DeferCleanup improves locality and reduces duplication, aligning with the rest of the file.
Also applies to: 393-402
576-582: Avoid Consistently with a blocking polling helperUsing Consistently with pollTillIstioCSRAvailable risks long blocking calls. Check Deployment readiness directly.
Apply:
- By("Verifying that IstioCSR deployment fails due to non-CA certificate") - // The deployment should fail and not become ready due to certificate validation - Consistently(func() error { - _, err := pollTillIstioCSRAvailable(ctx, loader, ns.Name, "default") - return err - }, "30s", "5s").Should(HaveOccurred(), "IstioCSR should fail to become ready due to non-CA certificate") + By("Verifying that IstioCSR deployment fails to become ready due to non-CA certificate") + Consistently(func() bool { + dep, err := clientset.AppsV1().Deployments(ns.Name).Get(ctx, "cert-manager-istio-csr", metav1.GetOptions{}) + if err != nil { + // treat missing deployment as not ready + return true + } + return dep.Status.AvailableReplicas == 0 + }, "30s", "5s").Should(BeTrue(), "Deployment should not become ready due to non-CA certificate")
740-760: Optional: assert mounted file content updates, not just the copied ConfigMapTo fully validate hot‑reload, read the mounted file from the pod (e.g., a short Job or exec) and compare with updatedCert.
I can add a tiny Job to cat /var/run/configmaps/istio-csr/ and assert equality.
539-539: Prefer GinkgoWriter over log.Printf for test logsAttach logs to the spec output and avoid global logger.
Apply:
- log.Printf("IstioCSR status: %+v", istioCSRStatus) + fmt.Fprintf(GinkgoWriter, "IstioCSR status: %+v\n", istioCSRStatus)Repeat for the second usage below.
After replacing, remove the unused log import.
Also applies to: 621-621
13-13: Remove unused log import (after switching to GinkgoWriter)If you adopt GinkgoWriter, drop this import.
- "log"
22-22: Avoid importing production controller package as “testutils”; extract or rename
Tests should not alias the entire controller pkg astestutils. Move shared helpers into a dedicated testutils package (e.g.,test/testutils), or importpkg/controller/istiocsrwith an alias matching its purpose (e.g.,istiocsr).
📜 Review details
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
📒 Files selected for processing (3)
pkg/controller/istiocsr/controller.go(3 hunks)test/e2e/istio_csr_test.go(9 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/istiocsr/controller.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml
[error] 16-16: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (7)
test/e2e/istio_csr_test.go (7)
168-170: Good: deterministic cleanup for IstioCSR YAMLUsing DeferCleanup here is correct and prevents leaks on failures.
188-191: Good: background deletion for JobAppropriate use of propagation to avoid blocking teardown.
241-247: Good: cleanup for custom clusterID resourceLooks correct.
267-270: Good: job cleanup for clusterID testLooks correct.
313-319: Good: cleanup for wrong clusterID resourceLooks correct.
339-342: Good: job cleanup for wrong clusterID testLooks correct.
483-506: Hard-coded container name is safe: the embedded Istio CSR deployment manifests define the container name as cert-manager-istio-csr, so no refactor needed.Likely an incorrect or invalid review comment.
Signed-off-by: chiragkyal <[email protected]>
chiragkyal
force-pushed
the
istio-ca-config
branch
from
5684141 to
ecfbee9
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/controller/istiocsr/test_utils.go (2)
274-310: Set SerialNumber before x509.CreateCertificate to avoid panic/errorx509.CreateCertificate requires a non-nil SerialNumber. Add a random serial.
// 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) if err != nil { panic(fmt.Sprintf("Failed to create certificate: %v", err)) }Note: Generating a new RSA key per call is expensive; consider caching a test-only key to speed tests.
5-12: Missing import for big.IntRequired for certificate serial numbers.
import ( "context" "crypto/rand" "crypto/rsa" "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "math/big" "fmt" "testing" "time" )
🧹 Nitpick comments (3)
pkg/controller/istiocsr/constants.go (2)
72-75: Fix comment: delimiter mismatch with implementationComment says value format uses “namespace/name” but code uses “namespace_name” and controller splits on “_”. Update the comment to avoid confusion.
- // 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 for IstiocsrResourceWatchLabelName. + // Format: <istiocsr_namespace>_<istiocsr_instance-name>
76-83: Minor typos and capitalization in commentsPolish comments for clarity and consistency with K8s object names.
- // 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 from the referenced Issuer/ClusterIssuer. @@ - // 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-managed CA ConfigMap.pkg/controller/istiocsr/controller.go (1)
193-201: Use structured logging; logr doesn’t format %s in messageThe message string won’t interpolate %s with logr. Use key/value pairs.
- r.log.Error(fmt.Errorf("invalid label format"), "%s label value(%s) not in expected format on %s resource", IstiocsrResourceWatchLabelName, value, obj.GetName()) + r.log.Error(fmt.Errorf("invalid watch label format"), + "watch label value not in expected format", + "label", IstiocsrResourceWatchLabelName, "value", value, "resource", obj.GetName())
📜 Review details
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
📒 Files selected for processing (7)
pkg/controller/istiocsr/constants.go(1 hunks)pkg/controller/istiocsr/controller.go(3 hunks)pkg/controller/istiocsr/deployments.go(10 hunks)pkg/controller/istiocsr/deployments_test.go(4 hunks)pkg/controller/istiocsr/test_utils.go(3 hunks)test/e2e/istio_csr_test.go(5 hunks)test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml
[error] 16-16: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (15)
pkg/controller/istiocsr/controller.go (2)
233-240: Predicate correctly accepts managed or watched ConfigMapsGood: predicate allows both operator-managed and watched ConfigMaps to trigger reconcile.
247-261: ConfigMap watch wiring looks correctAdds ConfigMap watch with tailored predicates; aligns with new CA ConfigMap flow.
test/e2e/istio_csr_test.go (1)
545-647: Nice: helper functions make the checks robust and readableGood use of Eventually/Consistently and explicit validations for labels, copy fidelity, and mounts.
pkg/controller/istiocsr/test_utils.go (1)
231-241: Key name usage is consistent with exported constantUsing IstiocsrCAKeyName in test-config data aligns with production code.
test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml (1)
14-18: Quote template values and keep the conditional on its own linesReplace the block under
istioCACertificate:with:istioCACertificate: - name: {{.ConfigMapName}} -{{- if .CustomNamespace}} - namespace: {{.CustomNamespace}} -{{- end}} - key: {{.ConfigMapKey}} + name: "{{ .ConfigMapName }}" +{{- if .CustomNamespace }} + namespace: "{{ .CustomNamespace }}" +{{- end }} + key: "{{ .ConfigMapKey }}"Run
yamllint test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yamlto confirm the parse error is resolved.pkg/controller/istiocsr/deployments_test.go (3)
6-6: LGTM!The addition of the
reflectpackage is appropriate for deep equality checks in the new test functionTestUpdateVolumeWithIssuerCA.
714-988: Excellent test coverage for CA certificate ConfigMap handling.The eight new test cases comprehensively cover:
- ✓ Success paths (default and custom namespace)
- ✓ Error scenarios (missing ConfigMap, missing key, invalid PEM data)
- ✓ Watch label update failures
- ✓ Certificate validation (non-CA, missing Certificate Sign key usage)
The test structure is clean, error messages are validated, and the mocking setup is appropriate for each scenario.
1123-1238: Well-structured test for volume mounting logic.The
TestUpdateVolumeWithIssuerCAfunction effectively validates:
- Adding CA volume when no volumes exist
- Preserving existing volumes while adding CA volume
- Updating CA volume while preserving other volumes
Using
reflect.DeepEqualfor verification is appropriate here as it ensures exact structural equality of the volume and mount configurations.pkg/controller/istiocsr/deployments.go (7)
4-5: LGTM!The new imports support the CA certificate validation and type handling requirements:
crypto/x509andencoding/pemfor certificate parsing and validationtypesforNamespacedNamein the user-provided CA flowAlso applies to: 16-16
267-282: Clean separation of CA certificate handling flows.The updated
updateVolumesfunction clearly distinguishes between user-provided CA certificates and issuer-based CA certificates with a logical fallback pattern. The early return when user-provided CA is configured prevents unnecessary issuer checks.
284-351: Well-documented and comprehensive user-provided CA handling.The
handleUserProvidedCAfunction correctly:
- Resolves namespace defaulting
- Validates ConfigMap existence with clear error messages
- Adds watch label before validation (enabling reconciliation on fixes)
- Validates key existence and PEM data
- Creates managed copy with detailed rationale in comments
The design choice to create a managed copy rather than directly mounting the user-provided ConfigMap is sound, as it enables validation before propagation and automatic reconciliation if users modify the managed copy.
353-380: Good refactoring of issuer-based CA flow.Extracting the issuer-based CA logic into a separate function improves code organization and makes the distinction between user-provided and issuer-based flows clearer.
382-451: Improved volume mounting logic with append/update strategy.The refactored
updateVolumeWithIssuerCAfunction now correctly:
- Updates existing CA volume/mount if present
- Appends new CA volume/mount if not present
- Preserves other volumes and mounts
This approach is more robust than wholesale replacement and prevents accidental deletion of user-configured volumes.
538-572: Thorough PEM data validation with appropriate checks.The
validatePEMDatafunction validates:
- ✓ PEM data is not empty
- ✓ Valid PEM block exists
- ✓ Block type is CERTIFICATE
- ✓ Certificate parses successfully
- ✓ BasicConstraints extension is valid
- ✓ IsCA flag is set
- ✓ Certificate Sign key usage is present
Note: The function validates only the first certificate in the PEM data. If a certificate chain is provided, subsequent certificates are not validated. This appears intentional since only the root CA certificate is needed.
574-587: Good generalization of watch label update logic.The updated
updateWatchLabelfunction now accepts anyclient.Objectinstead of just*corev1.Secret, making it reusable for both ConfigMaps and Secrets. The implementation correctly handles nil labels and updates the object.
Signed-off-by: chiragkyal <[email protected]>
|
/retest |
|
@chiragkyal: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary
This PR adds support for user-configurable Istio CA certificates by utilising the
istioCACertificatefield. Users can now reference a ConfigMap containing their CA certificate.Description
cert-manager-istio-csr-issuer-ca-copy) in the IstioCSR namespace/var/run/configmaps/istio-csristioCACertificateis not provided, the operator instead of looking in the configuredIssuer/ClusterIssuer, will instead look in the secret containing the certificates obtained from cert-manager for istiod server.EP: openshift/enhancements#1837