From 8a64905f6b15d4383758b3c5ec21971991abebb2 Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Thu, 10 Sep 2026 14:56:12 +0530 Subject: [PATCH 1/4] CM-1367: Add TrustManager filterNonCACerts API. Expose Enabled/Disabled on spec and status and pass --filter-non-ca-certs=true when Enabled. Requires a trust-manager operand that supports the flag (v0.21.0+). --- .../trustmanager.testsuite.yaml | 82 +++++++++++++++++++ api/operator/v1alpha1/trustmanager_types.go | 25 ++++++ .../operator.openshift.io_trustmanagers.yaml | 19 +++++ .../operator.openshift.io_trustmanagers.yaml | 19 +++++ pkg/controller/trustmanager/deployments.go | 4 + .../trustmanager/deployments_test.go | 19 ++++- .../trustmanager/install_trustmanager.go | 5 ++ .../trustmanager/install_trustmanager_test.go | 4 + pkg/controller/trustmanager/test_utils.go | 5 ++ .../operator/v1alpha1/trustmanagerconfig.go | 14 ++++ .../operator/v1alpha1/trustmanagerstatus.go | 10 +++ test/e2e/trustmanager_bundle_test.go | 82 ++++++++++++++++++- test/e2e/trustmanager_helpers_test.go | 7 +- test/e2e/trustmanager_test.go | 37 +++++++++ 14 files changed, 327 insertions(+), 5 deletions(-) diff --git a/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml b/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml index 6a22b6de9..edb326e31 100644 --- a/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml +++ b/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml @@ -22,6 +22,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled - name: Should not allow creating TrustManager with invalid name resourceName: invalid-name @@ -52,6 +53,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled - name: Should not allow logLevel below minimum resourceName: cluster @@ -93,6 +95,7 @@ tests: logFormat: json trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled - name: Should not allow invalid logFormat resourceName: cluster @@ -124,6 +127,7 @@ tests: logFormat: text trustNamespace: custom-namespace filterExpiredCertificates: Disabled + filterNonCACerts: Disabled # ========================================== # FilterExpiredCertificates Tests @@ -136,6 +140,7 @@ tests: spec: trustManagerConfig: filterExpiredCertificates: Enabled + filterNonCACerts: Disabled expected: | apiVersion: operator.openshift.io/v1alpha1 kind: TrustManager @@ -145,6 +150,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Enabled + filterNonCACerts: Disabled - name: Should not allow invalid filterExpiredCertificates value resourceName: cluster @@ -154,8 +160,41 @@ tests: spec: trustManagerConfig: filterExpiredCertificates: Invalid + filterNonCACerts: Disabled expectedError: "filterExpiredCertificates" + # ========================================== + # FilterNonCACerts Tests + # ========================================== + - name: Should create with filterNonCACerts Enabled + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + filterNonCACerts: Enabled + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + logLevel: 1 + logFormat: text + trustNamespace: cert-manager + filterExpiredCertificates: Disabled + filterNonCACerts: Enabled + + - name: Should not allow invalid filterNonCACerts value + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + filterNonCACerts: Invalid + expectedError: "filterNonCACerts" + # ========================================== # DefaultCAPackage Tests # ========================================== @@ -177,6 +216,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled defaultCAPackage: policy: Enabled @@ -215,6 +255,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled secretTargets: policy: Custom authorizedSecrets: @@ -282,6 +323,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled resources: requests: cpu: 100m @@ -315,6 +357,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled affinity: nodeAffinity: requiredDuringSchedulingIgnoredDuringExecution: @@ -345,6 +388,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled tolerations: - key: node-role.kubernetes.io/master operator: Exists @@ -368,6 +412,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled nodeSelector: kubernetes.io/os: linux @@ -393,6 +438,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled controllerConfig: labels: app.kubernetes.io/managed-by: cert-manager-operator @@ -416,6 +462,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled controllerConfig: annotations: custom-annotation: custom-value @@ -463,6 +510,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled - name: Should allow updating logFormat resourceName: cluster @@ -487,6 +535,7 @@ tests: logFormat: json trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled - name: Should allow updating filterExpiredCertificates resourceName: cluster @@ -496,12 +545,14 @@ tests: spec: trustManagerConfig: filterExpiredCertificates: Disabled + filterNonCACerts: Disabled updated: | apiVersion: operator.openshift.io/v1alpha1 kind: TrustManager spec: trustManagerConfig: filterExpiredCertificates: Enabled + filterNonCACerts: Disabled expected: | apiVersion: operator.openshift.io/v1alpha1 kind: TrustManager @@ -511,6 +562,32 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Enabled + filterNonCACerts: Disabled + + - name: Should allow updating filterNonCACerts + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + filterNonCACerts: Disabled + updated: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + filterNonCACerts: Enabled + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + logLevel: 1 + logFormat: text + trustNamespace: cert-manager + filterExpiredCertificates: Disabled + filterNonCACerts: Enabled - name: Should allow updating defaultCAPackage policy resourceName: cluster @@ -537,6 +614,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled defaultCAPackage: policy: Enabled @@ -567,6 +645,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled secretTargets: policy: Custom authorizedSecrets: @@ -602,6 +681,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled secretTargets: policy: Custom authorizedSecrets: @@ -635,6 +715,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled secretTargets: policy: Disabled @@ -710,6 +791,7 @@ tests: logFormat: text trustNamespace: cert-manager filterExpiredCertificates: Disabled + filterNonCACerts: Disabled secretTargets: policy: Custom authorizedSecrets: diff --git a/api/operator/v1alpha1/trustmanager_types.go b/api/operator/v1alpha1/trustmanager_types.go index a18caf370..ebf9332ee 100644 --- a/api/operator/v1alpha1/trustmanager_types.go +++ b/api/operator/v1alpha1/trustmanager_types.go @@ -118,6 +118,17 @@ type TrustManagerConfig struct { // +optional FilterExpiredCertificates FilterExpiredCertificatesPolicy `json:"filterExpiredCertificates,omitempty"` + // filterNonCACerts controls whether trust-manager filters out + // non-CA certificates from trust bundles before distributing them. + // When set to "Enabled", only certificates with the X.509 basicConstraints + // CA bit set are included in bundles. + // When set to "Disabled", non-CA certificates are included (default behavior). + // +kubebuilder:default:="Disabled" + // +kubebuilder:validation:Enum:=Enabled;Disabled + // +kubebuilder:validation:Optional + // +optional + FilterNonCACerts FilterNonCACertsPolicy `json:"filterNonCACerts,omitempty"` + // defaultCAPackage configures the default CA package for trust-manager. // When enabled, the operator will use OpenShift's trusted CA bundle injection mechanism. // +kubebuilder:validation:Optional @@ -225,6 +236,16 @@ const ( FilterExpiredCertificatesPolicyDisabled FilterExpiredCertificatesPolicy = "Disabled" ) +// FilterNonCACertsPolicy defines the policy for filtering non-CA certificates. +type FilterNonCACertsPolicy string + +const ( + // FilterNonCACertsPolicyEnabled filters out non-CA certificates from bundles. + FilterNonCACertsPolicyEnabled FilterNonCACertsPolicy = "Enabled" + // FilterNonCACertsPolicyDisabled includes non-CA certificates in bundles. + FilterNonCACertsPolicyDisabled FilterNonCACertsPolicy = "Disabled" +) + // SecretTargetsPolicy defines the policy for writing trust bundles to Secrets. type SecretTargetsPolicy string @@ -268,4 +289,8 @@ type TrustManagerStatus struct { // filterExpiredCertificatesPolicy indicates the current policy for filtering expired certificates. // +kubebuilder:validation:Enum:=Enabled;Disabled FilterExpiredCertificatesPolicy FilterExpiredCertificatesPolicy `json:"filterExpiredCertificatesPolicy,omitempty"` + + // filterNonCACertsPolicy indicates the current policy for filtering non-CA certificates. + // +kubebuilder:validation:Enum:=Enabled;Disabled + FilterNonCACertsPolicy FilterNonCACertsPolicy `json:"filterNonCACertsPolicy,omitempty"` } diff --git a/bundle/manifests/operator.openshift.io_trustmanagers.yaml b/bundle/manifests/operator.openshift.io_trustmanagers.yaml index 2da748fda..3ec28a0e4 100644 --- a/bundle/manifests/operator.openshift.io_trustmanagers.yaml +++ b/bundle/manifests/operator.openshift.io_trustmanagers.yaml @@ -1038,6 +1038,18 @@ spec: - Enabled - Disabled type: string + filterNonCACerts: + default: Disabled + description: |- + filterNonCACerts controls whether trust-manager filters out + non-CA certificates from trust bundles before distributing them. + When set to "Enabled", only certificates with the X.509 basicConstraints + CA bit set are included in bundles. + When set to "Disabled", non-CA certificates are included (default behavior). + enum: + - Enabled + - Disabled + type: string logFormat: default: text description: |- @@ -1304,6 +1316,13 @@ spec: - Enabled - Disabled type: string + filterNonCACertsPolicy: + description: filterNonCACertsPolicy indicates the current policy for + filtering non-CA certificates. + enum: + - Enabled + - Disabled + type: string secretTargetsPolicy: description: secretTargetsPolicy indicates the current secret targets policy. diff --git a/config/crd/bases/operator.openshift.io_trustmanagers.yaml b/config/crd/bases/operator.openshift.io_trustmanagers.yaml index 0a334f47d..94c96d274 100644 --- a/config/crd/bases/operator.openshift.io_trustmanagers.yaml +++ b/config/crd/bases/operator.openshift.io_trustmanagers.yaml @@ -1038,6 +1038,18 @@ spec: - Enabled - Disabled type: string + filterNonCACerts: + default: Disabled + description: |- + filterNonCACerts controls whether trust-manager filters out + non-CA certificates from trust bundles before distributing them. + When set to "Enabled", only certificates with the X.509 basicConstraints + CA bit set are included in bundles. + When set to "Disabled", non-CA certificates are included (default behavior). + enum: + - Enabled + - Disabled + type: string logFormat: default: text description: |- @@ -1304,6 +1316,13 @@ spec: - Enabled - Disabled type: string + filterNonCACertsPolicy: + description: filterNonCACertsPolicy indicates the current policy for + filtering non-CA certificates. + enum: + - Enabled + - Disabled + type: string secretTargetsPolicy: description: secretTargetsPolicy indicates the current secret targets policy. diff --git a/pkg/controller/trustmanager/deployments.go b/pkg/controller/trustmanager/deployments.go index da96689d4..841326c80 100644 --- a/pkg/controller/trustmanager/deployments.go +++ b/pkg/controller/trustmanager/deployments.go @@ -147,6 +147,10 @@ func updateDeploymentArgs(deployment *appsv1.Deployment, trustManager *v1alpha1. args = append(args, "--filter-expired-certificates=true") } + if config.FilterNonCACerts == v1alpha1.FilterNonCACertsPolicyEnabled { + args = append(args, "--filter-non-ca-certs=true") + } + if defaultCAPackageEnabled(config.DefaultCAPackage) { args = append(args, fmt.Sprintf("--default-package-location=%s", defaultCAPackageLocation)) } diff --git a/pkg/controller/trustmanager/deployments_test.go b/pkg/controller/trustmanager/deployments_test.go index 8e86c456d..3ffe761cd 100644 --- a/pkg/controller/trustmanager/deployments_test.go +++ b/pkg/controller/trustmanager/deployments_test.go @@ -148,6 +148,7 @@ func TestDeploymentContainerArgs(t *testing.T) { notExpectedArgs: []string{ "--secret-targets-enabled=true", "--filter-expired-certificates=true", + "--filter-non-ca-certs=true", fmt.Sprintf("--default-package-location=%s", defaultCAPackageLocation), }, }, @@ -157,12 +158,14 @@ func TestDeploymentContainerArgs(t *testing.T) { WithLogLevel(5). WithLogFormat("json"). WithTrustNamespace("custom-ns"). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled), + WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled), expectedArgs: []string{ "--log-level=5", "--log-format=json", "--trust-namespace=custom-ns", "--filter-expired-certificates=true", + "--filter-non-ca-certs=true", }, notExpectedArgs: []string{ "--log-level=1", @@ -210,6 +213,20 @@ func TestDeploymentContainerArgs(t *testing.T) { fmt.Sprintf("--default-package-location=%s", defaultCAPackageLocation), }, }, + { + name: "includes filter-non-ca-certs when filterNonCACerts is Enabled", + tmBuilder: testTrustManager().WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled), + expectedArgs: []string{ + "--filter-non-ca-certs=true", + }, + }, + { + name: "excludes filter-non-ca-certs when filterNonCACerts is Disabled", + tmBuilder: testTrustManager().WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyDisabled), + notExpectedArgs: []string{ + "--filter-non-ca-certs=true", + }, + }, } for _, tt := range tests { diff --git a/pkg/controller/trustmanager/install_trustmanager.go b/pkg/controller/trustmanager/install_trustmanager.go index 932253355..32294928e 100644 --- a/pkg/controller/trustmanager/install_trustmanager.go +++ b/pkg/controller/trustmanager/install_trustmanager.go @@ -112,6 +112,11 @@ func (r *Reconciler) updateStatusObservedState(trustManager *v1alpha1.TrustManag changed = true } + if policy := trustManager.Spec.TrustManagerConfig.FilterNonCACerts; trustManager.Status.FilterNonCACertsPolicy != policy { + trustManager.Status.FilterNonCACertsPolicy = policy + changed = true + } + if !changed { return nil } diff --git a/pkg/controller/trustmanager/install_trustmanager_test.go b/pkg/controller/trustmanager/install_trustmanager_test.go index 437022391..b5382f9d3 100644 --- a/pkg/controller/trustmanager/install_trustmanager_test.go +++ b/pkg/controller/trustmanager/install_trustmanager_test.go @@ -21,6 +21,7 @@ func TestUpdateStatusObservedState(t *testing.T) { SecretTargetsPolicy: "", DefaultCAPackagePolicy: "", FilterExpiredCertificatesPolicy: "", + FilterNonCACertsPolicy: "", } tests := []struct { @@ -45,6 +46,7 @@ func TestUpdateStatusObservedState(t *testing.T) { WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{"allowed-secret"}). WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled). WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled). Build() }, wantStatusUpdate: 1, @@ -54,6 +56,7 @@ func TestUpdateStatusObservedState(t *testing.T) { SecretTargetsPolicy: v1alpha1.SecretTargetsPolicyCustom, DefaultCAPackagePolicy: v1alpha1.DefaultCAPackagePolicyEnabled, FilterExpiredCertificatesPolicy: v1alpha1.FilterExpiredCertificatesPolicyEnabled, + FilterNonCACertsPolicy: v1alpha1.FilterNonCACertsPolicyEnabled, }, }, { @@ -65,6 +68,7 @@ func TestUpdateStatusObservedState(t *testing.T) { tm.Status.SecretTargetsPolicy = tm.Spec.TrustManagerConfig.SecretTargets.Policy tm.Status.DefaultCAPackagePolicy = tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy tm.Status.FilterExpiredCertificatesPolicy = tm.Spec.TrustManagerConfig.FilterExpiredCertificates + tm.Status.FilterNonCACertsPolicy = tm.Spec.TrustManagerConfig.FilterNonCACerts return tm }, wantStatusUpdate: 0, diff --git a/pkg/controller/trustmanager/test_utils.go b/pkg/controller/trustmanager/test_utils.go index e0b96cc9d..33e822070 100644 --- a/pkg/controller/trustmanager/test_utils.go +++ b/pkg/controller/trustmanager/test_utils.go @@ -89,6 +89,11 @@ func (b *trustManagerBuilder) WithFilterExpiredCertificates(policy v1alpha1.Filt return b } +func (b *trustManagerBuilder) WithFilterNonCACerts(policy v1alpha1.FilterNonCACertsPolicy) *trustManagerBuilder { + b.Spec.TrustManagerConfig.FilterNonCACerts = policy + return b +} + func (b *trustManagerBuilder) WithDefaultCAPackage(policy v1alpha1.DefaultCAPackagePolicy) *trustManagerBuilder { b.Spec.TrustManagerConfig.DefaultCAPackage.Policy = policy return b diff --git a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go index 4ac95edd8..e7b546b31 100644 --- a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go +++ b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go @@ -31,6 +31,12 @@ type TrustManagerConfigApplyConfiguration struct { // When set to "Enabled", expired certificates are removed from bundles. // When set to "Disabled", expired certificates are included (default behavior). FilterExpiredCertificates *operatorv1alpha1.FilterExpiredCertificatesPolicy `json:"filterExpiredCertificates,omitempty"` + // filterNonCACerts controls whether trust-manager filters out + // non-CA certificates from trust bundles before distributing them. + // When set to "Enabled", only certificates with the X.509 basicConstraints + // CA bit set are included in bundles. + // When set to "Disabled", non-CA certificates are included (default behavior). + FilterNonCACerts *operatorv1alpha1.FilterNonCACertsPolicy `json:"filterNonCACerts,omitempty"` // defaultCAPackage configures the default CA package for trust-manager. // When enabled, the operator will use OpenShift's trusted CA bundle injection mechanism. DefaultCAPackage *DefaultCAPackageConfigApplyConfiguration `json:"defaultCAPackage,omitempty"` @@ -94,6 +100,14 @@ func (b *TrustManagerConfigApplyConfiguration) WithFilterExpiredCertificates(val return b } +// WithFilterNonCACerts sets the FilterNonCACerts field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the FilterNonCACerts field is set to the value of the last call. +func (b *TrustManagerConfigApplyConfiguration) WithFilterNonCACerts(value operatorv1alpha1.FilterNonCACertsPolicy) *TrustManagerConfigApplyConfiguration { + b.FilterNonCACerts = &value + return b +} + // WithDefaultCAPackage sets the DefaultCAPackage field in the declarative configuration to the given value // and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the DefaultCAPackage field is set to the value of the last call. diff --git a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go index 05974408a..02e1b567d 100644 --- a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go +++ b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go @@ -24,6 +24,8 @@ type TrustManagerStatusApplyConfiguration struct { DefaultCAPackagePolicy *operatorv1alpha1.DefaultCAPackagePolicy `json:"defaultCAPackagePolicy,omitempty"` // filterExpiredCertificatesPolicy indicates the current policy for filtering expired certificates. FilterExpiredCertificatesPolicy *operatorv1alpha1.FilterExpiredCertificatesPolicy `json:"filterExpiredCertificatesPolicy,omitempty"` + // filterNonCACertsPolicy indicates the current policy for filtering non-CA certificates. + FilterNonCACertsPolicy *operatorv1alpha1.FilterNonCACertsPolicy `json:"filterNonCACertsPolicy,omitempty"` } // TrustManagerStatusApplyConfiguration constructs a declarative configuration of the TrustManagerStatus type for use with @@ -84,3 +86,11 @@ func (b *TrustManagerStatusApplyConfiguration) WithFilterExpiredCertificatesPoli b.FilterExpiredCertificatesPolicy = &value return b } + +// WithFilterNonCACertsPolicy sets the FilterNonCACertsPolicy field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the FilterNonCACertsPolicy field is set to the value of the last call. +func (b *TrustManagerStatusApplyConfiguration) WithFilterNonCACertsPolicy(value operatorv1alpha1.FilterNonCACertsPolicy) *TrustManagerStatusApplyConfiguration { + b.FilterNonCACertsPolicy = &value + return b +} diff --git a/test/e2e/trustmanager_bundle_test.go b/test/e2e/trustmanager_bundle_test.go index c6dea5840..86981a3f2 100644 --- a/test/e2e/trustmanager_bundle_test.go +++ b/test/e2e/trustmanager_bundle_test.go @@ -43,6 +43,10 @@ // Group 6 — FilterExpiredCertificates enabled: // - ConfigMap source with valid + expired certs → only valid cert in ConfigMap target // - Transition to Disabled → same Bundle re-syncs with both certs in target +// +// Group 7 — FilterNonCACerts enabled: +// - ConfigMap source with CA + leaf certs → only CA cert in ConfigMap target +// - Transition to Disabled → same Bundle re-syncs with both certs in target package e2e import ( @@ -58,8 +62,8 @@ import ( trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" configopenshiftv1 "github.com/openshift/api/config/v1" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" - "github.com/openshift/cert-manager-operator/test/library" testutils "github.com/openshift/cert-manager-operator/pkg/controller/istiocsr" + "github.com/openshift/cert-manager-operator/test/library" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -77,8 +81,8 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana ctx := context.TODO() var ( - testNS *corev1.Namespace - testCertPEM1, testCertPEM2, expiredCertPEM string + testNS *corev1.Namespace + testCertPEM1, testCertPEM2, expiredCertPEM, leafCertPEM string originalUnsupportedAddonFeatures string originalOperatorLogLevel string @@ -121,6 +125,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana cert.NotAfter = time.Now().Add(-24 * time.Hour) } expiredCertPEM = testutils.GenerateCertificate("e2e-expired-ca", []string{"cert-manager-operator-e2e"}, expiredCATweak) + leafCertPEM = testutils.GenerateCertificate("e2e-leaf", []string{"cert-manager-operator-e2e"}, func(*x509.Certificate) {}) By("creating test namespace for target verification") testNS = createNamespaceWithCleanup(ctx, "bundle-e2e-", map[string]string{bundleTestNamespaceLabel: "true"}) @@ -1073,4 +1078,75 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana verifyBundleSynced(ctx, filterBundleName) }) }) + + // ===== Group 7: FilterNonCACerts ===== + Context("with FilterNonCACerts enabled", Ordered, func() { + var ( + filterBundleName string + sourceCMName string + ) + + BeforeAll(func() { + createTrustManager(ctx, newTrustManagerCR(). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled)) + + sourceCMName = "filter-nca-src-cm-" + randomStr(5) + filterBundleName = "bundle-filter-non-ca-" + randomStr(5) + combinedPEM := testCertPEM1 + leafCertPEM + + By("creating source ConfigMap with CA + leaf certs in trust namespace") + createSourceConfigMap(ctx, trustManagerNamespace, sourceCMName, bundleSourceKey, combinedPEM) + + bundle := newBundle(filterBundleName). + WithConfigMapSource(sourceCMName, bundleSourceKey). + WithConfigMapTarget(bundleTargetKey). + Build() + + createBundleWithCleanup(ctx, bundle) + }) + AfterAll(func() { deleteTrustManager(ctx) }) + + It("should exclude non-CA certificates from ConfigMap target when using ConfigMap source", func() { + By("verifying target contains the CA certificate") + err := waitForConfigMapTarget(ctx, bundleClient, filterBundleName, testNS.Name, bundleTargetKey, testCertPEM1, highTimeout) + Expect(err).ShouldNot(HaveOccurred()) + + By("verifying target does NOT contain the leaf certificate") + Eventually(func(g Gomega) { + cm, err := k8sClientSet.CoreV1().ConfigMaps(testNS.Name).Get(ctx, filterBundleName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + data := cm.Data[bundleTargetKey] + g.Expect(strings.Contains(data, strings.TrimSpace(testCertPEM1))).Should(BeTrue(), "should contain CA cert") + g.Expect(strings.Contains(data, strings.TrimSpace(leafCertPEM))).Should(BeFalse(), "should not contain leaf cert") + }, highTimeout, fastPollInterval).Should(Succeed()) + + verifyBundleSynced(ctx, filterBundleName) + }) + + It("should re-sync same Bundle with leaf certs included after disabling filter", func() { + By("disabling filterNonCACerts on TrustManager CR") + Eventually(func() error { + tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + return err + } + tm.Spec.TrustManagerConfig.FilterNonCACerts = v1alpha1.FilterNonCACertsPolicyDisabled + _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) + return err + }, lowTimeout, fastPollInterval).Should(Succeed()) + + waitForTrustManagerReady(ctx) + + By("verifying the same Bundle's target now includes the leaf certificate") + Eventually(func(g Gomega) { + cm, err := k8sClientSet.CoreV1().ConfigMaps(testNS.Name).Get(ctx, filterBundleName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + data := cm.Data[bundleTargetKey] + g.Expect(strings.Contains(data, strings.TrimSpace(testCertPEM1))).Should(BeTrue(), "should contain CA cert") + g.Expect(strings.Contains(data, strings.TrimSpace(leafCertPEM))).Should(BeTrue(), "should contain leaf cert after disabling filter") + }, highTimeout, fastPollInterval).Should(Succeed()) + + verifyBundleSynced(ctx, filterBundleName) + }) + }) }) diff --git a/test/e2e/trustmanager_helpers_test.go b/test/e2e/trustmanager_helpers_test.go index 6a542594c..a84679f71 100644 --- a/test/e2e/trustmanager_helpers_test.go +++ b/test/e2e/trustmanager_helpers_test.go @@ -14,8 +14,8 @@ import ( . "github.com/onsi/gomega" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" - "github.com/openshift/cert-manager-operator/test/library" operatorclientv1alpha1 "github.com/openshift/cert-manager-operator/pkg/operator/clientset/versioned/typed/operator/v1alpha1" + "github.com/openshift/cert-manager-operator/test/library" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -97,6 +97,11 @@ func (b *trustManagerCRBuilder) WithFilterExpiredCertificates(policy v1alpha1.Fi return b } +func (b *trustManagerCRBuilder) WithFilterNonCACerts(policy v1alpha1.FilterNonCACertsPolicy) *trustManagerCRBuilder { + b.tm.Spec.TrustManagerConfig.FilterNonCACerts = policy + return b +} + func (b *trustManagerCRBuilder) Build() *v1alpha1.TrustManager { return b.tm } diff --git a/test/e2e/trustmanager_test.go b/test/e2e/trustmanager_test.go index 0b7026e7b..eb51a28e7 100644 --- a/test/e2e/trustmanager_test.go +++ b/test/e2e/trustmanager_test.go @@ -616,6 +616,31 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru }, lowTimeout, fastPollInterval).Should(Succeed()) }) + It("should add filter-non-ca-certs arg when filterNonCACerts is Enabled", func() { + createTrustManager(ctx, newTrustManagerCR(). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled)) + + By("verifying deployment args contain --filter-non-ca-certs=true") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(dep.Spec.Template.Spec.Containers).ShouldNot(BeEmpty()) + g.Expect(dep.Spec.Template.Spec.Containers[0].Args).Should(ContainElement("--filter-non-ca-certs=true")) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) + + It("should not have filter-non-ca-certs arg when filterNonCACerts is Disabled", func() { + createTrustManager(ctx, newTrustManagerCR()) + + By("verifying deployment args do not contain --filter-non-ca-certs=true") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(dep.Spec.Template.Spec.Containers).ShouldNot(BeEmpty()) + g.Expect(dep.Spec.Template.Spec.Containers[0].Args).ShouldNot(ContainElement("--filter-non-ca-certs=true")) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) + }) // ------------------------------------------------------------------------- @@ -1280,6 +1305,18 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru g.Expect(tm.Status.FilterExpiredCertificatesPolicy).Should(Equal(v1alpha1.FilterExpiredCertificatesPolicyEnabled)) }, lowTimeout, fastPollInterval).Should(Succeed()) }) + + It("should report filterNonCACerts policy in status", func() { + createTrustManager(ctx, newTrustManagerCR(). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled)) + + By("verifying status reports Enabled policy") + Eventually(func(g Gomega) { + tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(tm.Status.FilterNonCACertsPolicy).Should(Equal(v1alpha1.FilterNonCACertsPolicyEnabled)) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) }) // ------------------------------------------------------------------------- From 4ea3f276b64fa24d09ad1550dc881b4d501e8b7b Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Thu, 10 Sep 2026 17:13:53 +0530 Subject: [PATCH 2/4] CM-1367: Stop echoing TrustManager spec into status. Keep observed image only. Reuse shared Mode Enabled/Disabled for the filter and default-CA policy types instead of per-field constants. --- api/operator/v1alpha1/trustmanager_types.go | 49 ++-------- .../operator.openshift.io_trustmanagers.yaml | 32 ------ .../operator.openshift.io_trustmanagers.yaml | 32 ------ .../trustmanager/configmaps_test.go | 18 ++-- pkg/controller/trustmanager/deployments.go | 4 +- .../trustmanager/deployments_test.go | 18 ++-- .../trustmanager/install_trustmanager.go | 25 ----- .../trustmanager/install_trustmanager_test.go | 40 +++----- pkg/controller/trustmanager/utils.go | 2 +- .../operator/v1alpha1/trustmanagerstatus.go | 51 ---------- test/e2e/multiple_operands_test.go | 8 +- test/e2e/trustmanager_bundle_test.go | 12 +-- test/e2e/trustmanager_test.go | 98 +------------------ 13 files changed, 52 insertions(+), 337 deletions(-) diff --git a/api/operator/v1alpha1/trustmanager_types.go b/api/operator/v1alpha1/trustmanager_types.go index ebf9332ee..d8ffd6b42 100644 --- a/api/operator/v1alpha1/trustmanager_types.go +++ b/api/operator/v1alpha1/trustmanager_types.go @@ -226,26 +226,14 @@ type TrustManagerControllerConfig struct { Annotations map[string]string `json:"annotations,omitempty"` } -// FilterExpiredCertificatesPolicy defines the policy for filtering expired certificates. +// FilterExpiredCertificatesPolicy controls whether expired certificates are filtered from bundles. +// Allowed values are Enabled and Disabled. type FilterExpiredCertificatesPolicy string -const ( - // FilterExpiredCertificatesPolicyEnabled filters out expired certificates from bundles. - FilterExpiredCertificatesPolicyEnabled FilterExpiredCertificatesPolicy = "Enabled" - // FilterExpiredCertificatesPolicyDisabled includes expired certificates in bundles. - FilterExpiredCertificatesPolicyDisabled FilterExpiredCertificatesPolicy = "Disabled" -) - -// FilterNonCACertsPolicy defines the policy for filtering non-CA certificates. +// FilterNonCACertsPolicy controls whether non-CA certificates are filtered from bundles. +// Allowed values are Enabled and Disabled. type FilterNonCACertsPolicy string -const ( - // FilterNonCACertsPolicyEnabled filters out non-CA certificates from bundles. - FilterNonCACertsPolicyEnabled FilterNonCACertsPolicy = "Enabled" - // FilterNonCACertsPolicyDisabled includes non-CA certificates in bundles. - FilterNonCACertsPolicyDisabled FilterNonCACertsPolicy = "Disabled" -) - // SecretTargetsPolicy defines the policy for writing trust bundles to Secrets. type SecretTargetsPolicy string @@ -257,16 +245,10 @@ const ( SecretTargetsPolicyCustom SecretTargetsPolicy = "Custom" ) -// DefaultCAPackagePolicy defines the policy for the default CA package feature. +// DefaultCAPackagePolicy controls whether the default CA package feature is enabled. +// Allowed values are Enabled and Disabled. type DefaultCAPackagePolicy string -const ( - // DefaultCAPackagePolicyEnabled enables the default CA package feature. - DefaultCAPackagePolicyEnabled DefaultCAPackagePolicy = "Enabled" - // DefaultCAPackagePolicyDisabled disables the default CA package feature. - DefaultCAPackagePolicyDisabled DefaultCAPackagePolicy = "Disabled" -) - // TrustManagerStatus defines the observed state of TrustManager. type TrustManagerStatus struct { // conditions holds information about the current state of the trust-manager deployment. @@ -274,23 +256,4 @@ type TrustManagerStatus struct { // trustManagerImage is the container image (name:tag) used for trust-manager. TrustManagerImage string `json:"trustManagerImage,omitempty"` - - // trustNamespace is the namespace where trust-manager looks for trust sources. - TrustNamespace string `json:"trustNamespace,omitempty"` - - // secretTargetsPolicy indicates the current secret targets policy. - // +kubebuilder:validation:Enum:=Disabled;Custom - SecretTargetsPolicy SecretTargetsPolicy `json:"secretTargetsPolicy,omitempty"` - - // defaultCAPackagePolicy indicates the current default CA package policy. - // +kubebuilder:validation:Enum:=Enabled;Disabled - DefaultCAPackagePolicy DefaultCAPackagePolicy `json:"defaultCAPackagePolicy,omitempty"` - - // filterExpiredCertificatesPolicy indicates the current policy for filtering expired certificates. - // +kubebuilder:validation:Enum:=Enabled;Disabled - FilterExpiredCertificatesPolicy FilterExpiredCertificatesPolicy `json:"filterExpiredCertificatesPolicy,omitempty"` - - // filterNonCACertsPolicy indicates the current policy for filtering non-CA certificates. - // +kubebuilder:validation:Enum:=Enabled;Disabled - FilterNonCACertsPolicy FilterNonCACertsPolicy `json:"filterNonCACertsPolicy,omitempty"` } diff --git a/bundle/manifests/operator.openshift.io_trustmanagers.yaml b/bundle/manifests/operator.openshift.io_trustmanagers.yaml index 3ec28a0e4..680f0c22c 100644 --- a/bundle/manifests/operator.openshift.io_trustmanagers.yaml +++ b/bundle/manifests/operator.openshift.io_trustmanagers.yaml @@ -1302,42 +1302,10 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - defaultCAPackagePolicy: - description: defaultCAPackagePolicy indicates the current default - CA package policy. - enum: - - Enabled - - Disabled - type: string - filterExpiredCertificatesPolicy: - description: filterExpiredCertificatesPolicy indicates the current - policy for filtering expired certificates. - enum: - - Enabled - - Disabled - type: string - filterNonCACertsPolicy: - description: filterNonCACertsPolicy indicates the current policy for - filtering non-CA certificates. - enum: - - Enabled - - Disabled - type: string - secretTargetsPolicy: - description: secretTargetsPolicy indicates the current secret targets - policy. - enum: - - Disabled - - Custom - type: string trustManagerImage: description: trustManagerImage is the container image (name:tag) used for trust-manager. type: string - trustNamespace: - description: trustNamespace is the namespace where trust-manager looks - for trust sources. - type: string type: object required: - metadata diff --git a/config/crd/bases/operator.openshift.io_trustmanagers.yaml b/config/crd/bases/operator.openshift.io_trustmanagers.yaml index 94c96d274..b341d9004 100644 --- a/config/crd/bases/operator.openshift.io_trustmanagers.yaml +++ b/config/crd/bases/operator.openshift.io_trustmanagers.yaml @@ -1302,42 +1302,10 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - defaultCAPackagePolicy: - description: defaultCAPackagePolicy indicates the current default - CA package policy. - enum: - - Enabled - - Disabled - type: string - filterExpiredCertificatesPolicy: - description: filterExpiredCertificatesPolicy indicates the current - policy for filtering expired certificates. - enum: - - Enabled - - Disabled - type: string - filterNonCACertsPolicy: - description: filterNonCACertsPolicy indicates the current policy for - filtering non-CA certificates. - enum: - - Enabled - - Disabled - type: string - secretTargetsPolicy: - description: secretTargetsPolicy indicates the current secret targets - policy. - enum: - - Disabled - - Custom - type: string trustManagerImage: description: trustManagerImage is the container image (name:tag) used for trust-manager. type: string - trustNamespace: - description: trustNamespace is the namespace where trust-manager looks - for trust sources. - type: string type: object required: - metadata diff --git a/pkg/controller/trustmanager/configmaps_test.go b/pkg/controller/trustmanager/configmaps_test.go index 4629949b0..1a119017c 100644 --- a/pkg/controller/trustmanager/configmaps_test.go +++ b/pkg/controller/trustmanager/configmaps_test.go @@ -159,7 +159,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }{ { name: "skips when policy is Disabled", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyDisabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Disabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { }, wantExistsCount: 0, @@ -175,7 +175,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "returns error when injection ConfigMap is not found", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return errTestClient @@ -185,7 +185,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "returns error when CA bundle key is missing", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) @@ -198,7 +198,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "returns error when CA bundle is empty", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) @@ -211,7 +211,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "creates ConfigMap and returns hash when bundle is available", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) @@ -229,7 +229,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "skips patch when existing ConfigMap matches desired", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) @@ -251,7 +251,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "patches when existing ConfigMap data differs", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) @@ -271,7 +271,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "propagates Exists error", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) @@ -288,7 +288,7 @@ func TestDefaultCAPackageConfigMapReconciliation(t *testing.T) { }, { name: "propagates Patch error", - tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tm: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { cm := obj.(*corev1.ConfigMap) diff --git a/pkg/controller/trustmanager/deployments.go b/pkg/controller/trustmanager/deployments.go index 841326c80..1156689ad 100644 --- a/pkg/controller/trustmanager/deployments.go +++ b/pkg/controller/trustmanager/deployments.go @@ -143,11 +143,11 @@ func updateDeploymentArgs(deployment *appsv1.Deployment, trustManager *v1alpha1. args = append(args, "--secret-targets-enabled=true") } - if config.FilterExpiredCertificates == v1alpha1.FilterExpiredCertificatesPolicyEnabled { + if config.FilterExpiredCertificates == v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled) { args = append(args, "--filter-expired-certificates=true") } - if config.FilterNonCACerts == v1alpha1.FilterNonCACertsPolicyEnabled { + if config.FilterNonCACerts == v1alpha1.FilterNonCACertsPolicy(v1alpha1.Enabled) { args = append(args, "--filter-non-ca-certs=true") } diff --git a/pkg/controller/trustmanager/deployments_test.go b/pkg/controller/trustmanager/deployments_test.go index 3ffe761cd..cb8028a2f 100644 --- a/pkg/controller/trustmanager/deployments_test.go +++ b/pkg/controller/trustmanager/deployments_test.go @@ -158,8 +158,8 @@ func TestDeploymentContainerArgs(t *testing.T) { WithLogLevel(5). WithLogFormat("json"). WithTrustNamespace("custom-ns"). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled). - WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled), + WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled)). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicy(v1alpha1.Enabled)), expectedArgs: []string{ "--log-level=5", "--log-format=json", @@ -202,7 +202,7 @@ func TestDeploymentContainerArgs(t *testing.T) { }, { name: "includes default-package-location when defaultCAPackage is Enabled", - tmBuilder: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tmBuilder: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), expectedArgs: []string{ fmt.Sprintf("--default-package-location=%s", defaultCAPackageLocation), }, @@ -215,14 +215,14 @@ func TestDeploymentContainerArgs(t *testing.T) { }, { name: "includes filter-non-ca-certs when filterNonCACerts is Enabled", - tmBuilder: testTrustManager().WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled), + tmBuilder: testTrustManager().WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicy(v1alpha1.Enabled)), expectedArgs: []string{ "--filter-non-ca-certs=true", }, }, { name: "excludes filter-non-ca-certs when filterNonCACerts is Disabled", - tmBuilder: testTrustManager().WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyDisabled), + tmBuilder: testTrustManager().WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicy(v1alpha1.Disabled)), notExpectedArgs: []string{ "--filter-non-ca-certs=true", }, @@ -265,7 +265,7 @@ func TestDeploymentDefaultCAPackage(t *testing.T) { t.Run("adds arg, volume, mount, and hash annotation when enabled", func(t *testing.T) { r := testReconciler(t) - tm := testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled).Build() + tm := testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)).Build() dep, err := r.getDeploymentObject(tm, testResourceLabels(), testResourceAnnotations(), "abc123hash") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -517,7 +517,7 @@ func TestDeploymentReconciliation(t *testing.T) { setImage: true, preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { - tm := testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled).Build() + tm := testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)).Build() dep, err := r.getDeploymentObject(tm, testResourceLabels(), testResourceAnnotations(), "abc123hash") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -531,12 +531,12 @@ func TestDeploymentReconciliation(t *testing.T) { }, { name: "apply when existing has pod template annotation drift", - tmBuilder: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled), + tmBuilder: testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)), caBundleHash: "abc123hash", setImage: true, preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { - tm := testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled).Build() + tm := testTrustManager().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)).Build() dep, err := r.getDeploymentObject(tm, testResourceLabels(), testResourceAnnotations(), "abc123hash") if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/pkg/controller/trustmanager/install_trustmanager.go b/pkg/controller/trustmanager/install_trustmanager.go index 32294928e..0c4415314 100644 --- a/pkg/controller/trustmanager/install_trustmanager.go +++ b/pkg/controller/trustmanager/install_trustmanager.go @@ -92,31 +92,6 @@ func (r *Reconciler) updateStatusObservedState(trustManager *v1alpha1.TrustManag changed = true } - if ns := getTrustNamespace(trustManager); trustManager.Status.TrustNamespace != ns { - trustManager.Status.TrustNamespace = ns - changed = true - } - - if policy := trustManager.Spec.TrustManagerConfig.SecretTargets.Policy; trustManager.Status.SecretTargetsPolicy != policy { - trustManager.Status.SecretTargetsPolicy = policy - changed = true - } - - if policy := trustManager.Spec.TrustManagerConfig.DefaultCAPackage.Policy; trustManager.Status.DefaultCAPackagePolicy != policy { - trustManager.Status.DefaultCAPackagePolicy = policy - changed = true - } - - if policy := trustManager.Spec.TrustManagerConfig.FilterExpiredCertificates; trustManager.Status.FilterExpiredCertificatesPolicy != policy { - trustManager.Status.FilterExpiredCertificatesPolicy = policy - changed = true - } - - if policy := trustManager.Spec.TrustManagerConfig.FilterNonCACerts; trustManager.Status.FilterNonCACertsPolicy != policy { - trustManager.Status.FilterNonCACertsPolicy = policy - changed = true - } - if !changed { return nil } diff --git a/pkg/controller/trustmanager/install_trustmanager_test.go b/pkg/controller/trustmanager/install_trustmanager_test.go index b5382f9d3..d419f9b65 100644 --- a/pkg/controller/trustmanager/install_trustmanager_test.go +++ b/pkg/controller/trustmanager/install_trustmanager_test.go @@ -14,14 +14,8 @@ import ( func TestUpdateStatusObservedState(t *testing.T) { t.Setenv(trustManagerImageNameEnvVarName, testImage) - // Observed status after sync from testTrustManager() defaults (empty status fields + default spec). - wantStatusSyncedFromDefaultSpec := v1alpha1.TrustManagerStatus{ - TrustManagerImage: testImage, - TrustNamespace: defaultTrustNamespace, - SecretTargetsPolicy: "", - DefaultCAPackagePolicy: "", - FilterExpiredCertificatesPolicy: "", - FilterNonCACertsPolicy: "", + wantImageStatus := v1alpha1.TrustManagerStatus{ + TrustManagerImage: testImage, } tests := []struct { @@ -31,48 +25,36 @@ func TestUpdateStatusObservedState(t *testing.T) { wantStatus v1alpha1.TrustManagerStatus }{ { - name: "updates all observed fields when status is empty", + name: "sets trust-manager image when status is empty", trustManager: func() *v1alpha1.TrustManager { return testTrustManager().Build() }, wantStatusUpdate: 1, - wantStatus: wantStatusSyncedFromDefaultSpec, + wantStatus: wantImageStatus, }, { - name: "updates all observed fields for custom spec", + name: "does not echo spec fields into status", trustManager: func() *v1alpha1.TrustManager { return testTrustManager(). WithTrustNamespace("custom-trust-ns"). WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{"allowed-secret"}). - WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled). - WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled). + WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)). + WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled)). + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicy(v1alpha1.Enabled)). Build() }, wantStatusUpdate: 1, - wantStatus: v1alpha1.TrustManagerStatus{ - TrustManagerImage: testImage, - TrustNamespace: "custom-trust-ns", - SecretTargetsPolicy: v1alpha1.SecretTargetsPolicyCustom, - DefaultCAPackagePolicy: v1alpha1.DefaultCAPackagePolicyEnabled, - FilterExpiredCertificatesPolicy: v1alpha1.FilterExpiredCertificatesPolicyEnabled, - FilterNonCACertsPolicy: v1alpha1.FilterNonCACertsPolicyEnabled, - }, + wantStatus: wantImageStatus, }, { - name: "no-op when observed state already matches spec and env", + name: "no-op when image already matches env", trustManager: func() *v1alpha1.TrustManager { tm := testTrustManager().Build() tm.Status.TrustManagerImage = testImage - tm.Status.TrustNamespace = defaultTrustNamespace - tm.Status.SecretTargetsPolicy = tm.Spec.TrustManagerConfig.SecretTargets.Policy - tm.Status.DefaultCAPackagePolicy = tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy - tm.Status.FilterExpiredCertificatesPolicy = tm.Spec.TrustManagerConfig.FilterExpiredCertificates - tm.Status.FilterNonCACertsPolicy = tm.Spec.TrustManagerConfig.FilterNonCACerts return tm }, wantStatusUpdate: 0, - wantStatus: wantStatusSyncedFromDefaultSpec, + wantStatus: wantImageStatus, }, } diff --git a/pkg/controller/trustmanager/utils.go b/pkg/controller/trustmanager/utils.go index 3e4507959..1e921c7fd 100644 --- a/pkg/controller/trustmanager/utils.go +++ b/pkg/controller/trustmanager/utils.go @@ -146,7 +146,7 @@ func secretTargetsEnabled(config v1alpha1.SecretTargetsConfig) bool { // defaultCAPackageEnabled returns true when the defaultCAPackage policy is Enabled. func defaultCAPackageEnabled(config v1alpha1.DefaultCAPackageConfig) bool { - return config.Policy == v1alpha1.DefaultCAPackagePolicyEnabled + return config.Policy == v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled) } // getTrustNamespace returns the trust namespace from the TrustManager config. diff --git a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go index 02e1b567d..54e24f8ab 100644 --- a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go +++ b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go @@ -3,7 +3,6 @@ package v1alpha1 import ( - operatorv1alpha1 "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" v1 "k8s.io/client-go/applyconfigurations/meta/v1" ) @@ -16,16 +15,6 @@ type TrustManagerStatusApplyConfiguration struct { ConditionalStatusApplyConfiguration `json:",omitempty,inline"` // trustManagerImage is the container image (name:tag) used for trust-manager. TrustManagerImage *string `json:"trustManagerImage,omitempty"` - // trustNamespace is the namespace where trust-manager looks for trust sources. - TrustNamespace *string `json:"trustNamespace,omitempty"` - // secretTargetsPolicy indicates the current secret targets policy. - SecretTargetsPolicy *operatorv1alpha1.SecretTargetsPolicy `json:"secretTargetsPolicy,omitempty"` - // defaultCAPackagePolicy indicates the current default CA package policy. - DefaultCAPackagePolicy *operatorv1alpha1.DefaultCAPackagePolicy `json:"defaultCAPackagePolicy,omitempty"` - // filterExpiredCertificatesPolicy indicates the current policy for filtering expired certificates. - FilterExpiredCertificatesPolicy *operatorv1alpha1.FilterExpiredCertificatesPolicy `json:"filterExpiredCertificatesPolicy,omitempty"` - // filterNonCACertsPolicy indicates the current policy for filtering non-CA certificates. - FilterNonCACertsPolicy *operatorv1alpha1.FilterNonCACertsPolicy `json:"filterNonCACertsPolicy,omitempty"` } // TrustManagerStatusApplyConfiguration constructs a declarative configuration of the TrustManagerStatus type for use with @@ -54,43 +43,3 @@ func (b *TrustManagerStatusApplyConfiguration) WithTrustManagerImage(value strin b.TrustManagerImage = &value return b } - -// WithTrustNamespace sets the TrustNamespace field in the declarative configuration to the given value -// and returns the receiver, so that objects can be built by chaining "With" function invocations. -// If called multiple times, the TrustNamespace field is set to the value of the last call. -func (b *TrustManagerStatusApplyConfiguration) WithTrustNamespace(value string) *TrustManagerStatusApplyConfiguration { - b.TrustNamespace = &value - return b -} - -// WithSecretTargetsPolicy sets the SecretTargetsPolicy field in the declarative configuration to the given value -// and returns the receiver, so that objects can be built by chaining "With" function invocations. -// If called multiple times, the SecretTargetsPolicy field is set to the value of the last call. -func (b *TrustManagerStatusApplyConfiguration) WithSecretTargetsPolicy(value operatorv1alpha1.SecretTargetsPolicy) *TrustManagerStatusApplyConfiguration { - b.SecretTargetsPolicy = &value - return b -} - -// WithDefaultCAPackagePolicy sets the DefaultCAPackagePolicy field in the declarative configuration to the given value -// and returns the receiver, so that objects can be built by chaining "With" function invocations. -// If called multiple times, the DefaultCAPackagePolicy field is set to the value of the last call. -func (b *TrustManagerStatusApplyConfiguration) WithDefaultCAPackagePolicy(value operatorv1alpha1.DefaultCAPackagePolicy) *TrustManagerStatusApplyConfiguration { - b.DefaultCAPackagePolicy = &value - return b -} - -// WithFilterExpiredCertificatesPolicy sets the FilterExpiredCertificatesPolicy field in the declarative configuration to the given value -// and returns the receiver, so that objects can be built by chaining "With" function invocations. -// If called multiple times, the FilterExpiredCertificatesPolicy field is set to the value of the last call. -func (b *TrustManagerStatusApplyConfiguration) WithFilterExpiredCertificatesPolicy(value operatorv1alpha1.FilterExpiredCertificatesPolicy) *TrustManagerStatusApplyConfiguration { - b.FilterExpiredCertificatesPolicy = &value - return b -} - -// WithFilterNonCACertsPolicy sets the FilterNonCACertsPolicy field in the declarative configuration to the given value -// and returns the receiver, so that objects can be built by chaining "With" function invocations. -// If called multiple times, the FilterNonCACertsPolicy field is set to the value of the last call. -func (b *TrustManagerStatusApplyConfiguration) WithFilterNonCACertsPolicy(value operatorv1alpha1.FilterNonCACertsPolicy) *TrustManagerStatusApplyConfiguration { - b.FilterNonCACertsPolicy = &value - return b -} diff --git a/test/e2e/multiple_operands_test.go b/test/e2e/multiple_operands_test.go index a12b8aaa5..aeb62e74a 100644 --- a/test/e2e/multiple_operands_test.go +++ b/test/e2e/multiple_operands_test.go @@ -254,8 +254,8 @@ func multiOperandTrustManagerCR() *trustManagerCRBuilder { return newTrustManagerCR(). WithLabels(map[string]string{"env": "trustmanager-test"}). WithAnnotations(map[string]string{"trustmanager.operator.openshift.io/cluster": "trustmanager-test"}). - WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled). + WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled)). + WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled)). WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{"ca-bundle-secret", multiOperandBundleName}). WithTrustNamespace(trustManagerNamespace) } @@ -554,9 +554,9 @@ func assertTrustManagerCRConfigPropagation(ctx context.Context, clientset *kuber tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) Expect(err).NotTo(HaveOccurred()) - Expect(tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy).To(Equal(v1alpha1.DefaultCAPackagePolicyEnabled)) + Expect(tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy).To(Equal(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled))) Expect(tm.Spec.TrustManagerConfig.SecretTargets.Policy).To(Equal(v1alpha1.SecretTargetsPolicyCustom)) - Expect(tm.Spec.TrustManagerConfig.FilterExpiredCertificates).To(Equal(v1alpha1.FilterExpiredCertificatesPolicyEnabled)) + Expect(tm.Spec.TrustManagerConfig.FilterExpiredCertificates).To(Equal(v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled))) } func runMultiOperandBundleSecretTargetTest(ctx context.Context, sourcePEM string) { diff --git a/test/e2e/trustmanager_bundle_test.go b/test/e2e/trustmanager_bundle_test.go index 86981a3f2..67655a8d1 100644 --- a/test/e2e/trustmanager_bundle_test.go +++ b/test/e2e/trustmanager_bundle_test.go @@ -560,7 +560,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana // ===== Group 3: DefaultCAPackage enabled ===== Context("with DefaultCAPackage enabled", Ordered, func() { BeforeAll(func() { - createTrustManager(ctx, newTrustManagerCR().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled)) + createTrustManager(ctx, newTrustManagerCR().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled))) By("waiting for default CA package ConfigMap to be created") err := pollTillConfigMapAvailable(ctx, k8sClientSet, trustManagerNamespace, defaultCAPackageConfigMapName) @@ -871,7 +871,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana BeforeAll(func() { createTrustManager(ctx, newTrustManagerCR(). WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{bundleCombined}). - WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled)) + WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled))) By("waiting for default CA package ConfigMap to be created") err := pollTillConfigMapAvailable(ctx, k8sClientSet, trustManagerNamespace, defaultCAPackageConfigMapName) @@ -1017,7 +1017,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana BeforeAll(func() { createTrustManager(ctx, newTrustManagerCR(). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled)) + WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled))) sourceCMName = "filter-src-cm-" + randomStr(5) filterBundleName = "bundle-filter-expired-" + randomStr(5) @@ -1059,7 +1059,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana if err != nil { return err } - tm.Spec.TrustManagerConfig.FilterExpiredCertificates = v1alpha1.FilterExpiredCertificatesPolicyDisabled + tm.Spec.TrustManagerConfig.FilterExpiredCertificates = v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Disabled) _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) return err }, lowTimeout, fastPollInterval).Should(Succeed()) @@ -1088,7 +1088,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana BeforeAll(func() { createTrustManager(ctx, newTrustManagerCR(). - WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled)) + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicy(v1alpha1.Enabled))) sourceCMName = "filter-nca-src-cm-" + randomStr(5) filterBundleName = "bundle-filter-non-ca-" + randomStr(5) @@ -1130,7 +1130,7 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana if err != nil { return err } - tm.Spec.TrustManagerConfig.FilterNonCACerts = v1alpha1.FilterNonCACertsPolicyDisabled + tm.Spec.TrustManagerConfig.FilterNonCACerts = v1alpha1.FilterNonCACertsPolicy(v1alpha1.Disabled) _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) return err }, lowTimeout, fastPollInterval).Should(Succeed()) diff --git a/test/e2e/trustmanager_test.go b/test/e2e/trustmanager_test.go index eb51a28e7..b8feba608 100644 --- a/test/e2e/trustmanager_test.go +++ b/test/e2e/trustmanager_test.go @@ -593,7 +593,7 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru It("should add filter-expired-certificates arg when filterExpiredCertificates is Enabled", func() { createTrustManager(ctx, newTrustManagerCR(). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled)) + WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicy(v1alpha1.Enabled))) By("verifying deployment args contain --filter-expired-certificates=true") Eventually(func(g Gomega) { @@ -618,7 +618,7 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru It("should add filter-non-ca-certs arg when filterNonCACerts is Enabled", func() { createTrustManager(ctx, newTrustManagerCR(). - WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled)) + WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicy(v1alpha1.Enabled))) By("verifying deployment args contain --filter-non-ca-certs=true") Eventually(func(g Gomega) { @@ -693,7 +693,7 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru if err != nil { return err } - tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy = v1alpha1.DefaultCAPackagePolicyEnabled + tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy = v1alpha1.DefaultCAPackagePolicy(v1alpha1.Enabled) _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) return err }, lowTimeout, fastPollInterval).Should(Succeed()) @@ -775,7 +775,7 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru if err != nil { return err } - tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy = v1alpha1.DefaultCAPackagePolicyDisabled + tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy = v1alpha1.DefaultCAPackagePolicy(v1alpha1.Disabled) _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) return err }, lowTimeout, fastPollInterval).Should(Succeed()) @@ -1227,96 +1227,6 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru g.Expect(tm.Status.TrustManagerImage).ShouldNot(BeEmpty()) }, lowTimeout, fastPollInterval).Should(Succeed()) }) - - It("should report trust namespace in status", func() { - createTrustManager(ctx, newTrustManagerCR()) - - By("verifying TrustManager status has default trust namespace set") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.TrustNamespace).Should(Equal("cert-manager")) - }, lowTimeout, fastPollInterval).Should(Succeed()) - }) - - It("should report custom trust namespace in status", func() { - By("creating custom trust namespace") - customTrustNS := createUniqueNamespace("custom-trust-ns-status") - createAndDestroyTestNamespace(ctx, clientset, customTrustNS) - - createTrustManager(ctx, newTrustManagerCR().WithTrustNamespace(customTrustNS)) - - By("verifying TrustManager status has custom trust namespace set") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.TrustNamespace).Should(Equal(customTrustNS)) - }, lowTimeout, fastPollInterval).Should(Succeed()) - }) - - It("should report secretTargets policy in status", func() { - createTrustManager(ctx, newTrustManagerCR().WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{"status-test-secret"})) - - By("verifying TrustManager status reflects Custom secretTargets policy") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.SecretTargetsPolicy).Should(Equal(v1alpha1.SecretTargetsPolicyCustom)) - }, lowTimeout, fastPollInterval).Should(Succeed()) - }) - - It("should report default CA package policy in status", func() { - createTrustManager(ctx, newTrustManagerCR().WithDefaultCAPackage(v1alpha1.DefaultCAPackagePolicyEnabled)) - - By("verifying status reports Enabled policy") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.DefaultCAPackagePolicy).Should(Equal(v1alpha1.DefaultCAPackagePolicyEnabled)) - }, lowTimeout, fastPollInterval).Should(Succeed()) - - By("updating TrustManager CR to disable default CA package") - Eventually(func() error { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - return err - } - tm.Spec.TrustManagerConfig.DefaultCAPackage.Policy = v1alpha1.DefaultCAPackagePolicyDisabled - _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) - return err - }, lowTimeout, fastPollInterval).Should(Succeed()) - - By("verifying status reports Disabled policy") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.DefaultCAPackagePolicy).Should(Equal(v1alpha1.DefaultCAPackagePolicyDisabled)) - }, lowTimeout, fastPollInterval).Should(Succeed()) - }) - - It("should report filterExpiredCertificates policy in status", func() { - createTrustManager(ctx, newTrustManagerCR(). - WithFilterExpiredCertificates(v1alpha1.FilterExpiredCertificatesPolicyEnabled)) - - By("verifying status reports Enabled policy") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.FilterExpiredCertificatesPolicy).Should(Equal(v1alpha1.FilterExpiredCertificatesPolicyEnabled)) - }, lowTimeout, fastPollInterval).Should(Succeed()) - }) - - It("should report filterNonCACerts policy in status", func() { - createTrustManager(ctx, newTrustManagerCR(). - WithFilterNonCACerts(v1alpha1.FilterNonCACertsPolicyEnabled)) - - By("verifying status reports Enabled policy") - Eventually(func(g Gomega) { - tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(tm.Status.FilterNonCACertsPolicy).Should(Equal(v1alpha1.FilterNonCACertsPolicyEnabled)) - }, lowTimeout, fastPollInterval).Should(Succeed()) - }) }) // ------------------------------------------------------------------------- From dc091223fd655d5be348654139fccf22360bf7d6 Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Wed, 16 Sep 2026 15:30:21 +0530 Subject: [PATCH 3/4] CM-1367: Add TrustManager targetNamespaces API. Limit Bundle target writes to listed namespaces and scope operand RBAC to per-namespace Roles, matching upstream Helm. --- .../trustmanager.testsuite.yaml | 216 ++++++++++++ api/operator/v1alpha1/trustmanager_types.go | 18 + .../v1alpha1/zz_generated.deepcopy.go | 5 + .../operator.openshift.io_trustmanagers.yaml | 27 +- .../operator.openshift.io_trustmanagers.yaml | 19 + pkg/controller/trustmanager/constants.go | 5 + pkg/controller/trustmanager/controller.go | 2 +- pkg/controller/trustmanager/deployments.go | 7 + .../trustmanager/deployments_test.go | 32 +- pkg/controller/trustmanager/rbacs.go | 216 +++++++++++- pkg/controller/trustmanager/rbacs_test.go | 332 +++++++++++++++++- pkg/controller/trustmanager/test_utils.go | 5 + .../operator/v1alpha1/trustmanagerconfig.go | 19 + test/e2e/trustmanager_bundle_test.go | 49 +++ test/e2e/trustmanager_helpers_test.go | 9 + test/e2e/trustmanager_test.go | 113 +++++- 16 files changed, 1041 insertions(+), 33 deletions(-) diff --git a/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml b/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml index edb326e31..1497c97ae 100644 --- a/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml +++ b/api/operator/v1alpha1/tests/trustmanagers.operator.openshift.io/trustmanager.testsuite.yaml @@ -195,6 +195,139 @@ tests: filterNonCACerts: Invalid expectedError: "filterNonCACerts" + # ========================================== + # TargetNamespaces Tests + # ========================================== + - name: Should create with targetNamespaces + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - app-ns + - other-ns + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + logLevel: 1 + logFormat: text + trustNamespace: cert-manager + targetNamespaces: + - app-ns + - other-ns + filterExpiredCertificates: Disabled + filterNonCACerts: Disabled + + - name: Should not allow invalid targetNamespaces value + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - Invalid_NS + expectedError: "targetNamespaces" + + - name: Should not allow empty targetNamespaces item + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - "" + expectedError: "targetNamespaces" + + - name: Should not allow duplicate targetNamespaces items + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - app-ns + - app-ns + expectedError: "targetNamespaces" + + - name: Should not allow targetNamespaces item longer than 63 characters + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + expectedError: "targetNamespaces" + + - name: Should not allow more than 50 targetNamespaces items + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - ns-00 + - ns-01 + - ns-02 + - ns-03 + - ns-04 + - ns-05 + - ns-06 + - ns-07 + - ns-08 + - ns-09 + - ns-10 + - ns-11 + - ns-12 + - ns-13 + - ns-14 + - ns-15 + - ns-16 + - ns-17 + - ns-18 + - ns-19 + - ns-20 + - ns-21 + - ns-22 + - ns-23 + - ns-24 + - ns-25 + - ns-26 + - ns-27 + - ns-28 + - ns-29 + - ns-30 + - ns-31 + - ns-32 + - ns-33 + - ns-34 + - ns-35 + - ns-36 + - ns-37 + - ns-38 + - ns-39 + - ns-40 + - ns-41 + - ns-42 + - ns-43 + - ns-44 + - ns-45 + - ns-46 + - ns-47 + - ns-48 + - ns-49 + - ns-50 + expectedError: "targetNamespaces" + # ========================================== # DefaultCAPackage Tests # ========================================== @@ -589,6 +722,89 @@ tests: filterExpiredCertificates: Disabled filterNonCACerts: Enabled + - name: Should allow updating targetNamespaces + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - app-ns + updated: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - app-ns + - other-ns + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + logLevel: 1 + logFormat: text + trustNamespace: cert-manager + targetNamespaces: + - app-ns + - other-ns + filterExpiredCertificates: Disabled + filterNonCACerts: Disabled + + - name: Should allow clearing targetNamespaces to restore cluster-wide + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - app-ns + updated: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: {} + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + logLevel: 1 + logFormat: text + trustNamespace: cert-manager + filterExpiredCertificates: Disabled + filterNonCACerts: Disabled + + - name: Should allow clearing targetNamespaces with an empty list + resourceName: cluster + initial: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: + - app-ns + updated: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + targetNamespaces: [] + expected: | + apiVersion: operator.openshift.io/v1alpha1 + kind: TrustManager + spec: + trustManagerConfig: + logLevel: 1 + logFormat: text + trustNamespace: cert-manager + targetNamespaces: [] + filterExpiredCertificates: Disabled + filterNonCACerts: Disabled + - name: Should allow updating defaultCAPackage policy resourceName: cluster initial: | diff --git a/api/operator/v1alpha1/trustmanager_types.go b/api/operator/v1alpha1/trustmanager_types.go index d8ffd6b42..44a83a7ff 100644 --- a/api/operator/v1alpha1/trustmanager_types.go +++ b/api/operator/v1alpha1/trustmanager_types.go @@ -103,6 +103,24 @@ type TrustManagerConfig struct { // +optional TrustNamespace string `json:"trustNamespace,omitempty"` + // targetNamespaces limits where trust-manager writes Bundle targets + // (ConfigMaps, and Secrets when secretTargets is enabled). + // When empty or omitted, trust-manager writes targets in all namespaces + // (default behavior). + // When set, Bundle targets are written only in the listed namespaces. + // Trust sources are still read from trustNamespace. + // Removing a namespace from this list does not delete existing target + // ConfigMaps or Secrets in that namespace. + // +listType=set + // +kubebuilder:validation:MinItems:=0 + // +kubebuilder:validation:MaxItems:=50 + // +kubebuilder:validation:items:MinLength:=1 + // +kubebuilder:validation:items:MaxLength:=63 + // +kubebuilder:validation:items:Pattern:=^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + // +kubebuilder:validation:Optional + // +optional + TargetNamespaces []string `json:"targetNamespaces,omitempty"` + // secretTargets configures whether trust-manager can write trust bundles to Secrets. // +kubebuilder:validation:Optional // +optional diff --git a/api/operator/v1alpha1/zz_generated.deepcopy.go b/api/operator/v1alpha1/zz_generated.deepcopy.go index 883cddf1b..bada02be0 100644 --- a/api/operator/v1alpha1/zz_generated.deepcopy.go +++ b/api/operator/v1alpha1/zz_generated.deepcopy.go @@ -615,6 +615,11 @@ func (in *TrustManager) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TrustManagerConfig) DeepCopyInto(out *TrustManagerConfig) { *out = *in + if in.TargetNamespaces != nil { + in, out := &in.TargetNamespaces, &out.TargetNamespaces + *out = make([]string, len(*in)) + copy(*out, *in) + } in.SecretTargets.DeepCopyInto(&out.SecretTargets) out.DefaultCAPackage = in.DefaultCAPackage in.Resources.DeepCopyInto(&out.Resources) diff --git a/bundle/manifests/operator.openshift.io_trustmanagers.yaml b/bundle/manifests/operator.openshift.io_trustmanagers.yaml index 680f0c22c..d8bb1d62c 100644 --- a/bundle/manifests/operator.openshift.io_trustmanagers.yaml +++ b/bundle/manifests/operator.openshift.io_trustmanagers.yaml @@ -1,9 +1,9 @@ +--- apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: controller-gen.kubebuilder.io/version: v0.19.0 - creationTimestamp: null labels: app.kubernetes.io/name: trustmanager app.kubernetes.io/part-of: cert-manager-operator @@ -1175,6 +1175,25 @@ spec: Custom rule: self.policy == 'Custom' || !has(self.authorizedSecrets) || size(self.authorizedSecrets) == 0 + targetNamespaces: + description: |- + targetNamespaces limits where trust-manager writes Bundle targets + (ConfigMaps, and Secrets when secretTargets is enabled). + When empty or omitted, trust-manager writes targets in all namespaces + (default behavior). + When set, Bundle targets are written only in the listed namespaces. + Trust sources are still read from trustNamespace. + Removing a namespace from this list does not delete existing target + ConfigMaps or Secrets in that namespace. + items: + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string + maxItems: 50 + minItems: 0 + type: array + x-kubernetes-list-type: set tolerations: description: |- tolerations allows the trust-manager pod to be scheduled on tainted nodes. @@ -1318,9 +1337,3 @@ spec: storage: true subresources: status: {} -status: - acceptedNames: - kind: "" - plural: "" - conditions: null - storedVersions: null diff --git a/config/crd/bases/operator.openshift.io_trustmanagers.yaml b/config/crd/bases/operator.openshift.io_trustmanagers.yaml index b341d9004..d8bb1d62c 100644 --- a/config/crd/bases/operator.openshift.io_trustmanagers.yaml +++ b/config/crd/bases/operator.openshift.io_trustmanagers.yaml @@ -1175,6 +1175,25 @@ spec: Custom rule: self.policy == 'Custom' || !has(self.authorizedSecrets) || size(self.authorizedSecrets) == 0 + targetNamespaces: + description: |- + targetNamespaces limits where trust-manager writes Bundle targets + (ConfigMaps, and Secrets when secretTargets is enabled). + When empty or omitted, trust-manager writes targets in all namespaces + (default behavior). + When set, Bundle targets are written only in the listed namespaces. + Trust sources are still read from trustNamespace. + Removing a namespace from this list does not delete existing target + ConfigMaps or Secrets in that namespace. + items: + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string + maxItems: 50 + minItems: 0 + type: array + x-kubernetes-list-type: set tolerations: description: |- tolerations allows the trust-manager pod to be scheduled on tainted nodes. diff --git a/pkg/controller/trustmanager/constants.go b/pkg/controller/trustmanager/constants.go index 11bfccc25..1dc63382f 100644 --- a/pkg/controller/trustmanager/constants.go +++ b/pkg/controller/trustmanager/constants.go @@ -92,6 +92,11 @@ const ( trustManagerRoleName = trustManagerCommonResourceName trustManagerRoleBindingName = trustManagerCommonResourceName + // Namespaced RBAC used when targetNamespaces is set. Distinct from + // trustManagerRoleName, which is the trust-namespace source-secret Role. + trustManagerTargetRoleName = trustManagerCommonResourceName + "-target" + trustManagerTargetRoleBindingName = trustManagerCommonResourceName + "-target" + trustManagerLeaderElectionRoleName = trustManagerCommonResourceName + ":leaderelection" trustManagerLeaderElectionRoleBindingName = trustManagerCommonResourceName + ":leaderelection" diff --git a/pkg/controller/trustmanager/controller.go b/pkg/controller/trustmanager/controller.go index 9445542f9..ead739685 100644 --- a/pkg/controller/trustmanager/controller.go +++ b/pkg/controller/trustmanager/controller.go @@ -53,7 +53,7 @@ type Reconciler struct { // +kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles;clusterrolebindings,verbs=get;list;watch;create;update;patch -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates;issuers,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=trust.cert-manager.io,resources=bundles,verbs=get;list;watch diff --git a/pkg/controller/trustmanager/deployments.go b/pkg/controller/trustmanager/deployments.go index 1156689ad..d8f00904c 100644 --- a/pkg/controller/trustmanager/deployments.go +++ b/pkg/controller/trustmanager/deployments.go @@ -6,6 +6,7 @@ import ( "os" "reflect" "slices" + "strings" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -151,6 +152,12 @@ func updateDeploymentArgs(deployment *appsv1.Deployment, trustManager *v1alpha1. args = append(args, "--filter-non-ca-certs=true") } + if len(config.TargetNamespaces) > 0 { + ns := slices.Clone(config.TargetNamespaces) + slices.Sort(ns) + args = append(args, "--target-namespaces="+strings.Join(ns, ",")) + } + if defaultCAPackageEnabled(config.DefaultCAPackage) { args = append(args, fmt.Sprintf("--default-package-location=%s", defaultCAPackageLocation)) } diff --git a/pkg/controller/trustmanager/deployments_test.go b/pkg/controller/trustmanager/deployments_test.go index cb8028a2f..ea699552f 100644 --- a/pkg/controller/trustmanager/deployments_test.go +++ b/pkg/controller/trustmanager/deployments_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "slices" + "strings" "testing" appsv1 "k8s.io/api/apps/v1" @@ -130,10 +131,11 @@ func TestDeploymentSpec(t *testing.T) { func TestDeploymentContainerArgs(t *testing.T) { tests := []struct { - name string - tmBuilder *trustManagerBuilder - expectedArgs []string - notExpectedArgs []string + name string + tmBuilder *trustManagerBuilder + expectedArgs []string + notExpectedArgs []string + notExpectedArgPrefixes []string }{ { name: "default values", @@ -151,6 +153,7 @@ func TestDeploymentContainerArgs(t *testing.T) { "--filter-non-ca-certs=true", fmt.Sprintf("--default-package-location=%s", defaultCAPackageLocation), }, + notExpectedArgPrefixes: []string{"--target-namespaces="}, }, { name: "custom values", @@ -227,6 +230,20 @@ func TestDeploymentContainerArgs(t *testing.T) { "--filter-non-ca-certs=true", }, }, + { + name: "includes sorted target-namespaces when set", + tmBuilder: testTrustManager().WithTargetNamespaces("zeta-ns", "alpha-ns"), + expectedArgs: []string{ + "--target-namespaces=alpha-ns,zeta-ns", + }, + }, + { + name: "excludes target-namespaces when unset", + tmBuilder: testTrustManager(), + notExpectedArgPrefixes: []string{ + "--target-namespaces=", + }, + }, } for _, tt := range tests { @@ -256,6 +273,13 @@ func TestDeploymentContainerArgs(t *testing.T) { t.Errorf("unexpected arg %q found in %v", notExpected, args) } } + for _, prefix := range tt.notExpectedArgPrefixes { + for _, arg := range args { + if strings.HasPrefix(arg, prefix) { + t.Errorf("unexpected arg prefix %q found in %v", prefix, args) + } + } + } }) } } diff --git a/pkg/controller/trustmanager/rbacs.go b/pkg/controller/trustmanager/rbacs.go index d8c3bcc04..7aca6811a 100644 --- a/pkg/controller/trustmanager/rbacs.go +++ b/pkg/controller/trustmanager/rbacs.go @@ -7,6 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" @@ -45,13 +46,23 @@ func (r *Reconciler) createOrApplyRBACResources(trustManager *v1alpha1.TrustMana return err } + if err := r.createOrApplyTargetNamespaceRBAC(trustManager, resourceLabels, resourceAnnotations, trustNamespace); err != nil { + r.log.Error(err, "failed to reconcile target namespace RBAC resources") + return err + } + + if err := r.cleanupStaleTargetNamespaceRBAC(trustManager, trustNamespace); err != nil { + r.log.Error(err, "failed to clean up stale target namespace RBAC resources") + return err + } + return nil } // ClusterRole func (r *Reconciler) createOrApplyClusterRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { - desired := getClusterRoleObject(trustManager.Spec.TrustManagerConfig.SecretTargets, resourceLabels, resourceAnnotations) + desired := getClusterRoleObject(trustManager.Spec.TrustManagerConfig, resourceLabels, resourceAnnotations) resourceName := desired.GetName() r.log.V(4).Info("reconciling clusterrole resource", "name", resourceName) @@ -74,24 +85,36 @@ func (r *Reconciler) createOrApplyClusterRole(trustManager *v1alpha1.TrustManage return nil } -func getClusterRoleObject(secretTargets v1alpha1.SecretTargetsConfig, resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRole { +func getClusterRoleObject(config v1alpha1.TrustManagerConfig, resourceLabels, resourceAnnotations map[string]string) *rbacv1.ClusterRole { clusterRole := common.DecodeObjBytes[*rbacv1.ClusterRole](codecs, rbacv1.SchemeGroupVersion, assets.MustAsset(clusterRoleAssetName)) common.UpdateName(clusterRole, trustManagerClusterRoleName) common.UpdateResourceLabels(clusterRole, resourceLabels) updateResourceAnnotations(clusterRole, resourceAnnotations) - appendSecretTargetRules(clusterRole, secretTargets) + if len(config.TargetNamespaces) > 0 { + // Match upstream Helm: drop cluster-wide ConfigMap/Event write and + // emit those rules as per-namespace Roles instead. + clusterRole.Rules = slices.DeleteFunc(clusterRole.Rules, isNamespacedTargetRule) + } else { + appendSecretTargetRules(&clusterRole.Rules, config.SecretTargets) + } return clusterRole } -// appendSecretTargetRules adds cluster-wide secret read and scoped write rules -// to the ClusterRole when the secretTargets policy is Custom. The authorizedSecrets -// list is sorted to ensure deterministic rule ordering for comparison. -func appendSecretTargetRules(clusterRole *rbacv1.ClusterRole, secretTargets v1alpha1.SecretTargetsConfig) { +// isNamespacedTargetRule reports whether the rule is the ConfigMap or Event +// write that Helm moves onto per-namespace Roles when targetNamespaces is set. +func isNamespacedTargetRule(rule rbacv1.PolicyRule) bool { + return slices.Contains(rule.Resources, "configmaps") || slices.Contains(rule.Resources, "events") +} + +// appendSecretTargetRules adds secret read and scoped write rules when the +// secretTargets policy is Custom. The authorizedSecrets list is sorted to +// ensure deterministic rule ordering for comparison. +func appendSecretTargetRules(rules *[]rbacv1.PolicyRule, secretTargets v1alpha1.SecretTargetsConfig) { if !secretTargetsEnabled(secretTargets) { return } - clusterRole.Rules = append(clusterRole.Rules, rbacv1.PolicyRule{ + *rules = append(*rules, rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"secrets"}, Verbs: []string{"get", "list", "watch"}, @@ -100,7 +123,7 @@ func appendSecretTargetRules(clusterRole *rbacv1.ClusterRole, secretTargets v1al sortedSecrets := slices.Clone(secretTargets.AuthorizedSecrets) slices.Sort(sortedSecrets) - clusterRole.Rules = append(clusterRole.Rules, rbacv1.PolicyRule{ + *rules = append(*rules, rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"secrets"}, ResourceNames: sortedSecrets, @@ -108,6 +131,25 @@ func appendSecretTargetRules(clusterRole *rbacv1.ClusterRole, secretTargets v1al }) } +// namespacedTargetRules is the ConfigMap write (+ Events, optional Secrets) +// granted in each target namespace when targetNamespaces is set. +func namespacedTargetRules(secretTargets v1alpha1.SecretTargetsConfig) []rbacv1.PolicyRule { + rules := []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + Verbs: []string{"get", "list", "create", "patch", "watch", "delete"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"events"}, + Verbs: []string{"create", "patch"}, + }, + } + appendSecretTargetRules(&rules, secretTargets) + return rules +} + // ClusterRoleBinding func (r *Reconciler) createOrApplyClusterRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string) error { @@ -288,6 +330,162 @@ func getLeaderElectionRoleBindingObject(resourceLabels, resourceAnnotations map[ return roleBinding } +// Per-namespace Role/RoleBinding for Bundle target writes when targetNamespaces is set. + +func (r *Reconciler) createOrApplyTargetNamespaceRBAC(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, trustNamespace string) error { + for _, ns := range targetRBACNamespaces(trustManager, trustNamespace) { + if err := r.createOrApplyTargetNamespaceRole(trustManager, resourceLabels, resourceAnnotations, ns); err != nil { + return err + } + if err := r.createOrApplyTargetNamespaceRoleBinding(trustManager, resourceLabels, resourceAnnotations, ns); err != nil { + return err + } + } + return nil +} + +// targetRBACNamespaces is the unique, sorted set of namespaces that receive +// namespaced target write Roles: listed targetNamespaces plus trustNamespace +// (upstream cache always includes the trust namespace). +func targetRBACNamespaces(trustManager *v1alpha1.TrustManager, trustNamespace string) []string { + if len(trustManager.Spec.TrustManagerConfig.TargetNamespaces) == 0 { + return nil + } + ns := slices.Clone(trustManager.Spec.TrustManagerConfig.TargetNamespaces) + ns = append(ns, trustNamespace) + slices.Sort(ns) + return slices.Compact(ns) +} + +func (r *Reconciler) createOrApplyTargetNamespaceRole(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, namespace string) error { + desired := getTargetNamespaceRoleObject(namespace, trustManager.Spec.TrustManagerConfig.SecretTargets, resourceLabels, resourceAnnotations) + resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) + r.log.V(4).Info("reconciling target namespace role resource", "name", resourceName) + + existing := &rbacv1.Role{} + exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) + if err != nil { + return common.FromClientError(err, "failed to check if target namespace role %q exists", resourceName) + } + if exists && !roleModified(desired, existing) { + r.log.V(4).Info("target namespace role resource exists and is in desired state", "name", resourceName) + return nil + } + + r.log.V(2).Info("target namespace role resource has been modified, updating to desired state", "name", resourceName) + if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { + return common.FromClientError(err, "failed to apply target namespace role %q", resourceName) + } + + r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "target namespace role resource %s applied", resourceName) + return nil +} + +func getTargetNamespaceRoleObject(namespace string, secretTargets v1alpha1.SecretTargetsConfig, resourceLabels, resourceAnnotations map[string]string) *rbacv1.Role { + role := common.DecodeObjBytes[*rbacv1.Role](codecs, rbacv1.SchemeGroupVersion, assets.MustAsset(roleAssetName)) + common.UpdateName(role, trustManagerTargetRoleName) + common.UpdateNamespace(role, namespace) + common.UpdateResourceLabels(role, resourceLabels) + updateResourceAnnotations(role, resourceAnnotations) + role.Rules = namespacedTargetRules(secretTargets) + return role +} + +func (r *Reconciler) createOrApplyTargetNamespaceRoleBinding(trustManager *v1alpha1.TrustManager, resourceLabels, resourceAnnotations map[string]string, namespace string) error { + desired := getTargetNamespaceRoleBindingObject(namespace, resourceLabels, resourceAnnotations) + resourceName := fmt.Sprintf("%s/%s", desired.GetNamespace(), desired.GetName()) + r.log.V(4).Info("reconciling target namespace rolebinding resource", "name", resourceName) + + existing := &rbacv1.RoleBinding{} + exists, err := r.Exists(r.ctx, client.ObjectKeyFromObject(desired), existing) + if err != nil { + return common.FromClientError(err, "failed to check if target namespace rolebinding %q exists", resourceName) + } + if exists && !roleBindingModified(desired, existing) { + r.log.V(4).Info("target namespace rolebinding resource exists and is in desired state", "name", resourceName) + return nil + } + + r.log.V(2).Info("target namespace rolebinding resource has been modified, updating to desired state", "name", resourceName) + if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil { + return common.FromClientError(err, "failed to apply target namespace rolebinding %q", resourceName) + } + + r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "target namespace rolebinding resource %s applied", resourceName) + return nil +} + +func getTargetNamespaceRoleBindingObject(namespace string, resourceLabels, resourceAnnotations map[string]string) *rbacv1.RoleBinding { + roleBinding := common.DecodeObjBytes[*rbacv1.RoleBinding](codecs, rbacv1.SchemeGroupVersion, assets.MustAsset(roleBindingAssetName)) + common.UpdateName(roleBinding, trustManagerTargetRoleBindingName) + common.UpdateNamespace(roleBinding, namespace) + common.UpdateResourceLabels(roleBinding, resourceLabels) + updateResourceAnnotations(roleBinding, resourceAnnotations) + roleBinding.RoleRef.Name = trustManagerTargetRoleName + updateBindingSubjects(roleBinding.Subjects, trustManagerServiceAccountName, operandNamespace) + return roleBinding +} + +// cleanupStaleTargetNamespaceRBAC deletes leftover trust-manager-target Roles +// and RoleBindings in namespaces that are no longer in the desired set. +// Shrinking targetNamespaces does not delete leftover target ConfigMaps/Secrets; +// leftover write permission would, so the Roles must go. +func (r *Reconciler) cleanupStaleTargetNamespaceRBAC(trustManager *v1alpha1.TrustManager, trustNamespace string) error { + desired := make(map[string]struct{}) + for _, ns := range targetRBACNamespaces(trustManager, trustNamespace) { + desired[ns] = struct{}{} + } + + if err := r.deleteStaleTargetRoles(trustManager, desired); err != nil { + return err + } + return r.deleteStaleTargetRoleBindings(trustManager, desired) +} + +func (r *Reconciler) deleteStaleTargetRoles(trustManager *v1alpha1.TrustManager, desired map[string]struct{}) error { + var roles rbacv1.RoleList + if err := r.List(r.ctx, &roles, client.MatchingLabels{common.ManagedResourceLabelKey: RequestEnqueueLabelValue}); err != nil { + return common.FromClientError(err, "failed to list target namespace roles") + } + for i := range roles.Items { + role := &roles.Items[i] + if role.Name != trustManagerTargetRoleName { + continue + } + if _, keep := desired[role.Namespace]; keep { + continue + } + if err := r.Delete(r.ctx, role); err != nil && !apierrors.IsNotFound(err) { + return common.FromClientError(err, "failed to delete target namespace role %s/%s", role.Namespace, role.Name) + } + r.log.V(2).Info("deleted stale target namespace role", "namespace", role.Namespace, "name", role.Name) + r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "stale target namespace role %s/%s deleted", role.Namespace, role.Name) + } + return nil +} + +func (r *Reconciler) deleteStaleTargetRoleBindings(trustManager *v1alpha1.TrustManager, desired map[string]struct{}) error { + var roleBindings rbacv1.RoleBindingList + if err := r.List(r.ctx, &roleBindings, client.MatchingLabels{common.ManagedResourceLabelKey: RequestEnqueueLabelValue}); err != nil { + return common.FromClientError(err, "failed to list target namespace rolebindings") + } + for i := range roleBindings.Items { + rb := &roleBindings.Items[i] + if rb.Name != trustManagerTargetRoleBindingName { + continue + } + if _, keep := desired[rb.Namespace]; keep { + continue + } + if err := r.Delete(r.ctx, rb); err != nil && !apierrors.IsNotFound(err) { + return common.FromClientError(err, "failed to delete target namespace rolebinding %s/%s", rb.Namespace, rb.Name) + } + r.log.V(2).Info("deleted stale target namespace rolebinding", "namespace", rb.Namespace, "name", rb.Name) + r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "stale target namespace rolebinding %s/%s deleted", rb.Namespace, rb.Name) + } + return nil +} + // updateBindingSubjects sets the ServiceAccount name and namespace on RBAC binding subjects. func updateBindingSubjects(subjects []rbacv1.Subject, serviceAccountName, namespace string) { for i := range subjects { diff --git a/pkg/controller/trustmanager/rbacs_test.go b/pkg/controller/trustmanager/rbacs_test.go index 634bffc30..63aa1372e 100644 --- a/pkg/controller/trustmanager/rbacs_test.go +++ b/pkg/controller/trustmanager/rbacs_test.go @@ -7,6 +7,7 @@ import ( "testing" rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -28,7 +29,7 @@ func TestRoleObject(t *testing.T) { name: "cluster role has correct metadata", tm: testTrustManager(), getRole: func(l, a map[string]string) client.Object { - return getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, l, a) + return getClusterRoleObject(v1alpha1.TrustManagerConfig{}, l, a) }, wantName: trustManagerClusterRoleName, wantLabels: map[string]string{ @@ -63,7 +64,7 @@ func TestRoleObject(t *testing.T) { name: "cluster role default labels take precedence over user labels", tm: testTrustManager().WithLabels(map[string]string{"app": "should-be-overridden"}), getRole: func(l, a map[string]string) client.Object { - return getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, l, a) + return getClusterRoleObject(v1alpha1.TrustManagerConfig{}, l, a) }, wantLabels: map[string]string{ "app": trustManagerCommonName, @@ -75,7 +76,7 @@ func TestRoleObject(t *testing.T) { WithLabels(map[string]string{"user-label": "test-value"}). WithAnnotations(map[string]string{"user-annotation": "test-value"}), getRole: func(l, a map[string]string) client.Object { - return getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, l, a) + return getClusterRoleObject(v1alpha1.TrustManagerConfig{}, l, a) }, wantLabels: map[string]string{"user-label": "test-value"}, wantAnnotations: map[string]string{"user-annotation": "test-value"}, @@ -122,6 +123,18 @@ func TestRoleObject(t *testing.T) { wantLabels: map[string]string{"user-label": "test-value"}, wantAnnotations: map[string]string{"user-annotation": "test-value"}, }, + { + name: "target namespace role has correct metadata", + tm: testTrustManager().WithTargetNamespaces("app-ns"), + getRole: func(l, a map[string]string) client.Object { + return getTargetNamespaceRoleObject("app-ns", v1alpha1.SecretTargetsConfig{}, l, a) + }, + wantName: trustManagerTargetRoleName, + wantNamespace: "app-ns", + wantLabels: map[string]string{ + "app": trustManagerCommonName, + }, + }, } for _, tt := range tests { @@ -273,6 +286,22 @@ func TestRoleBindingObject(t *testing.T) { wantLabels: map[string]string{"user-label": "test-value"}, wantAnnotations: map[string]string{"user-annotation": "test-value"}, }, + { + name: "target namespace role binding has correct metadata and subjects", + tm: testTrustManager().WithTargetNamespaces("app-ns"), + getBinding: func(l, a map[string]string) client.Object { + return getTargetNamespaceRoleBindingObject("app-ns", l, a) + }, + wantName: trustManagerTargetRoleBindingName, + wantNamespace: "app-ns", + wantRoleRefName: trustManagerTargetRoleName, + wantRoleRefKind: "Role", + wantSubjectName: trustManagerServiceAccountName, + wantSubjectNS: operandNamespace, + wantLabels: map[string]string{ + "app": trustManagerCommonName, + }, + }, } for _, tt := range tests { @@ -350,7 +379,7 @@ func TestRBACReconciliation(t *testing.T) { existsCall++ switch existsCall { case 1: - cr := getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{}, testResourceLabels(), testResourceAnnotations()) cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: crb := getClusterRoleBindingObject(testResourceLabels(), testResourceAnnotations()) @@ -382,7 +411,7 @@ func TestRBACReconciliation(t *testing.T) { existsCall++ switch existsCall { case 1: - cr := getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{}, testResourceLabels(), testResourceAnnotations()) cr.Labels["app.kubernetes.io/instance"] = "modified-value" cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: @@ -419,7 +448,7 @@ func TestRBACReconciliation(t *testing.T) { existsCall++ switch existsCall { case 1: - cr := getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, labels, annotations) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{}, labels, annotations) cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: crb := getClusterRoleBindingObject(labels, annotations) @@ -452,7 +481,7 @@ func TestRBACReconciliation(t *testing.T) { existsCall++ switch existsCall { case 1: - cr := getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{}, testResourceLabels(), testResourceAnnotations()) cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: crb := getClusterRoleBindingObject(testResourceLabels(), testResourceAnnotations()) @@ -626,7 +655,7 @@ func TestClusterRoleSecretTargetRules(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { tm := tt.tm.Build() - cr := getClusterRoleObject(tm.Spec.TrustManagerConfig.SecretTargets, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(tm.Spec.TrustManagerConfig, testResourceLabels(), testResourceAnnotations()) hasSecretRead := false hasSecretWrite := false @@ -676,7 +705,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { switch existsCall { case 1: // Existing ClusterRole has no secret rules (was Disabled) - cr := getClusterRoleObject(v1alpha1.SecretTargetsConfig{}, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{}, testResourceLabels(), testResourceAnnotations()) cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: crb := getClusterRoleBindingObject(testResourceLabels(), testResourceAnnotations()) @@ -710,7 +739,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { existsCall++ switch existsCall { case 1: - cr := getClusterRoleObject(secretTargets, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{SecretTargets: secretTargets}, testResourceLabels(), testResourceAnnotations()) cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: crb := getClusterRoleBindingObject(testResourceLabels(), testResourceAnnotations()) @@ -747,7 +776,7 @@ func TestRBACReconciliationWithSecretTargets(t *testing.T) { existsCall++ switch existsCall { case 1: - cr := getClusterRoleObject(oldSecretTargets, testResourceLabels(), testResourceAnnotations()) + cr := getClusterRoleObject(v1alpha1.TrustManagerConfig{SecretTargets: oldSecretTargets}, testResourceLabels(), testResourceAnnotations()) cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) case 2: crb := getClusterRoleBindingObject(testResourceLabels(), testResourceAnnotations()) @@ -817,3 +846,284 @@ func assertSubjects(t *testing.T, subjects []rbacv1.Subject, expectedName, expec t.Error("expected to find a ServiceAccount subject") } } + +func TestClusterRoleTargetNamespacesRules(t *testing.T) { + tests := []struct { + name string + tm *trustManagerBuilder + wantConfigMapWrite bool + wantEvents bool + wantSecretRead bool + wantSecretWrite bool + }{ + { + name: "cluster-wide ConfigMap write and events when targetNamespaces is unset", + tm: testTrustManager(), + wantConfigMapWrite: true, + wantEvents: true, + }, + { + name: "drops cluster-wide ConfigMap write and events when targetNamespaces is set", + tm: testTrustManager().WithTargetNamespaces("app-ns"), + }, + { + name: "secret rules stay on ClusterRole when targetNamespaces is unset and secretTargets is Custom", + tm: testTrustManager().WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{"bundle-secret"}), + wantConfigMapWrite: true, + wantEvents: true, + wantSecretRead: true, + wantSecretWrite: true, + }, + { + name: "secret rules move off ClusterRole when targetNamespaces is set and secretTargets is Custom", + tm: testTrustManager(). + WithTargetNamespaces("app-ns"). + WithSecretTargets(v1alpha1.SecretTargetsPolicyCustom, []string{"bundle-secret"}), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tm := tt.tm.Build() + cr := getClusterRoleObject(tm.Spec.TrustManagerConfig, testResourceLabels(), testResourceAnnotations()) + + if got := hasResourceVerb(cr.Rules, "configmaps", "create"); got != tt.wantConfigMapWrite { + t.Errorf("expected configmap write=%v, got %v", tt.wantConfigMapWrite, got) + } + if got := hasResourceVerb(cr.Rules, "events", "create"); got != tt.wantEvents { + t.Errorf("expected events write=%v, got %v", tt.wantEvents, got) + } + if got := hasResourceVerb(cr.Rules, "secrets", "get"); got != tt.wantSecretRead { + t.Errorf("expected secret read=%v, got %v", tt.wantSecretRead, got) + } + if got := hasResourceVerb(cr.Rules, "secrets", "create"); got != tt.wantSecretWrite { + t.Errorf("expected secret write=%v, got %v", tt.wantSecretWrite, got) + } + }) + } +} + +func TestTargetNamespaceRoleRules(t *testing.T) { + t.Run("includes ConfigMap write and events", func(t *testing.T) { + role := getTargetNamespaceRoleObject("app-ns", v1alpha1.SecretTargetsConfig{}, testResourceLabels(), testResourceAnnotations()) + if !hasResourceVerb(role.Rules, "configmaps", "create") { + t.Error("expected configmap write on target namespace Role") + } + if !hasResourceVerb(role.Rules, "events", "create") { + t.Error("expected events write on target namespace Role") + } + if hasResourceVerb(role.Rules, "secrets", "create") { + t.Error("expected no secret write on target namespace Role when secretTargets is Disabled") + } + }) + + t.Run("includes secret rules when secretTargets is Custom", func(t *testing.T) { + secretTargets := v1alpha1.SecretTargetsConfig{ + Policy: v1alpha1.SecretTargetsPolicyCustom, + AuthorizedSecrets: []string{"bundle-secret"}, + } + role := getTargetNamespaceRoleObject("app-ns", secretTargets, testResourceLabels(), testResourceAnnotations()) + if !hasResourceVerb(role.Rules, "secrets", "get") { + t.Error("expected secret read on target namespace Role") + } + if !hasResourceVerb(role.Rules, "secrets", "create") { + t.Error("expected secret write on target namespace Role") + } + }) +} + +func TestTargetRBACNamespaces(t *testing.T) { + tests := []struct { + name string + tm *trustManagerBuilder + trustNamespace string + want []string + }{ + { + name: "empty when targetNamespaces is unset", + tm: testTrustManager(), + trustNamespace: defaultTrustNamespace, + }, + { + name: "includes listed namespaces and trust namespace, sorted unique", + tm: testTrustManager().WithTargetNamespaces("zeta-ns", "app-ns"), + trustNamespace: defaultTrustNamespace, + want: []string{"app-ns", defaultTrustNamespace, "zeta-ns"}, + }, + { + name: "does not duplicate trust namespace when it is already listed", + tm: testTrustManager().WithTargetNamespaces(defaultTrustNamespace, "app-ns"), + trustNamespace: defaultTrustNamespace, + want: []string{"app-ns", defaultTrustNamespace}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := targetRBACNamespaces(tt.tm.Build(), tt.trustNamespace) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("expected %v, got %v", tt.want, got) + } + }) + } +} + +func TestRBACReconciliationWithTargetNamespaces(t *testing.T) { + targetNS := "app-ns" + // Sorted: app-ns, cert-manager + targetNamespaces := []string{targetNS, defaultTrustNamespace} + + tests := []struct { + name string + preReq func(*Reconciler, *fakes.FakeCtrlClient) + wantErr string + wantExistsCount int + wantPatchCount int + wantDeleteCount int + }{ + { + name: "successful apply of cluster RBAC plus per-namespace target Roles", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + return false, nil + }) + }, + wantExistsCount: 10, + wantPatchCount: 10, + }, + { + name: "skip apply when all RBAC resources including target Roles match desired", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + tm := testTrustManager().WithTargetNamespaces(targetNS).Build() + config := tm.Spec.TrustManagerConfig + existsCall := 0 + m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + existsCall++ + switch existsCall { + case 1: + cr := getClusterRoleObject(config, testResourceLabels(), testResourceAnnotations()) + cr.DeepCopyInto(obj.(*rbacv1.ClusterRole)) + case 2: + crb := getClusterRoleBindingObject(testResourceLabels(), testResourceAnnotations()) + crb.DeepCopyInto(obj.(*rbacv1.ClusterRoleBinding)) + case 3: + role := getTrustNamespaceRoleObject(testResourceLabels(), testResourceAnnotations(), defaultTrustNamespace) + role.DeepCopyInto(obj.(*rbacv1.Role)) + case 4: + rb := getTrustNamespaceRoleBindingObject(testResourceLabels(), testResourceAnnotations(), defaultTrustNamespace) + rb.DeepCopyInto(obj.(*rbacv1.RoleBinding)) + case 5: + role := getLeaderElectionRoleObject(testResourceLabels(), testResourceAnnotations()) + role.DeepCopyInto(obj.(*rbacv1.Role)) + case 6: + rb := getLeaderElectionRoleBindingObject(testResourceLabels(), testResourceAnnotations()) + rb.DeepCopyInto(obj.(*rbacv1.RoleBinding)) + case 7: + role := getTargetNamespaceRoleObject(targetNamespaces[0], config.SecretTargets, testResourceLabels(), testResourceAnnotations()) + role.DeepCopyInto(obj.(*rbacv1.Role)) + case 8: + rb := getTargetNamespaceRoleBindingObject(targetNamespaces[0], testResourceLabels(), testResourceAnnotations()) + rb.DeepCopyInto(obj.(*rbacv1.RoleBinding)) + case 9: + role := getTargetNamespaceRoleObject(targetNamespaces[1], config.SecretTargets, testResourceLabels(), testResourceAnnotations()) + role.DeepCopyInto(obj.(*rbacv1.Role)) + case 10: + rb := getTargetNamespaceRoleBindingObject(targetNamespaces[1], testResourceLabels(), testResourceAnnotations()) + rb.DeepCopyInto(obj.(*rbacv1.RoleBinding)) + } + return true, nil + }) + }, + wantExistsCount: 10, + wantPatchCount: 0, + }, + { + name: "deletes leftover target Role and RoleBinding outside the desired set", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + return false, nil + }) + m.ListCalls(func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + switch l := list.(type) { + case *rbacv1.RoleList: + l.Items = []rbacv1.Role{ + { + ObjectMeta: metav1.ObjectMeta{Name: trustManagerTargetRoleName, Namespace: "stale-ns"}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: trustManagerRoleName, Namespace: defaultTrustNamespace}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: trustManagerTargetRoleName, Namespace: targetNS}, + }, + } + case *rbacv1.RoleBindingList: + l.Items = []rbacv1.RoleBinding{ + { + ObjectMeta: metav1.ObjectMeta{Name: trustManagerTargetRoleBindingName, Namespace: "stale-ns"}, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: trustManagerRoleBindingName, Namespace: defaultTrustNamespace}, + }, + } + } + return nil + }) + }, + wantExistsCount: 10, + wantPatchCount: 10, + wantDeleteCount: 2, + }, + { + name: "target namespace role patch error propagates", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + return false, nil + }) + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if role, ok := obj.(*rbacv1.Role); ok && role.Name == trustManagerTargetRoleName { + return errTestClient + } + return nil + }) + }, + wantErr: "failed to apply target namespace role", + wantExistsCount: 7, + wantPatchCount: 7, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := testReconciler(t) + mock := &fakes.FakeCtrlClient{} + if tt.preReq != nil { + tt.preReq(r, mock) + } + r.CtrlClient = mock + + tm := testTrustManager().WithTargetNamespaces(targetNS).Build() + err := r.createOrApplyRBACResources(tm, getResourceLabels(tm), getResourceAnnotations(tm), defaultTrustNamespace) + assertError(t, err, tt.wantErr) + + if got := mock.ExistsCallCount(); got != tt.wantExistsCount { + t.Errorf("expected %d Exists calls, got %d", tt.wantExistsCount, got) + } + if got := mock.PatchCallCount(); got != tt.wantPatchCount { + t.Errorf("expected %d Patch calls, got %d", tt.wantPatchCount, got) + } + if got := mock.DeleteCallCount(); got != tt.wantDeleteCount { + t.Errorf("expected %d Delete calls, got %d", tt.wantDeleteCount, got) + } + }) + } +} + +func hasResourceVerb(rules []rbacv1.PolicyRule, resource, verb string) bool { + for _, rule := range rules { + if slices.Contains(rule.Resources, resource) && slices.Contains(rule.Verbs, verb) { + return true + } + } + return false +} diff --git a/pkg/controller/trustmanager/test_utils.go b/pkg/controller/trustmanager/test_utils.go index 33e822070..b9f9b9aac 100644 --- a/pkg/controller/trustmanager/test_utils.go +++ b/pkg/controller/trustmanager/test_utils.go @@ -94,6 +94,11 @@ func (b *trustManagerBuilder) WithFilterNonCACerts(policy v1alpha1.FilterNonCACe return b } +func (b *trustManagerBuilder) WithTargetNamespaces(namespaces ...string) *trustManagerBuilder { + b.Spec.TrustManagerConfig.TargetNamespaces = namespaces + return b +} + func (b *trustManagerBuilder) WithDefaultCAPackage(policy v1alpha1.DefaultCAPackagePolicy) *trustManagerBuilder { b.Spec.TrustManagerConfig.DefaultCAPackage.Policy = policy return b diff --git a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go index e7b546b31..e8e748903 100644 --- a/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go +++ b/pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go @@ -24,6 +24,15 @@ type TrustManagerConfigApplyConfiguration struct { // This field is immutable once set. // This field can have a maximum of 63 characters. TrustNamespace *string `json:"trustNamespace,omitempty"` + // targetNamespaces limits where trust-manager writes Bundle targets + // (ConfigMaps, and Secrets when secretTargets is enabled). + // When empty or omitted, trust-manager writes targets in all namespaces + // (default behavior). + // When set, Bundle targets are written only in the listed namespaces. + // Trust sources are still read from trustNamespace. + // Removing a namespace from this list does not delete existing target + // ConfigMaps or Secrets in that namespace. + TargetNamespaces []string `json:"targetNamespaces,omitempty"` // secretTargets configures whether trust-manager can write trust bundles to Secrets. SecretTargets *SecretTargetsConfigApplyConfiguration `json:"secretTargets,omitempty"` // filterExpiredCertificates controls whether trust-manager filters out @@ -84,6 +93,16 @@ func (b *TrustManagerConfigApplyConfiguration) WithTrustNamespace(value string) return b } +// WithTargetNamespaces adds the given value to the TargetNamespaces field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the TargetNamespaces field. +func (b *TrustManagerConfigApplyConfiguration) WithTargetNamespaces(values ...string) *TrustManagerConfigApplyConfiguration { + for i := range values { + b.TargetNamespaces = append(b.TargetNamespaces, values[i]) + } + return b +} + // WithSecretTargets sets the SecretTargets field in the declarative configuration to the given value // and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the SecretTargets field is set to the value of the last call. diff --git a/test/e2e/trustmanager_bundle_test.go b/test/e2e/trustmanager_bundle_test.go index 67655a8d1..e9b36193b 100644 --- a/test/e2e/trustmanager_bundle_test.go +++ b/test/e2e/trustmanager_bundle_test.go @@ -47,6 +47,10 @@ // Group 7 — FilterNonCACerts enabled: // - ConfigMap source with CA + leaf certs → only CA cert in ConfigMap target // - Transition to Disabled → same Bundle re-syncs with both certs in target +// +// Group 8 — TargetNamespaces set: +// - Bundle ConfigMap target is written only in listed namespaces +// - Bundle ConfigMap is absent from a denied namespace and the suite testNS package e2e import ( @@ -1149,4 +1153,49 @@ var _ = Describe("Bundle", Ordered, Label("Platform:Generic", "Feature:TrustMana verifyBundleSynced(ctx, filterBundleName) }) }) + + // ===== Group 8: TargetNamespaces ===== + Context("with TargetNamespaces set", Ordered, func() { + var ( + allowedNS *corev1.Namespace + deniedNS *corev1.Namespace + ) + + BeforeAll(func() { + By("creating allowed and denied target namespaces") + allowedNS = createNamespaceWithCleanup(ctx, "tm-target-allow-", nil) + deniedNS = createNamespaceWithCleanup(ctx, "tm-target-deny-", nil) + + createTrustManager(ctx, newTrustManagerCR().WithTargetNamespaces(allowedNS.Name)) + }) + AfterAll(func() { deleteTrustManager(ctx) }) + + It("should sync Bundle ConfigMap only into listed target namespaces", func() { + bundleName := "bundle-target-ns-" + randomStr(5) + bundle := newBundle(bundleName). + WithInLineSource(testCertPEM1). + WithConfigMapTarget(bundleTargetKey). + Build() + + createBundleWithCleanup(ctx, bundle) + + By("verifying ConfigMap exists in the listed target namespace") + err := waitForConfigMapTarget(ctx, bundleClient, bundleName, allowedNS.Name, bundleTargetKey, testCertPEM1, highTimeout) + Expect(err).ShouldNot(HaveOccurred()) + + By("verifying ConfigMap does not exist in a namespace outside the list") + Consistently(func(g Gomega) { + _, err := k8sClientSet.CoreV1().ConfigMaps(deniedNS.Name).Get(ctx, bundleName, metav1.GetOptions{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "ConfigMap %s should not exist in denied namespace %s", bundleName, deniedNS.Name) + }, "30s", fastPollInterval).Should(Succeed()) + + By("verifying ConfigMap does not exist in the suite test namespace") + Consistently(func(g Gomega) { + _, err := k8sClientSet.CoreV1().ConfigMaps(testNS.Name).Get(ctx, bundleName, metav1.GetOptions{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "ConfigMap %s should not exist in suite test namespace %s", bundleName, testNS.Name) + }, "30s", fastPollInterval).Should(Succeed()) + }) + }) }) diff --git a/test/e2e/trustmanager_helpers_test.go b/test/e2e/trustmanager_helpers_test.go index a84679f71..f2ae55fa7 100644 --- a/test/e2e/trustmanager_helpers_test.go +++ b/test/e2e/trustmanager_helpers_test.go @@ -102,6 +102,11 @@ func (b *trustManagerCRBuilder) WithFilterNonCACerts(policy v1alpha1.FilterNonCA return b } +func (b *trustManagerCRBuilder) WithTargetNamespaces(namespaces ...string) *trustManagerCRBuilder { + b.tm.Spec.TrustManagerConfig.TargetNamespaces = namespaces + return b +} + func (b *trustManagerCRBuilder) Build() *v1alpha1.TrustManager { return b.tm } @@ -150,6 +155,10 @@ func cleanupTrustManagerOperandLeavings(ctx context.Context) { }, lowTimeout, fastPollInterval).Should(BeTrue()) deleteTrustManagerDefaultCAPackageConfigMap(ctx) + + By("cleaning up leftover target namespace Role and RoleBinding in the trust namespace") + _ = k8sClientSet.RbacV1().Roles(trustManagerNamespace).Delete(ctx, "trust-manager-target", metav1.DeleteOptions{}) + _ = k8sClientSet.RbacV1().RoleBindings(trustManagerNamespace).Delete(ctx, "trust-manager-target", metav1.DeleteOptions{}) } // deleteTrustManagerDefaultCAPackageConfigMap removes the operand ConfigMap created when diff --git a/test/e2e/trustmanager_test.go b/test/e2e/trustmanager_test.go index b8feba608..3bda2e692 100644 --- a/test/e2e/trustmanager_test.go +++ b/test/e2e/trustmanager_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "slices" + "strings" "time" . "github.com/onsi/ginkgo/v2" @@ -38,6 +39,9 @@ const ( trustManagerRoleName = "trust-manager" trustManagerRoleBindingName = "trust-manager" + trustManagerTargetRoleName = "trust-manager-target" + trustManagerTargetRoleBindingName = "trust-manager-target" + trustManagerLeaderElectionRoleName = "trust-manager:leaderelection" trustManagerLeaderElectionRoleBindingName = "trust-manager:leaderelection" @@ -641,6 +645,109 @@ var _ = Describe("TrustManager", Ordered, Label("Platform:Generic", "Feature:Tru }, lowTimeout, fastPollInterval).Should(Succeed()) }) + It("should add target-namespaces arg when targetNamespaces is set", func() { + targetNS := createUniqueNamespace("tm-target-ns") + createAndDestroyTestNamespace(ctx, clientset, targetNS) + + createTrustManager(ctx, newTrustManagerCR().WithTargetNamespaces(targetNS)) + + By("verifying deployment args contain --target-namespaces") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(dep.Spec.Template.Spec.Containers).ShouldNot(BeEmpty()) + g.Expect(dep.Spec.Template.Spec.Containers[0].Args).Should(ContainElement("--target-namespaces=" + targetNS)) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) + + It("should update target-namespaces arg when the list changes", func() { + ns1 := createUniqueNamespace("tm-target-a-") + ns2 := createUniqueNamespace("tm-target-b-") + createAndDestroyTestNamespace(ctx, clientset, ns1) + createAndDestroyTestNamespace(ctx, clientset, ns2) + + createTrustManager(ctx, newTrustManagerCR().WithTargetNamespaces(ns1)) + + By("verifying deployment args contain the initial target namespace") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(dep.Spec.Template.Spec.Containers).ShouldNot(BeEmpty()) + g.Expect(dep.Spec.Template.Spec.Containers[0].Args).Should(ContainElement("--target-namespaces=" + ns1)) + }, lowTimeout, fastPollInterval).Should(Succeed()) + + By("updating TrustManager CR with an additional target namespace") + Eventually(func() error { + tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + return err + } + tm.Spec.TrustManagerConfig.TargetNamespaces = []string{ns2, ns1} + _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) + return err + }, lowTimeout, fastPollInterval).Should(Succeed()) + + wanted := slices.Clone([]string{ns1, ns2}) + slices.Sort(wanted) + By("verifying deployment args contain the updated sorted target namespace list") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(dep.Spec.Template.Spec.Containers).ShouldNot(BeEmpty()) + g.Expect(dep.Spec.Template.Spec.Containers[0].Args).Should(ContainElement("--target-namespaces=" + strings.Join(wanted, ","))) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) + + It("should scope ConfigMap write RBAC to targetNamespaces when set", func() { + targetNS := createUniqueNamespace("tm-target-rbac-") + createAndDestroyTestNamespace(ctx, clientset, targetNS) + + createTrustManager(ctx, newTrustManagerCR().WithTargetNamespaces(targetNS)) + + By("verifying ClusterRole has no cluster-wide ConfigMap write") + Eventually(func(g Gomega) { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, trustManagerClusterRoleName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(hasResourceVerb(cr.Rules, "configmaps", "create")).Should(BeFalse(), + "ClusterRole should not have ConfigMap create when targetNamespaces is set") + }, lowTimeout, fastPollInterval).Should(Succeed()) + + By("verifying namespaced Role exists in the listed target namespace") + Eventually(func(g Gomega) { + role, err := clientset.RbacV1().Roles(targetNS).Get(ctx, trustManagerTargetRoleName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(hasResourceVerb(role.Rules, "configmaps", "create")).Should(BeTrue(), + "target namespace Role should have ConfigMap create") + }, lowTimeout, fastPollInterval).Should(Succeed()) + + By("verifying namespaced Role exists in the trust namespace") + Eventually(func(g Gomega) { + role, err := clientset.RbacV1().Roles(trustManagerNamespace).Get(ctx, trustManagerTargetRoleName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(hasResourceVerb(role.Rules, "configmaps", "create")).Should(BeTrue(), + "trust namespace Role should have ConfigMap create") + }, lowTimeout, fastPollInterval).Should(Succeed()) + + By("verifying RoleBinding in the listed target namespace") + Eventually(func(g Gomega) { + rb, err := clientset.RbacV1().RoleBindings(targetNS).Get(ctx, trustManagerTargetRoleBindingName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(rb.RoleRef.Name).Should(Equal(trustManagerTargetRoleName)) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) + + It("should not have target-namespaces arg when targetNamespaces is unset", func() { + createTrustManager(ctx, newTrustManagerCR()) + + By("verifying deployment args do not contain --target-namespaces") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(dep.Spec.Template.Spec.Containers).ShouldNot(BeEmpty()) + g.Expect(dep.Spec.Template.Spec.Containers[0].Args).ShouldNot(ContainElement(HavePrefix("--target-namespaces="))) + }, lowTimeout, fastPollInterval).Should(Succeed()) + }) + }) // ------------------------------------------------------------------------- @@ -1566,8 +1673,12 @@ func verifyTrustManagerResourceRecreation(deleteFunc func() error, getFunc func( // hasSecretRule returns true if any rule in the list targets the "secrets" resource. func hasSecretRule(rules []rbacv1.PolicyRule) bool { + return hasResourceVerb(rules, "secrets", "get") || hasResourceVerb(rules, "secrets", "create") +} + +func hasResourceVerb(rules []rbacv1.PolicyRule, resource, verb string) bool { for _, rule := range rules { - if slices.Contains(rule.Resources, "secrets") { + if slices.Contains(rule.Resources, resource) && slices.Contains(rule.Verbs, verb) { return true } } From 019c7df1e15a608f2132958de80fd7dcc7cfe4b9 Mon Sep 17 00:00:00 2001 From: Arun Maurya Date: Wed, 16 Sep 2026 15:50:49 +0530 Subject: [PATCH 4/4] CM-1367: Regenerate TrustManager bundle CRD via make bundle. --- bundle/manifests/operator.openshift.io_trustmanagers.yaml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bundle/manifests/operator.openshift.io_trustmanagers.yaml b/bundle/manifests/operator.openshift.io_trustmanagers.yaml index d8bb1d62c..ec33b14c9 100644 --- a/bundle/manifests/operator.openshift.io_trustmanagers.yaml +++ b/bundle/manifests/operator.openshift.io_trustmanagers.yaml @@ -1,9 +1,9 @@ ---- apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: controller-gen.kubebuilder.io/version: v0.19.0 + creationTimestamp: null labels: app.kubernetes.io/name: trustmanager app.kubernetes.io/part-of: cert-manager-operator @@ -1337,3 +1337,9 @@ spec: storage: true subresources: status: {} +status: + acceptedNames: + kind: "" + plural: "" + conditions: null + storedVersions: null