diff --git a/pkg/controller/certmanager/credentials_request_test.go b/pkg/controller/certmanager/credentials_request_test.go new file mode 100644 index 000000000..bb0f1d438 --- /dev/null +++ b/pkg/controller/certmanager/credentials_request_test.go @@ -0,0 +1,241 @@ +package certmanager + +import ( + "strings" + "testing" + + 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/client-go/informers" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" + + configv1 "github.com/openshift/api/config/v1" + configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" + configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" +) + +const testSecretName = "cloud-creds" + +func TestWithCloudCredentials(t *testing.T) { + tests := []struct { + name string + deploymentName string + secretName string + secretInStore bool + decoySecretOnly bool // lister has a different secret name so Get(secretName) fails (not brittle on tt.name) + platformType configv1.PlatformType + wantErr string + wantNotFoundOK bool // if true, err must be NotFound and still contain wantErr in the message + wantVolumes int + wantMountPath string + wantAWSEnv bool + noInfra bool // if true, infra indexer is left empty so Get("cluster") fails + }{ + { + name: "non-controller deployment no-op", + deploymentName: certmanagerWebhookDeployment, + secretName: testSecretName, + platformType: configv1.AWSPlatformType, + wantVolumes: 0, + }, + { + name: "empty secret name returns nil", + deploymentName: certmanagerControllerDeployment, + secretName: "", + platformType: configv1.AWSPlatformType, + wantVolumes: 0, + }, + { + name: "secret not found returns retry error", + deploymentName: certmanagerControllerDeployment, + secretName: "missing-secret", + secretInStore: false, + decoySecretOnly: true, + platformType: configv1.AWSPlatformType, + wantErr: "Retrying", + wantVolumes: 0, + }, + { + name: "AWS adds volume, mount and env", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AWSPlatformType, + wantVolumes: 1, + wantMountPath: awsCredentialsDir, + wantAWSEnv: true, + }, + { + name: "GCP adds volume and mount, no AWS env", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.GCPPlatformType, + wantVolumes: 1, + wantMountPath: gcpCredentialsDir, + wantAWSEnv: false, + }, + { + name: "unsupported platform returns error", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.PlatformType("Unsupported"), + wantErr: "unsupported cloud provider", + wantVolumes: 0, + }, + { + name: "infra not found returns error", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AWSPlatformType, + wantErr: "cluster", + wantNotFoundOK: true, + noInfra: true, + wantVolumes: 0, + }, + { + name: "Azure platform is unsupported", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AzurePlatformType, + wantErr: "unsupported cloud provider", + wantVolumes: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var kubeClient *fake.Clientset + if tt.secretInStore { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: "cert-manager"}, + } + kubeClient = fake.NewSimpleClientset(secret) + } else if tt.decoySecretOnly { + kubeClient = fake.NewSimpleClientset(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"}, + }) + } else { + kubeClient = fake.NewSimpleClientset() + } + kubeInformers := informers.NewSharedInformerFactory(kubeClient, 0) + if tt.secretInStore || tt.wantErr != "" { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: "cert-manager"}, + } + if tt.decoySecretOnly { + secret.Name = "other" + } + kubeInformers.Core().V1().Secrets().Informer().GetStore().Add(secret) + } + stopCh := make(chan struct{}) + defer close(stopCh) + kubeInformers.Start(stopCh) + if tt.secretInStore || tt.wantErr != "" { + if !cache.WaitForCacheSync(stopCh, kubeInformers.Core().V1().Secrets().Informer().HasSynced) { + t.Fatal("secret informer did not sync") + } + } + + infraIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + if !tt.noInfra { + infra := &configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: configv1.InfrastructureStatus{ + PlatformStatus: &configv1.PlatformStatus{Type: tt.platformType}, + }, + } + _ = infraIndexer.Add(infra) + } + infraInformer := &fakeInfrastructureInformer{lister: configlistersv1.NewInfrastructureLister(infraIndexer)} + + hook := withCloudCredentials( + kubeInformers.Core().V1().Secrets(), + infraInformer, + tt.deploymentName, + tt.secretName, + ) + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "controller"}}, + }, + }, + }, + } + err := hook(nil, deployment) + + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErr) + } + matchSubstring := strings.Contains(err.Error(), tt.wantErr) + var ok bool + if tt.wantNotFoundOK { + ok = apierrors.IsNotFound(err) && matchSubstring + } else { + ok = matchSubstring + } + if !ok { + if tt.wantNotFoundOK { + t.Errorf("error = %v, want NotFound with message containing %q", err, tt.wantErr) + } else { + t.Errorf("error = %v, want substring %q", err, tt.wantErr) + } + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if n := len(deployment.Spec.Template.Spec.Volumes); n != tt.wantVolumes { + t.Errorf("volumes count = %d, want %d", n, tt.wantVolumes) + } + if tt.wantVolumes > 0 { + if deployment.Spec.Template.Spec.Volumes[0].Name != cloudCredentialsVolumeName { + t.Errorf("volume name = %q, want %q", deployment.Spec.Template.Spec.Volumes[0].Name, cloudCredentialsVolumeName) + } + if tt.wantMountPath != "" { + mounts := deployment.Spec.Template.Spec.Containers[0].VolumeMounts + if len(mounts) == 0 { + t.Fatalf("expected VolumeMount for mount path %q, got none (containers[0].VolumeMounts is empty)", tt.wantMountPath) + } + if mounts[0].MountPath != tt.wantMountPath { + t.Errorf("mount path = %q, want %q", mounts[0].MountPath, tt.wantMountPath) + } + } + var hasAWS bool + for _, e := range deployment.Spec.Template.Spec.Containers[0].Env { + if e.Name == "AWS_SDK_LOAD_CONFIG" { + hasAWS = true + break + } + } + if hasAWS != tt.wantAWSEnv { + t.Errorf("AWS_SDK_LOAD_CONFIG present = %v, want %v", hasAWS, tt.wantAWSEnv) + } + } + }) + } +} + +// fakeInfrastructureInformer implements configinformersv1.InfrastructureInformer using a fixed lister. +type fakeInfrastructureInformer struct { + lister configlistersv1.InfrastructureLister +} + +func (f *fakeInfrastructureInformer) Informer() cache.SharedIndexInformer { + return nil +} + +func (f *fakeInfrastructureInformer) Lister() configlistersv1.InfrastructureLister { + return f.lister +} + +var _ configinformersv1.InfrastructureInformer = (*fakeInfrastructureInformer)(nil) diff --git a/pkg/controller/certmanager/deployment_overrides_test.go b/pkg/controller/certmanager/deployment_overrides_test.go index aa8c111c9..7de566490 100644 --- a/pkg/controller/certmanager/deployment_overrides_test.go +++ b/pkg/controller/certmanager/deployment_overrides_test.go @@ -2,6 +2,7 @@ package certmanager import ( "reflect" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -9,9 +10,14 @@ import ( "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/client-go/tools/cache" + corelistersv1 "k8s.io/client-go/listers/core/v1" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/operator/assets" + "github.com/openshift/cert-manager-operator/pkg/operator/operatorclient" ) func TestUnsupportedConfigOverrides(t *testing.T) { @@ -457,3 +463,154 @@ func TestParseArgMap(t *testing.T) { t.Fatalf("unexpected update to arg map, diff = %v", cmp.Diff(wantMap, argMap)) } } + +// TestWithOperandImageOverrideHook covers the modified hook (parameter rename). +func TestWithOperandImageOverrideHook(t *testing.T) { + originalImage := "quay.io/jetstack/cert-manager-controller:v1.19.2" + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: certmanagerControllerDeployment}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "controller", Image: originalImage, Args: []string{"--v=2"}}, + }, + }, + }, + }, + } + err := withOperandImageOverrideHook(nil, deployment) + require.NoError(t, err) + require.Len(t, deployment.Spec.Template.Spec.Containers, 1) + + // Verify image was overridden + newImage := deployment.Spec.Template.Spec.Containers[0].Image + require.NotEmpty(t, newImage, "image should be set") + + // certManagerImage resolves image; at least args should contain acme solver for controller + args := deployment.Spec.Template.Spec.Containers[0].Args + var hasAcme bool + var acmeImage string + for _, a := range args { + if strings.HasPrefix(a, "--acme-http01-solver-image=") { + hasAcme = true + acmeImage = strings.TrimPrefix(a, "--acme-http01-solver-image=") + break + } + } + require.True(t, hasAcme, "controller deployment should get acme-http01-solver-image arg") + require.NotEmpty(t, acmeImage, "acme solver image value should not be empty") +} + +// TestWithProxyEnv covers the modified hook (parameter rename). +func TestWithProxyEnv(t *testing.T) { + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "c", Env: []corev1.EnvVar{{Name: "EXISTING", Value: "x"}}}}, + }, + }, + }, + } + err := withProxyEnv(nil, deployment) + require.NoError(t, err) + // mergeContainerEnvs merges; we should have at least EXISTING plus any proxy vars from env + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers[0].Env) +} + +// TestWithCAConfigMap covers the modified hook (parameter rename); empty name and success path. +func TestWithCAConfigMap(t *testing.T) { + t.Run("empty configmap name returns nil", func(t *testing.T) { + hook := withCAConfigMap(nil, nil, "") + dep := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c"}}}, + }, + }, + } + err := hook(nil, dep) + require.NoError(t, err) + }) + t.Run("configmap not found returns retry error", func(t *testing.T) { + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + // empty indexer -> Get("my-ca") will return not found + lister := corelistersv1.NewConfigMapLister(indexer) + fakeInformer := &fakeConfigMapInformer{lister: lister} + hook := withCAConfigMap(fakeInformer, nil, "my-ca") + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "c"}}, + }, + }, + }, + } + err := hook(nil, deployment) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), "Retrying") || apierrors.IsNotFound(err), "expected retry or NotFound") + }) + t.Run("configmap exists adds volume and volume mounts", func(t *testing.T) { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "trusted-ca", Namespace: operatorclient.TargetNamespace}, + } + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + require.NoError(t, indexer.Add(cm)) + lister := corelistersv1.NewConfigMapLister(indexer) + fakeInformer := &fakeConfigMapInformer{lister: lister} + hook := withCAConfigMap(fakeInformer, nil, "trusted-ca") + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "c1"}, {Name: "c2"}}, + }, + }, + }, + } + err := hook(nil, deployment) + require.NoError(t, err) + require.NotEmpty(t, deployment.Spec.Template.Spec.Volumes) + require.Equal(t, trustedCAVolumeName, deployment.Spec.Template.Spec.Volumes[len(deployment.Spec.Template.Spec.Volumes)-1].Name) + for i := range deployment.Spec.Template.Spec.Containers { + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers[i].VolumeMounts) + } + }) +} + +// fakeConfigMapInformer implements coreinformersv1.ConfigMapInformer for tests. +type fakeConfigMapInformer struct { + lister corelistersv1.ConfigMapLister +} + +func (f *fakeConfigMapInformer) Informer() cache.SharedIndexInformer { return nil } +func (f *fakeConfigMapInformer) Lister() corelistersv1.ConfigMapLister { + return f.lister +} + +// TestWithSABoundToken covers the modified hook (parameter rename). +func TestWithSABoundToken(t *testing.T) { + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "controller"}}, + }, + }, + }, + } + err := withSABoundToken(nil, deployment) + require.NoError(t, err) + require.NotEmpty(t, deployment.Spec.Template.Spec.Volumes) + var found bool + for _, v := range deployment.Spec.Template.Spec.Volumes { + if v.Name == boundSATokenVolumeName { + found = true + break + } + } + require.True(t, found) + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers[0].VolumeMounts) +} diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go new file mode 100644 index 000000000..585448716 --- /dev/null +++ b/pkg/controller/common/client_test.go @@ -0,0 +1,118 @@ +package common + +import ( + "context" + "net/http" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/config" + "sigs.k8s.io/controller-runtime/pkg/healthz" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" +) + +// fakeManager implements manager.Manager for unit testing NewClient. +// Only GetClient is used by NewClient; other methods return nil/zero/no-op. +type fakeManager struct { + client client.Client +} + +func (f *fakeManager) GetClient() client.Client { return f.client } +func (f *fakeManager) GetCache() cache.Cache { return nil } +func (f *fakeManager) GetScheme() *runtime.Scheme { return nil } +func (f *fakeManager) GetConfig() *rest.Config { return &rest.Config{} } +func (f *fakeManager) GetHTTPClient() *http.Client { return &http.Client{} } +func (f *fakeManager) GetFieldIndexer() client.FieldIndexer { return nil } +func (f *fakeManager) GetEventRecorderFor(string) record.EventRecorder { return nil } +func (f *fakeManager) GetRESTMapper() meta.RESTMapper { return nil } +func (f *fakeManager) GetAPIReader() client.Reader { return f.client } +func (f *fakeManager) Start(context.Context) error { return nil } +func (f *fakeManager) Add(manager.Runnable) error { return nil } +func (f *fakeManager) Elected() <-chan struct{} { ch := make(chan struct{}); close(ch); return ch } +func (f *fakeManager) AddMetricsServerExtraHandler(string, http.Handler) error { return nil } +func (f *fakeManager) AddHealthzCheck(string, healthz.Checker) error { return nil } +func (f *fakeManager) AddReadyzCheck(string, healthz.Checker) error { return nil } +func (f *fakeManager) GetWebhookServer() webhook.Server { return nil } +func (f *fakeManager) GetLogger() logr.Logger { return logr.Discard() } +func (f *fakeManager) GetControllerOptions() config.Controller { return config.Controller{} } + +// sentinelClient is a non-nil client.Client stub so NewClient tests can assert manager wiring +// (pointer identity) without controller-runtime's fake client (not vendored). +type sentinelClient struct{} + +type noopSubResourceWriter struct{} + +func (noopSubResourceWriter) Create(context.Context, client.Object, client.Object, ...client.SubResourceCreateOption) error { + return nil +} +func (noopSubResourceWriter) Update(context.Context, client.Object, ...client.SubResourceUpdateOption) error { + return nil +} +func (noopSubResourceWriter) Patch(context.Context, client.Object, client.Patch, ...client.SubResourcePatchOption) error { + return nil +} + +type noopSubResourceClient struct{} + +func (noopSubResourceClient) Get(context.Context, client.Object, client.Object, ...client.SubResourceGetOption) error { + return nil +} +func (noopSubResourceClient) Create(context.Context, client.Object, client.Object, ...client.SubResourceCreateOption) error { + return nil +} +func (noopSubResourceClient) Update(context.Context, client.Object, ...client.SubResourceUpdateOption) error { + return nil +} +func (noopSubResourceClient) Patch(context.Context, client.Object, client.Patch, ...client.SubResourcePatchOption) error { + return nil +} + +func (*sentinelClient) Get(context.Context, client.ObjectKey, client.Object, ...client.GetOption) error { + return nil +} +func (*sentinelClient) List(context.Context, client.ObjectList, ...client.ListOption) error { return nil } +func (*sentinelClient) Apply(context.Context, runtime.ApplyConfiguration, ...client.ApplyOption) error { + return nil +} +func (*sentinelClient) Create(context.Context, client.Object, ...client.CreateOption) error { return nil } +func (*sentinelClient) Delete(context.Context, client.Object, ...client.DeleteOption) error { return nil } +func (*sentinelClient) Update(context.Context, client.Object, ...client.UpdateOption) error { return nil } +func (*sentinelClient) Patch(context.Context, client.Object, client.Patch, ...client.PatchOption) error { + return nil +} +func (*sentinelClient) DeleteAllOf(context.Context, client.Object, ...client.DeleteAllOfOption) error { + return nil +} +func (*sentinelClient) Status() client.SubResourceWriter { return noopSubResourceWriter{} } +func (*sentinelClient) SubResource(string) client.SubResourceClient { return noopSubResourceClient{} } +func (*sentinelClient) Scheme() *runtime.Scheme { return nil } +func (*sentinelClient) RESTMapper() meta.RESTMapper { return nil } +func (*sentinelClient) GroupVersionKindFor(runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{}, nil +} +func (*sentinelClient) IsObjectNamespaced(runtime.Object) (bool, error) { return false, nil } + +// TestNewClient verifies NewClient returns a CtrlClient that wraps the same client instance +// as manager.GetClient() (cache-consistent wiring). +func TestNewClient(t *testing.T) { + var cl client.Client = &sentinelClient{} + mgr := &fakeManager{client: cl} + + got, err := NewClient(mgr) + require.NoError(t, err) + require.NotNil(t, got) + var _ CtrlClient = got + impl, ok := got.(*ctrlClientImpl) + require.True(t, ok, "NewClient must return *ctrlClientImpl") + assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") +} diff --git a/pkg/controller/common/errors_test.go b/pkg/controller/common/errors_test.go new file mode 100644 index 000000000..00db91b18 --- /dev/null +++ b/pkg/controller/common/errors_test.go @@ -0,0 +1,332 @@ +package common + +import ( + "fmt" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestReconcileErrors(t *testing.T) { + tests := []struct { + name string + fn func() (*ReconcileError, error) + checkReason ErrorReason + checkErr bool + wantNil bool + }{ + { + name: "NewIrrecoverableError", + fn: func() (*ReconcileError, error) { + e := NewIrrecoverableError(fmt.Errorf("x"), "message: %s", "arg") + return e, nil + }, + checkReason: IrrecoverableError, + checkErr: true, + }, + { + name: "NewIrrecoverableError nil input returns nil", + fn: func() (*ReconcileError, error) { + return NewIrrecoverableError(nil, "msg"), nil + }, + wantNil: true, + }, + { + name: "NewMultipleInstanceError", + fn: func() (*ReconcileError, error) { + return NewMultipleInstanceError(fmt.Errorf("multiple")), nil + }, + checkReason: MultipleInstanceError, + checkErr: true, + }, + { + name: "NewMultipleInstanceError nil input returns nil", + fn: func() (*ReconcileError, error) { + return NewMultipleInstanceError(nil), nil + }, + wantNil: true, + }, + { + name: "NewRetryRequiredError", + fn: func() (*ReconcileError, error) { + return NewRetryRequiredError(fmt.Errorf("x"), "retry: %d", 1), nil + }, + checkReason: RetryRequiredError, + checkErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, _ := tt.fn() + if tt.wantNil { + if got != nil { + t.Errorf("expected nil, got %v", got) + } + return + } + if got == nil { + t.Fatal("expected non-nil error") + } + if tt.checkReason != "" && got.Reason != tt.checkReason { + t.Errorf("Reason = %v, want %v", got.Reason, tt.checkReason) + } + if tt.checkErr && got.Err == nil { + t.Error("Err should be set") + } + }) + } +} + +func TestFromClientError(t *testing.T) { + tests := []struct { + name string + clientErr error + wantIrrecover bool + wantRetry bool + }{ + { + name: "Unauthorized is irrecoverable", + clientErr: apierrors.NewUnauthorized("forbidden"), + wantIrrecover: true, + }, + { + name: "Forbidden is irrecoverable", + clientErr: apierrors.NewForbidden(schema.GroupResource{Resource: "test"}, "resource", fmt.Errorf("forbidden")), + wantIrrecover: true, + }, + { + name: "Invalid is irrecoverable", + clientErr: apierrors.NewInvalid(schema.GroupKind{Kind: "Pod"}, "test-pod", nil), + wantIrrecover: true, + }, + { + name: "BadRequest is irrecoverable", + clientErr: apierrors.NewBadRequest("bad request"), + wantIrrecover: true, + }, + { + name: "ServiceUnavailable is irrecoverable", + clientErr: apierrors.NewServiceUnavailable("service unavailable"), + wantIrrecover: true, + }, + { + name: "Conflict is retry required", + clientErr: apierrors.NewConflict(schema.GroupResource{Resource: "test"}, "resource", fmt.Errorf("conflict")), + wantRetry: true, + }, + { + name: "NotFound is retry required", + clientErr: apierrors.NewNotFound(schema.GroupResource{Resource: "test"}, "resource"), + wantRetry: true, + }, + { + name: "AlreadyExists is retry required", + clientErr: apierrors.NewAlreadyExists(schema.GroupResource{Resource: "test"}, "resource"), + wantRetry: true, + }, + { + name: "nil error returns nil", + clientErr: nil, + wantRetry: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := FromClientError(tt.clientErr, "client err: %s", "context") + if tt.clientErr == nil { + if err != nil { + t.Errorf("expected nil for nil input, got %v", err) + } + return + } + if err == nil { + t.Fatal("expected non-nil error") + } + if tt.wantIrrecover && !IsIrrecoverableError(err) { + t.Error("expected irrecoverable") + } + if tt.wantRetry && !IsRetryRequiredError(err) { + t.Error("expected retry required") + } + // Verify message formatting works + if err.Message != "client err: context" { + t.Errorf("Message = %q, want %q", err.Message, "client err: context") + } + }) + } +} + +func TestFromError(t *testing.T) { + tests := []struct { + name string + inputErr error + wantNil bool + wantReason ErrorReason + wantMessage string + }{ + { + name: "preserves irrecoverable error", + inputErr: NewIrrecoverableError(fmt.Errorf("base"), "irrecoverable"), + wantReason: IrrecoverableError, + wantMessage: "wrapped: %s", + }, + { + name: "preserves retry error as retry", + inputErr: NewRetryRequiredError(fmt.Errorf("base"), "retry"), + wantReason: RetryRequiredError, + wantMessage: "wrapped: %s", + }, + { + name: "converts plain error to retry", + inputErr: fmt.Errorf("plain error"), + wantReason: RetryRequiredError, + wantMessage: "converted", + }, + { + name: "nil error returns nil", + inputErr: nil, + wantNil: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := FromError(tt.inputErr, tt.wantMessage, "arg") + if tt.wantNil { + if err != nil { + t.Errorf("expected nil, got %v", err) + } + return + } + if err == nil { + t.Fatal("expected non-nil error") + } + if err.Reason != tt.wantReason { + t.Errorf("Reason = %v, want %v", err.Reason, tt.wantReason) + } + }) + } +} + +func TestIsIrrecoverableError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "true for ReconcileError with IrrecoverableError reason", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + want: true, + }, + { + name: "false for plain error", + err: fmt.Errorf("plain"), + want: false, + }, + { + name: "false for RetryRequiredError", + err: NewRetryRequiredError(fmt.Errorf("x"), "msg"), + want: false, + }, + { + name: "false for nil", + err: nil, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsIrrecoverableError(tt.err) + if got != tt.want { + t.Errorf("IsIrrecoverableError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsRetryRequiredError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "true for RetryRequiredError", + err: NewRetryRequiredError(fmt.Errorf("x"), "msg"), + want: true, + }, + { + name: "false for IrrecoverableError", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + want: false, + }, + { + name: "false for plain error", + err: fmt.Errorf("plain"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsRetryRequiredError(tt.err) + if got != tt.want { + t.Errorf("IsRetryRequiredError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsMultipleInstanceError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "true for MultipleInstanceError", + err: NewMultipleInstanceError(fmt.Errorf("multiple")), + want: true, + }, + { + name: "false for IrrecoverableError", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsMultipleInstanceError(tt.err) + if got != tt.want { + t.Errorf("IsMultipleInstanceError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestReconcileError_Error(t *testing.T) { + tests := []struct { + name string + err *ReconcileError + wantMsg string + }{ + { + name: "formats message with error", + err: NewIrrecoverableError(fmt.Errorf("base"), "message"), + wantMsg: "message: base", + }, + { + name: "formats with arguments", + err: NewRetryRequiredError(fmt.Errorf("err"), "retry: %d", 3), + wantMsg: "retry: 3: err", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.err.Error() + if got != tt.wantMsg { + t.Errorf("Error() = %q, want %q", got, tt.wantMsg) + } + }) + } +} diff --git a/pkg/controller/common/fakes/fake_ctrl_client_test.go b/pkg/controller/common/fakes/fake_ctrl_client_test.go new file mode 100644 index 000000000..014ba332b --- /dev/null +++ b/pkg/controller/common/fakes/fake_ctrl_client_test.go @@ -0,0 +1,166 @@ +package fakes + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// TestFakeCtrlClient_Exists documents behavior of Exists with client.ObjectKey (refactor safety). +func TestFakeCtrlClient_Exists(t *testing.T) { + tests := []struct { + name string + setupStub func(*FakeCtrlClient) + key client.ObjectKey + obj client.Object + expectedOk bool + expectError bool + errorMsg string + }{ + { + name: "happy path - stub returns true", + setupStub: func(f *FakeCtrlClient) { + f.ExistsReturns(true, nil) + }, + key: client.ObjectKey{Namespace: "ns", Name: "cm"}, + obj: &corev1.ConfigMap{}, + expectedOk: true, + expectError: false, + }, + { + name: "happy path - stub returns false (not found)", + setupStub: func(f *FakeCtrlClient) { + f.ExistsReturns(false, nil) + }, + key: client.ObjectKey{Namespace: "ns", Name: "missing"}, + obj: &corev1.ConfigMap{}, + expectedOk: false, + expectError: false, + }, + { + name: "error case - stub returns error", + setupStub: func(f *FakeCtrlClient) { + f.ExistsReturns(false, errors.New("api server error")) + }, + key: client.ObjectKey{Namespace: "ns", Name: "x"}, + obj: &corev1.ConfigMap{}, + expectError: true, + errorMsg: "api server error", + }, + { + name: "boundary - empty ObjectKey", + setupStub: func(f *FakeCtrlClient) { f.ExistsReturns(false, nil) }, + key: client.ObjectKey{}, + obj: &corev1.ConfigMap{}, + expectedOk: false, + expectError: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake := &FakeCtrlClient{} + if tt.setupStub != nil { + tt.setupStub(fake) + } + ok, err := fake.Exists(context.Background(), tt.key, tt.obj) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + assert.Equal(t, tt.expectedOk, ok) + }) + } +} + +// TestFakeCtrlClient_ExistsArgsForCall verifies ExistsCalls / ExistsArgsForCall use client.ObjectKey. +func TestFakeCtrlClient_ExistsArgsForCall(t *testing.T) { + fake := &FakeCtrlClient{} + fake.ExistsReturns(true, nil) + key := client.ObjectKey{Namespace: "cert-manager", Name: "trusted-ca"} + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace}} + _, _ = fake.Exists(context.Background(), key, obj) + require.Equal(t, 1, fake.ExistsCallCount()) + ctx, gotKey, gotObj := fake.ExistsArgsForCall(0) + require.NotNil(t, ctx) + assert.Equal(t, key, gotKey, "ExistsArgsForCall must return client.ObjectKey") + assert.Same(t, obj, gotObj) +} + +// TestFakeCtrlClient_Get documents behavior of Get with client.ObjectKey. +func TestFakeCtrlClient_Get(t *testing.T) { + tests := []struct { + name string + setupStub func(*FakeCtrlClient) + key client.ObjectKey + obj client.Object + expectError bool + errorMsg string + }{ + { + name: "happy path - stub returns nil", + setupStub: func(f *FakeCtrlClient) { + f.GetStub = func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + cm := &corev1.ConfigMap{} + cm.SetName("x") + cm.SetNamespace("ns") + cm.DeepCopyInto(obj.(*corev1.ConfigMap)) + return nil + } + }, + key: client.ObjectKey{Namespace: "ns", Name: "x"}, + obj: &corev1.ConfigMap{}, + expectError: false, + }, + { + name: "error case - stub returns error", + setupStub: func(f *FakeCtrlClient) { + f.GetReturns(errors.New("not found")) + }, + key: client.ObjectKey{Namespace: "ns", Name: "missing"}, + obj: &corev1.ConfigMap{}, + expectError: true, + errorMsg: "not found", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake := &FakeCtrlClient{} + if tt.setupStub != nil { + tt.setupStub(fake) + } + err := fake.Get(context.Background(), tt.key, tt.obj) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + }) + } +} + +// TestFakeCtrlClient_GetArgsForCall verifies GetCalls / GetArgsForCall use client.ObjectKey. +func TestFakeCtrlClient_GetArgsForCall(t *testing.T) { + fake := &FakeCtrlClient{} + fake.GetReturns(nil) + key := client.ObjectKey{Namespace: "default", Name: "secret"} + obj := &corev1.Secret{} + _ = fake.Get(context.Background(), key, obj) + require.Equal(t, 1, fake.GetCallCount()) + ctx, gotKey, gotObj := fake.GetArgsForCall(0) + require.NotNil(t, ctx) + assert.Equal(t, key, gotKey, "GetArgsForCall must return client.ObjectKey") + assert.Same(t, obj, gotObj) +} diff --git a/pkg/controller/common/utils_test.go b/pkg/controller/common/utils_test.go new file mode 100644 index 000000000..582b950d8 --- /dev/null +++ b/pkg/controller/common/utils_test.go @@ -0,0 +1,269 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "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" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// TestUpdateNamespace provides table-driven tests for UpdateNamespace(obj, newNamespace). +// Refactor may move this to ctrlutil; tests ensure behavior (side-effect on obj) is preserved. +func TestUpdateNamespace(t *testing.T) { + tests := []struct { + name string + obj client.Object + newNamespace string + expectedNs string + description string + }{ + { + name: "happy path - set new namespace", + obj: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "old-ns"}, + }, + newNamespace: "new-ns", + expectedNs: "new-ns", + description: "namespace is updated in place", + }, + { + name: "edge case - empty string namespace", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, + }, + newNamespace: "", + expectedNs: "", + description: "empty namespace is allowed", + }, + { + name: "boundary - long namespace name", + obj: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "ns"}, + }, + newNamespace: "openshift-cert-manager-operator", + expectedNs: "openshift-cert-manager-operator", + description: "long namespace is set correctly", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + UpdateNamespace(tt.obj, tt.newNamespace) + assert.Equal(t, tt.expectedNs, tt.obj.GetNamespace(), tt.description) + }) + } +} + +// TestUpdateResourceLabels provides table-driven tests for UpdateResourceLabels(obj, labels). +func TestUpdateResourceLabels(t *testing.T) { + tests := []struct { + name string + obj client.Object + labels map[string]string + expectedLen int + checkLabels map[string]string + description string + }{ + { + name: "happy path - set labels", + obj: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + }, + labels: map[string]string{"key": "value", "a": "b"}, + expectedLen: 2, + checkLabels: map[string]string{"key": "value", "a": "b"}, + description: "labels replace existing", + }, + { + name: "edge case - nil labels", + obj: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"old": "v"}}, + }, + labels: nil, + expectedLen: 0, + checkLabels: nil, + description: "nil labels clears (SetLabels(nil) behavior)", + }, + { + name: "edge case - empty map", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Labels: map[string]string{"a": "1"}}, + }, + labels: map[string]string{}, + expectedLen: 0, + checkLabels: map[string]string{}, + description: "empty map clears labels", + }, + { + name: "single element", + obj: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "s"}}, + labels: map[string]string{"only": "one"}, + expectedLen: 1, + checkLabels: map[string]string{"only": "one"}, + description: "single label", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + UpdateResourceLabels(tt.obj, tt.labels) + got := tt.obj.GetLabels() + require.Len(t, got, tt.expectedLen, tt.description) + for k, v := range tt.checkLabels { + assert.Equal(t, v, got[k], "label %q", k) + } + }) + } +} + +func TestObjectMetadataModified_DifferentTypes(t *testing.T) { + sa := &corev1.ServiceAccount{} + cm := &corev1.ConfigMap{} + if ObjectMetadataModified(sa, cm) { + t.Error("ObjectMetadataModified should return false when both objects have the same labels (including both empty)") + } +} + +func TestObjectMetadataModified_SameTypeDifferentLabels(t *testing.T) { + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, + } + fetched := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "2"}}, + } + if !ObjectMetadataModified(desired, fetched) { + t.Error("ObjectMetadataModified should return true when labels differ") + } +} + +func TestObjectMetadataModified_SameTypeSameLabels(t *testing.T) { + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, + } + fetched := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, + } + if ObjectMetadataModified(desired, fetched) { + t.Error("ObjectMetadataModified should return false when labels match") + } +} + +func TestObjectMetadataModified(t *testing.T) { + tests := []struct { + name string + desired map[string]string + fetched map[string]string + wantDiff bool + }{ + {"same", map[string]string{"a": "1"}, map[string]string{"a": "1"}, false}, + {"different value", map[string]string{"a": "1"}, map[string]string{"a": "2"}, true}, + {"different key", map[string]string{"a": "1"}, map[string]string{"b": "1"}, true}, + {"both nil", nil, nil, false}, + {"desired nil fetched has labels", nil, map[string]string{"a": "1"}, true}, + {"desired has labels fetched nil", map[string]string{"a": "1"}, nil, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.desired}} + f := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.fetched}} + got := ObjectMetadataModified(d, f) + if got != tt.wantDiff { + t.Errorf("ObjectMetadataModified() = %v, want %v", got, tt.wantDiff) + } + }) + } +} + +func TestContainsAnnotation(t *testing.T) { + obj := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"present": "yes"}, + }, + } + if !ContainsAnnotation(obj, "present") { + t.Error("ContainsAnnotation should be true for present key") + } + if ContainsAnnotation(obj, "absent") { + t.Error("ContainsAnnotation should be false for absent key") + } + empty := &corev1.ServiceAccount{} + if ContainsAnnotation(empty, "any") { + t.Error("ContainsAnnotation should be false when annotations nil") + } +} + +func TestAddAnnotation(t *testing.T) { + t.Run("adds when missing", func(t *testing.T) { + obj := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{}} + added := AddAnnotation(obj, "key", "value") + if !added { + t.Error("AddAnnotation should return true when adding") + } + if obj.GetAnnotations()["key"] != "value" { + t.Errorf("annotation value = %q, want %q", obj.GetAnnotations()["key"], "value") + } + }) + t.Run("no change when present", func(t *testing.T) { + obj := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"key": "existing"}, + }, + } + added := AddAnnotation(obj, "key", "new") + if added { + t.Error("AddAnnotation should return false when already present") + } + if obj.GetAnnotations()["key"] != "existing" { + t.Errorf("annotation should be unchanged: %q", obj.GetAnnotations()["key"]) + } + }) +} + +// Ensure we can pass any client.Object to ObjectMetadataModified (e.g. *appsv1.Deployment). +func TestObjectMetadataModified_WithDifferentObjectTypes(t *testing.T) { + tests := []struct { + name string + desired client.Object + fetched client.Object + wantChanged bool + }{ + { + name: "Deployment with different labels", + desired: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "1"}}, + }, + fetched: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "2"}}, + }, + wantChanged: true, + }, + { + name: "Deployment with same labels", + desired: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "1"}}, + }, + fetched: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "1"}}, + }, + wantChanged: false, + }, + { + name: "ConfigMap with different labels", + desired: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Labels: map[string]string{"key": "val1"}}, + }, + fetched: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Labels: map[string]string{"key": "val2"}}, + }, + wantChanged: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ObjectMetadataModified(tt.desired, tt.fetched) + assert.Equal(t, tt.wantChanged, got) + }) + } +} diff --git a/pkg/controller/istiocsr/utils_test.go b/pkg/controller/istiocsr/utils_test.go new file mode 100644 index 000000000..4c4e00f69 --- /dev/null +++ b/pkg/controller/istiocsr/utils_test.go @@ -0,0 +1,523 @@ +package istiocsr + +import ( + "fmt" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" +) + +// baseDeployment returns a minimal deployment for spec comparison tests. +func baseDeployment(replicas int32, image string) *appsv1.Deployment { + return &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: &replicas, + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "x"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "x"}}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "c", + Image: image, + Args: []string{"--arg"}, + Ports: []corev1.ContainerPort{{ContainerPort: 8080}}, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{HTTPGet: &corev1.HTTPGetAction{Path: "/ready"}}, + InitialDelaySeconds: 0, + PeriodSeconds: 1, + }, + SecurityContext: &corev1.SecurityContext{}, + Resources: corev1.ResourceRequirements{}, + }}, + ServiceAccountName: "sa", + }, + }, + }, + } +} + +func TestHasObjectChanged(t *testing.T) { + tests := []struct { + name string + desired client.Object + fetched client.Object + wantPanic bool + panicSubstr string + wantChanged bool + }{ + { + name: "different types panics", + desired: &rbacv1.ClusterRole{}, + fetched: &rbacv1.ClusterRoleBinding{}, + wantPanic: true, + panicSubstr: "same type", + }, + { + name: "mismatched types (CR vs CRB) panics", + desired: &rbacv1.ClusterRole{}, + fetched: &rbacv1.ClusterRoleBinding{}, + wantPanic: true, + panicSubstr: "same type", + }, + { + name: "matching ClusterRole identical", + desired: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}}}, + }, + fetched: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}}}, + }, + wantChanged: false, + }, + { + name: "ClusterRole rules different", + desired: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}, APIGroups: []string{""}}}, + }, + fetched: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"list"}}}, + }, + wantChanged: true, + }, + { + name: "matching ClusterRoleBinding identical", + desired: &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + }, + fetched: &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + }, + wantChanged: false, + }, + { + name: "Deployment labels different", + desired: func() *appsv1.Deployment { d := baseDeployment(1, "img"); d.Labels = map[string]string{"a": "1"}; return d }(), + fetched: func() *appsv1.Deployment { d := baseDeployment(1, "img"); d.Labels = map[string]string{"a": "2"}; return d }(), + wantChanged: true, + }, + { + name: "ConfigMap identical", + desired: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Data: map[string]string{"k": "v"}, + }, + fetched: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Data: map[string]string{"k": "v"}, + }, + wantChanged: false, + }, + { + name: "Service identical", + desired: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + fetched: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + wantChanged: false, + }, + { + name: "unsupported type (Pod) panics", + desired: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "a"}}, + fetched: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "a"}}, + wantPanic: true, + panicSubstr: "unsupported", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.wantPanic { + defer func() { + r := recover() + if r == nil { + t.Error("expected panic") + } + if msg := fmt.Sprintf("%v", r); tt.panicSubstr != "" && !strings.Contains(msg, tt.panicSubstr) { + t.Errorf("panic message = %q, want substring %q", msg, tt.panicSubstr) + } + }() + } + got := hasObjectChanged(tt.desired, tt.fetched) + if !tt.wantPanic && got != tt.wantChanged { + t.Errorf("hasObjectChanged() = %v, want %v", got, tt.wantChanged) + } + }) + } +} + +func TestDeploymentSpecModified(t *testing.T) { + tests := []struct { + name string + desired *appsv1.Deployment + fetched *appsv1.Deployment + wantTrue bool + }{ + { + name: "identical", + desired: baseDeployment(1, "img"), + fetched: baseDeployment(1, "img"), + wantTrue: false, + }, + { + name: "replicas different", + desired: baseDeployment(1, "img"), + fetched: baseDeployment(2, "img"), + wantTrue: true, + }, + { + name: "image different", + desired: baseDeployment(1, "img:v1"), + fetched: baseDeployment(1, "img:v2"), + wantTrue: true, + }, + { + name: "selector match labels different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Selector.MatchLabels = map[string]string{"app": "x"} + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Selector.MatchLabels = map[string]string{"app": "y"} + return d + }(), + wantTrue: true, + }, + { + name: "template labels different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Labels = map[string]string{"app": "x"} + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Labels = map[string]string{"app": "other"} + return d + }(), + wantTrue: true, + }, + { + name: "container count different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers = append(d.Spec.Template.Spec.Containers, corev1.Container{Name: "sidecar"}) + return d + }(), + wantTrue: true, + }, + { + name: "args different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Args = []string{"--arg=a"} + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Args = []string{"--arg=b"} + return d + }(), + wantTrue: true, + }, + { + name: "container name different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Name = "c" + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Name = "other" + return d + }(), + wantTrue: true, + }, + { + name: "ports length different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Ports = append(d.Spec.Template.Spec.Containers[0].Ports, corev1.ContainerPort{ContainerPort: 9090}) + return d + }(), + wantTrue: true, + }, + { + name: "ports content different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Ports = []corev1.ContainerPort{{ContainerPort: 9090}} + return d + }(), + wantTrue: true, + }, + { + name: "readiness probe path different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].ReadinessProbe.HTTPGet.Path = "/other" + return d + }(), + wantTrue: true, + }, + { + name: "serviceAccountName different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.ServiceAccountName = "other-sa" + return d + }(), + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := deploymentSpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("deploymentSpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestHasObjectChanged_RoleAndRoleBinding(t *testing.T) { + t.Run("Role rules different", func(t *testing.T) { + desired := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}, APIGroups: []string{""}}}, + } + fetched := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"list"}}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for Role rules different") + } + }) + t.Run("ClusterRoleBinding RoleRef different", func(t *testing.T) { + desired := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "role-a"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + fetched := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "role-b"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for RoleRef different") + } + }) + t.Run("ClusterRoleBinding Subjects different", func(t *testing.T) { + desired := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + fetched := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "other", Namespace: "z"}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for Subjects different") + } + }) + t.Run("RoleBinding RoleRef different", func(t *testing.T) { + desired := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "Role", Name: "role-a"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + fetched := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "Role", Name: "role-b"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for RoleBinding RoleRef different") + } + }) +} + +func TestServiceSpecModified(t *testing.T) { + tests := []struct { + name string + desired *corev1.Service + fetched *corev1.Service + wantTrue bool + }{ + { + name: "identical", + desired: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + fetched: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + wantTrue: false, + }, + { + name: "different type", + desired: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + fetched: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := serviceSpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("serviceSpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestCertificateSpecModified(t *testing.T) { + tests := []struct { + name string + desired *certmanagerv1.Certificate + fetched *certmanagerv1.Certificate + wantTrue bool + }{ + { + name: "identical", + desired: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"a.example.com"}}}, + fetched: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"a.example.com"}}}, + wantTrue: false, + }, + { + name: "different DNSNames", + desired: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"a.example.com"}}}, + fetched: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"b.example.com"}}}, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := certificateSpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("certificateSpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestConfigMapDataModified(t *testing.T) { + tests := []struct { + name string + desired *corev1.ConfigMap + fetched *corev1.ConfigMap + wantTrue bool + }{ + { + name: "identical", + desired: &corev1.ConfigMap{Data: map[string]string{"key": "v1"}}, + fetched: &corev1.ConfigMap{Data: map[string]string{"key": "v1"}}, + wantTrue: false, + }, + { + name: "different value", + desired: &corev1.ConfigMap{Data: map[string]string{"key": "v1"}}, + fetched: &corev1.ConfigMap{Data: map[string]string{"key": "v2"}}, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := configMapDataModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("configMapDataModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestNetworkPolicySpecModified(t *testing.T) { + tests := []struct { + name string + desired *networkingv1.NetworkPolicy + fetched *networkingv1.NetworkPolicy + wantTrue bool + }{ + { + name: "identical", + desired: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}}}, + fetched: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}}}, + wantTrue: false, + }, + { + name: "different PolicyTypes", + desired: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}}}, + fetched: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeEgress}}}, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := networkPolicySpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("networkPolicySpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} diff --git a/pkg/controller/trustmanager/controller_test.go b/pkg/controller/trustmanager/controller_test.go index 65fe79c76..3efd817a0 100644 --- a/pkg/controller/trustmanager/controller_test.go +++ b/pkg/controller/trustmanager/controller_test.go @@ -10,6 +10,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -370,3 +371,33 @@ func TestProcessReconcileRequest(t *testing.T) { }) } } + +func TestCleanUp(t *testing.T) { + tests := []struct { + name string + trustManager *v1alpha1.TrustManager + wantRequeue bool + wantErr bool + }{ + { + name: "returns false and nil", + trustManager: &v1alpha1.TrustManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + }, + wantRequeue: false, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Reconciler{eventRecorder: record.NewFakeRecorder(10)} + requeue, err := r.cleanUp(tt.trustManager) + if (err != nil) != tt.wantErr { + t.Errorf("cleanUp() error = %v, wantErr %v", err, tt.wantErr) + } + if requeue != tt.wantRequeue { + t.Errorf("cleanUp() requeue = %v, want %v", requeue, tt.wantRequeue) + } + }) + } +} diff --git a/pkg/controller/trustmanager/utils_test.go b/pkg/controller/trustmanager/utils_test.go index d791ddeb0..a113ace02 100644 --- a/pkg/controller/trustmanager/utils_test.go +++ b/pkg/controller/trustmanager/utils_test.go @@ -1,9 +1,16 @@ package trustmanager import ( + "fmt" + "strings" "testing" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" + "github.com/openshift/cert-manager-operator/pkg/controller/common" ) func TestGetTrustNamespace(t *testing.T) { @@ -112,6 +119,62 @@ func TestGetResourceAnnotations(t *testing.T) { } } +func TestManagedLabelsModified(t *testing.T) { + tests := []struct { + name string + desired map[string]string + existing map[string]string + want bool + }{ + { + name: "identical labels returns not modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{"a": "1"}, + want: false, + }, + { + name: "different value for same key returns modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{"a": "2"}, + want: true, + }, + { + name: "existing has extra labels beyond desired still not modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{"a": "1", "b": "2"}, + want: false, + }, + { + name: "desired label missing on existing returns modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{}, + want: true, + }, + { + name: "nil desired labels returns not modified", + desired: nil, + existing: map[string]string{"a": "1"}, + want: false, + }, + { + name: "both nil labels returns not modified", + desired: nil, + existing: nil, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + desired := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.desired}} + existing := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.existing}} + got := managedLabelsModified(desired, existing) + if got != tt.want { + t.Errorf("managedLabelsModified() = %v, want %v", got, tt.want) + } + }) + } +} + func TestValidateTrustManagerConfig(t *testing.T) { tests := []struct { name string @@ -149,6 +212,18 @@ func TestValidateTrustManagerConfig(t *testing.T) { tm: testTrustManager().WithAnnotations(map[string]string{"invalid/key/with/extra/slash": "val"}), wantErr: `spec.controllerConfig.annotations: Invalid value:`, }, + { + name: "non-empty trustManagerConfig with explicit fields is valid", + tm: func() *trustManagerBuilder { + b := testTrustManager() + b.Spec.TrustManagerConfig = v1alpha1.TrustManagerConfig{ + LogLevel: 2, + LogFormat: "json", + TrustNamespace: "cert-manager", + } + return b + }(), + }, } for _, tt := range tests { @@ -199,3 +274,83 @@ func TestUpdateResourceAnnotations(t *testing.T) { }) } } + +func mustEncode(t *testing.T, encoder runtime.Encoder, obj runtime.Object) []byte { + t.Helper() + b, err := runtime.Encode(encoder, obj) + if err != nil { + t.Fatalf("encode: %v", err) + } + return b +} + +// TestDecodeServiceAccountObjBytes exercises common.DecodeObjBytes for ServiceAccount assets (same path as serviceaccounts.go). +func TestDecodeServiceAccountObjBytes(t *testing.T) { + validSA := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + configMapObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "ns"}, + Data: map[string]string{"k": "v"}, + } + gv := corev1.SchemeGroupVersion + encoder := codecs.LegacyCodec(gv) + tests := []struct { + name string + getBytes func(t *testing.T) []byte + wantPanic bool + panicSubstr string + wantName string + wantNamespace string + }{ + { + name: "valid ServiceAccount bytes returns SA", + getBytes: func(t *testing.T) []byte { + return mustEncode(t, encoder, validSA) + }, + wantPanic: false, + wantName: "test-sa", + wantNamespace: "test-ns", + }, + { + name: "wrong type bytes panics", + getBytes: func(t *testing.T) []byte { + return mustEncode(t, encoder, configMapObj) + }, + wantPanic: true, + panicSubstr: "ServiceAccount", + }, + { + name: "invalid bytes panics", + getBytes: func(t *testing.T) []byte { + return []byte("not valid yaml or json") + }, + wantPanic: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objBytes := tt.getBytes(t) + if tt.wantPanic { + defer func() { + r := recover() + if r == nil { + t.Error("expected panic") + } + if tt.panicSubstr != "" { + msg := fmt.Sprintf("%v", r) + if !strings.Contains(msg, tt.panicSubstr) && !strings.Contains(msg, "interface") { + t.Errorf("panic message = %q, want substring %q", msg, tt.panicSubstr) + } + } + }() + } + got := common.DecodeObjBytes[*corev1.ServiceAccount](codecs, gv, objBytes) + if !tt.wantPanic { + if got.Name != tt.wantName || got.Namespace != tt.wantNamespace { + t.Errorf("DecodeObjBytes() = %s/%s, want %s/%s", got.Name, got.Namespace, tt.wantName, tt.wantNamespace) + } + } + }) + } +}