From 33c3d47ceeab029df984beb002201e2f6c56cb6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20S=C4=99k?= Date: Wed, 25 Dec 2019 14:40:46 +0100 Subject: [PATCH] #197 Improve checking the ImagePullSecret --- go.mod | 2 +- .../backuprestore/backuprestore.go | 6 ++-- .../jenkins/configuration/base/reconcile.go | 29 +++++++++++---- .../configuration/base/reconcile_test.go | 35 +++++++++++++++++++ .../configuration/base/validate_test.go | 21 ++++++----- .../configuration/user/seedjobs/seedjobs.go | 7 ++-- pkg/controller/jenkins/jenkins_controller.go | 2 +- test/e2e/jenkins.go | 7 ++-- test/e2e/main_test.go | 3 +- 9 files changed, 79 insertions(+), 33 deletions(-) diff --git a/go.mod b/go.mod index 8ec0de98..232945bb 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( go.uber.org/zap v1.9.1 golang.org/x/crypto v0.0.0-20190909091759-094676da4a83 // indirect golang.org/x/lint v0.0.0-20190930215403-16217165b5de // indirect - golang.org/x/net v0.0.0-20190909003024-a7b16738d86b // indirect + golang.org/x/net v0.0.0-20190909003024-a7b16738d86b golang.org/x/sys v0.0.0-20190910064555-bbd175535a8b // indirect golang.org/x/text v0.3.2 // indirect golang.org/x/tools v0.0.0-20191004055002-72853e10c5a3 // indirect diff --git a/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go b/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go index 0ad43ffd..3177d897 100644 --- a/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go +++ b/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go @@ -90,7 +90,7 @@ func (bar *BackupAndRestore) Validate() []string { messages = append(messages, fmt.Sprintf("restore container '%s' not found in CR spec.master.containers", restore.ContainerName)) } if restore.Action.Exec == nil { - messages = append(messages, fmt.Sprintf("spec.restore.action.exec is not configured")) + messages = append(messages, "spec.restore.action.exec is not configured") } } @@ -101,10 +101,10 @@ func (bar *BackupAndRestore) Validate() []string { messages = append(messages, fmt.Sprintf("backup container '%s' not found in CR spec.master.containers", backup.ContainerName)) } if backup.Action.Exec == nil { - messages = append(messages, fmt.Sprintf("spec.backup.action.exec is not configured")) + messages = append(messages, "spec.backup.action.exec is not configured") } if backup.Interval == 0 { - messages = append(messages, fmt.Sprintf("spec.backup.interval is not configured")) + messages = append(messages, "spec.backup.interval is not configured") } } diff --git a/pkg/controller/jenkins/configuration/base/reconcile.go b/pkg/controller/jenkins/configuration/base/reconcile.go index 42b17b54..970da1e3 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile.go +++ b/pkg/controller/jenkins/configuration/base/reconcile.go @@ -25,7 +25,6 @@ import ( "github.com/go-logr/logr" stackerr "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -337,7 +336,7 @@ func (r *ReconcileJenkinsBaseConfiguration) addLabelForWatchesResources(customiz func (r *ReconcileJenkinsBaseConfiguration) createRBAC(meta metav1.ObjectMeta) error { serviceAccount := resources.NewServiceAccount(meta) err := r.CreateResource(serviceAccount) - if err != nil && !errors.IsAlreadyExists(err) { + if err != nil && !apierrors.IsAlreadyExists(err) { return stackerr.WithStack(err) } @@ -359,7 +358,7 @@ func (r *ReconcileJenkinsBaseConfiguration) createRBAC(meta metav1.ObjectMeta) e func (r *ReconcileJenkinsBaseConfiguration) createService(meta metav1.ObjectMeta, name string, config v1alpha2.Service) error { service := corev1.Service{} err := r.Client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: meta.Namespace}, &service) - if err != nil && errors.IsNotFound(err) { + if err != nil && apierrors.IsNotFound(err) { service = resources.UpdateService(corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -400,7 +399,7 @@ func (r *ReconcileJenkinsBaseConfiguration) ensureJenkinsMasterPod(meta metav1.O // Check if this Pod already exists currentJenkinsMasterPod, err := r.getJenkinsMasterPod() - if err != nil && errors.IsNotFound(err) { + if err != nil && apierrors.IsNotFound(err) { jenkinsMasterPod := resources.NewJenkinsMasterPod(meta, r.Configuration.Jenkins) if !reflect.DeepEqual(jenkinsMasterPod.Spec.Containers[0].Command, resources.GetJenkinsMasterContainerBaseCommand()) { r.logger.Info(fmt.Sprintf("spec.master.containers[%s].command has been overridden make sure the command looks like: '%v', otherwise the operator won't configure default user and install plugins", @@ -430,7 +429,7 @@ func (r *ReconcileJenkinsBaseConfiguration) ensureJenkinsMasterPod(meta metav1.O return reconcile.Result{Requeue: true}, err } return reconcile.Result{}, nil - } else if err != nil && !errors.IsNotFound(err) { + } else if err != nil && !apierrors.IsNotFound(err) { return reconcile.Result{}, stackerr.WithStack(err) } @@ -519,12 +518,12 @@ func (r *ReconcileJenkinsBaseConfiguration) checkForPodRecreation(currentJenkins } if !reflect.DeepEqual(r.Configuration.Jenkins.Spec.Master.SecurityContext, currentJenkinsMasterPod.Spec.SecurityContext) { - messages = append(messages, fmt.Sprintf("Jenkins pod security context has changed")) + messages = append(messages, "Jenkins pod security context has changed") verbose = append(verbose, fmt.Sprintf("Jenkins pod security context has changed, actual '%+v' required '%+v'", currentJenkinsMasterPod.Spec.SecurityContext, r.Configuration.Jenkins.Spec.Master.SecurityContext)) } - if !reflect.DeepEqual(r.Configuration.Jenkins.Spec.Master.ImagePullSecrets, currentJenkinsMasterPod.Spec.ImagePullSecrets) { + if !compareImagePullSecrets(r.Configuration.Jenkins.Spec.Master.ImagePullSecrets, currentJenkinsMasterPod.Spec.ImagePullSecrets) { messages = append(messages, "Jenkins Pod ImagePullSecrets has changed") verbose = append(verbose, fmt.Sprintf("Jenkins Pod ImagePullSecrets has changed, actual '%+v' required '%+v'", currentJenkinsMasterPod.Spec.ImagePullSecrets, r.Configuration.Jenkins.Spec.Master.ImagePullSecrets)) @@ -656,6 +655,22 @@ func (r *ReconcileJenkinsBaseConfiguration) compareContainers(expected corev1.Co return messages, verbose } +func compareImagePullSecrets(expected, actual []corev1.LocalObjectReference) bool { + for _, expected := range expected { + found := false + for _, actual := range actual { + if expected.Name == actual.Name { + found = true + break + } + } + if !found { + return false + } + } + return true +} + func comparePodAnnotations(expected, actual map[string]string) bool { for expectedKey, expectedValue := range expected { actualValue, found := actual[expectedKey] diff --git a/pkg/controller/jenkins/configuration/base/reconcile_test.go b/pkg/controller/jenkins/configuration/base/reconcile_test.go index 2b6a073a..205f3d36 100644 --- a/pkg/controller/jenkins/configuration/base/reconcile_test.go +++ b/pkg/controller/jenkins/configuration/base/reconcile_test.go @@ -698,3 +698,38 @@ func TestComparePodAnnotations(t *testing.T) { assert.False(t, got) }) } + +func TestCompareImagePullSecrets(t *testing.T) { + t.Run("empty", func(t *testing.T) { + var expected []corev1.LocalObjectReference + var actual []corev1.LocalObjectReference + + got := compareImagePullSecrets(expected, actual) + + assert.True(t, got) + }) + t.Run("the same", func(t *testing.T) { + expected := []corev1.LocalObjectReference{{Name: "test"}} + actual := []corev1.LocalObjectReference{{Name: "test"}} + + got := compareImagePullSecrets(expected, actual) + + assert.True(t, got) + }) + t.Run("one extra pull secret in pod", func(t *testing.T) { + var expected []corev1.LocalObjectReference + actual := []corev1.LocalObjectReference{{Name: "test"}} + + got := compareImagePullSecrets(expected, actual) + + assert.True(t, got) + }) + t.Run("one missing image pull secret", func(t *testing.T) { + expected := []corev1.LocalObjectReference{{Name: "test"}} + var actual []corev1.LocalObjectReference + + got := compareImagePullSecrets(expected, actual) + + assert.False(t, got) + }) +} diff --git a/pkg/controller/jenkins/configuration/base/validate_test.go b/pkg/controller/jenkins/configuration/base/validate_test.go index b6d4415e..a5f131b3 100644 --- a/pkg/controller/jenkins/configuration/base/validate_test.go +++ b/pkg/controller/jenkins/configuration/base/validate_test.go @@ -15,7 +15,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/api/core/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -351,7 +350,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { Master: v1alpha2.JenkinsMaster{ Containers: []v1alpha2.Container{ { - Env: []v1.EnvVar{ + Env: []corev1.EnvVar{ { Name: "SOME_VALUE", Value: "", @@ -378,7 +377,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { Master: v1alpha2.JenkinsMaster{ Containers: []v1alpha2.Container{ { - Env: []v1.EnvVar{ + Env: []corev1.EnvVar{ { Name: constants.JavaOpsVariableName, Value: "-Djenkins.install.runSetupWizard=false", @@ -402,7 +401,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { Master: v1alpha2.JenkinsMaster{ Containers: []v1alpha2.Container{ { - Env: []v1.EnvVar{ + Env: []corev1.EnvVar{ { Name: constants.JavaOpsVariableName, Value: "-Djava.awt.headless=true", @@ -427,7 +426,7 @@ func TestValidateReservedVolumes(t *testing.T) { jenkins := v1alpha2.Jenkins{ Spec: v1alpha2.JenkinsSpec{ Master: v1alpha2.JenkinsMaster{ - Volumes: []v1.Volume{ + Volumes: []corev1.Volume{ { Name: "not-used-name", }, @@ -445,7 +444,7 @@ func TestValidateReservedVolumes(t *testing.T) { jenkins := v1alpha2.Jenkins{ Spec: v1alpha2.JenkinsSpec{ Master: v1alpha2.JenkinsMaster{ - Volumes: []v1.Volume{ + Volumes: []corev1.Volume{ { Name: resources.JenkinsHomeVolumeName, }, @@ -479,14 +478,14 @@ func TestValidateContainerVolumeMounts(t *testing.T) { jenkins := v1alpha2.Jenkins{ Spec: v1alpha2.JenkinsSpec{ Master: v1alpha2.JenkinsMaster{ - Volumes: []v1.Volume{ + Volumes: []corev1.Volume{ { Name: "example", }, }, Containers: []v1alpha2.Container{ { - VolumeMounts: []v1.VolumeMount{ + VolumeMounts: []corev1.VolumeMount{ { Name: "example", MountPath: "/test", @@ -507,14 +506,14 @@ func TestValidateContainerVolumeMounts(t *testing.T) { jenkins := v1alpha2.Jenkins{ Spec: v1alpha2.JenkinsSpec{ Master: v1alpha2.JenkinsMaster{ - Volumes: []v1.Volume{ + Volumes: []corev1.Volume{ { Name: "example", }, }, Containers: []v1alpha2.Container{ { - VolumeMounts: []v1.VolumeMount{ + VolumeMounts: []corev1.VolumeMount{ { Name: "example", MountPath: "", // empty @@ -537,7 +536,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { Master: v1alpha2.JenkinsMaster{ Containers: []v1alpha2.Container{ { - VolumeMounts: []v1.VolumeMount{ + VolumeMounts: []corev1.VolumeMount{ { Name: "missing-volume", MountPath: "/test", diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/seedjobs.go b/pkg/controller/jenkins/configuration/user/seedjobs/seedjobs.go index 0cc1d2ae..7c2f71b3 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/seedjobs.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/seedjobs.go @@ -19,7 +19,6 @@ import ( "github.com/go-logr/logr" stackerr "github.com/pkg/errors" - apps "k8s.io/api/apps/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -361,8 +360,8 @@ func agentDeploymentName(jenkins v1alpha2.Jenkins, agentName string) string { return fmt.Sprintf("%s-%s", agentName, jenkins.Name) } -func agentDeployment(jenkins *v1alpha2.Jenkins, namespace string, agentName string, secret string) *apps.Deployment { - return &apps.Deployment{ +func agentDeployment(jenkins *v1alpha2.Jenkins, namespace string, agentName string, secret string) *appsv1.Deployment { + return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: agentDeploymentName(*jenkins, agentName), Namespace: namespace, @@ -377,7 +376,7 @@ func agentDeployment(jenkins *v1alpha2.Jenkins, namespace string, agentName stri }, }, }, - Spec: apps.DeploymentSpec{ + Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 39f32a09..c9e5b884 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -297,7 +297,7 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg return reconcile.Result{}, jenkins, err } if len(messages) > 0 { - message := fmt.Sprintf("Validation of user configuration failed, please correct Jenkins CR") + message := "Validation of user configuration failed, please correct Jenkins CR" *r.notificationEvents <- event.Event{ Jenkins: *jenkins, Phase: event.PhaseUser, diff --git a/test/e2e/jenkins.go b/test/e2e/jenkins.go index df1b5784..66c3df56 100644 --- a/test/e2e/jenkins.go +++ b/test/e2e/jenkins.go @@ -9,7 +9,6 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base/resources" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/constants" framework "github.com/operator-framework/operator-sdk/pkg/test" - "k8s.io/api/core/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -32,7 +31,7 @@ func getJenkins(t *testing.T, namespace, name string) *v1alpha2.Jenkins { return jenkins } -func getJenkinsMasterPod(t *testing.T, jenkins *v1alpha2.Jenkins) *v1.Pod { +func getJenkinsMasterPod(t *testing.T, jenkins *v1alpha2.Jenkins) *corev1.Pod { lo := metav1.ListOptions{ LabelSelector: labels.SelectorFromSet(resources.BuildResourceLabels(jenkins)).String(), } @@ -47,7 +46,7 @@ func getJenkinsMasterPod(t *testing.T, jenkins *v1alpha2.Jenkins) *v1.Pod { } func createJenkinsAPIClient(jenkins *v1alpha2.Jenkins, hostname string, port int, useNodePort bool) (jenkinsclient.Jenkins, error) { - adminSecret := &v1.Secret{} + adminSecret := &corev1.Secret{} namespaceName := types.NamespacedName{Namespace: jenkins.Namespace, Name: resources.GetOperatorCredentialsSecretName(jenkins)} if err := framework.Global.Client.Get(context.TODO(), namespaceName, adminSecret); err != nil { return nil, err @@ -97,7 +96,7 @@ func createJenkinsCR(t *testing.T, name, namespace string, seedJob *[]v1alpha2.S Containers: []v1alpha2.Container{ { Name: resources.JenkinsMasterContainerName, - Env: []v1.EnvVar{ + Env: []corev1.EnvVar{ { Name: "TEST_ENV", Value: "test_env_value", diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index d0764078..a58506a2 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -8,7 +8,6 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/constants" - f "github.com/operator-framework/operator-sdk/pkg/test" framework "github.com/operator-framework/operator-sdk/pkg/test" "github.com/operator-framework/operator-sdk/pkg/test/e2eutil" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -35,7 +34,7 @@ func TestMain(m *testing.M) { port = flag.Int(portParameterName, -1, "The port on which Jenkins API is working. Note: If you want to use nodePort don't set this setting and --jenkins-api-use-nodeport must be false.") useNodePort = flag.Bool(nodePortParameterName, false, "Connect to Jenkins API using the nodePort instead of service port. If you want to set this as true - don't set --jenkins-api-port.") - f.MainEntry(m) + framework.MainEntry(m) } func setupTest(t *testing.T) (string, *framework.TestCtx) {