From eb63a33966391d17d77ccf419dc7fcfad0b660a7 Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Wed, 8 Oct 2025 20:59:25 +0530 Subject: [PATCH 1/3] feat: user-configurable istio-ca certificate Signed-off-by: chiragkyal --- pkg/controller/istiocsr/constants.go | 12 +- pkg/controller/istiocsr/controller.go | 6 +- pkg/controller/istiocsr/deployments.go | 233 ++++++++++-- pkg/controller/istiocsr/deployments_test.go | 395 +++++++++++++++++++- pkg/controller/istiocsr/test_utils.go | 90 ++++- 5 files changed, 690 insertions(+), 46 deletions(-) diff --git a/pkg/controller/istiocsr/constants.go b/pkg/controller/istiocsr/constants.go index 8e6ae744c..d4dd2b510 100644 --- a/pkg/controller/istiocsr/constants.go +++ b/pkg/controller/istiocsr/constants.go @@ -65,21 +65,21 @@ const ( // created in other namespaces by the controller. istiocsrNamespaceMappingLabelName = "cert-manager-istio-csr-namespace" - // istiocsrResourceWatchLabelName is the label name for identifying the resources of interest for the + // IstiocsrResourceWatchLabelName is the label name for identifying the resources of interest for the // controller but does not create or manage the resource. - istiocsrResourceWatchLabelName = "istiocsr.openshift.operator.io/watched-by" + IstiocsrResourceWatchLabelName = "istiocsr.openshift.operator.io/watched-by" // istiocsrResourceWatchLabelName is the value format assigned to istiocsrResourceWatchLabelName label, which // will be of the form / istiocsrResourceWatchLabelValueFmt = "%s_%s" - // istiocsrCAConfigMapName is the name o the configmap which is mounted in istiocsr container, containing the + // 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 = istiocsrCommonName + "-issuer-ca-copy" + IstiocsrCAConfigMapName = istiocsrCommonName + "-issuer-ca-copy" - // istiocsrCAKeyName is the key name holding the CA certificate in the issuer secret or the controller + // IstiocsrCAKeyName is the key name holding the CA certificate in the issuer secret or the controller // CA configmap. - istiocsrCAKeyName = "ca.crt" + IstiocsrCAKeyName = "ca.crt" ) var ( diff --git a/pkg/controller/istiocsr/controller.go b/pkg/controller/istiocsr/controller.go index 5099d166e..7bf013b7f 100644 --- a/pkg/controller/istiocsr/controller.go +++ b/pkg/controller/istiocsr/controller.go @@ -190,13 +190,13 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { if objLabels[requestEnqueueLabelKey] == requestEnqueueLabelValue { return true } - value := objLabels[istiocsrResourceWatchLabelName] + value := objLabels[IstiocsrResourceWatchLabelName] if value == "" { return false } key := strings.Split(value, "_") if len(key) != 2 { - 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"), "%s label value(%s) not in expected format on %s resource", IstiocsrResourceWatchLabelName, value, obj.GetName()) return false } namespace = key[0] @@ -227,7 +227,7 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // predicate function to filter events for objects which controller is interested in, but // not managed or created by controller. controllerWatchResources := predicate.NewPredicateFuncs(func(object client.Object) bool { - return object.GetLabels() != nil && object.GetLabels()[istiocsrResourceWatchLabelName] != "" + return object.GetLabels() != nil && object.GetLabels()[IstiocsrResourceWatchLabelName] != "" }) withIgnoreStatusUpdatePredicates := builder.WithPredicates(predicate.GenerationChangedPredicate{}, controllerManagedResources) diff --git a/pkg/controller/istiocsr/deployments.go b/pkg/controller/istiocsr/deployments.go index 6d0c997c3..bbd8e9d3c 100644 --- a/pkg/controller/istiocsr/deployments.go +++ b/pkg/controller/istiocsr/deployments.go @@ -1,6 +1,8 @@ package istiocsr import ( + "crypto/x509" + "encoding/pem" "fmt" "os" "reflect" @@ -11,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/apis/core" corevalidation "k8s.io/kubernetes/pkg/apis/core/validation" @@ -147,7 +150,7 @@ func updateArgList(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR) { fmt.Sprintf("--issuer-name=%s", istiocsrConfigs.CertManager.IssuerRef.Name), fmt.Sprintf("--issuer-kind=%s", istiocsrConfigs.CertManager.IssuerRef.Kind), fmt.Sprintf("--issuer-group=%s", istiocsrConfigs.CertManager.IssuerRef.Group), - fmt.Sprintf("--root-ca-file=%s/%s", caVolumeMountPath, istiocsrCAKeyName), + fmt.Sprintf("--root-ca-file=%s/%s", caVolumeMountPath, IstiocsrCAKeyName), fmt.Sprintf("--serving-certificate-dns-names=cert-manager-istio-csr.%s.svc", istiocsr.GetNamespace()), fmt.Sprintf("--serving-certificate-duration=%.0fm", istiocsrConfigs.IstiodTLSConfig.CertificateDuration.Minutes()), fmt.Sprintf("--trust-domain=%s", istiocsrConfigs.IstiodTLSConfig.TrustDomain), @@ -261,6 +264,94 @@ func (r *Reconciler) assertIssuerRefExists(istiocsr *v1alpha1.IstioCSR) error { } 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 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 nil +} + +// handleUserProvidedCA processes user-provided CA certificate configuration. +// +// It performs the following operations: +// 1. Validates that the source ConfigMap exists +// 2. Adds a watch label to the source ConfigMap for change tracking +// 3. Validates that the ConfigMap contains the specified key and is a valid CA certificate +// 4. Creates or updates a copy of the ConfigMap in the IstioCSR namespace +// 5. Configures the deployment to mount the CA certificate volume +// +// The caller must ensure that `istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate` +// is not nil before calling this function to avoid a panic. +// +// Returns an error if any validation fails or if ConfigMap operations fail. +func (r *Reconciler) handleUserProvidedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { + caCertConfig := istiocsr.Spec.IstioCSRConfig.CertManager.IstioCACertificate + + // Determine the namespace - use specified namespace or default to IstioCSR namespace + namespace := caCertConfig.Namespace + if namespace == "" { + namespace = istiocsr.GetNamespace() + } + + // Validate that the source ConfigMap exists + sourceConfigMapKey := types.NamespacedName{ + Name: caCertConfig.Name, + Namespace: namespace, + } + + sourceConfigMap := &corev1.ConfigMap{} + if err := r.Get(r.ctx, sourceConfigMapKey, sourceConfigMap); err != nil { + return NewIrrecoverableError(err, "failed to fetch CA certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name) + } + + // Add watch label to the source ConfigMap to trigger reconciliation on changes. + // This is done before validation so that if validation fails now, fixing the ConfigMap + // will trigger reconciliation. + 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 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), "invalid CA certificate ConfigMap %s/%s", sourceConfigMapKey.Namespace, sourceConfigMapKey.Name) + } + + // 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) + } + + // Create a managed copy of the ConfigMap in the IstioCSR namespace. + // The operator does not directly mount the user-provided ConfigMap but creates a managed copy + // (cert-manager-istio-csr-issuer-ca-copy) in the IstioCSR namespace. This approach enables the + // operator to validate any changes to the source ConfigMap before propagating them to the istio-csr + // agent, as ConfigMap changes automatically propagate to mounted pods. The watch label on the source + // ConfigMap triggers reconciliation when modified, allowing the operator to re-validate and update + // its managed copy. Additionally, if a user directly modifies the operator-managed copy, it will be + // reconciled back to the desired state derived from the validated source ConfigMap. + if err := r.createOrUpdateCAConfigMap(istiocsr, pemData, resourceLabels); err != nil { + return FromClientError(err, "failed to create CA certificate ConfigMap copy") + } + + // Mount the copied CA certificate ConfigMap (always uses the standard name) + updateVolumeWithIssuerCA(deployment) + + return nil +} + +// handleIssuerBasedCA handles the creation of CA ConfigMap from issuer secret and volume mounting +func (r *Reconciler) handleIssuerBasedCA(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioCSR, resourceLabels map[string]string) error { var ( issuerConfig certmanagerv1.IssuerConfig ) @@ -279,7 +370,7 @@ func (r *Reconciler) updateVolumes(deployment *appsv1.Deployment, istiocsr *v1al } if issuerConfig.CA != nil && issuerConfig.CA.SecretName != "" { - if err := r.createCAConfigMap(istiocsr, issuerConfig, resourceLabels); err != nil { + if err := r.createCAConfigMapFromIssuerSecret(istiocsr, issuerConfig, resourceLabels); err != nil { return FromClientError(err, "failed to create CA ConfigMap") } updateVolumeWithIssuerCA(deployment) @@ -296,41 +387,67 @@ func updateVolumeWithIssuerCA(deployment *appsv1.Deployment) { defaultMode = int32(420) ) - volumeMounts := []corev1.VolumeMount{ - { - Name: caVolumeName, - MountPath: caVolumeMountPath, - ReadOnly: true, - }, + desiredVolumeMount := corev1.VolumeMount{ + Name: caVolumeName, + MountPath: caVolumeMountPath, + ReadOnly: true, } - volumes := []corev1.Volume{ - { - Name: caVolumeName, - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: istiocsrCAConfigMapName, - }, - Items: []corev1.KeyToPath{ - { - Key: istiocsrCAKeyName, - Path: istiocsrCAKeyName, - Mode: &defaultMode, - }, + desiredVolume := corev1.Volume{ + Name: caVolumeName, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: IstiocsrCAConfigMapName, + }, + Items: []corev1.KeyToPath{ + { + Key: IstiocsrCAKeyName, + Path: IstiocsrCAKeyName, + Mode: &defaultMode, }, - DefaultMode: &defaultMode, }, + DefaultMode: &defaultMode, }, }, } + // Update or append volume mount in the istio-csr container for i, container := range deployment.Spec.Template.Spec.Containers { if container.Name == istiocsrContainerName { - deployment.Spec.Template.Spec.Containers[i].VolumeMounts = volumeMounts + volumeMountExists := false + for j, vm := range container.VolumeMounts { + if vm.Name == caVolumeName { + deployment.Spec.Template.Spec.Containers[i].VolumeMounts[j] = desiredVolumeMount + volumeMountExists = true + break + } + } + if !volumeMountExists { + deployment.Spec.Template.Spec.Containers[i].VolumeMounts = append( + deployment.Spec.Template.Spec.Containers[i].VolumeMounts, + desiredVolumeMount, + ) + } + break + } + } + + // Update or append volume in the deployment + volumeExists := false + for i, vol := range deployment.Spec.Template.Spec.Volumes { + if vol.Name == caVolumeName { + deployment.Spec.Template.Spec.Volumes[i] = desiredVolume + volumeExists = true + break } } - deployment.Spec.Template.Spec.Volumes = volumes + if !volumeExists { + deployment.Spec.Template.Spec.Volumes = append( + deployment.Spec.Template.Spec.Volumes, + desiredVolume, + ) + } } func (r *Reconciler) getIssuer(istiocsr *v1alpha1.IstioCSR) (client.Object, error) { @@ -354,7 +471,7 @@ func (r *Reconciler) getIssuer(istiocsr *v1alpha1.IstioCSR) (client.Object, erro return object, nil } -func (r *Reconciler) createCAConfigMap(istiocsr *v1alpha1.IstioCSR, issuerConfig certmanagerv1.IssuerConfig, resourceLabels map[string]string) error { +func (r *Reconciler) createCAConfigMapFromIssuerSecret(istiocsr *v1alpha1.IstioCSR, issuerConfig certmanagerv1.IssuerConfig, resourceLabels map[string]string) error { if issuerConfig.CA == nil || issuerConfig.CA.SecretName == "" { return nil } @@ -367,12 +484,18 @@ func (r *Reconciler) createCAConfigMap(istiocsr *v1alpha1.IstioCSR, issuerConfig if err := r.Get(r.ctx, secretKey, secret); err != nil { return fmt.Errorf("failed to fetch secret in issuer: %w", err) } - if err := r.updateWatchLabelOnSecret(secret, istiocsr); err != nil { + if err := r.updateWatchLabel(secret, istiocsr); err != nil { return err } + certData := string(secret.Data[IstiocsrCAKeyName]) + return r.createOrUpdateCAConfigMap(istiocsr, certData, resourceLabels) +} + +// createOrUpdateCAConfigMap creates or updates the CA ConfigMap with the provided certificate data +func (r *Reconciler) createOrUpdateCAConfigMap(istiocsr *v1alpha1.IstioCSR, certData string, resourceLabels map[string]string) error { configmapKey := client.ObjectKey{ - Name: istiocsrCAConfigMapName, + Name: IstiocsrCAConfigMapName, Namespace: istiocsr.GetNamespace(), } fetched := &corev1.ConfigMap{} @@ -388,9 +511,10 @@ func (r *Reconciler) createCAConfigMap(istiocsr *v1alpha1.IstioCSR, issuerConfig Labels: resourceLabels, }, Data: map[string]string{ - istiocsrCAKeyName: string(secret.Data[istiocsrCAKeyName]), + IstiocsrCAKeyName: certData, }, } + if exist && hasObjectChanged(desired, fetched) { r.log.V(1).Info("ca configmap need update", "name", configmapKey) if err := r.UpdateWithRetry(r.ctx, desired); err != nil { @@ -400,25 +524,64 @@ func (r *Reconciler) createCAConfigMap(istiocsr *v1alpha1.IstioCSR, issuerConfig } else { r.log.V(4).Info("configmap resource already exists and is in expected state", "name", configmapKey) } + if !exist { if err := r.Create(r.ctx, desired); err != nil { return fmt.Errorf("failed to create %s configmap resource: %w", configmapKey, err) } r.eventRecorder.Eventf(istiocsr, corev1.EventTypeNormal, "Reconciled", "configmap resource %s created", configmapKey) } + + return nil +} + +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.BasicConstraintsValid { + return fmt.Errorf("certificate does not have valid Basic Constraints extension") + } + + if !cert.IsCA { + return fmt.Errorf("certificate is not a CA certificate") + } + + // Check Key Usage for certificate signing + if cert.KeyUsage&x509.KeyUsageCertSign == 0 { + return fmt.Errorf("certificate does not have Certificate Sign key usage") + } + return nil } -func (r *Reconciler) updateWatchLabelOnSecret(secret *corev1.Secret, istiocsr *v1alpha1.IstioCSR) error { - labels := secret.GetLabels() +// updateWatchLabel adds a watch label to any Kubernetes object that supports labels +func (r *Reconciler) updateWatchLabel(obj client.Object, istiocsr *v1alpha1.IstioCSR) error { + labels := obj.GetLabels() if labels == nil { labels = make(map[string]string) } - labels[istiocsrResourceWatchLabelName] = fmt.Sprintf(istiocsrResourceWatchLabelValueFmt, istiocsr.GetNamespace(), istiocsr.GetName()) - secret.SetLabels(labels) + labels[IstiocsrResourceWatchLabelName] = fmt.Sprintf(istiocsrResourceWatchLabelValueFmt, istiocsr.GetNamespace(), istiocsr.GetName()) + obj.SetLabels(labels) - if err := r.UpdateWithRetry(r.ctx, secret); err != nil { - return fmt.Errorf("failed to update %s secret with custom watch label: %w", secret.GetName(), err) + 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 } diff --git a/pkg/controller/istiocsr/deployments_test.go b/pkg/controller/istiocsr/deployments_test.go index 0eaba416d..8f4ae5da2 100644 --- a/pkg/controller/istiocsr/deployments_test.go +++ b/pkg/controller/istiocsr/deployments_test.go @@ -3,6 +3,7 @@ package istiocsr import ( "context" "fmt" + "reflect" "slices" "strings" "testing" @@ -448,7 +449,7 @@ func TestCreateOrApplyDeployments(t *testing.T) { return nil }) }, - 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 secret with custom 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`, }, { name: "deployment reconciliation fails while checking configmap exists", @@ -710,6 +711,281 @@ func TestCreateOrApplyDeployments(t *testing.T) { }, wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update resource requirements: [spec.istioCSRConfig.resources.requests[test]: Invalid value: test: must be a standard resource type or fully qualified, spec.istioCSRConfig.resources.requests[test]: Invalid value: test: must be a standard resource for containers]`, }, + { + name: "deployment reconciliation successful with CA certificate ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + case *corev1.ConfigMap: + // Return false for the copied ConfigMap to simulate creation + return false, nil + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "ca-cert-test" { + configMap := testCACertificateConfigMap() + configMap.DeepCopyInto(o) + } + } + return nil + }) + m.UpdateWithRetryReturns(nil) + m.CreateReturns(nil) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "ca-cert-test", + Key: "ca-cert.pem", + } + }, + }, + { + name: "deployment reconciliation successful with CA certificate ConfigMap in custom namespace", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "ca-cert-test" && ns.Namespace == "custom-namespace" { + configMap := testCACertificateConfigMap() + configMap.Namespace = "custom-namespace" + configMap.DeepCopyInto(o) + } + } + return nil + }) + m.UpdateWithRetryReturns(nil) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "ca-cert-test", + Namespace: "custom-namespace", + Key: "ca-cert.pem", + } + }, + }, + { + name: "deployment reconciliation fails with missing CA certificate ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "ca-cert-test" { + return apierrors.NewNotFound(corev1.Resource("configmaps"), "ca-cert-test") + } + } + return nil + }) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "ca-cert-test", + Key: "ca-cert.pem", + } + }, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to validate and mount CA certificate ConfigMap: failed to fetch CA certificate ConfigMap istiocsr-test-ns/ca-cert-test: configmaps "ca-cert-test" not found`, + }, + { + name: "deployment reconciliation fails with missing key in CA certificate ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "ca-cert-test" { + configMap := testCACertificateConfigMap() + // Remove the expected key + delete(configMap.Data, "ca-cert.pem") + configMap.DeepCopyInto(o) + } + } + return nil + }) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "ca-cert-test", + Key: "ca-cert.pem", + } + }, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to validate and mount CA certificate ConfigMap: invalid CA certificate ConfigMap istiocsr-test-ns/ca-cert-test: key "ca-cert.pem" not found in ConfigMap istiocsr-test-ns/ca-cert-test`, + }, + { + name: "deployment reconciliation fails with invalid PEM data in CA certificate ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "ca-cert-test" { + configMap := testCACertificateConfigMap() + // Set invalid PEM data + configMap.Data["ca-cert.pem"] = "invalid-pem-data" + configMap.DeepCopyInto(o) + } + } + return nil + }) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "ca-cert-test", + Key: "ca-cert.pem", + } + }, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to validate and mount CA certificate ConfigMap: invalid PEM data in CA certificate ConfigMap istiocsr-test-ns/ca-cert-test key "ca-cert.pem": no valid PEM data found`, + }, + + { + name: "deployment reconciliation fails while updating watch label on CA certificate ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + case *corev1.ConfigMap: + // Return false for the copied ConfigMap to simulate creation + return false, nil + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "watch-label-fail-test" { + configMap := testCACertificateConfigMap() + configMap.Name = "watch-label-fail-test" + configMap.DeepCopyInto(o) + } + } + return nil + }) + m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, _ ...client.UpdateOption) error { + switch obj.(type) { + case *corev1.ConfigMap: + // Fail when trying to update the source ConfigMap with watch label + if obj.GetName() == "watch-label-fail-test" { + return apierrors.NewUnauthorized("no access to update watch label") + } + } + return nil + }) + m.CreateReturns(nil) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "watch-label-fail-test", + Key: "ca-cert.pem", + } + }, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to validate and mount CA certificate ConfigMap: failed to update watch label on CA certificate ConfigMap istiocsr-test-ns/watch-label-fail-test: failed to update watch-label-fail-test resource with watch label: no access to update watch label`, + }, + + { + name: "deployment reconciliation fails with non-CA certificate in ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "non-ca-cert-test" { + configMap := testNonCACertificateConfigMap() + configMap.DeepCopyInto(o) + } + } + return nil + }) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "non-ca-cert-test", + Key: "ca-cert.pem", + } + }, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to validate and mount CA certificate ConfigMap: invalid PEM data in CA certificate ConfigMap istiocsr-test-ns/non-ca-cert-test key "ca-cert.pem": certificate is not a CA certificate`, + }, + + { + name: "deployment reconciliation fails with certificate missing KeyUsageCertSign in ConfigMap", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *appsv1.Deployment: + deployment := testDeployment() + deployment.DeepCopyInto(o) + } + return true, nil + }) + m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { + switch o := obj.(type) { + case *corev1.ConfigMap: + if ns.Name == "ca-without-certsign-test" { + configMap := testCertificateWithoutCertSignConfigMap() + configMap.DeepCopyInto(o) + } + } + return nil + }) + }, + updateIstioCSR: func(i *v1alpha1.IstioCSR) { + i.Status.IstioCSRImage = image + i.Spec.IstioCSRConfig.CertManager.IstioCACertificate = &v1alpha1.ConfigMapReference{ + Name: "ca-without-certsign-test", + Key: "ca-cert.pem", + } + }, + wantErr: `failed to generate deployment resource for creation in istiocsr-test-ns: failed to update volume istiocsr-test-ns/istiocsr-test-resource: failed to validate and mount CA certificate ConfigMap: invalid PEM data in CA certificate ConfigMap istiocsr-test-ns/ca-without-certsign-test key "ca-cert.pem": certificate does not have Certificate Sign key usage`, + }, } for _, tt := range tests { @@ -843,3 +1119,120 @@ func TestUpdateArgList(t *testing.T) { func containsArg(args []string, targetArg string) bool { return slices.Contains(args, targetArg) } + +func TestUpdateVolumeWithIssuerCA(t *testing.T) { + defaultMode := int32(420) + expectedCAVolume := corev1.Volume{ + Name: "root-ca", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "cert-manager-istio-csr-issuer-ca-copy", + }, + Items: []corev1.KeyToPath{ + { + Key: "ca.crt", + Path: "ca.crt", + Mode: &defaultMode, + }, + }, + DefaultMode: &defaultMode, + }, + }, + } + expectedCAMount := corev1.VolumeMount{ + Name: "root-ca", + MountPath: "/var/run/configmaps/istio-csr", + ReadOnly: true, + } + + tests := []struct { + name string + existingVolumes []corev1.Volume + existingVolumeMounts []corev1.VolumeMount + expectedVolumes []corev1.Volume + expectedMounts []corev1.VolumeMount + }{ + { + name: "add CA volume when no volumes exist", + expectedVolumes: []corev1.Volume{expectedCAVolume}, + expectedMounts: []corev1.VolumeMount{expectedCAMount}, + }, + { + name: "add CA volume preserving other volumes", + existingVolumes: []corev1.Volume{ + {Name: "other-volume", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + }, + existingVolumeMounts: []corev1.VolumeMount{ + {Name: "other-volume", MountPath: "/other"}, + }, + expectedVolumes: []corev1.Volume{ + {Name: "other-volume", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + expectedCAVolume, + }, + expectedMounts: []corev1.VolumeMount{ + {Name: "other-volume", MountPath: "/other"}, + expectedCAMount, + }, + }, + { + name: "update CA volume preserving others", + existingVolumes: []corev1.Volume{ + {Name: "volume-1", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + {Name: "root-ca", VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: "old-configmap"}, + }, + }}, + }, + existingVolumeMounts: []corev1.VolumeMount{ + {Name: "volume-1", MountPath: "/path1"}, + {Name: "root-ca", MountPath: "/old/path"}, + }, + expectedVolumes: []corev1.Volume{ + {Name: "volume-1", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + expectedCAVolume, + }, + expectedMounts: []corev1.VolumeMount{ + {Name: "volume-1", MountPath: "/path1"}, + expectedCAMount, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deployment := testDeployment() + deployment.Spec.Template.Spec.Volumes = tt.existingVolumes + + // Set existing volume mounts on istio-csr container + for i, container := range deployment.Spec.Template.Spec.Containers { + if container.Name == istiocsrContainerName { + deployment.Spec.Template.Spec.Containers[i].VolumeMounts = tt.existingVolumeMounts + break + } + } + + updateVolumeWithIssuerCA(deployment) + + // Verify volumes match expected + if !reflect.DeepEqual(deployment.Spec.Template.Spec.Volumes, tt.expectedVolumes) { + t.Errorf("volumes mismatch:\ngot: %+v\nwant: %+v", deployment.Spec.Template.Spec.Volumes, tt.expectedVolumes) + } + + // Find istio-csr container and verify mounts + var containerMounts []corev1.VolumeMount + for _, container := range deployment.Spec.Template.Spec.Containers { + if container.Name == istiocsrContainerName { + containerMounts = container.VolumeMounts + break + } + } + + // Verify mounts match expected + if !reflect.DeepEqual(containerMounts, tt.expectedMounts) { + t.Errorf("volume mounts mismatch:\ngot: %+v\nwant: %+v", containerMounts, tt.expectedMounts) + } + }) + } +} diff --git a/pkg/controller/istiocsr/test_utils.go b/pkg/controller/istiocsr/test_utils.go index a72656284..4d4828f7b 100644 --- a/pkg/controller/istiocsr/test_utils.go +++ b/pkg/controller/istiocsr/test_utils.go @@ -2,6 +2,11 @@ package istiocsr import ( "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" "fmt" "testing" "time" @@ -34,6 +39,8 @@ var ( testError = fmt.Errorf("test client error") ) +type CertificateTweak func(*x509.Certificate) + func testReconciler(t *testing.T) *Reconciler { return &Reconciler{ ctx: context.Background(), @@ -228,7 +235,88 @@ func testConfigMap() *corev1.ConfigMap { Namespace: testIstioCSRNamespace, }, Data: map[string]string{ - istiocsrCAKeyName: "testCAData", + IstiocsrCAKeyName: "testCAData", + }, + } +} + +// testCACertificateConfigMap creates a ConfigMap with a CA certificate +func testCACertificateConfigMap() *corev1.ConfigMap { + caPEM := GenerateCertificate("Test CA", []string{"cert-manager-operator"}, + func(cert *x509.Certificate) { + cert.IsCA = true + cert.KeyUsage |= x509.KeyUsageCertSign + }, + ) + return createCertificateConfigMap("ca-cert-test", testIstioCSRNamespace, caPEM) +} + +// testNonCACertificateConfigMap creates a ConfigMap with a non-CA certificate +func testNonCACertificateConfigMap() *corev1.ConfigMap { + certPEM := GenerateCertificate("Test non-CA", []string{"cert-manager-operator"}, + func(cert *x509.Certificate) { + cert.IsCA = false + }, + ) + return createCertificateConfigMap("non-ca-cert-test", testIstioCSRNamespace, certPEM) +} + +// testCertificateWithoutCertSignConfigMap creates a ConfigMap with a CA certificate missing CertSign +func testCertificateWithoutCertSignConfigMap() *corev1.ConfigMap { + certPEM := GenerateCertificate("Test CA without CertSign", []string{"cert-manager-operator"}, + func(cert *x509.Certificate) { + cert.IsCA = true + }, + ) + return createCertificateConfigMap("ca-without-certsign-test", testIstioCSRNamespace, certPEM) +} + +// GenerateCertificate creates a certificate with specified tweaks +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) + + certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey) + if err != nil { + panic(fmt.Sprintf("Failed to create certificate: %v", err)) + } + + // Encode certificate to PEM format + certPEM := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: certDER, + }) + + return string(certPEM) +} + +func createCertificateConfigMap(name string, namespace string, pemData string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Data: map[string]string{ + "ca-cert.pem": pemData, }, } } From ecfbee9449f6fb61084b781d95b427625e081899 Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Wed, 8 Oct 2025 20:59:52 +0530 Subject: [PATCH 2/3] Add e2e for istioCACertificate feature Signed-off-by: chiragkyal --- pkg/controller/istiocsr/controller.go | 11 + test/e2e/istio_csr_test.go | 419 +++++++++++++++++- ...istiocsr_with_ca_certificate_template.yaml | 22 + 3 files changed, 446 insertions(+), 6 deletions(-) create mode 100644 test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml diff --git a/pkg/controller/istiocsr/controller.go b/pkg/controller/istiocsr/controller.go index 7bf013b7f..9b3795ba7 100644 --- a/pkg/controller/istiocsr/controller.go +++ b/pkg/controller/istiocsr/controller.go @@ -230,9 +230,19 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { return object.GetLabels() != nil && object.GetLabels()[IstiocsrResourceWatchLabelName] != "" }) + controllerConfigMapPredicates := predicate.NewPredicateFuncs(func(object client.Object) bool { + if object.GetLabels() == nil { + return false + } + // Accept if it's a managed ConfigMap OR a watched ConfigMap + return object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue || + object.GetLabels()[IstiocsrResourceWatchLabelName] != "" + }) + withIgnoreStatusUpdatePredicates := builder.WithPredicates(predicate.GenerationChangedPredicate{}, controllerManagedResources) controllerWatchResourcePredicates := builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}, controllerWatchResources) controllerManagedResourcePredicates := builder.WithPredicates(controllerManagedResources) + controllerConfigMapWatchPredicates := builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}, controllerConfigMapPredicates) return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.IstioCSR{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). @@ -245,6 +255,7 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&rbacv1.RoleBinding{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerManagedResourcePredicates). Watches(&corev1.Service{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerManagedResourcePredicates). Watches(&corev1.ServiceAccount{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerManagedResourcePredicates). + Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerConfigMapWatchPredicates). WatchesMetadata(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(mapFunc), controllerWatchResourcePredicates). Complete(r) } diff --git a/test/e2e/istio_csr_test.go b/test/e2e/istio_csr_test.go index a49c2301d..8e2a40a5a 100644 --- a/test/e2e/istio_csr_test.go +++ b/test/e2e/istio_csr_test.go @@ -14,6 +14,7 @@ import ( "net/url" "path/filepath" + testutils "github.com/openshift/cert-manager-operator/pkg/controller/istiocsr" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -187,8 +188,10 @@ var _ = Describe("Istio-CSR", Ordered, Label("Feature:IstioCSR"), func() { IstioCSRStatus: istioCSRStatus, }, ), filepath.Join("testdata", "istio", "grpcurl_job.yaml"), ns.Name) - policy := metav1.DeletePropagationBackground - defer clientset.BatchV1().Jobs(ns.Name).Delete(ctx, grpcAppName, metav1.DeleteOptions{PropagationPolicy: &policy}) + DeferCleanup(func() { + policy := metav1.DeletePropagationBackground + clientset.BatchV1().Jobs(ns.Name).Delete(ctx, grpcAppName, metav1.DeleteOptions{PropagationPolicy: &policy}) + }) By("waiting for the job to be completed") err = pollTillJobCompleted(ctx, clientset, ns.Name, grpcAppName) @@ -262,8 +265,10 @@ var _ = Describe("Istio-CSR", Ordered, Label("Feature:IstioCSR"), func() { JobName: grpcAppName, }, ), filepath.Join("testdata", "istio", "grpcurl_job_with_cluster_id.yaml"), ns.Name) - policy := metav1.DeletePropagationBackground - defer clientset.BatchV1().Jobs(ns.Name).Delete(ctx, grpcAppName, metav1.DeleteOptions{PropagationPolicy: &policy}) + DeferCleanup(func() { + policy := metav1.DeletePropagationBackground + clientset.BatchV1().Jobs(ns.Name).Delete(ctx, grpcAppName, metav1.DeleteOptions{PropagationPolicy: &policy}) + }) By("waiting for the job to be completed") err = pollTillJobCompleted(ctx, clientset, ns.Name, grpcAppName) @@ -330,8 +335,10 @@ var _ = Describe("Istio-CSR", Ordered, Label("Feature:IstioCSR"), func() { JobName: grpcAppName, }, ), filepath.Join("testdata", "istio", "grpcurl_job_with_cluster_id.yaml"), ns.Name) - policy := metav1.DeletePropagationBackground - defer clientset.BatchV1().Jobs(ns.Name).Delete(ctx, grpcAppName, metav1.DeleteOptions{PropagationPolicy: &policy}) + DeferCleanup(func() { + policy := metav1.DeletePropagationBackground + clientset.BatchV1().Jobs(ns.Name).Delete(ctx, grpcAppName, metav1.DeleteOptions{PropagationPolicy: &policy}) + }) By("waiting for the job to fail or timeout") // This job should fail because clusterID doesn't match @@ -495,4 +502,404 @@ var _ = Describe("Istio-CSR", Ordered, Label("Feature:IstioCSR"), func() { Expect(configMapCount).Should(Equal(3), "ConfigMap should exist in all 3 test namespaces when no selector is configured") }) }) + + Context("with CA Certificate ConfigMap", func() { + + const ( + configMapRefName = "test-ca-certificate" + configMapRefKey = "ca-cert.pem" + ) + + type istioCSRTemplateData struct { + CustomNamespace string + ConfigMapName string + ConfigMapKey string + } + + BeforeEach(func() { + By("creating cluster issuer") + loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "cluster_issuer.yaml"), ns.Name) + + By("issuing TLS certificate") + loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) + + err := waitForCertificateReadiness(ctx, "my-selfsigned-ca", ns.Name) + Expect(err).NotTo(HaveOccurred()) + + By("creating istio-ca issuer") + loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "istio", "istio_ca_issuer.yaml"), ns.Name) + }) + + AfterEach(func() { + By("cleaning up cluster issuer") + loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "cluster_issuer.yaml"), ns.Name) + + By("cleaning up TLS certificate") + loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) + + By("cleaning up istio-ca issuer") + loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "istio", "istio_ca_issuer.yaml"), ns.Name) + }) + + // Helper functions for CA ConfigMap verification + verifyConfigMapHasWatchLabel := func(namespace, configMapName string) { + By(fmt.Sprintf("Verifying watch label on ConfigMap %s in namespace %s", configMapName, namespace)) + Eventually(func() bool { + configMap, err := clientset.CoreV1().ConfigMaps(namespace).Get(ctx, configMapName, metav1.GetOptions{}) + if err != nil { + return false + } + _, hasWatchLabel := configMap.Labels[testutils.IstiocsrResourceWatchLabelName] + return hasWatchLabel + }, "1m", "5s").Should(BeTrue(), fmt.Sprintf("ConfigMap %s should have watch label", configMapName)) + } + + verifyConfigMapCopied := func(sourceNamespace, sourceConfigMapName, sourceKey, destNamespace string) { + By(fmt.Sprintf("Verifying ConfigMap %s from namespace %s is copied to namespace %s", sourceConfigMapName, sourceNamespace, destNamespace)) + Eventually(func() bool { + // Get the source ConfigMap data + sourceConfigMap, err := clientset.CoreV1().ConfigMaps(sourceNamespace).Get(ctx, sourceConfigMapName, metav1.GetOptions{}) + if err != nil { + return false + } + sourceCertData := sourceConfigMap.Data[sourceKey] + + // Check if the copied ConfigMap exists and has the same data + copiedConfigMap, err := clientset.CoreV1().ConfigMaps(destNamespace).Get(ctx, testutils.IstiocsrCAConfigMapName, metav1.GetOptions{}) + if err != nil { + return false + } + copiedCertData := copiedConfigMap.Data[testutils.IstiocsrCAKeyName] + + return sourceCertData != "" && copiedCertData != "" && copiedCertData == sourceCertData + }, "2m", "5s").Should(BeTrue(), "CA certificate should be copied with identical content") + } + + verifyConfigMapMountedInPod := func(namespace string) { + By("Verifying ConfigMap is mounted correctly in the pod") + Eventually(func() error { + pods, err := clientset.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{ + LabelSelector: "app.kubernetes.io/name=cert-manager-istio-csr", + }) + if err != nil { + return err + } + if len(pods.Items) == 0 { + return fmt.Errorf("no istio-csr pods found") + } + + // Get the first running pod + var runningPod *corev1.Pod + for i := range pods.Items { + if pods.Items[i].Status.Phase == corev1.PodRunning { + runningPod = &pods.Items[i] + break + } + } + if runningPod == nil { + return fmt.Errorf("no running istio-csr pod found") + } + + // Verify volume is configured + volumeFound := false + for _, vol := range runningPod.Spec.Volumes { + if vol.Name == "root-ca" { + if vol.ConfigMap == nil { + return fmt.Errorf("volume root-ca is not a ConfigMap volume") + } + if vol.ConfigMap.Name != testutils.IstiocsrCAConfigMapName { + return fmt.Errorf("volume root-ca references wrong ConfigMap: got %s, want %s", + vol.ConfigMap.Name, testutils.IstiocsrCAConfigMapName) + } + volumeFound = true + break + } + } + if !volumeFound { + return fmt.Errorf("volume root-ca not found in pod spec") + } + + // Verify volume mount in container + for _, container := range runningPod.Spec.Containers { + if container.Name == "cert-manager-istio-csr" { + volumeMountFound := false + for _, vm := range container.VolumeMounts { + if vm.Name == "root-ca" { + if vm.MountPath != "/var/run/configmaps/istio-csr" { + return fmt.Errorf("volume mount root-ca has wrong path: got %s, want /var/run/configmaps/istio-csr", + vm.MountPath) + } + if !vm.ReadOnly { + return fmt.Errorf("volume mount root-ca should be read-only") + } + volumeMountFound = true + break + } + } + if !volumeMountFound { + return fmt.Errorf("volume mount root-ca not found in container cert-manager-istio-csr") + } + return nil + } + } + return fmt.Errorf("container cert-manager-istio-csr not found in pod") + }, "2m", "10s").Should(Succeed(), "ConfigMap should be correctly mounted in the pod") + } + + It("should successfully use CA certificate from ConfigMap in same namespace", func() { + By("Creating a CA certificate ConfigMap") + caCertConfigMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: configMapRefName, + Namespace: ns.Name, // same namespace as IstioCSR + }, + Data: map[string]string{ + configMapRefKey: testutils.GenerateCertificate("Test CA E2E", []string{"cert-manager-operator"}, func(cert *x509.Certificate) { + cert.IsCA = true + cert.KeyUsage |= x509.KeyUsageCertSign + }), + }, + } + _, err := clientset.CoreV1().ConfigMaps(caCertConfigMap.Namespace).Create(ctx, caCertConfigMap, metav1.CreateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Creating IstioCSR resource") + templateData := istioCSRTemplateData{ + CustomNamespace: "", // Empty string for same namespace + ConfigMapName: configMapRefName, + ConfigMapKey: configMapRefKey, + } + loader.CreateFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + DeferCleanup(func() { + loader.DeleteFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + }) + + By("waiting for IstioCSR to be ready and deployment to be created") + istioCSRStatus := waitForIstioCSRReady(ns) + log.Printf("IstioCSR status: %+v", istioCSRStatus) + + // Verify that the source ConfigMap data is copied to the operator-managed ConfigMap + verifyConfigMapCopied(caCertConfigMap.Namespace, caCertConfigMap.Name, configMapRefKey, ns.Name) + // Verify that the source ConfigMap has a watch label to trigger reconciliation on changes + verifyConfigMapHasWatchLabel(caCertConfigMap.Namespace, caCertConfigMap.Name) + // Verify that the certificate is correctly mounted in the istio-csr pod + verifyConfigMapMountedInPod(ns.Name) + }) + + It("should fail when CA certificate is not actually a CA certificate", func() { + By("Creating a ConfigMap with a non-CA certificate") + nonCACertConfigMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: configMapRefName, + Namespace: ns.Name, // same namespace as IstioCSR + }, + Data: map[string]string{ + configMapRefKey: testutils.GenerateCertificate("Test Non-CA E2E", []string{"cert-manager-operator"}, func(cert *x509.Certificate) { + cert.IsCA = false + }), + }, + } + _, err := clientset.CoreV1().ConfigMaps(nonCACertConfigMap.Namespace).Create(ctx, nonCACertConfigMap, metav1.CreateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Creating IstioCSR resource") + templateData := istioCSRTemplateData{ + CustomNamespace: "", // Empty string for same namespace + ConfigMapName: configMapRefName, + ConfigMapKey: configMapRefKey, + } + loader.CreateFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + DeferCleanup(func() { + loader.DeleteFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + }) + + 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") + }) + + It("should successfully use CA certificate from ConfigMap in custom namespace", func() { + By("Creating a custom namespace for the source ConfigMap") + customNamespace, err := loader.CreateTestingNS("custom-ca-ns", false) + Expect(err).ShouldNot(HaveOccurred()) + DeferCleanup(func() { + loader.DeleteTestingNS(customNamespace.Name, func() bool { return false }) + }) + + By("Creating a CA certificate ConfigMap in the custom namespace") + caCertConfigMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: configMapRefName, + Namespace: customNamespace.Name, + }, + Data: map[string]string{ + configMapRefKey: testutils.GenerateCertificate("Custom Namespace CA E2E", []string{"cert-manager-operator"}, func(cert *x509.Certificate) { + cert.IsCA = true + cert.KeyUsage |= x509.KeyUsageCertSign + }), + }, + } + _, err = clientset.CoreV1().ConfigMaps(caCertConfigMap.Namespace).Create(ctx, caCertConfigMap, metav1.CreateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Creating IstioCSR resource with custom namespace reference") + templateData := istioCSRTemplateData{ + CustomNamespace: customNamespace.Name, + ConfigMapName: configMapRefName, + ConfigMapKey: configMapRefKey, + } + loader.CreateFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + DeferCleanup(func() { + loader.DeleteFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + }) + + By("waiting for IstioCSR to be ready and deployment to be created") + istioCSRStatus := waitForIstioCSRReady(ns) + log.Printf("IstioCSR status: %+v", istioCSRStatus) + + // Verify that the source ConfigMap data is copied from custom namespace to the operator-managed ConfigMap + verifyConfigMapCopied(customNamespace.Name, caCertConfigMap.Name, configMapRefKey, ns.Name) + // Verify that the source ConfigMap in custom namespace has a watch label to trigger reconciliation on changes + verifyConfigMapHasWatchLabel(customNamespace.Name, caCertConfigMap.Name) + // Verify that the certificate from custom namespace is correctly mounted in the istio-csr pod + verifyConfigMapMountedInPod(ns.Name) + }) + + It("should reconcile copied ConfigMap when manually modified", func() { + By("Creating initial CA certificate ConfigMap") + initialCert := testutils.GenerateCertificate("Initial CA E2E", []string{"cert-manager-operator"}, func(cert *x509.Certificate) { + cert.IsCA = true + cert.KeyUsage |= x509.KeyUsageCertSign + }) + + caCertConfigMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: configMapRefName, + Namespace: ns.Name, + }, + Data: map[string]string{ + configMapRefKey: initialCert, + }, + } + _, err := clientset.CoreV1().ConfigMaps(caCertConfigMap.Namespace).Create(ctx, caCertConfigMap, metav1.CreateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Creating IstioCSR resource") + templateData := istioCSRTemplateData{ + CustomNamespace: "", + ConfigMapName: configMapRefName, + ConfigMapKey: configMapRefKey, + } + loader.CreateFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + DeferCleanup(func() { + loader.DeleteFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + }) + + By("Waiting for IstioCSR to be ready") + _ = waitForIstioCSRReady(ns) + + By("Verifying initial ConfigMap is copied") + Eventually(func() bool { + copiedConfigMap, err := clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, testutils.IstiocsrCAConfigMapName, metav1.GetOptions{}) + if err != nil { + return false + } + return copiedConfigMap.Data[testutils.IstiocsrCAKeyName] == initialCert + }, "2m", "5s").Should(BeTrue(), "Initial CA certificate should be copied") + + By("Manually modifying the copied ConfigMap") + tamperedCert := "-----BEGIN CERTIFICATE-----\nTAMPERED DATA\n-----END CERTIFICATE-----" + copiedConfigMap, err := clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, testutils.IstiocsrCAConfigMapName, metav1.GetOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + copiedConfigMap.Data[testutils.IstiocsrCAKeyName] = tamperedCert + _, err = clientset.CoreV1().ConfigMaps(ns.Name).Update(ctx, copiedConfigMap, metav1.UpdateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Verifying operator automatically reconciles the copied ConfigMap back to the desired state") + Eventually(func() bool { + reconciledConfigMap, err := clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, testutils.IstiocsrCAConfigMapName, metav1.GetOptions{}) + if err != nil { + return false + } + // Should be reconciled back to match the source ConfigMap, not the tampered data + return reconciledConfigMap.Data[testutils.IstiocsrCAKeyName] == initialCert + }, "2m", "5s").Should(BeTrue(), "Copied ConfigMap should be reconciled back to match source ConfigMap") + + By("Verifying the ConfigMap volume mount is still correctly configured") + verifyConfigMapMountedInPod(ns.Name) + }) + + It("should update mounted CA certificate when source ConfigMap is modified", func() { + By("Creating initial CA certificate ConfigMap") + initialCert := testutils.GenerateCertificate("Initial CA E2E", []string{"cert-manager-operator"}, func(cert *x509.Certificate) { + cert.IsCA = true + cert.KeyUsage |= x509.KeyUsageCertSign + }) + + caCertConfigMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: configMapRefName, + Namespace: ns.Name, + }, + Data: map[string]string{ + configMapRefKey: initialCert, + }, + } + _, err := clientset.CoreV1().ConfigMaps(caCertConfigMap.Namespace).Create(ctx, caCertConfigMap, metav1.CreateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Creating IstioCSR resource") + templateData := istioCSRTemplateData{ + CustomNamespace: "", + ConfigMapName: configMapRefName, + ConfigMapKey: configMapRefKey, + } + loader.CreateFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + DeferCleanup(func() { + loader.DeleteFromFile(AssetFunc(testassets.ReadFile).WithTemplateValues(templateData), filepath.Join("testdata", "istio", "istiocsr_with_ca_certificate_template.yaml"), ns.Name) + }) + + By("Waiting for IstioCSR to be ready") + _ = waitForIstioCSRReady(ns) + + By("Verifying initial ConfigMap is copied and mounted") + Eventually(func() bool { + copiedConfigMap, err := clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, testutils.IstiocsrCAConfigMapName, metav1.GetOptions{}) + if err != nil { + return false + } + return copiedConfigMap.Data[testutils.IstiocsrCAKeyName] == initialCert + }, "2m", "5s").Should(BeTrue(), "Initial CA certificate should be copied") + + // Verify that the ConfigMap volume mount is correctly configured + verifyConfigMapMountedInPod(ns.Name) + + By("Updating the source ConfigMap with new CA certificate") + updatedCert := testutils.GenerateCertificate("Updated CA E2E", []string{"cert-manager-operator-updated"}, func(cert *x509.Certificate) { + cert.IsCA = true + cert.KeyUsage |= x509.KeyUsageCertSign + }) + + sourceConfigMap, err := clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, configMapRefName, metav1.GetOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + sourceConfigMap.Data[configMapRefKey] = updatedCert + _, err = clientset.CoreV1().ConfigMaps(ns.Name).Update(ctx, sourceConfigMap, metav1.UpdateOptions{}) + Expect(err).ShouldNot(HaveOccurred()) + + By("Verifying copied ConfigMap is updated with new certificate") + Eventually(func() bool { + copiedConfigMap, err := clientset.CoreV1().ConfigMaps(ns.Name).Get(ctx, testutils.IstiocsrCAConfigMapName, metav1.GetOptions{}) + if err != nil { + return false + } + return copiedConfigMap.Data[testutils.IstiocsrCAKeyName] == updatedCert + }, "2m", "5s").Should(BeTrue(), "Updated CA certificate should be copied") + + // Verify that the ConfigMap volume mount is still correctly configured + verifyConfigMapMountedInPod(ns.Name) + }) + }) }) diff --git a/test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml b/test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml new file mode 100644 index 000000000..5fb74efa6 --- /dev/null +++ b/test/e2e/testdata/istio/istiocsr_with_ca_certificate_template.yaml @@ -0,0 +1,22 @@ +apiVersion: operator.openshift.io/v1alpha1 +kind: IstioCSR +metadata: + name: default + namespace: istio-system +spec: + istioCSRConfig: + certManager: + issuerRef: + name: istio-ca + kind: Issuer + group: cert-manager.io + istioCACertificate: + name: {{.ConfigMapName}} +{{- if .CustomNamespace}} + namespace: {{.CustomNamespace}} +{{- end}} + key: {{.ConfigMapKey}} + istiodTLSConfig: + trustDomain: cluster.local + istio: + namespace: istio-system From 9ce4d2eca8e68be8b3b98079cf0d6982ae72304e Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Mon, 13 Oct 2025 17:50:04 +0530 Subject: [PATCH 3/3] Remove duplicate BeforeEach Signed-off-by: chiragkyal --- test/e2e/istio_csr_test.go | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/test/e2e/istio_csr_test.go b/test/e2e/istio_csr_test.go index 8e2a40a5a..60b0b6d26 100644 --- a/test/e2e/istio_csr_test.go +++ b/test/e2e/istio_csr_test.go @@ -516,31 +516,6 @@ var _ = Describe("Istio-CSR", Ordered, Label("Feature:IstioCSR"), func() { ConfigMapKey string } - BeforeEach(func() { - By("creating cluster issuer") - loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "cluster_issuer.yaml"), ns.Name) - - By("issuing TLS certificate") - loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) - - err := waitForCertificateReadiness(ctx, "my-selfsigned-ca", ns.Name) - Expect(err).NotTo(HaveOccurred()) - - By("creating istio-ca issuer") - loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "istio", "istio_ca_issuer.yaml"), ns.Name) - }) - - AfterEach(func() { - By("cleaning up cluster issuer") - loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "cluster_issuer.yaml"), ns.Name) - - By("cleaning up TLS certificate") - loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) - - By("cleaning up istio-ca issuer") - loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "istio", "istio_ca_issuer.yaml"), ns.Name) - }) - // Helper functions for CA ConfigMap verification verifyConfigMapHasWatchLabel := func(namespace, configMapName string) { By(fmt.Sprintf("Verifying watch label on ConfigMap %s in namespace %s", configMapName, namespace))