From f945972efb7c8cdd3f0a66037e79af750625908b Mon Sep 17 00:00:00 2001 From: Brandon Palm Date: Thu, 23 Jul 2026 16:17:03 -0500 Subject: [PATCH] NO-JIRA: Add unit tests across codebase to close coverage gaps Full-codebase unit test audit identified 42 coverage gaps across 12 packages. This commit addresses the high and medium priority gaps that can be covered without modifying production code. New test files (11): - pkg/controller/certmanager: network policy validation, default CertManager controller, deployment log level hook - pkg/controller/common: client methods (Exists, Get, Create, Update, UpdateWithRetry, Patch, StatusUpdate), HandleReconcileResult, validation functions, utility functions - pkg/controller/istiocsr: network policy CRUD, validateIstioCSRConfig, updateCondition - pkg/operator/operatorclient: GetOperatorState, EnsureFinalizer, RemoveFinalizer, ApplyOperatorStatus, GetUnsupportedConfigOverrides - pkg/operator: buildCacheObjectList, addControllerCacheConfig, findExistingCacheEntry Modified test files (7): - pkg/controller/certmanager: deployment helper override functions (args, env, labels), unsupported overrides invalid JSON, related images for webhook/cainjector/acmesolver - pkg/controller/trustmanager: managedAnnotationsModified, addFinalizer/removeFinalizer edge cases, updateStatus retry, containerPortsMatch, readinessProbeModified, webhook rules and AdmissionReviewVersions drift - pkg/features: IsIstioCSRFeatureGateEnabled, SetupWithFlagValue invalid flag Notable finding: istiocsr utils.go:484 updateCondition aggregates {err, errUpdate} instead of {prependErr, errUpdate}, losing the original reconcile error when both fail. The http01proxy version correctly uses prependErr. Test documents this bug. --- .../cert_manager_networkpolicy_test.go | 308 +++++++++++++ .../default_cert_manager_controller_test.go | 58 +++ .../certmanager/deployment_helper_test.go | 432 ++++++++++++++++++ .../certmanager/deployment_log_level_test.go | 118 +++++ .../certmanager/deployment_overrides_test.go | 23 + .../certmanager/related_images_test.go | 105 +++++ pkg/controller/common/client_test.go | 355 ++++++++++++++ .../common/reconcile_result_test.go | 162 +++++++ pkg/controller/common/utils_test.go | 42 ++ pkg/controller/common/validation_test.go | 214 +++++++++ .../istiocsr/networkpolicies_test.go | 225 +++++++++ pkg/controller/istiocsr/utils_test.go | 209 +++++++++ .../trustmanager/deployments_test.go | 138 ++++++ pkg/controller/trustmanager/utils_test.go | 171 +++++++ pkg/controller/trustmanager/webhooks_test.go | 35 ++ pkg/features/features_test.go | 51 +++ .../operatorclient/operatorclient_test.go | 373 +++++++++++++++ pkg/operator/setup_manager_test.go | 232 ++++++++++ 18 files changed, 3251 insertions(+) create mode 100644 pkg/controller/certmanager/cert_manager_networkpolicy_test.go create mode 100644 pkg/controller/certmanager/default_cert_manager_controller_test.go create mode 100644 pkg/controller/certmanager/deployment_log_level_test.go create mode 100644 pkg/controller/common/reconcile_result_test.go create mode 100644 pkg/controller/common/validation_test.go create mode 100644 pkg/controller/istiocsr/networkpolicies_test.go create mode 100644 pkg/operator/operatorclient/operatorclient_test.go create mode 100644 pkg/operator/setup_manager_test.go diff --git a/pkg/controller/certmanager/cert_manager_networkpolicy_test.go b/pkg/controller/certmanager/cert_manager_networkpolicy_test.go new file mode 100644 index 000000000..92c238d1b --- /dev/null +++ b/pkg/controller/certmanager/cert_manager_networkpolicy_test.go @@ -0,0 +1,308 @@ +package certmanager + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" +) + +func TestValidateComponentName(t *testing.T) { + tests := []struct { + name string + componentName v1alpha1.ComponentName + expectError bool + }{ + { + name: "CoreController is valid", + componentName: v1alpha1.CoreController, + expectError: false, + }, + { + name: "CAInjector is valid", + componentName: v1alpha1.CAInjector, + expectError: false, + }, + { + name: "Webhook is valid", + componentName: v1alpha1.Webhook, + expectError: false, + }, + { + name: "empty string is invalid", + componentName: "", + expectError: true, + }, + { + name: "unknown component name is invalid", + componentName: "UnknownComponent", + expectError: true, + }, + { + name: "lowercase controller is invalid", + componentName: "controller", + expectError: true, + }, + } + + c := &CertManagerNetworkPolicyUserDefinedController{} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := c.validateComponentName(tc.componentName) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported component name") + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestGetPodSelectorForComponent(t *testing.T) { + tests := []struct { + name string + componentName v1alpha1.ComponentName + expectedLabels map[string]string + }{ + { + name: "CoreController returns cert-manager app label", + componentName: v1alpha1.CoreController, + expectedLabels: map[string]string{ + "app": "cert-manager", + }, + }, + { + name: "CAInjector returns cainjector app label", + componentName: v1alpha1.CAInjector, + expectedLabels: map[string]string{ + "app": "cainjector", + }, + }, + { + name: "Webhook returns webhook app label", + componentName: v1alpha1.Webhook, + expectedLabels: map[string]string{ + "app": "webhook", + }, + }, + { + name: "unknown component returns default label", + componentName: "Unknown", + expectedLabels: map[string]string{ + "app.kubernetes.io/name": "cert-manager", + }, + }, + } + + c := &CertManagerNetworkPolicyUserDefinedController{} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + selector := c.getPodSelectorForComponent(tc.componentName) + require.Equal(t, tc.expectedLabels, selector.MatchLabels) + }) + } +} + +func TestValidateNetworkPolicyConfig(t *testing.T) { + tests := []struct { + name string + certManager *v1alpha1.CertManager + expectError bool + errContains string + }{ + { + name: "valid config with single policy passes", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + NetworkPolicies: []v1alpha1.NetworkPolicy{ + { + Name: "allow-egress", + ComponentName: v1alpha1.CoreController, + }, + }, + }, + }, + expectError: false, + }, + { + name: "valid config with multiple policies passes", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + NetworkPolicies: []v1alpha1.NetworkPolicy{ + { + Name: "allow-egress-controller", + ComponentName: v1alpha1.CoreController, + }, + { + Name: "allow-egress-webhook", + ComponentName: v1alpha1.Webhook, + }, + { + Name: "allow-egress-cainjector", + ComponentName: v1alpha1.CAInjector, + }, + }, + }, + }, + expectError: false, + }, + { + name: "empty network policies list passes", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + NetworkPolicies: []v1alpha1.NetworkPolicy{}, + }, + }, + expectError: false, + }, + { + name: "nil network policies list passes", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + expectError: false, + }, + { + name: "invalid component name fails", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + NetworkPolicies: []v1alpha1.NetworkPolicy{ + { + Name: "bad-policy", + ComponentName: "InvalidComponent", + }, + }, + }, + }, + expectError: true, + errContains: "invalid component name", + }, + { + name: "empty policy name fails", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + NetworkPolicies: []v1alpha1.NetworkPolicy{ + { + Name: "", + ComponentName: v1alpha1.CoreController, + }, + }, + }, + }, + expectError: true, + errContains: "name cannot be empty", + }, + { + name: "second policy with invalid component fails", + certManager: &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + NetworkPolicies: []v1alpha1.NetworkPolicy{ + { + Name: "good-policy", + ComponentName: v1alpha1.CoreController, + }, + { + Name: "bad-policy", + ComponentName: "BadComponent", + }, + }, + }, + }, + expectError: true, + errContains: "network policy at index 1", + }, + } + + c := &CertManagerNetworkPolicyUserDefinedController{} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := c.validateNetworkPolicyConfig(tc.certManager) + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestCreateUserNetworkPolicy(t *testing.T) { + tests := []struct { + name string + userPolicy v1alpha1.NetworkPolicy + expectedName string + expectedLabels map[string]string + expectedPodLabels map[string]string + }{ + { + name: "creates policy for CoreController", + userPolicy: v1alpha1.NetworkPolicy{ + Name: "allow-dns", + ComponentName: v1alpha1.CoreController, + }, + expectedName: "cert-manager-user-allow-dns", + expectedLabels: map[string]string{ + networkPolicyOwnerLabel: "cert-manager", + }, + expectedPodLabels: map[string]string{ + "app": "cert-manager", + }, + }, + { + name: "creates policy for Webhook", + userPolicy: v1alpha1.NetworkPolicy{ + Name: "allow-api", + ComponentName: v1alpha1.Webhook, + }, + expectedName: "cert-manager-user-allow-api", + expectedLabels: map[string]string{ + networkPolicyOwnerLabel: "cert-manager", + }, + expectedPodLabels: map[string]string{ + "app": "webhook", + }, + }, + { + name: "creates policy for CAInjector", + userPolicy: v1alpha1.NetworkPolicy{ + Name: "allow-egress", + ComponentName: v1alpha1.CAInjector, + }, + expectedName: "cert-manager-user-allow-egress", + expectedLabels: map[string]string{ + networkPolicyOwnerLabel: "cert-manager", + }, + expectedPodLabels: map[string]string{ + "app": "cainjector", + }, + }, + } + + c := &CertManagerNetworkPolicyUserDefinedController{} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + policy := c.createUserNetworkPolicy(tc.userPolicy) + + require.Equal(t, tc.expectedName, policy.Name) + require.Equal(t, certManagerNamespace, policy.Namespace) + require.Equal(t, tc.expectedLabels, policy.Labels) + require.Equal(t, tc.expectedPodLabels, policy.Spec.PodSelector.MatchLabels) + }) + } +} diff --git a/pkg/controller/certmanager/default_cert_manager_controller_test.go b/pkg/controller/certmanager/default_cert_manager_controller_test.go new file mode 100644 index 000000000..a9113fee2 --- /dev/null +++ b/pkg/controller/certmanager/default_cert_manager_controller_test.go @@ -0,0 +1,58 @@ +package certmanager + +import ( + "context" + "testing" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" + fakeclientset "github.com/openshift/cert-manager-operator/pkg/operator/clientset/versioned/fake" +) + +func TestCreateDefaultCertManager(t *testing.T) { + fakeClient := fakeclientset.NewSimpleClientset() + + controller := &DefaultCertManagerController{ + certManagerClient: fakeClient.OperatorV1alpha1(), + } + + ctx := context.Background() + cm, err := controller.createDefaultCertManager(ctx) + require.NoError(t, err) + require.NotNil(t, cm) + + assert.Equal(t, "cluster", cm.Name) + assert.Equal(t, operatorv1.Managed, cm.Spec.ManagementState) + + // Verify the resource was actually created in the fake client + got, err := fakeClient.OperatorV1alpha1().CertManagers().Get(ctx, "cluster", metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, "cluster", got.Name) + assert.Equal(t, operatorv1.Managed, got.Spec.ManagementState) +} + +func TestCreateDefaultCertManagerAlreadyExists(t *testing.T) { + existing := &v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Spec: v1alpha1.CertManagerSpec{ + OperatorSpec: operatorv1.OperatorSpec{ + ManagementState: operatorv1.Managed, + }, + }, + } + fakeClient := fakeclientset.NewSimpleClientset(existing) + + controller := &DefaultCertManagerController{ + certManagerClient: fakeClient.OperatorV1alpha1(), + } + + ctx := context.Background() + _, err := controller.createDefaultCertManager(ctx) + require.Error(t, err, "creating a duplicate CertManager should fail") +} diff --git a/pkg/controller/certmanager/deployment_helper_test.go b/pkg/controller/certmanager/deployment_helper_test.go index b48041d0a..eb587ae9b 100644 --- a/pkg/controller/certmanager/deployment_helper_test.go +++ b/pkg/controller/certmanager/deployment_helper_test.go @@ -878,6 +878,438 @@ func TestGetOverrideSchedulingFor(t *testing.T) { } } +func TestGetOverrideArgsFor(t *testing.T) { + tests := []struct { + name string + certManagerObj v1alpha1.CertManager + deploymentName string + expectedArgs []string + expectError bool + errContains string + }{ + { + name: "get override args for controller", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + ControllerConfig: &v1alpha1.DeploymentConfig{ + OverrideArgs: []string{"--v=4", "--feature-gates=ExperimentalGatewayAPISupport=true"}, + }, + }, + }, + deploymentName: certmanagerControllerDeployment, + expectedArgs: []string{"--v=4", "--feature-gates=ExperimentalGatewayAPISupport=true"}, + }, + { + name: "get override args for webhook", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + WebhookConfig: &v1alpha1.DeploymentConfig{ + OverrideArgs: []string{"--secure-port=10251"}, + }, + }, + }, + deploymentName: certmanagerWebhookDeployment, + expectedArgs: []string{"--secure-port=10251"}, + }, + { + name: "get override args for cainjector", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + CAInjectorConfig: &v1alpha1.DeploymentConfig{ + OverrideArgs: []string{"--leader-elect=false"}, + }, + }, + }, + deploymentName: certmanagerCAinjectorDeployment, + expectedArgs: []string{"--leader-elect=false"}, + }, + { + name: "nil config returns nil args for controller", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + deploymentName: certmanagerControllerDeployment, + expectedArgs: nil, + }, + { + name: "unsupported deployment name returns error", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + deploymentName: "unknown-deployment", + expectError: true, + errContains: "unsupported deployment name", + }, + } + + ctx := t.Context() + + watcherStarted := make(chan struct{}) + fakeClient := fake.NewSimpleClientset() + fakeClient.PrependWatchReactor("certmanagers", func(action clienttesting.Action) (handled bool, ret watch.Interface, err error) { + gvr := action.GetResource() + ns := action.GetNamespace() + watch, err := fakeClient.Tracker().Watch(gvr, ns) + if err != nil { + return false, nil, err + } + close(watcherStarted) + return true, watch, nil + }) + + certManagerInformers := certmanoperatorinformer.NewSharedInformerFactory(fakeClient, 0).Operator().V1alpha1().CertManagers() + certManagerChan := make(chan *v1alpha1.CertManager, 1) + + certManagerInformers.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj any) { + certManagerChan <- obj.(*v1alpha1.CertManager) + }, + DeleteFunc: func(obj any) { + certManagerChan <- obj.(*v1alpha1.CertManager) + }, + }) + + go certManagerInformers.Informer().Run(ctx.Done()) + cache.WaitForCacheSync(ctx.Done(), certManagerInformers.Informer().HasSynced) + <-watcherStarted + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := fakeClient.OperatorV1alpha1().CertManagers().Create(ctx, &tc.certManagerObj, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("error injecting cert manager add: %v", err) + } + + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("Informer did not get the added cert manager object") + } + + actualArgs, err := getOverrideArgsFor(certManagerInformers, tc.deploymentName) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + } else { + assert.NoError(t, err) + require.Equal(t, tc.expectedArgs, actualArgs) + } + + err = fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name, metav1.DeleteOptions{}) + if err != nil { + t.Fatalf("error deleting cert manager: %v", err) + } + + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("Informer did not get the deleted cert manager") + } + }) + } +} + +func TestGetOverrideEnvFor(t *testing.T) { + tests := []struct { + name string + certManagerObj v1alpha1.CertManager + deploymentName string + expectedEnv []corev1.EnvVar + expectError bool + errContains string + }{ + { + name: "get override env for controller", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + ControllerConfig: &v1alpha1.DeploymentConfig{ + OverrideEnv: []corev1.EnvVar{ + {Name: "HTTP_PROXY", Value: "http://proxy:3128"}, + }, + }, + }, + }, + deploymentName: certmanagerControllerDeployment, + expectedEnv: []corev1.EnvVar{ + {Name: "HTTP_PROXY", Value: "http://proxy:3128"}, + }, + }, + { + name: "get override env for webhook", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + WebhookConfig: &v1alpha1.DeploymentConfig{ + OverrideEnv: []corev1.EnvVar{ + {Name: "MY_VAR", Value: "my-value"}, + }, + }, + }, + }, + deploymentName: certmanagerWebhookDeployment, + expectedEnv: []corev1.EnvVar{ + {Name: "MY_VAR", Value: "my-value"}, + }, + }, + { + name: "get override env for cainjector", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + CAInjectorConfig: &v1alpha1.DeploymentConfig{ + OverrideEnv: []corev1.EnvVar{ + {Name: "NO_PROXY", Value: "localhost"}, + }, + }, + }, + }, + deploymentName: certmanagerCAinjectorDeployment, + expectedEnv: []corev1.EnvVar{ + {Name: "NO_PROXY", Value: "localhost"}, + }, + }, + { + name: "nil config returns nil env", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + deploymentName: certmanagerControllerDeployment, + expectedEnv: nil, + }, + { + name: "unsupported deployment name returns error", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + deploymentName: "unknown-deployment", + expectError: true, + errContains: "unsupported deployment name", + }, + } + + ctx := t.Context() + + watcherStarted := make(chan struct{}) + fakeClient := fake.NewSimpleClientset() + fakeClient.PrependWatchReactor("certmanagers", func(action clienttesting.Action) (handled bool, ret watch.Interface, err error) { + gvr := action.GetResource() + ns := action.GetNamespace() + watch, err := fakeClient.Tracker().Watch(gvr, ns) + if err != nil { + return false, nil, err + } + close(watcherStarted) + return true, watch, nil + }) + + certManagerInformers := certmanoperatorinformer.NewSharedInformerFactory(fakeClient, 0).Operator().V1alpha1().CertManagers() + certManagerChan := make(chan *v1alpha1.CertManager, 1) + + certManagerInformers.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj any) { + certManagerChan <- obj.(*v1alpha1.CertManager) + }, + DeleteFunc: func(obj any) { + certManagerChan <- obj.(*v1alpha1.CertManager) + }, + }) + + go certManagerInformers.Informer().Run(ctx.Done()) + cache.WaitForCacheSync(ctx.Done(), certManagerInformers.Informer().HasSynced) + <-watcherStarted + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := fakeClient.OperatorV1alpha1().CertManagers().Create(ctx, &tc.certManagerObj, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("error injecting cert manager add: %v", err) + } + + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("Informer did not get the added cert manager object") + } + + actualEnv, err := getOverrideEnvFor(certManagerInformers, tc.deploymentName) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + } else { + assert.NoError(t, err) + require.Equal(t, tc.expectedEnv, actualEnv) + } + + err = fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name, metav1.DeleteOptions{}) + if err != nil { + t.Fatalf("error deleting cert manager: %v", err) + } + + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("Informer did not get the deleted cert manager") + } + }) + } +} + +func TestGetOverridePodLabelsFor(t *testing.T) { + tests := []struct { + name string + certManagerObj v1alpha1.CertManager + deploymentName string + expectedLabels map[string]string + expectError bool + errContains string + }{ + { + name: "get override labels for controller", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + ControllerConfig: &v1alpha1.DeploymentConfig{ + OverrideLabels: map[string]string{ + "custom-label": "custom-value", + }, + }, + }, + }, + deploymentName: certmanagerControllerDeployment, + expectedLabels: map[string]string{ + "custom-label": "custom-value", + }, + }, + { + name: "get override labels for webhook", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + WebhookConfig: &v1alpha1.DeploymentConfig{ + OverrideLabels: map[string]string{ + "env": "production", + }, + }, + }, + }, + deploymentName: certmanagerWebhookDeployment, + expectedLabels: map[string]string{ + "env": "production", + }, + }, + { + name: "get override labels for cainjector", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{ + CAInjectorConfig: &v1alpha1.DeploymentConfig{ + OverrideLabels: map[string]string{ + "team": "security", + }, + }, + }, + }, + deploymentName: certmanagerCAinjectorDeployment, + expectedLabels: map[string]string{ + "team": "security", + }, + }, + { + name: "nil config returns nil labels", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + deploymentName: certmanagerControllerDeployment, + expectedLabels: nil, + }, + { + name: "unsupported deployment name returns error", + certManagerObj: v1alpha1.CertManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: v1alpha1.CertManagerSpec{}, + }, + deploymentName: "unknown-deployment", + expectError: true, + errContains: "unsupported deployment name", + }, + } + + ctx := t.Context() + + watcherStarted := make(chan struct{}) + fakeClient := fake.NewSimpleClientset() + fakeClient.PrependWatchReactor("certmanagers", func(action clienttesting.Action) (handled bool, ret watch.Interface, err error) { + gvr := action.GetResource() + ns := action.GetNamespace() + watch, err := fakeClient.Tracker().Watch(gvr, ns) + if err != nil { + return false, nil, err + } + close(watcherStarted) + return true, watch, nil + }) + + certManagerInformers := certmanoperatorinformer.NewSharedInformerFactory(fakeClient, 0).Operator().V1alpha1().CertManagers() + certManagerChan := make(chan *v1alpha1.CertManager, 1) + + certManagerInformers.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj any) { + certManagerChan <- obj.(*v1alpha1.CertManager) + }, + DeleteFunc: func(obj any) { + certManagerChan <- obj.(*v1alpha1.CertManager) + }, + }) + + go certManagerInformers.Informer().Run(ctx.Done()) + cache.WaitForCacheSync(ctx.Done(), certManagerInformers.Informer().HasSynced) + <-watcherStarted + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := fakeClient.OperatorV1alpha1().CertManagers().Create(ctx, &tc.certManagerObj, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("error injecting cert manager add: %v", err) + } + + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("Informer did not get the added cert manager object") + } + + actualLabels, err := getOverridePodLabelsFor(certManagerInformers, tc.deploymentName) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.errContains) + } else { + assert.NoError(t, err) + require.Equal(t, tc.expectedLabels, actualLabels) + } + + err = fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name, metav1.DeleteOptions{}) + if err != nil { + t.Fatalf("error deleting cert manager: %v", err) + } + + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("Informer did not get the deleted cert manager") + } + }) + } +} + func TestGetOverrideReplicasFor(t *testing.T) { tests := []struct { name string diff --git a/pkg/controller/certmanager/deployment_log_level_test.go b/pkg/controller/certmanager/deployment_log_level_test.go new file mode 100644 index 000000000..66a4a919d --- /dev/null +++ b/pkg/controller/certmanager/deployment_log_level_test.go @@ -0,0 +1,118 @@ +package certmanager + +import ( + "testing" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func newTestDeploymentWithArgs(args []string) *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-manager", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "cert-manager-controller", + Args: args, + }, + }, + }, + }, + }, + } +} + +func TestWithLogLevel(t *testing.T) { + tests := []struct { + name string + logLevel operatorv1.LogLevel + existingArgs []string + wantArgs []string + wantChanged bool + }{ + { + name: "Normal log level sets --v=2", + logLevel: operatorv1.Normal, + existingArgs: []string{"--cluster-resource-namespace=cert-manager"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=2"}, + wantChanged: true, + }, + { + name: "Debug log level sets --v=4", + logLevel: operatorv1.Debug, + existingArgs: []string{"--cluster-resource-namespace=cert-manager"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=4"}, + wantChanged: true, + }, + { + name: "Trace log level sets --v=6", + logLevel: operatorv1.Trace, + existingArgs: []string{"--cluster-resource-namespace=cert-manager"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=6"}, + wantChanged: true, + }, + { + name: "TraceAll log level sets --v=8", + logLevel: operatorv1.TraceAll, + existingArgs: []string{"--cluster-resource-namespace=cert-manager"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=8"}, + wantChanged: true, + }, + { + name: "empty log level does not modify args", + logLevel: "", + existingArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=2"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=2"}, + wantChanged: false, + }, + { + name: "unknown log level does not modify args", + logLevel: "UnknownLevel", + existingArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=2"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=2"}, + wantChanged: false, + }, + { + name: "log level overrides existing --v arg", + logLevel: operatorv1.Debug, + existingArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=2"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--v=4"}, + wantChanged: true, + }, + { + name: "log level merges with multiple existing args", + logLevel: operatorv1.Trace, + existingArgs: []string{"--leader-election-namespace=kube-system", "--v=2", "--cluster-resource-namespace=cert-manager"}, + wantArgs: []string{"--cluster-resource-namespace=cert-manager", "--leader-election-namespace=kube-system", "--v=6"}, + wantChanged: true, + }, + { + name: "log level works with empty existing args", + logLevel: operatorv1.Normal, + existingArgs: nil, + wantArgs: []string{"--v=2"}, + wantChanged: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deployment := newTestDeploymentWithArgs(tc.existingArgs) + operatorSpec := &operatorv1.OperatorSpec{ + LogLevel: tc.logLevel, + } + + err := withLogLevel(operatorSpec, deployment) + require.NoError(t, err) + require.Equal(t, tc.wantArgs, deployment.Spec.Template.Spec.Containers[0].Args) + }) + } +} diff --git a/pkg/controller/certmanager/deployment_overrides_test.go b/pkg/controller/certmanager/deployment_overrides_test.go index e51fc669f..e1254ab3f 100644 --- a/pkg/controller/certmanager/deployment_overrides_test.go +++ b/pkg/controller/certmanager/deployment_overrides_test.go @@ -4,12 +4,14 @@ import ( "strings" "testing" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/library-go/pkg/operator/resource/resourceread" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" corelistersv1 "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" @@ -226,6 +228,27 @@ func TestUnsupportedConfigOverrides(t *testing.T) { } } +func TestWithUnsupportedArgsOverrideHookInvalidJSON(t *testing.T) { + deployment := newTestDeploymentWithArgs([]string{"--v=2"}) + operatorSpec := &operatorv1.OperatorSpec{ + UnsupportedConfigOverrides: runtime.RawExtension{ + Raw: []byte(`{invalid-json`), + }, + } + + err := withUnsupportedArgsOverrideHook(operatorSpec, deployment) + require.Error(t, err, "expected error for invalid JSON in UnsupportedConfigOverrides") +} + +func TestWithUnsupportedArgsOverrideHookEmptyRaw(t *testing.T) { + deployment := newTestDeploymentWithArgs([]string{"--v=2"}) + operatorSpec := &operatorv1.OperatorSpec{} + + err := withUnsupportedArgsOverrideHook(operatorSpec, deployment) + require.NoError(t, err) + require.Equal(t, []string{"--v=2"}, deployment.Spec.Template.Spec.Containers[0].Args) +} + func TestParseEnvMap(t *testing.T) { tests := []struct { name string diff --git a/pkg/controller/certmanager/related_images_test.go b/pkg/controller/certmanager/related_images_test.go index 3727dbfc3..afbac59ff 100644 --- a/pkg/controller/certmanager/related_images_test.go +++ b/pkg/controller/certmanager/related_images_test.go @@ -42,3 +42,108 @@ func Test_certManagerImage(t *testing.T) { }) } } + +func Test_certManagerWebhookImage(t *testing.T) { + tests := []struct { + name string + defaultImage string + envVarValue string + want string + }{ + { + name: "Use default webhook image when env var is empty", + defaultImage: "quay.io/jetstack/cert-manager-webhook:latest", + envVarValue: "", + want: "quay.io/jetstack/cert-manager-webhook:latest", + }, + { + name: "Use override webhook image when env var is set", + defaultImage: "quay.io/jetstack/cert-manager-webhook:latest", + envVarValue: "registry.redhat.io/cert-manager/cert-manager-webhook-rhel-8:latest", + want: "registry.redhat.io/cert-manager/cert-manager-webhook-rhel-8:latest", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + os.Setenv("RELATED_IMAGE_CERT_MANAGER_WEBHOOK", tt.envVarValue) + defer os.Unsetenv("RELATED_IMAGE_CERT_MANAGER_WEBHOOK") + if got := certManagerImage(tt.defaultImage); got != tt.want { + t.Errorf("certManagerImage() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_certManagerCAInjectorImage(t *testing.T) { + tests := []struct { + name string + defaultImage string + envVarValue string + want string + }{ + { + name: "Use default cainjector image when env var is empty", + defaultImage: "quay.io/jetstack/cert-manager-cainjector:latest", + envVarValue: "", + want: "quay.io/jetstack/cert-manager-cainjector:latest", + }, + { + name: "Use override cainjector image when env var is set", + defaultImage: "quay.io/jetstack/cert-manager-cainjector:latest", + envVarValue: "registry.redhat.io/cert-manager/cert-manager-cainjector-rhel-8:latest", + want: "registry.redhat.io/cert-manager/cert-manager-cainjector-rhel-8:latest", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + os.Setenv("RELATED_IMAGE_CERT_MANAGER_CA_INJECTOR", tt.envVarValue) + defer os.Unsetenv("RELATED_IMAGE_CERT_MANAGER_CA_INJECTOR") + if got := certManagerImage(tt.defaultImage); got != tt.want { + t.Errorf("certManagerImage() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_certManagerACMESolverImage(t *testing.T) { + tests := []struct { + name string + defaultImage string + envVarValue string + want string + }{ + { + name: "Use default acmesolver image when env var is empty", + defaultImage: "quay.io/jetstack/cert-manager-acmesolver:latest", + envVarValue: "", + want: "quay.io/jetstack/cert-manager-acmesolver:latest", + }, + { + name: "Use override acmesolver image when env var is set", + defaultImage: "quay.io/jetstack/cert-manager-acmesolver:latest", + envVarValue: "registry.redhat.io/cert-manager/cert-manager-acmesolver-rhel-8:latest", + want: "registry.redhat.io/cert-manager/cert-manager-acmesolver-rhel-8:latest", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + os.Setenv("RELATED_IMAGE_CERT_MANAGER_ACMESOLVER", tt.envVarValue) + defer os.Unsetenv("RELATED_IMAGE_CERT_MANAGER_ACMESOLVER") + if got := certManagerImage(tt.defaultImage); got != tt.want { + t.Errorf("certManagerImage() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_certManagerImageUnknownImage(t *testing.T) { + // An unknown image should be returned as-is regardless of env vars + os.Setenv("RELATED_IMAGE_CERT_MANAGER_CONTROLLER", "override:latest") + defer os.Unsetenv("RELATED_IMAGE_CERT_MANAGER_CONTROLLER") + + defaultImage := "quay.io/some-other/image:v1.0" + got := certManagerImage(defaultImage) + if got != defaultImage { + t.Errorf("certManagerImage() = %v, want %v", got, defaultImage) + } +} diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go index d0ea892ba..092660d94 100644 --- a/pkg/controller/common/client_test.go +++ b/pkg/controller/common/client_test.go @@ -2,15 +2,20 @@ package common import ( "context" + "fmt" "net/http" "testing" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" "k8s.io/client-go/tools/events" "k8s.io/client-go/tools/record" @@ -134,3 +139,353 @@ func TestNewClient(t *testing.T) { require.True(t, ok, "NewClient must return *ctrlClientImpl") assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") } + +// mockClient implements client.Client with configurable behavior for testing. +type mockClient struct { + client.Client // embed to satisfy interface; only tested methods are overridden + + getFunc func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error + createFunc func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error + updateFunc func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error + deleteFunc func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error + listFunc func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error + patchFunc func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error + + statusClient *mockStatusClient +} + +type mockStatusClient struct { + updateFunc func(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error +} + +func (m *mockStatusClient) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if m.updateFunc != nil { + return m.updateFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockStatusClient) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { + return nil +} + +func (m *mockStatusClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + return nil +} + +func (m *mockStatusClient) Apply(ctx context.Context, cfg runtime.ApplyConfiguration, opts ...client.SubResourceApplyOption) error { + return nil +} + +func (m *mockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if m.getFunc != nil { + return m.getFunc(ctx, key, obj, opts...) + } + return nil +} + +func (m *mockClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if m.createFunc != nil { + return m.createFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + if m.updateFunc != nil { + return m.updateFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + if m.deleteFunc != nil { + return m.deleteFunc(ctx, obj, opts...) + } + return nil +} + +func (m *mockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if m.listFunc != nil { + return m.listFunc(ctx, list, opts...) + } + return nil +} + +func (m *mockClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if m.patchFunc != nil { + return m.patchFunc(ctx, obj, patch, opts...) + } + return nil +} + +func (m *mockClient) Status() client.SubResourceWriter { + if m.statusClient != nil { + return m.statusClient + } + return &mockStatusClient{} +} + +func newCtrlClient(mc *mockClient) *ctrlClientImpl { + return &ctrlClientImpl{Client: mc} +} + +func TestClientExists_Found(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return nil + }, + } + c := newCtrlClient(mc) + + found, err := c.Exists(context.Background(), types.NamespacedName{Name: "test", Namespace: "default"}, &corev1.ConfigMap{}) + require.NoError(t, err) + assert.True(t, found) +} + +func TestClientExists_NotFound(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "missing") + }, + } + c := newCtrlClient(mc) + + found, err := c.Exists(context.Background(), types.NamespacedName{Name: "missing", Namespace: "default"}, &corev1.ConfigMap{}) + require.NoError(t, err) + assert.False(t, found) +} + +func TestClientExists_Error(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return fmt.Errorf("connection refused") + }, + } + c := newCtrlClient(mc) + + found, err := c.Exists(context.Background(), types.NamespacedName{Name: "test", Namespace: "default"}, &corev1.ConfigMap{}) + require.Error(t, err) + assert.False(t, found) +} + +func TestClientGet_Success(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, key client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + cm := obj.(*corev1.ConfigMap) + cm.Name = key.Name + cm.Namespace = key.Namespace + cm.Data = map[string]string{"k": "v"} + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{} + err := c.Get(context.Background(), types.NamespacedName{Name: "myconfig", Namespace: "default"}, cm) + require.NoError(t, err) + assert.Equal(t, "myconfig", cm.Name) + assert.Equal(t, "v", cm.Data["k"]) +} + +func TestClientGet_NotFound(t *testing.T) { + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + return apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "missing") + }, + } + c := newCtrlClient(mc) + + err := c.Get(context.Background(), types.NamespacedName{Name: "missing", Namespace: "default"}, &corev1.ConfigMap{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to get object") +} + +func TestClientCreate_Success(t *testing.T) { + var created bool + mc := &mockClient{ + createFunc: func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + created = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "new", Namespace: "default"}} + err := c.Create(context.Background(), cm) + require.NoError(t, err) + assert.True(t, created) +} + +func TestClientCreate_AlreadyExists(t *testing.T) { + mc := &mockClient{ + createFunc: func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, "dup") + }, + } + c := newCtrlClient(mc) + + err := c.Create(context.Background(), &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "dup", Namespace: "default"}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to create object") +} + +func TestClientUpdate_Success(t *testing.T) { + var updated bool + mc := &mockClient{ + updateFunc: func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + updated = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "upd", Namespace: "default"}} + err := c.Update(context.Background(), cm) + require.NoError(t, err) + assert.True(t, updated) +} + +func TestClientDelete_Success(t *testing.T) { + var deleted bool + mc := &mockClient{ + deleteFunc: func(_ context.Context, _ client.Object, _ ...client.DeleteOption) error { + deleted = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "del", Namespace: "default"}} + err := c.Delete(context.Background(), cm) + require.NoError(t, err) + assert.True(t, deleted) +} + +func TestClientList_Success(t *testing.T) { + mc := &mockClient{ + listFunc: func(_ context.Context, list client.ObjectList, _ ...client.ListOption) error { + cmList := list.(*corev1.ConfigMapList) + cmList.Items = []corev1.ConfigMap{ + {ObjectMeta: metav1.ObjectMeta{Name: "cm1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "cm2"}}, + } + return nil + }, + } + c := newCtrlClient(mc) + + list := &corev1.ConfigMapList{} + err := c.List(context.Background(), list) + require.NoError(t, err) + assert.Len(t, list.Items, 2) +} + +func TestClientUpdateWithRetry_Success(t *testing.T) { + updateCallCount := 0 + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + obj.SetResourceVersion("123") + return nil + }, + updateFunc: func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + updateCallCount++ + // Succeed on the first try. + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "retry-cm", Namespace: "default"}} + err := c.UpdateWithRetry(context.Background(), cm) + require.NoError(t, err) + assert.Equal(t, 1, updateCallCount) + assert.Equal(t, "123", cm.GetResourceVersion()) +} + +func TestClientUpdateWithRetry_ConflictThenSuccess(t *testing.T) { + callCount := 0 + mc := &mockClient{ + getFunc: func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { + obj.SetResourceVersion(fmt.Sprintf("rv-%d", callCount)) + return nil + }, + updateFunc: func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + callCount++ + if callCount == 1 { + return apierrors.NewConflict(schema.GroupResource{Resource: "configmaps"}, "retry-cm", fmt.Errorf("conflict")) + } + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "retry-cm", Namespace: "default"}} + err := c.UpdateWithRetry(context.Background(), cm) + require.NoError(t, err) + assert.Equal(t, 2, callCount, "update should be called twice: conflict then success") +} + +func TestClientPatch_Success(t *testing.T) { + var patched bool + mc := &mockClient{ + patchFunc: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + patched = true + return nil + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "patch-cm", Namespace: "default"}} + err := c.Patch(context.Background(), cm, client.MergeFrom(cm)) + require.NoError(t, err) + assert.True(t, patched) +} + +func TestClientPatch_Error(t *testing.T) { + mc := &mockClient{ + patchFunc: func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return fmt.Errorf("patch failed") + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "patch-cm", Namespace: "default"}} + err := c.Patch(context.Background(), cm, client.MergeFrom(cm)) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to patch object") +} + +func TestClientStatusUpdate_Success(t *testing.T) { + var updated bool + mc := &mockClient{ + statusClient: &mockStatusClient{ + updateFunc: func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + updated = true + return nil + }, + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "status-cm", Namespace: "default"}} + err := c.StatusUpdate(context.Background(), cm) + require.NoError(t, err) + assert.True(t, updated) +} + +func TestClientStatusUpdate_Error(t *testing.T) { + mc := &mockClient{ + statusClient: &mockStatusClient{ + updateFunc: func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + return fmt.Errorf("status update failed") + }, + }, + } + c := newCtrlClient(mc) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "status-cm", Namespace: "default"}} + err := c.StatusUpdate(context.Background(), cm) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to update status") +} diff --git a/pkg/controller/common/reconcile_result_test.go b/pkg/controller/common/reconcile_result_test.go new file mode 100644 index 000000000..856d603aa --- /dev/null +++ b/pkg/controller/common/reconcile_result_test.go @@ -0,0 +1,162 @@ +package common + +import ( + "fmt" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + ctrl "sigs.k8s.io/controller-runtime" + + v1alpha1 "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" +) + +func TestHandleReconcileResult(t *testing.T) { + const requeueDuration = 5 * time.Second + log := logr.Discard() + + tests := map[string]struct { + reconcileErr error + updateFnErr error + expectResult ctrl.Result + expectErr bool + expectCallCount int + }{ + "irrecoverable error sets degraded and does not requeue": { + reconcileErr: NewIrrecoverableError(fmt.Errorf("perm fail"), "something broke"), + expectResult: ctrl.Result{}, + expectErr: false, + expectCallCount: 1, + }, + "recoverable error sets progressing and requeues after duration": { + reconcileErr: NewRetryRequiredError(fmt.Errorf("temp fail"), "retry later"), + expectResult: ctrl.Result{RequeueAfter: requeueDuration}, + expectErr: false, + expectCallCount: 1, + }, + "nil error sets ready and does not requeue": { + reconcileErr: nil, + expectResult: ctrl.Result{}, + expectErr: false, + expectCallCount: 1, + }, + "updateConditionFn error on irrecoverable is propagated": { + reconcileErr: NewIrrecoverableError(fmt.Errorf("perm fail"), "broke"), + updateFnErr: fmt.Errorf("status update failed"), + expectResult: ctrl.Result{}, + expectErr: true, + expectCallCount: 1, + }, + "updateConditionFn error on recoverable is propagated instead of requeue": { + reconcileErr: NewRetryRequiredError(fmt.Errorf("temp fail"), "retry"), + updateFnErr: fmt.Errorf("status update failed"), + expectResult: ctrl.Result{}, + expectErr: true, + expectCallCount: 1, + }, + "updateConditionFn error on success is propagated": { + reconcileErr: nil, + updateFnErr: fmt.Errorf("status update failed"), + expectResult: ctrl.Result{}, + expectErr: true, + expectCallCount: 1, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + // Fresh status so conditions are always unset and SetCondition returns true. + status := &v1alpha1.ConditionalStatus{} + + callCount := 0 + updateFn := func(prependErr error) error { + callCount++ + return tc.updateFnErr + } + + result, err := HandleReconcileResult(status, tc.reconcileErr, log, updateFn, requeueDuration) + + assert.Equal(t, tc.expectResult, result) + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + assert.Equal(t, tc.expectCallCount, callCount, "updateConditionFn call count mismatch") + }) + } +} + +func TestHandleReconcileResult_ConditionsSet(t *testing.T) { + const requeueDuration = 5 * time.Second + log := logr.Discard() + + noopUpdate := func(_ error) error { return nil } + + t.Run("irrecoverable error sets Degraded=True and Ready=False", func(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + _, _ = HandleReconcileResult(status, NewIrrecoverableError(fmt.Errorf("fail"), "broke"), log, noopUpdate, requeueDuration) + + degraded := status.GetCondition(v1alpha1.Degraded) + require.NotNil(t, degraded) + assert.Equal(t, "True", string(degraded.Status)) + assert.Equal(t, v1alpha1.ReasonFailed, degraded.Reason) + + ready := status.GetCondition(v1alpha1.Ready) + require.NotNil(t, ready) + assert.Equal(t, "False", string(ready.Status)) + }) + + t.Run("recoverable error sets Degraded=False and Ready=False", func(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + _, _ = HandleReconcileResult(status, NewRetryRequiredError(fmt.Errorf("temp"), "retry"), log, noopUpdate, requeueDuration) + + degraded := status.GetCondition(v1alpha1.Degraded) + require.NotNil(t, degraded) + assert.Equal(t, "False", string(degraded.Status)) + + ready := status.GetCondition(v1alpha1.Ready) + require.NotNil(t, ready) + assert.Equal(t, "False", string(ready.Status)) + assert.Equal(t, v1alpha1.ReasonInProgress, ready.Reason) + }) + + t.Run("success sets Degraded=False and Ready=True", func(t *testing.T) { + status := &v1alpha1.ConditionalStatus{} + _, _ = HandleReconcileResult(status, nil, log, noopUpdate, requeueDuration) + + degraded := status.GetCondition(v1alpha1.Degraded) + require.NotNil(t, degraded) + assert.Equal(t, "False", string(degraded.Status)) + + ready := status.GetCondition(v1alpha1.Ready) + require.NotNil(t, ready) + assert.Equal(t, "True", string(ready.Status)) + assert.Equal(t, v1alpha1.ReasonReady, ready.Reason) + }) +} + +func TestHandleReconcileResult_NoUpdateWhenConditionsUnchanged(t *testing.T) { + const requeueDuration = 5 * time.Second + log := logr.Discard() + + // Pre-set conditions to match what success would set. + status := &v1alpha1.ConditionalStatus{} + status.SetCondition(v1alpha1.Degraded, "False", v1alpha1.ReasonReady, "") + status.SetCondition(v1alpha1.Ready, "True", v1alpha1.ReasonReady, "reconciliation successful") + + callCount := 0 + updateFn := func(_ error) error { + callCount++ + return nil + } + + result, err := HandleReconcileResult(status, nil, log, updateFn, requeueDuration) + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + assert.Equal(t, 0, callCount, "updateConditionFn should not be called when conditions are unchanged") +} diff --git a/pkg/controller/common/utils_test.go b/pkg/controller/common/utils_test.go index d4117d34d..0368921ab 100644 --- a/pkg/controller/common/utils_test.go +++ b/pkg/controller/common/utils_test.go @@ -8,9 +8,18 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" "sigs.k8s.io/controller-runtime/pkg/client" ) +func TestUpdateName(t *testing.T) { + cm := &corev1.ConfigMap{} + UpdateName(cm, "test-name") + assert.Equal(t, "test-name", cm.Name) +} + // TestUpdateNamespace provides table-driven tests for UpdateNamespace(obj, newNamespace). func TestUpdateNamespace(t *testing.T) { tests := []struct { @@ -292,3 +301,36 @@ func TestAddAnnotation(t *testing.T) { }) } } + +func TestDecodeObjBytes_ValidConfigMap(t *testing.T) { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + codecs := serializer.NewCodecFactory(scheme) + gv := schema.GroupVersion{Group: "", Version: "v1"} + + yamlBytes := []byte(`apiVersion: v1 +kind: ConfigMap +metadata: + name: decoded-cm + namespace: test-ns +data: + key: value +`) + + cm := DecodeObjBytes[*corev1.ConfigMap](codecs, gv, yamlBytes) + require.NotNil(t, cm) + assert.Equal(t, "decoded-cm", cm.Name) + assert.Equal(t, "test-ns", cm.Namespace) + assert.Equal(t, "value", cm.Data["key"]) +} + +func TestDecodeObjBytes_InvalidBytes(t *testing.T) { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + codecs := serializer.NewCodecFactory(scheme) + gv := schema.GroupVersion{Group: "", Version: "v1"} + + require.Panics(t, func() { + DecodeObjBytes[*corev1.ConfigMap](codecs, gv, []byte("not valid yaml or json {{{")) + }) +} diff --git a/pkg/controller/common/validation_test.go b/pkg/controller/common/validation_test.go new file mode 100644 index 000000000..949e23af5 --- /dev/null +++ b/pkg/controller/common/validation_test.go @@ -0,0 +1,214 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +func TestValidateLabelsConfig(t *testing.T) { + fldPath := field.NewPath("spec") + + t.Run("valid labels pass", func(t *testing.T) { + labels := map[string]string{ + "app": "test", + "example.com/my-component": "frontend", + } + err := ValidateLabelsConfig(labels, fldPath) + require.NoError(t, err) + }) + + t.Run("invalid label key returns error", func(t *testing.T) { + labels := map[string]string{ + "INVALID KEY WITH SPACES": "value", + } + err := ValidateLabelsConfig(labels, fldPath) + require.Error(t, err) + }) + + t.Run("empty labels pass", func(t *testing.T) { + err := ValidateLabelsConfig(map[string]string{}, fldPath) + require.NoError(t, err) + }) +} + +func TestValidateAnnotationsConfig(t *testing.T) { + fldPath := field.NewPath("spec") + + t.Run("valid annotations pass", func(t *testing.T) { + annotations := map[string]string{ + "kubectl.kubernetes.io/last-applied-configuration": "{}", + "my-annotation": "some-value", + } + err := ValidateAnnotationsConfig(annotations, fldPath) + require.NoError(t, err) + }) + + t.Run("invalid annotation key returns error", func(t *testing.T) { + annotations := map[string]string{ + "invalid key!@#$": "value", + } + err := ValidateAnnotationsConfig(annotations, fldPath) + require.Error(t, err) + }) +} + +func TestValidateNodeSelectorConfig(t *testing.T) { + fldPath := field.NewPath("spec") + + t.Run("valid nodeSelector passes", func(t *testing.T) { + nodeSelector := map[string]string{ + "kubernetes.io/os": "linux", + "node-role": "worker", + } + err := ValidateNodeSelectorConfig(nodeSelector, fldPath) + require.NoError(t, err) + }) + + t.Run("empty value key passes", func(t *testing.T) { + nodeSelector := map[string]string{ + "node.kubernetes.io/instance-type": "", + } + err := ValidateNodeSelectorConfig(nodeSelector, fldPath) + require.NoError(t, err) + }) + + t.Run("invalid key returns error", func(t *testing.T) { + nodeSelector := map[string]string{ + "BAD KEY!!!": "value", + } + err := ValidateNodeSelectorConfig(nodeSelector, fldPath) + require.Error(t, err) + }) +} + +func TestValidateTolerationsConfig(t *testing.T) { + fldPath := field.NewPath("spec") + + t.Run("valid tolerations pass", func(t *testing.T) { + tolerations := []corev1.Toleration{ + { + Key: "node.kubernetes.io/not-ready", + Operator: corev1.TolerationOpExists, + Effect: corev1.TaintEffectNoSchedule, + }, + { + Key: "dedicated", + Operator: corev1.TolerationOpEqual, + Value: "cert-manager", + Effect: corev1.TaintEffectNoSchedule, + }, + } + err := ValidateTolerationsConfig(tolerations, fldPath) + require.NoError(t, err) + }) + + t.Run("invalid operator returns error", func(t *testing.T) { + tolerations := []corev1.Toleration{ + { + Key: "key", + Operator: corev1.TolerationOperator("InvalidOp"), + Effect: corev1.TaintEffectNoSchedule, + }, + } + err := ValidateTolerationsConfig(tolerations, fldPath) + require.Error(t, err) + }) + + t.Run("empty tolerations pass", func(t *testing.T) { + err := ValidateTolerationsConfig([]corev1.Toleration{}, fldPath) + require.NoError(t, err) + }) +} + +func TestValidateResourceRequirements(t *testing.T) { + fldPath := field.NewPath("spec") + + t.Run("valid cpu and memory pass", func(t *testing.T) { + reqs := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("128Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("512Mi"), + }, + } + err := ValidateResourceRequirements(reqs, fldPath) + require.NoError(t, err) + }) + + t.Run("negative cpu returns error", func(t *testing.T) { + reqs := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("-100m"), + }, + } + err := ValidateResourceRequirements(reqs, fldPath) + require.Error(t, err) + }) + + t.Run("empty requirements pass", func(t *testing.T) { + err := ValidateResourceRequirements(corev1.ResourceRequirements{}, fldPath) + require.NoError(t, err) + }) +} + +func TestValidateAffinityRules(t *testing.T) { + fldPath := field.NewPath("spec") + + t.Run("nil affinity passes", func(t *testing.T) { + err := ValidateAffinityRules(nil, fldPath) + require.NoError(t, err) + }) + + t.Run("valid node affinity passes", func(t *testing.T) { + affinity := &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "kubernetes.io/os", + Operator: corev1.NodeSelectorOpIn, + Values: []string{"linux"}, + }, + }, + }, + }, + }, + }, + } + err := ValidateAffinityRules(affinity, fldPath) + require.NoError(t, err) + }) + + t.Run("invalid label selector in node affinity returns error", func(t *testing.T) { + affinity := &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: "kubernetes.io/os", + Operator: corev1.NodeSelectorOperator("BadOp"), + Values: []string{"linux"}, + }, + }, + }, + }, + }, + }, + } + err := ValidateAffinityRules(affinity, fldPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a valid selector operator") + }) +} diff --git a/pkg/controller/istiocsr/networkpolicies_test.go b/pkg/controller/istiocsr/networkpolicies_test.go new file mode 100644 index 000000000..7a803ce48 --- /dev/null +++ b/pkg/controller/istiocsr/networkpolicies_test.go @@ -0,0 +1,225 @@ +package istiocsr + +import ( + "context" + "strings" + "testing" + + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/openshift/cert-manager-operator/pkg/controller/common/fakes" +) + +func TestCreateOrApplyNetworkPolicies(t *testing.T) { + tests := []struct { + name string + preReq func(*Reconciler, *fakes.FakeCtrlClient) + wantErr string + }{ + { + name: "happy path all network policies applied", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + // Exists returns false so Create is called, Create succeeds by default + }, + }, + { + name: "create error propagates", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + switch obj.(type) { + case *networkingv1.NetworkPolicy: + return errTestClient + } + return nil + }) + }, + wantErr: "failed to create/update network policy from", + }, + } + + 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 + istiocsr := testIstioCSR() + + err := r.createOrApplyNetworkPolicies(istiocsr, controllerDefaultResourceLabels, false) + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErr) + } + if got := err.Error(); !strings.Contains(got, tt.wantErr) { + t.Errorf("createOrApplyNetworkPolicies() err = %q, want substring %q", got, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Verify Create was called for each network policy asset + if mock.CreateCallCount() != len(istioCSRNetworkPolicyAssets) { + t.Errorf("expected %d Create calls, got %d", len(istioCSRNetworkPolicyAssets), mock.CreateCallCount()) + } + }) + } +} + +func TestCreateOrUpdateNetworkPolicy(t *testing.T) { + tests := []struct { + name string + istioCSRCreateRecon bool + preReq func(*Reconciler, *fakes.FakeCtrlClient) + wantErr string + assertCalls func(t *testing.T, mock *fakes.FakeCtrlClient) + }{ + { + name: "policy does not exist creates it", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + }, + assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { + if mock.CreateCallCount() != 1 { + t.Errorf("expected 1 Create call, got %d", mock.CreateCallCount()) + } + }, + }, + { + name: "policy exists and matches is a no-op", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *networkingv1.NetworkPolicy: + np := testNetworkPolicy() + np.DeepCopyInto(o) + } + return true, nil + }) + }, + assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { + if mock.CreateCallCount() != 0 { + t.Errorf("expected 0 Create calls for matching policy, got %d", mock.CreateCallCount()) + } + if mock.UpdateWithRetryCallCount() != 0 { + t.Errorf("expected 0 UpdateWithRetry calls for matching policy, got %d", mock.UpdateWithRetryCallCount()) + } + }, + }, + { + name: "policy exists but differs triggers update", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *networkingv1.NetworkPolicy: + np := testNetworkPolicy() + // Modify labels so hasObjectChanged returns true + np.SetLabels(map[string]string{"modified": "true"}) + np.DeepCopyInto(o) + } + return true, nil + }) + m.UpdateWithRetryReturns(nil) + }, + assertCalls: func(t *testing.T, mock *fakes.FakeCtrlClient) { + if mock.UpdateWithRetryCallCount() != 1 { + t.Errorf("expected 1 UpdateWithRetry call for modified policy, got %d", mock.UpdateWithRetryCallCount()) + } + }, + }, + { + name: "Exists error propagates", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, errTestClient + }) + }, + wantErr: "failed to check", + }, + { + name: "Create error propagates", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + return errTestClient + }) + }, + wantErr: "failed to create", + }, + { + name: "UpdateWithRetry error propagates", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + switch o := obj.(type) { + case *networkingv1.NetworkPolicy: + np := testNetworkPolicy() + np.SetLabels(map[string]string{"modified": "true"}) + np.DeepCopyInto(o) + } + return true, nil + }) + m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, option ...client.UpdateOption) error { + return errTestClient + }) + }, + wantErr: "failed to update", + }, + } + + 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 + + policy := testNetworkPolicy() + err := r.createOrUpdateNetworkPolicy(policy, tt.istioCSRCreateRecon) + + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErr) + } + if got := err.Error(); !strings.Contains(got, tt.wantErr) { + t.Errorf("createOrUpdateNetworkPolicy() err = %q, want substring %q", got, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if tt.assertCalls != nil { + tt.assertCalls(t, mock) + } + }) + } +} + +// testNetworkPolicy returns a NetworkPolicy with expected labels and spec for tests. +func testNetworkPolicy() *networkingv1.NetworkPolicy { + return &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-network-policy", + Namespace: testIstioCSRNamespace, + Labels: controllerDefaultResourceLabels, + }, + Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + PodSelector: metav1.LabelSelector{}, + }, + } +} + + diff --git a/pkg/controller/istiocsr/utils_test.go b/pkg/controller/istiocsr/utils_test.go index d141e20bc..9409a2682 100644 --- a/pkg/controller/istiocsr/utils_test.go +++ b/pkg/controller/istiocsr/utils_test.go @@ -13,6 +13,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + certmanagermetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + + "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" + "github.com/openshift/cert-manager-operator/pkg/controller/common/fakes" ) // baseDeployment returns a minimal deployment for spec comparison tests. @@ -529,3 +533,208 @@ func TestNetworkPolicySpecModified(t *testing.T) { }) } } + +func TestValidateIstioCSRConfig(t *testing.T) { + tests := []struct { + name string + istiocsr *v1alpha1.IstioCSR + wantErr bool + wantErrMsg string + }{ + { + name: "empty IstioCSRConfig", + istiocsr: &v1alpha1.IstioCSR{ + Spec: v1alpha1.IstioCSRSpec{}, + }, + wantErr: true, + wantErrMsg: "istioCSRConfig config cannot be empty", + }, + { + name: "empty IstiodTLSConfig", + istiocsr: &v1alpha1.IstioCSR{ + Spec: v1alpha1.IstioCSRSpec{ + IstioCSRConfig: v1alpha1.IstioCSRConfig{ + CertManager: v1alpha1.CertManagerConfig{ + IssuerRef: certmanagermetav1.ObjectReference{ + Name: "test", + Kind: "issuer", + }, + }, + Istio: v1alpha1.IstioConfig{ + Namespace: "istio-system", + Revisions: []string{"default"}, + }, + }, + }, + }, + wantErr: true, + wantErrMsg: "istiodTLSConfig config cannot be empty", + }, + { + name: "empty Istio config", + istiocsr: &v1alpha1.IstioCSR{ + Spec: v1alpha1.IstioCSRSpec{ + IstioCSRConfig: v1alpha1.IstioCSRConfig{ + IstiodTLSConfig: v1alpha1.IstiodTLSConfig{ + TrustDomain: "cluster.local", + PrivateKeySize: 2048, + CertificateDuration: &metav1.Duration{Duration: DefaultCertificateDuration}, + }, + CertManager: v1alpha1.CertManagerConfig{ + IssuerRef: certmanagermetav1.ObjectReference{ + Name: "test", + Kind: "issuer", + }, + }, + }, + }, + }, + wantErr: true, + wantErrMsg: "istio config cannot be empty", + }, + { + name: "empty CertManager config", + istiocsr: &v1alpha1.IstioCSR{ + Spec: v1alpha1.IstioCSRSpec{ + IstioCSRConfig: v1alpha1.IstioCSRConfig{ + IstiodTLSConfig: v1alpha1.IstiodTLSConfig{ + TrustDomain: "cluster.local", + PrivateKeySize: 2048, + CertificateDuration: &metav1.Duration{Duration: DefaultCertificateDuration}, + }, + Istio: v1alpha1.IstioConfig{ + Namespace: "istio-system", + Revisions: []string{"default"}, + }, + }, + }, + }, + wantErr: true, + wantErrMsg: "certManager config cannot be empty", + }, + { + name: "valid config passes", + istiocsr: testIstioCSR(), + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateIstioCSRConfig(tt.istiocsr) + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.wantErrMsg) { + t.Errorf("validateIstioCSRConfig() err = %q, want substring %q", err.Error(), tt.wantErrMsg) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + +func TestUpdateCondition(t *testing.T) { + tests := []struct { + name string + prependErr error + setupFake func(*fakes.FakeCtrlClient) + wantErr bool + wantErrMsg string + // checkBug verifies that the aggregate error contains prependErr when both + // prependErr is non-nil and status update fails. The current implementation + // has a bug: it uses `err` (the raw status update error) instead of + // `prependErr` in utilerrors.NewAggregate, so the original reconcile error + // is lost from the aggregate. Compare with pkg/controller/http01proxy/utils.go + // which correctly uses `prependErr`. + checkBuggyAggregate bool + }{ + { + name: "prependErr nil and status update succeeds returns nil", + prependErr: nil, + wantErr: false, + }, + { + name: "prependErr non-nil and status update succeeds returns prependErr", + prependErr: fmt.Errorf("original reconcile error"), + wantErr: true, + wantErrMsg: "original reconcile error", + }, + { + name: "prependErr nil and status update fails returns update error", + prependErr: nil, + setupFake: func(fc *fakes.FakeCtrlClient) { + fc.StatusUpdateReturns(fmt.Errorf("status update conflict")) + }, + wantErr: true, + wantErrMsg: "failed to update", + }, + { + name: "prependErr non-nil and status update fails returns aggregate error", + prependErr: fmt.Errorf("original reconcile error"), + setupFake: func(fc *fakes.FakeCtrlClient) { + fc.StatusUpdateReturns(fmt.Errorf("status update conflict")) + }, + wantErr: true, + // BUG: The istiocsr updateCondition at utils.go line 484 passes `err` + // (the raw updateStatus error) instead of `prependErr` into the aggregate. + // This means the returned aggregate contains the status update error twice + // (once raw and once wrapped) but LOSES the original prependErr. + // + // The http01proxy version correctly uses `prependErr`: + // return utilerrors.NewAggregate([]error{prependErr, errUpdate}) + // + // The istiocsr version incorrectly uses `err`: + // return utilerrors.NewAggregate([]error{err, errUpdate}) + // + // This test documents the current (buggy) behavior. + wantErrMsg: "failed to update", + checkBuggyAggregate: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &fakes.FakeCtrlClient{} + if tt.setupFake != nil { + tt.setupFake(mock) + } + r := testReconciler(t) + r.CtrlClient = mock + + istiocsr := testIstioCSR() + err := r.updateCondition(istiocsr, tt.prependErr) + + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.wantErrMsg) { + t.Errorf("updateCondition() err = %q, want substring %q", err.Error(), tt.wantErrMsg) + } + + // Document the bug: when prependErr is non-nil and status update fails, + // the aggregate should contain prependErr but doesn't due to the bug. + if tt.checkBuggyAggregate { + errStr := err.Error() + // The correct behavior would include "original reconcile error" in the aggregate. + // Due to the bug (using `err` instead of `prependErr`), it does NOT. + if strings.Contains(errStr, "original reconcile error") { + // If this assertion fires, the bug has been fixed -- update this test. + t.Log("NOTE: updateCondition now correctly includes prependErr in aggregate -- bug is fixed") + } else { + t.Log("KNOWN BUG: updateCondition uses `err` instead of `prependErr` in aggregate (istiocsr utils.go line 484)") + } + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/pkg/controller/trustmanager/deployments_test.go b/pkg/controller/trustmanager/deployments_test.go index 8e86c456d..7d2a6b7f2 100644 --- a/pkg/controller/trustmanager/deployments_test.go +++ b/pkg/controller/trustmanager/deployments_test.go @@ -421,6 +421,144 @@ func TestDeploymentOverrides(t *testing.T) { } } +func TestContainerPortsMatch(t *testing.T) { + tests := []struct { + name string + desired []corev1.ContainerPort + existing []corev1.ContainerPort + want bool + }{ + { + name: "identical ports match", + desired: []corev1.ContainerPort{{Name: "https", ContainerPort: 6443}}, + existing: []corev1.ContainerPort{{Name: "https", ContainerPort: 6443}}, + want: true, + }, + { + name: "mismatched port counts do not match", + desired: []corev1.ContainerPort{{Name: "https", ContainerPort: 6443}}, + existing: []corev1.ContainerPort{{Name: "https", ContainerPort: 6443}, {Name: "metrics", ContainerPort: 9402}}, + want: false, + }, + { + name: "same count but unmatched port name", + desired: []corev1.ContainerPort{{Name: "https", ContainerPort: 6443}}, + existing: []corev1.ContainerPort{{Name: "webhook", ContainerPort: 6443}}, + want: false, + }, + { + name: "same count but unmatched port number", + desired: []corev1.ContainerPort{{Name: "https", ContainerPort: 6443}}, + existing: []corev1.ContainerPort{{Name: "https", ContainerPort: 8443}}, + want: false, + }, + { + name: "both empty match", + desired: nil, + existing: nil, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := containerPortsMatch(tt.desired, tt.existing) + if got != tt.want { + t.Errorf("containerPortsMatch() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestReadinessProbeModified(t *testing.T) { + tests := []struct { + name string + desired *corev1.Probe + existing *corev1.Probe + want bool + }{ + { + name: "both nil not modified", + desired: nil, + existing: nil, + want: false, + }, + { + name: "desired nil existing non-nil is modified", + desired: nil, + existing: &corev1.Probe{}, + want: true, + }, + { + name: "desired non-nil existing nil is modified", + desired: &corev1.Probe{}, + existing: nil, + want: true, + }, + { + name: "HTTPGet path difference is modified", + desired: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/readyz"}, + }, + }, + existing: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/healthz"}, + }, + }, + want: true, + }, + { + name: "identical HTTPGet probes not modified", + desired: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/readyz"}, + }, + InitialDelaySeconds: 3, + PeriodSeconds: 7, + }, + existing: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/readyz"}, + }, + InitialDelaySeconds: 3, + PeriodSeconds: 7, + }, + want: false, + }, + { + name: "desired has HTTPGet existing does not is modified", + desired: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/readyz"}, + }, + }, + existing: &corev1.Probe{}, + want: true, + }, + { + name: "different PeriodSeconds is modified", + desired: &corev1.Probe{ + PeriodSeconds: 10, + }, + existing: &corev1.Probe{ + PeriodSeconds: 5, + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := readinessProbeModified(tt.desired, tt.existing) + if got != tt.want { + t.Errorf("readinessProbeModified() = %v, want %v", got, tt.want) + } + }) + } +} + func TestDeploymentReconciliation(t *testing.T) { tests := []struct { name string diff --git a/pkg/controller/trustmanager/utils_test.go b/pkg/controller/trustmanager/utils_test.go index a113ace02..4e3f12583 100644 --- a/pkg/controller/trustmanager/utils_test.go +++ b/pkg/controller/trustmanager/utils_test.go @@ -1,6 +1,7 @@ package trustmanager import ( + "context" "fmt" "strings" "testing" @@ -8,9 +9,11 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/controller/common" + "github.com/openshift/cert-manager-operator/pkg/controller/common/fakes" ) func TestGetTrustNamespace(t *testing.T) { @@ -354,3 +357,171 @@ func TestDecodeServiceAccountObjBytes(t *testing.T) { }) } } + +func TestManagedAnnotationsModified(t *testing.T) { + tests := []struct { + name string + desiredAnnotations map[string]string + currentAnnotations map[string]string + wantModified bool + }{ + { + name: "identical annotations not modified", + desiredAnnotations: map[string]string{"key": "value"}, + currentAnnotations: map[string]string{"key": "value"}, + wantModified: false, + }, + { + name: "missing managed annotation is modified", + desiredAnnotations: map[string]string{"managed": "value"}, + currentAnnotations: map[string]string{"other": "value"}, + wantModified: true, + }, + { + name: "changed managed annotation is modified", + desiredAnnotations: map[string]string{"key": "desired"}, + currentAnnotations: map[string]string{"key": "tampered"}, + wantModified: true, + }, + { + name: "extra annotation on existing is allowed", + desiredAnnotations: map[string]string{"managed": "value"}, + currentAnnotations: map[string]string{"managed": "value", "extra": "ok"}, + wantModified: false, + }, + { + name: "nil desired annotations not modified", + desiredAnnotations: nil, + currentAnnotations: map[string]string{"any": "value"}, + wantModified: false, + }, + { + name: "nil existing annotations with desired is modified", + desiredAnnotations: map[string]string{"key": "value"}, + currentAnnotations: nil, + wantModified: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + desired := testTrustManager().Build() + desired.SetAnnotations(tt.desiredAnnotations) + + existing := testTrustManager().Build() + existing.SetAnnotations(tt.currentAnnotations) + + got := managedAnnotationsModified(desired, existing) + if got != tt.wantModified { + t.Errorf("expected modified=%v, got %v", tt.wantModified, got) + } + }) + } +} + +func TestAddFinalizerAlreadyPresent(t *testing.T) { + r := testReconciler(t) + mock := &fakes.FakeCtrlClient{} + r.CtrlClient = mock + + tm := testTrustManager().Build() + tm.Finalizers = []string{finalizer} + + err := r.addFinalizer(context.Background(), tm) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // When the finalizer is already present, no Update or Get calls should occur. + if got := mock.UpdateWithRetryCallCount(); got != 0 { + t.Errorf("expected 0 UpdateWithRetry calls, got %d", got) + } + if got := mock.GetCallCount(); got != 0 { + t.Errorf("expected 0 Get calls, got %d", got) + } +} + +func TestAddFinalizerGetAfterUpdateFails(t *testing.T) { + r := testReconciler(t) + mock := &fakes.FakeCtrlClient{} + // UpdateWithRetry succeeds, but Get after update fails. + mock.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { + return errTestClient + }) + r.CtrlClient = mock + + tm := testTrustManager().Build() + + err := r.addFinalizer(context.Background(), tm) + if err == nil { + t.Fatal("expected error, got nil") + } + assertError(t, err, "failed to fetch trustmanager.openshift.operator.io") +} + +func TestRemoveFinalizerNotPresent(t *testing.T) { + r := testReconciler(t) + mock := &fakes.FakeCtrlClient{} + r.CtrlClient = mock + + tm := testTrustManager().Build() + // No finalizers set — removeFinalizer should be a no-op. + + err := r.removeFinalizer(context.Background(), tm, finalizer) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if got := mock.UpdateWithRetryCallCount(); got != 0 { + t.Errorf("expected 0 UpdateWithRetry calls, got %d", got) + } +} + +func TestUpdateStatusGetFailure(t *testing.T) { + r := testReconciler(t) + mock := &fakes.FakeCtrlClient{} + mock.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { + return errTestClient + }) + r.CtrlClient = mock + + tm := testTrustManager().Build() + tm.Name = trustManagerObjectName + tm.Status.Conditions = []metav1.Condition{ + { + Type: v1alpha1.Ready, + Status: metav1.ConditionTrue, + }, + } + + err := r.updateStatus(context.Background(), tm) + if err == nil { + t.Fatal("expected error, got nil") + } + assertError(t, err, "failed to fetch trustmanager.openshift.operator.io") +} + +func TestUpdateStatusStatusUpdateFailure(t *testing.T) { + r := testReconciler(t) + mock := &fakes.FakeCtrlClient{} + mock.GetCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) error { + switch o := obj.(type) { + case *v1alpha1.TrustManager: + testTrustManager().Build().DeepCopyInto(o) + } + return nil + }) + mock.StatusUpdateCalls(func(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + return errTestClient + }) + r.CtrlClient = mock + + tm := testTrustManager().Build() + tm.Name = trustManagerObjectName + + err := r.updateStatus(context.Background(), tm) + if err == nil { + t.Fatal("expected error, got nil") + } + assertError(t, err, "failed to update trustmanager.openshift.operator.io") +} diff --git a/pkg/controller/trustmanager/webhooks_test.go b/pkg/controller/trustmanager/webhooks_test.go index 17841c685..c357bb550 100644 --- a/pkg/controller/trustmanager/webhooks_test.go +++ b/pkg/controller/trustmanager/webhooks_test.go @@ -197,6 +197,41 @@ func TestValidatingWebhookConfigReconciliation(t *testing.T) { wantExistsCount: 1, wantPatchCount: 1, }, + { + name: "apply when existing has rules drift", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + vwc := getValidatingWebhookConfigObject(testResourceLabels(), testResourceAnnotations()) + vwc.Webhooks[0].Rules = []admissionregistrationv1.RuleWithOperations{ + { + Operations: []admissionregistrationv1.OperationType{admissionregistrationv1.Create}, + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"tampered.io"}, + APIVersions: []string{"v1"}, + Resources: []string{"fakes"}, + }, + }, + } + vwc.DeepCopyInto(obj.(*admissionregistrationv1.ValidatingWebhookConfiguration)) + return true, nil + }) + }, + wantExistsCount: 1, + wantPatchCount: 1, + }, + { + name: "apply when existing has admission review versions drift", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, key client.ObjectKey, obj client.Object) (bool, error) { + vwc := getValidatingWebhookConfigObject(testResourceLabels(), testResourceAnnotations()) + vwc.Webhooks[0].AdmissionReviewVersions = []string{"v1beta1"} + vwc.DeepCopyInto(obj.(*admissionregistrationv1.ValidatingWebhookConfiguration)) + return true, nil + }) + }, + wantExistsCount: 1, + wantPatchCount: 1, + }, { name: "exists error propagates", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { diff --git a/pkg/features/features_test.go b/pkg/features/features_test.go index b064b67d0..0011e3321 100644 --- a/pkg/features/features_test.go +++ b/pkg/features/features_test.go @@ -293,3 +293,54 @@ func TestIsTrustManagerFeatureGateEnabled(t *testing.T) { }) } } + +// TestIsIstioCSRFeatureGateEnabled covers the IstioCSR operator featuregate +// (--unsupported-addon-features). IstioCSR is GA and enabled by default, +// so it does not depend on cluster FeatureSet (unlike TrustManager). +func TestIsIstioCSRFeatureGateEnabled(t *testing.T) { + defer func() { + // IstioCSR defaults to true; restore after test + _ = SetupWithFlagValue("IstioCSR=true") + }() + + tests := []struct { + name string + prep func(t *testing.T) + assert func(t *testing.T) + }{ + { + name: "returns true when operator featuregate is on", + prep: func(t *testing.T) { + t.Helper() + require.NoError(t, SetupWithFlagValue("IstioCSR=true")) + }, + assert: func(t *testing.T) { + t.Helper() + assert.True(t, IsIstioCSRFeatureGateEnabled()) + }, + }, + { + name: "returns false when operator featuregate is off", + prep: func(t *testing.T) { + t.Helper() + require.NoError(t, SetupWithFlagValue("IstioCSR=false")) + }, + assert: func(t *testing.T) { + t.Helper() + assert.False(t, IsIstioCSRFeatureGateEnabled()) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.prep(t) + tt.assert(t) + }) + } +} + +func TestSetupWithFlagValue_invalidFlag(t *testing.T) { + err := SetupWithFlagValue("InvalidFeature=true") + require.Error(t, err) +} diff --git a/pkg/operator/operatorclient/operatorclient_test.go b/pkg/operator/operatorclient/operatorclient_test.go new file mode 100644 index 000000000..7d16a004e --- /dev/null +++ b/pkg/operator/operatorclient/operatorclient_test.go @@ -0,0 +1,373 @@ +package operatorclient + +import ( + "context" + "encoding/json" + "fmt" + "testing" + + operatorv1 "github.com/openshift/api/operator/v1" + applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ktesting "k8s.io/client-go/testing" + "k8s.io/utils/clock" + + "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" + fakeclientset "github.com/openshift/cert-manager-operator/pkg/operator/clientset/versioned/fake" + informers "github.com/openshift/cert-manager-operator/pkg/operator/informers/externalversions" +) + +// newTestOperatorClient creates an OperatorClient backed by a fake clientset. +// The provided objects are seeded into the fake client and informer cache. +func newTestOperatorClient(t *testing.T, objects ...runtime.Object) (OperatorClient, *fakeclientset.Clientset) { + t.Helper() + + fakeClient := fakeclientset.NewClientset(objects...) + factory := informers.NewSharedInformerFactory(fakeClient, 0) + + // Pre-populate the informer cache with the objects so the lister works. + informer := factory.Operator().V1alpha1().CertManagers().Informer() + for _, obj := range objects { + if err := informer.GetIndexer().Add(obj); err != nil { + t.Fatalf("failed to add object to indexer: %v", err) + } + } + + oc := OperatorClient{ + Informers: factory, + Client: fakeClient.OperatorV1alpha1(), + Clock: clock.RealClock{}, + } + return oc, fakeClient +} + +// newCertManager creates a CertManager resource for testing. +func newCertManager(opts ...func(*v1alpha1.CertManager)) *v1alpha1.CertManager { + cm := &v1alpha1.CertManager{ + TypeMeta: metav1.TypeMeta{ + Kind: "CertManager", + APIVersion: "operator.openshift.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + ResourceVersion: "1", + }, + Spec: v1alpha1.CertManagerSpec{ + OperatorSpec: operatorv1.OperatorSpec{ + ManagementState: operatorv1.Managed, + }, + }, + } + for _, fn := range opts { + fn(cm) + } + return cm +} + +func TestGetUnsupportedConfigOverrides(t *testing.T) { + tests := []struct { + name string + rawBytes []byte + expectNil bool + expectErr bool + expectValue *v1alpha1.UnsupportedConfigOverrides + }{ + { + name: "empty raw bytes returns nil config", + rawBytes: nil, + expectNil: true, + }, + { + name: "valid JSON returns parsed UnsupportedConfigOverrides", + rawBytes: []byte(`{"controller":{"args":["--foo","--bar"]}}`), + expectValue: &v1alpha1.UnsupportedConfigOverrides{ + Controller: v1alpha1.UnsupportedConfigOverridesForCertManagerController{ + Args: []string{"--foo", "--bar"}, + }, + }, + }, + { + name: "invalid JSON returns unmarshal error", + rawBytes: []byte(`{not-valid-json`), + expectErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + spec := &operatorv1.OperatorSpec{ + UnsupportedConfigOverrides: runtime.RawExtension{ + Raw: tc.rawBytes, + }, + } + result, err := GetUnsupportedConfigOverrides(spec) + + if tc.expectErr { + if err == nil { + t.Fatal("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if tc.expectNil { + if result != nil { + t.Fatalf("expected nil result, got %+v", result) + } + return + } + + got, _ := json.Marshal(result) + want, _ := json.Marshal(tc.expectValue) + if string(got) != string(want) { + t.Errorf("expected %s, got %s", want, got) + } + }) + } +} + +func TestGetOperatorState(t *testing.T) { + t.Run("success returns spec, status, and resource version", func(t *testing.T) { + cm := newCertManager(func(cm *v1alpha1.CertManager) { + cm.ResourceVersion = "42" + cm.Spec.ManagementState = operatorv1.Unmanaged + }) + oc, _ := newTestOperatorClient(t, cm) + + spec, status, rv, err := oc.GetOperatorState() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rv != "42" { + t.Errorf("expected resource version 42, got %s", rv) + } + if spec.ManagementState != operatorv1.Unmanaged { + t.Errorf("expected ManagementState Unmanaged, got %v", spec.ManagementState) + } + if status == nil { + t.Fatal("expected non-nil status") + } + }) + + t.Run("get error propagates when resource not in lister", func(t *testing.T) { + oc, _ := newTestOperatorClient(t) + + _, _, _, err := oc.GetOperatorState() + if err == nil { + t.Fatal("expected error when no resource exists, got nil") + } + }) +} + +func TestEnsureFinalizer(t *testing.T) { + ctx := context.Background() + + t.Run("adds finalizer when not present", func(t *testing.T) { + cm := newCertManager() + oc, fakeClient := newTestOperatorClient(t, cm) + + if err := oc.EnsureFinalizer(ctx, "test-finalizer"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + updated, err := fakeClient.OperatorV1alpha1().CertManagers().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + t.Fatalf("failed to get updated resource: %v", err) + } + found := false + for _, f := range updated.GetFinalizers() { + if f == "test-finalizer" { + found = true + break + } + } + if !found { + t.Error("expected finalizer 'test-finalizer' to be present on updated resource") + } + }) + + t.Run("no-op when finalizer already present", func(t *testing.T) { + cm := newCertManager(func(cm *v1alpha1.CertManager) { + cm.SetFinalizers([]string{"test-finalizer"}) + }) + oc, fakeClient := newTestOperatorClient(t, cm) + + if err := oc.EnsureFinalizer(ctx, "test-finalizer"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + updated, err := fakeClient.OperatorV1alpha1().CertManagers().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + t.Fatalf("failed to get resource: %v", err) + } + if updated.ResourceVersion != "1" { + t.Errorf("expected no update (resource version 1), got %s", updated.ResourceVersion) + } + }) + + t.Run("save error propagates", func(t *testing.T) { + cm := newCertManager() + oc, fakeClient := newTestOperatorClient(t, cm) + + fakeClient.PrependReactor("update", "certmanagers", + func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("simulated update error") + }) + + err := oc.EnsureFinalizer(ctx, "new-finalizer") + if err == nil { + t.Fatal("expected error from save, got nil") + } + if err.Error() != "simulated update error" { + t.Errorf("unexpected error message: %v", err) + } + }) +} + +func TestRemoveFinalizer(t *testing.T) { + ctx := context.Background() + + t.Run("removes finalizer when present", func(t *testing.T) { + cm := newCertManager(func(cm *v1alpha1.CertManager) { + cm.SetFinalizers([]string{"keep-me", "remove-me", "also-keep"}) + }) + oc, fakeClient := newTestOperatorClient(t, cm) + + if err := oc.RemoveFinalizer(ctx, "remove-me"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + updated, err := fakeClient.OperatorV1alpha1().CertManagers().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + t.Fatalf("failed to get updated resource: %v", err) + } + for _, f := range updated.GetFinalizers() { + if f == "remove-me" { + t.Error("finalizer 'remove-me' should have been removed") + } + } + if len(updated.GetFinalizers()) != 2 { + t.Errorf("expected 2 remaining finalizers, got %d", len(updated.GetFinalizers())) + } + }) + + t.Run("no-op when finalizer not present", func(t *testing.T) { + cm := newCertManager(func(cm *v1alpha1.CertManager) { + cm.SetFinalizers([]string{"other-finalizer"}) + }) + oc, fakeClient := newTestOperatorClient(t, cm) + + if err := oc.RemoveFinalizer(ctx, "not-present"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + updated, err := fakeClient.OperatorV1alpha1().CertManagers().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + t.Fatalf("failed to get resource: %v", err) + } + if updated.ResourceVersion != "1" { + t.Errorf("expected no update (resource version 1), got %s", updated.ResourceVersion) + } + }) + + t.Run("save error propagates", func(t *testing.T) { + cm := newCertManager(func(cm *v1alpha1.CertManager) { + cm.SetFinalizers([]string{"test-finalizer"}) + }) + oc, fakeClient := newTestOperatorClient(t, cm) + + fakeClient.PrependReactor("update", "certmanagers", + func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("simulated update error") + }) + + err := oc.RemoveFinalizer(ctx, "test-finalizer") + if err == nil { + t.Fatal("expected error from save, got nil") + } + if err.Error() != "simulated update error" { + t.Errorf("unexpected error message: %v", err) + } + }) +} + +func TestApplyOperatorStatus(t *testing.T) { + ctx := context.Background() + + t.Run("nil desiredConfiguration returns error", func(t *testing.T) { + cm := newCertManager() + oc, _ := newTestOperatorClient(t, cm) + + err := oc.ApplyOperatorStatus(ctx, "test-manager", nil) + if err == nil { + t.Fatal("expected error for nil desiredConfiguration, got nil") + } + expected := "applyConfiguration must have a value" + if err.Error() != expected { + t.Errorf("expected error %q, got %q", expected, err.Error()) + } + }) + + t.Run("not-found error creates new status", func(t *testing.T) { + // No CertManager resource exists in the fake client. + oc, _ := newTestOperatorClient(t) + + desired := applyoperatorv1.OperatorStatus() + err := oc.ApplyOperatorStatus(ctx, "test-manager", desired) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("get error (non-NotFound) propagates", func(t *testing.T) { + cm := newCertManager() + oc, fakeClient := newTestOperatorClient(t, cm) + + fakeClient.PrependReactor("get", "certmanagers", + func(action ktesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("simulated server error") + }) + + desired := applyoperatorv1.OperatorStatus() + err := oc.ApplyOperatorStatus(ctx, "test-manager", desired) + if err == nil { + t.Fatal("expected error, got nil") + } + if expected := "unable to get operator configuration: simulated server error"; err.Error() != expected { + t.Errorf("expected error %q, got %q", expected, err.Error()) + } + }) + + t.Run("deep-equal status skips update", func(t *testing.T) { + cm := newCertManager() + oc, fakeClient := newTestOperatorClient(t, cm) + + // Apply once to establish the baseline. + desired := applyoperatorv1.OperatorStatus() + if err := oc.ApplyOperatorStatus(ctx, "test-manager", desired); err != nil { + t.Fatalf("first apply failed: %v", err) + } + + // Track whether a second apply-status call is made. + applyStatusCalled := false + fakeClient.PrependReactor("patch", "certmanagers", + func(action ktesting.Action) (bool, runtime.Object, error) { + applyStatusCalled = true + return false, nil, nil + }) + + // Apply the same status again -- should be a no-op. + desired2 := applyoperatorv1.OperatorStatus() + if err := oc.ApplyOperatorStatus(ctx, "test-manager", desired2); err != nil { + t.Fatalf("second apply failed: %v", err) + } + + if applyStatusCalled { + t.Error("expected no apply-status call when status is unchanged") + } + }) +} diff --git a/pkg/operator/setup_manager_test.go b/pkg/operator/setup_manager_test.go new file mode 100644 index 000000000..4b7489d7f --- /dev/null +++ b/pkg/operator/setup_manager_test.go @@ -0,0 +1,232 @@ +package operator + +import ( + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/openshift/cert-manager-operator/pkg/controller/common" + "github.com/openshift/cert-manager-operator/pkg/controller/istiocsr" + "github.com/openshift/cert-manager-operator/pkg/controller/trustmanager" +) + +func TestBuildCacheObjectList(t *testing.T) { + t.Run("single controller enabled (IstioCSR)", func(t *testing.T) { + config := ControllerConfig{ + EnableIstioCSR: true, + EnableTrustManager: false, + } + + objectList, err := buildCacheObjectList(config) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // IstioCSR manages 9 resource types plus 1 CR type. + // Verify at least the Deployment entry exists with the correct label selector. + found := false + for key, byObj := range objectList { + if _, ok := key.(*appsv1.Deployment); ok { + found = true + sel := byObj.Label + if sel == nil { + t.Fatal("expected label selector for Deployment, got nil") + } + selectorStr := sel.String() + if selectorStr == "" { + t.Fatal("expected non-empty label selector for Deployment") + } + // Should match only the IstioCSR label value. + testLabels := map[string]string{common.ManagedResourceLabelKey: istiocsr.RequestEnqueueLabelValue} + if !sel.Matches(labelSet(testLabels)) { + t.Errorf("expected selector to match IstioCSR label value %q, selector: %s", + istiocsr.RequestEnqueueLabelValue, selectorStr) + } + break + } + } + if !found { + t.Error("expected Deployment entry in cache object list") + } + }) + + t.Run("multiple controllers enabled (labels merge)", func(t *testing.T) { + config := ControllerConfig{ + EnableIstioCSR: true, + EnableTrustManager: true, + } + + objectList, err := buildCacheObjectList(config) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Deployment is shared between IstioCSR and TrustManager. + // The label selector should match both values via the In operator. + for key, byObj := range objectList { + if _, ok := key.(*appsv1.Deployment); ok { + sel := byObj.Label + if sel == nil { + t.Fatal("expected label selector for Deployment, got nil") + } + + istioLabels := map[string]string{common.ManagedResourceLabelKey: istiocsr.RequestEnqueueLabelValue} + trustLabels := map[string]string{common.ManagedResourceLabelKey: trustmanager.RequestEnqueueLabelValue} + + if !sel.Matches(labelSet(istioLabels)) { + t.Errorf("expected merged selector to match IstioCSR label, selector: %s", sel.String()) + } + if !sel.Matches(labelSet(trustLabels)) { + t.Errorf("expected merged selector to match TrustManager label, selector: %s", sel.String()) + } + break + } + } + }) + + t.Run("no controllers enabled", func(t *testing.T) { + config := ControllerConfig{ + EnableIstioCSR: false, + EnableTrustManager: false, + } + + objectList, err := buildCacheObjectList(config) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(objectList) != 0 { + t.Errorf("expected empty object list when no controllers enabled, got %d entries", len(objectList)) + } + }) +} + +func TestAddControllerCacheConfig(t *testing.T) { + t.Run("adds new resource type", func(t *testing.T) { + objectList := make(map[client.Object]cache.ByObject) + resources := []client.Object{&appsv1.Deployment{}, &corev1.Service{}} + + err := addControllerCacheConfig(objectList, "test-controller", resources) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(objectList) != 2 { + t.Errorf("expected 2 entries, got %d", len(objectList)) + } + + // Verify the Deployment entry has the correct label selector. + _, byObj, found := findExistingCacheEntry(objectList, &appsv1.Deployment{}) + if !found { + t.Fatal("expected Deployment entry in object list") + } + testLabels := map[string]string{common.ManagedResourceLabelKey: "test-controller"} + if !byObj.Label.Matches(labelSet(testLabels)) { + t.Errorf("expected selector to match label value %q, selector: %s", "test-controller", byObj.Label.String()) + } + }) + + t.Run("merges existing resource type with In operator", func(t *testing.T) { + objectList := make(map[client.Object]cache.ByObject) + resources1 := []client.Object{&appsv1.Deployment{}} + resources2 := []client.Object{&appsv1.Deployment{}} + + err := addControllerCacheConfig(objectList, "controller-a", resources1) + if err != nil { + t.Fatalf("first addControllerCacheConfig failed: %v", err) + } + + err = addControllerCacheConfig(objectList, "controller-b", resources2) + if err != nil { + t.Fatalf("second addControllerCacheConfig failed: %v", err) + } + + // Should still be 1 entry (merged), not 2. + if len(objectList) != 1 { + t.Errorf("expected 1 merged entry, got %d", len(objectList)) + } + + _, byObj, found := findExistingCacheEntry(objectList, &appsv1.Deployment{}) + if !found { + t.Fatal("expected Deployment entry in object list") + } + + // Selector should match both label values. + labelsA := map[string]string{common.ManagedResourceLabelKey: "controller-a"} + labelsB := map[string]string{common.ManagedResourceLabelKey: "controller-b"} + if !byObj.Label.Matches(labelSet(labelsA)) { + t.Errorf("expected merged selector to match controller-a, selector: %s", byObj.Label.String()) + } + if !byObj.Label.Matches(labelSet(labelsB)) { + t.Errorf("expected merged selector to match controller-b, selector: %s", byObj.Label.String()) + } + + // Should NOT match a label value that was never added. + labelsC := map[string]string{common.ManagedResourceLabelKey: "controller-c"} + if byObj.Label.Matches(labelSet(labelsC)) { + t.Errorf("expected merged selector NOT to match controller-c, selector: %s", byObj.Label.String()) + } + }) +} + +func TestFindExistingCacheEntry(t *testing.T) { + t.Run("returns entry when type found", func(t *testing.T) { + objectList := map[client.Object]cache.ByObject{ + &appsv1.Deployment{}: {}, + &corev1.Service{}: {}, + &rbacv1.ClusterRole{}: {}, + } + + key, _, found := findExistingCacheEntry(objectList, &appsv1.Deployment{}) + if !found { + t.Fatal("expected to find Deployment entry") + } + if _, ok := key.(*appsv1.Deployment); !ok { + t.Errorf("expected key to be *appsv1.Deployment, got %T", key) + } + }) + + t.Run("returns not-found when type absent", func(t *testing.T) { + objectList := map[client.Object]cache.ByObject{ + &appsv1.Deployment{}: {}, + &corev1.Service{}: {}, + } + + _, _, found := findExistingCacheEntry(objectList, &rbacv1.ClusterRole{}) + if found { + t.Error("expected not-found for ClusterRole, but it was found") + } + }) + + t.Run("distinguishes different pointer types of same kind", func(t *testing.T) { + objectList := map[client.Object]cache.ByObject{ + &rbacv1.ClusterRole{}: {}, + } + + _, _, found := findExistingCacheEntry(objectList, &rbacv1.ClusterRoleBinding{}) + if found { + t.Error("expected not-found for ClusterRoleBinding when only ClusterRole exists") + } + }) +} + +// labelSet wraps a map to satisfy labels.Labels interface via k8s.io/apimachinery. +type labelSet map[string]string + +func (ls labelSet) Has(label string) bool { + _, ok := ls[label] + return ok +} + +func (ls labelSet) Get(label string) string { + return ls[label] +} + +func (ls labelSet) Lookup(label string) (string, bool) { + v, ok := ls[label] + return v, ok +}