diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ac5634e2..ccab3762 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -120,10 +120,10 @@ func main() { } c := make(chan notifications.Event) - go notifications.Listen(c, mgr.GetClient()) + go notifications.Listen(c, events, mgr.GetClient()) // setup Jenkins controller - if err := jenkins.Add(mgr, *local, *minikube, events, *clientSet, *cfg, &c); err != nil { + if err := jenkins.Add(mgr, *local, *minikube, *clientSet, *cfg, &c); err != nil { fatal(errors.Wrap(err, "failed to setup controllers"), *debug) } diff --git a/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go b/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go index d9f4701f..85b85d49 100644 --- a/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go +++ b/pkg/controller/jenkins/configuration/backuprestore/backuprestore.go @@ -46,8 +46,8 @@ func New(k8sClient k8s.Client, clientSet kubernetes.Clientset, } // Validate validates backup and restore configuration -func (bar *BackupAndRestore) Validate() bool { - valid := true +func (bar *BackupAndRestore) Validate() []string { + var messages []string allContainers := map[string]v1alpha2.Container{} for _, container := range bar.jenkins.Spec.Master.Containers { allContainers[container.Name] = container @@ -57,12 +57,10 @@ func (bar *BackupAndRestore) Validate() bool { if len(restore.ContainerName) > 0 { _, found := allContainers[restore.ContainerName] if !found { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("restore container '%s' not found in CR spec.master.containers", restore.ContainerName)) + messages = append(messages, fmt.Sprintf("restore container '%s' not found in CR spec.master.containers", restore.ContainerName)) } if restore.Action.Exec == nil { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("spec.restore.action.exec is not configured")) + messages = append(messages, fmt.Sprintf("spec.restore.action.exec is not configured")) } } @@ -70,29 +68,24 @@ func (bar *BackupAndRestore) Validate() bool { if len(backup.ContainerName) > 0 { _, found := allContainers[backup.ContainerName] if !found { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("backup container '%s' not found in CR spec.master.containers", backup.ContainerName)) + messages = append(messages, fmt.Sprintf("backup container '%s' not found in CR spec.master.containers", backup.ContainerName)) } if backup.Action.Exec == nil { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("spec.backup.action.exec is not configured")) + messages = append(messages, fmt.Sprintf("spec.backup.action.exec is not configured")) } if backup.Interval == 0 { - valid = false - bar.logger.V(log.VWarn).Info(fmt.Sprintf("spec.backup.interval is not configured")) + messages = append(messages, fmt.Sprintf("spec.backup.interval is not configured")) } } if len(restore.ContainerName) > 0 && len(backup.ContainerName) == 0 { - valid = false - bar.logger.V(log.VWarn).Info("spec.backup.containerName is not configured") + messages = append(messages, "spec.backup.containerName is not configured") } if len(backup.ContainerName) > 0 && len(restore.ContainerName) == 0 { - valid = false - bar.logger.V(log.VWarn).Info("spec.restore.containerName is not configured") + messages = append(messages, "spec.restore.containerName is not configured") } - return valid + return messages } // Restore performs Jenkins restore backup operation diff --git a/pkg/controller/jenkins/configuration/base/validate.go b/pkg/controller/jenkins/configuration/base/validate.go index 31a2a803..47c4df7b 100644 --- a/pkg/controller/jenkins/configuration/base/validate.go +++ b/pkg/controller/jenkins/configuration/base/validate.go @@ -24,213 +24,207 @@ var ( ) // Validate validates Jenkins CR Spec.master section -func (r *ReconcileJenkinsBaseConfiguration) Validate(jenkins *v1alpha2.Jenkins) (bool, error) { - if !r.validateReservedVolumes() { - return false, nil +func (r *ReconcileJenkinsBaseConfiguration) Validate(jenkins *v1alpha2.Jenkins) ([]string, error) { + var messages []string + + if msg := r.validateReservedVolumes(); msg != nil { + messages = append(messages, msg...) } - if valid, err := r.validateVolumes(); err != nil { - return false, err - } else if !valid { - return false, nil + + if msg, err := r.validateVolumes(); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } for _, container := range jenkins.Spec.Master.Containers { - if !r.validateContainer(container) { - return false, nil + if msg := r.validateContainer(container); msg != nil { + messages = append(messages, msg...) } } - if !r.validatePlugins(plugins.BasePlugins(), jenkins.Spec.Master.BasePlugins, jenkins.Spec.Master.Plugins) { - return false, nil + if msg := r.validatePlugins(plugins.BasePlugins(), jenkins.Spec.Master.BasePlugins, jenkins.Spec.Master.Plugins); msg != nil { + messages = append(messages, msg...) } - if !r.validateJenkinsMasterPodEnvs() { - return false, nil + if msg := r.validateJenkinsMasterPodEnvs(); msg != nil { + messages = append(messages, msg...) } - if valid, err := r.validateCustomization(r.jenkins.Spec.GroovyScripts.Customization, "spec.groovyScripts"); err != nil { - return false, err - } else if !valid { - return false, nil + if msg, err := r.validateCustomization(r.jenkins.Spec.GroovyScripts.Customization, "spec.groovyScripts"); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } - if valid, err := r.validateCustomization(r.jenkins.Spec.ConfigurationAsCode.Customization, "spec.configurationAsCode"); err != nil { - return false, err - } else if !valid { - return false, nil + if msg, err := r.validateCustomization(r.jenkins.Spec.ConfigurationAsCode.Customization, "spec.configurationAsCode"); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecrets() (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecrets() ([]string, error) { + var messages []string for _, sr := range r.jenkins.Spec.Master.ImagePullSecrets { - valid, err := r.validateImagePullSecret(sr.Name) + msg, err := r.validateImagePullSecret(sr.Name) if err != nil { - return false, err + return nil, err } - if !valid { - return false, nil + if msg != nil { + messages = append(messages, msg...) } } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecret(secretName string) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateImagePullSecret(secretName string) ([]string, error) { + var messages []string secret := &corev1.Secret{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: r.jenkins.ObjectMeta.Namespace}, secret) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret %s not found defined in spec.master.imagePullSecrets", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret %s not found defined in spec.master.imagePullSecrets", secretName)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } if secret.Data["docker-server"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-server' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-server' key.", secretName)) } if secret.Data["docker-username"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-username' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-username' key.", secretName)) } if secret.Data["docker-password"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-password' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-password' key.", secretName)) } if secret.Data["docker-email"] == nil { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-email' key.", secretName)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' defined in spec.master.imagePullSecrets doesn't have 'docker-email' key.", secretName)) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateVolumes() (bool, error) { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validateVolumes() ([]string, error) { + var messages []string for _, volume := range r.jenkins.Spec.Master.Volumes { switch { case volume.ConfigMap != nil: - if ok, err := r.validateConfigMapVolume(volume); err != nil { - return false, err - } else if !ok { - valid = false + if msg, err := r.validateConfigMapVolume(volume); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } case volume.Secret != nil: - if ok, err := r.validateSecretVolume(volume); err != nil { - return false, err - } else if !ok { - valid = false + if msg, err := r.validateSecretVolume(volume); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } case volume.PersistentVolumeClaim != nil: - if ok, err := r.validatePersistentVolumeClaim(volume); err != nil { - return false, err - } else if !ok { - valid = false + if msg, err := r.validatePersistentVolumeClaim(volume); err != nil { + return nil, err + } else if msg != nil { + messages = append(messages, msg...) } default: //TODO add support for rest of volumes - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Unsupported volume '%v'", volume)) + messages = append(messages, fmt.Sprintf("Unsupported volume '%v'", volume)) } } - return valid, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validatePersistentVolumeClaim(volume corev1.Volume) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validatePersistentVolumeClaim(volume corev1.Volume) ([]string, error) { + var messages []string + pvc := &corev1.PersistentVolumeClaim{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: volume.PersistentVolumeClaim.ClaimName, Namespace: r.jenkins.ObjectMeta.Namespace}, pvc) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("PersistentVolumeClaim '%s' not found for volume '%v'", volume.PersistentVolumeClaim.ClaimName, volume)) - return false, nil + messages = append(messages, fmt.Sprintf("PersistentVolumeClaim '%s' not found for volume '%v'", volume.PersistentVolumeClaim.ClaimName, volume)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateConfigMapVolume(volume corev1.Volume) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateConfigMapVolume(volume corev1.Volume) ([]string, error) { + var messages []string if volume.ConfigMap.Optional != nil && *volume.ConfigMap.Optional { - return true, nil + return nil, nil } configMap := &corev1.ConfigMap{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: volume.ConfigMap.Name, Namespace: r.jenkins.ObjectMeta.Namespace}, configMap) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("ConfigMap '%s' not found for volume '%v'", volume.ConfigMap.Name, volume)) - return false, nil + messages = append(messages, fmt.Sprintf("ConfigMap '%s' not found for volume '%v'", volume.ConfigMap.Name, volume)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateSecretVolume(volume corev1.Volume) (bool, error) { +func (r *ReconcileJenkinsBaseConfiguration) validateSecretVolume(volume corev1.Volume) ([]string, error) { + var messages []string if volume.Secret.Optional != nil && *volume.Secret.Optional { - return true, nil + return nil, nil } secret := &corev1.Secret{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: volume.Secret.SecretName, Namespace: r.jenkins.ObjectMeta.Namespace}, secret) if err != nil && apierrors.IsNotFound(err) { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' not found for volume '%v'", volume.Secret.SecretName, volume)) - return false, nil + messages = append(messages, fmt.Sprintf("Secret '%s' not found for volume '%v'", volume.Secret.SecretName, volume)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } - return true, nil + return messages, nil } -func (r *ReconcileJenkinsBaseConfiguration) validateReservedVolumes() bool { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validateReservedVolumes() []string { + var messages []string for _, baseVolume := range resources.GetJenkinsMasterPodBaseVolumes(r.jenkins) { for _, volume := range r.jenkins.Spec.Master.Volumes { if baseVolume.Name == volume.Name { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Jenkins Master pod volume '%s' is reserved please choose different one", volume.Name)) - valid = false + messages = append(messages, fmt.Sprintf("Jenkins Master pod volume '%s' is reserved please choose different one", volume.Name)) } } } - return valid + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validateContainer(container v1alpha2.Container) bool { - logger := r.logger.WithValues("container", container.Name) +func (r *ReconcileJenkinsBaseConfiguration) validateContainer(container v1alpha2.Container) []string { + var messages []string if container.Image == "" { - logger.V(log.VWarn).Info("Image not set") - return false + messages = append(messages, "Image not set") } if !dockerImageRegexp.MatchString(container.Image) && !docker.ReferenceRegexp.MatchString(container.Image) { - logger.V(log.VWarn).Info("Invalid image") - return false + messages = append(messages, "Invalid image") } if container.ImagePullPolicy == "" { - logger.V(log.VWarn).Info("Image pull policy not set") - return false + messages = append(messages, "Image pull policy not set") } - if !r.validateContainerVolumeMounts(container) { - return false + if msg := r.validateContainerVolumeMounts(container); msg != nil { + messages = append(messages, msg...) } - return true + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validateContainerVolumeMounts(container v1alpha2.Container) bool { - logger := r.logger.WithValues("container", container.Name) +func (r *ReconcileJenkinsBaseConfiguration) validateContainerVolumeMounts(container v1alpha2.Container) []string { + var messages []string allVolumes := append(resources.GetJenkinsMasterPodBaseVolumes(r.jenkins), r.jenkins.Spec.Master.Volumes...) - valid := true for _, volumeMount := range container.VolumeMounts { if len(volumeMount.MountPath) == 0 { - logger.V(log.VWarn).Info(fmt.Sprintf("mountPath not set for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) - valid = false + messages = append(messages, fmt.Sprintf("mountPath not set for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) } foundVolume := false @@ -241,15 +235,15 @@ func (r *ReconcileJenkinsBaseConfiguration) validateContainerVolumeMounts(contai } if !foundVolume { - logger.V(log.VWarn).Info(fmt.Sprintf("Not found volume for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) - valid = false + messages = append(messages, fmt.Sprintf("Not found volume for '%s' volume mount in container '%s'", volumeMount.Name, container.Name)) } } - return valid + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() bool { +func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() []string { + var messages []string baseEnvs := resources.GetJenkinsMasterContainerBaseEnvs(r.jenkins) baseEnvNames := map[string]string{} for _, env := range baseEnvs { @@ -257,14 +251,12 @@ func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() bool } javaOpts := corev1.EnvVar{} - valid := true for _, userEnv := range r.jenkins.Spec.Master.Containers[0].Env { if userEnv.Name == constants.JavaOpsVariableName { javaOpts = userEnv } if _, overriding := baseEnvNames[userEnv.Name]; overriding { - r.logger.V(log.VWarn).Info(fmt.Sprintf("Jenkins Master container env '%s' cannot be overridden", userEnv.Name)) - valid = false + messages = append(messages, fmt.Sprintf("Jenkins Master container env '%s' cannot be overridden", userEnv.Name)) } } @@ -282,23 +274,21 @@ func (r *ReconcileJenkinsBaseConfiguration) validateJenkinsMasterPodEnvs() bool } for requiredFlag, set := range requiredFlags { if !set { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Jenkins Master container env '%s' doesn't have required flag '%s'", constants.JavaOpsVariableName, requiredFlag)) + messages = append(messages, fmt.Sprintf("Jenkins Master container env '%s' doesn't have required flag '%s'", constants.JavaOpsVariableName, requiredFlag)) } } - return valid + return messages } -func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins []plugins.Plugin, basePlugins, userPlugins []v1alpha2.Plugin) bool { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins []plugins.Plugin, basePlugins, userPlugins []v1alpha2.Plugin) []string { + var messages []string allPlugins := map[plugins.Plugin][]plugins.Plugin{} for _, jenkinsPlugin := range basePlugins { plugin, err := plugins.NewPlugin(jenkinsPlugin.Name, jenkinsPlugin.Version) if err != nil { - r.logger.V(log.VWarn).Info(err.Error()) - valid = false + messages = append(messages, err.Error()) } if plugin != nil { @@ -309,8 +299,7 @@ func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins for _, jenkinsPlugin := range userPlugins { plugin, err := plugins.NewPlugin(jenkinsPlugin.Name, jenkinsPlugin.Version) if err != nil { - r.logger.V(log.VWarn).Info(err.Error()) - valid = false + messages = append(messages, err.Error()) } if plugin != nil { @@ -319,14 +308,14 @@ func (r *ReconcileJenkinsBaseConfiguration) validatePlugins(requiredBasePlugins } if !plugins.VerifyDependencies(allPlugins) { - valid = false + messages = append(messages, "Invalid dependencies") } if !r.verifyBasePlugins(requiredBasePlugins, basePlugins) { - valid = false + messages = append(messages, "Failed to verify base plugins") } - return valid + return messages } func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugins []plugins.Plugin, basePlugins []v1alpha2.Plugin) bool { @@ -349,44 +338,39 @@ func (r *ReconcileJenkinsBaseConfiguration) verifyBasePlugins(requiredBasePlugin return valid } -func (r *ReconcileJenkinsBaseConfiguration) validateCustomization(customization v1alpha2.Customization, name string) (bool, error) { - valid := true +func (r *ReconcileJenkinsBaseConfiguration) validateCustomization(customization v1alpha2.Customization, name string) ([]string, error) { + var messages []string if len(customization.Secret.Name) == 0 && len(customization.Configurations) == 0 { - return true, nil + return nil, nil } if len(customization.Secret.Name) > 0 && len(customization.Configurations) == 0 { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("%s.secret.name is set but %s.configurations is empty", name, name)) + messages = append(messages, fmt.Sprintf("%s.secret.name is set but %s.configurations is empty", name, name)) } if len(customization.Secret.Name) > 0 { secret := &corev1.Secret{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: customization.Secret.Name, Namespace: r.jenkins.ObjectMeta.Namespace}, secret) if err != nil && apierrors.IsNotFound(err) { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("Secret '%s' configured in %s.secret.name not found", customization.Secret.Name, name)) + messages = append(messages, fmt.Sprintf("Secret '%s' configured in %s.secret.name not found", customization.Secret.Name, name)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } } for index, configMapRef := range customization.Configurations { if len(configMapRef.Name) == 0 { - r.logger.V(log.VWarn).Info(fmt.Sprintf("%s.configurations[%d] name is empty", name, index)) - valid = false + messages = append(messages, fmt.Sprintf("%s.configurations[%d] name is empty", name, index)) continue } configMap := &corev1.ConfigMap{} err := r.k8sClient.Get(context.TODO(), types.NamespacedName{Name: configMapRef.Name, Namespace: r.jenkins.ObjectMeta.Namespace}, configMap) if err != nil && apierrors.IsNotFound(err) { - valid = false - r.logger.V(log.VWarn).Info(fmt.Sprintf("ConfigMap '%s' configured in %s.configurations[%d] not found", configMapRef.Name, name, index)) - return false, nil + messages = append(messages, fmt.Sprintf("ConfigMap '%s' configured in %s.configurations[%d] not found", configMapRef.Name, name, index)) } else if err != nil && !apierrors.IsNotFound(err) { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } } - return valid, nil + return messages, nil } diff --git a/pkg/controller/jenkins/configuration/base/validate_test.go b/pkg/controller/jenkins/configuration/base/validate_test.go index 145534f8..ca231a73 100644 --- a/pkg/controller/jenkins/configuration/base/validate_test.go +++ b/pkg/controller/jenkins/configuration/base/validate_test.go @@ -30,7 +30,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("valid user plugin", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -39,7 +39,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("invalid user plugin name", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -48,7 +48,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("invalid user plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -57,7 +57,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("valid base plugin", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -66,7 +66,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("invalid base plugin name", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -75,7 +75,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("invalid base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -84,7 +84,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("valid user and base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -93,7 +93,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("invalid user and base plugin version", func(t *testing.T) { var requiredBasePlugins []plugins.Plugin @@ -102,7 +102,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("required base plugin set with the same version", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} @@ -111,7 +111,7 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("required base plugin set with different version", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} @@ -120,16 +120,16 @@ func TestValidatePlugins(t *testing.T) { got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.True(t, got) + assert.Nil(t, got) }) - t.Run("missign required base plugin", func(t *testing.T) { + t.Run("missing required base plugin", func(t *testing.T) { requiredBasePlugins := []plugins.Plugin{{Name: "simple-plugin", Version: "0.0.1"}} var basePlugins []v1alpha2.Plugin var userPlugins []v1alpha2.Plugin got := baseReconcileLoop.validatePlugins(requiredBasePlugins, basePlugins, userPlugins) - assert.False(t, got) + assert.NotNil(t, got) }) } @@ -165,7 +165,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, err := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, true) + assert.Nil(t, got) assert.NoError(t, err) }) @@ -186,7 +186,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker email", func(t *testing.T) { @@ -219,7 +219,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker password", func(t *testing.T) { @@ -252,7 +252,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker username", func(t *testing.T) { @@ -285,7 +285,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) t.Run("no docker server", func(t *testing.T) { @@ -318,7 +318,7 @@ func TestReconcileJenkinsBaseConfiguration_validateImagePullSecrets(t *testing.T &jenkins, false, false, nil, nil) got, _ := baseReconcileLoop.validateImagePullSecrets() - assert.Equal(t, got, false) + assert.NotNil(t, got) }) } @@ -348,7 +348,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("override JENKINS_HOME env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -374,7 +374,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("missing -Djava.awt.headless=true in JAVA_OPTS env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -396,7 +396,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("missing -Djenkins.install.runSetupWizard=false in JAVA_OPTS env", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -418,7 +418,7 @@ func TestValidateJenkinsMasterPodEnvs(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateJenkinsMasterPodEnvs() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } @@ -438,7 +438,7 @@ func TestValidateReservedVolumes(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateReservedVolumes() - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("used reserved name", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -455,7 +455,7 @@ func TestValidateReservedVolumes(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateReservedVolumes() - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } @@ -469,7 +469,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(v1alpha2.Container{}) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("one extra volume", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -496,7 +496,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("empty mountPath", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -523,7 +523,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) t.Run("missing volume", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -545,7 +545,7 @@ func TestValidateContainerVolumeMounts(t *testing.T) { baseReconcileLoop := New(nil, nil, logf.ZapLogger(false), &jenkins, false, false, nil, nil) got := baseReconcileLoop.validateContainerVolumeMounts(jenkins.Spec.Master.Containers[0]) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } @@ -568,7 +568,7 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("happy, required", func(t *testing.T) { optional := false @@ -594,7 +594,7 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("missing configmap", func(t *testing.T) { optional := false @@ -618,7 +618,7 @@ func TestValidateConfigMapVolume(t *testing.T) { got, err := baseReconcileLoop.validateConfigMapVolume(volume) assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) } @@ -641,7 +641,7 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("happy, required", func(t *testing.T) { optional := false @@ -665,7 +665,7 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("missing secret", func(t *testing.T) { optional := false @@ -687,7 +687,7 @@ func TestValidateSecretVolume(t *testing.T) { got, err := baseReconcileLoop.validateSecretVolume(volume) assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) } @@ -709,7 +709,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("secret set but configurations is empty", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -731,7 +731,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("secret and configmap exists", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -761,7 +761,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.True(t, got) + assert.Nil(t, got) }) t.Run("secret not exists and configmap exists", func(t *testing.T) { configMapName := "configmap-name" @@ -784,7 +784,7 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) t.Run("secret exists and configmap not exists", func(t *testing.T) { customization := v1alpha2.Customization{ @@ -806,6 +806,6 @@ func TestValidateCustomization(t *testing.T) { got, err := baseReconcileLoop.validateCustomization(customization, "spec.groovyScripts") assert.NoError(t, err) - assert.False(t, got) + assert.NotNil(t, got) }) } diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate.go index 75fa4b68..d97dfd56 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate.go @@ -19,51 +19,44 @@ import ( ) // ValidateSeedJobs verify seed jobs configuration -func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) (bool, error) { - valid := true +func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) ([]string, error) { + var messages []string - if !s.validateIfIDIsUnique(jenkins.Spec.SeedJobs) { - valid = false + if msg := s.validateIfIDIsUnique(jenkins.Spec.SeedJobs); msg != nil { + messages = append(messages, msg...) } for _, seedJob := range jenkins.Spec.SeedJobs { logger := s.logger.WithValues("seedJob", seedJob.ID).V(log.VWarn) if len(seedJob.ID) == 0 { - logger.Info("id can't be empty") - valid = false + messages = append(messages, "id can't be empty") } if len(seedJob.RepositoryBranch) == 0 { - logger.Info("repository branch can't be empty") - valid = false + messages = append(messages, "repository branch can't be empty") } if len(seedJob.RepositoryURL) == 0 { - logger.Info("repository URL branch can't be empty") - valid = false + messages = append(messages, "repository URL branch can't be empty") } if len(seedJob.Targets) == 0 { - logger.Info("targets can't be empty") - valid = false + messages = append(messages, "targets can't be empty") } if _, ok := v1alpha2.AllowedJenkinsCredentialMap[string(seedJob.JenkinsCredentialType)]; !ok { - logger.Info("unknown credential type") - return false, nil + messages = append(messages, "unknown credential type") } if (seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType || seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType) && len(seedJob.CredentialID) == 0 { - logger.Info("credential ID can't be empty") - valid = false + messages = append(messages, "credential ID can't be empty") } // validate repository url match private key if strings.Contains(seedJob.RepositoryURL, "git@") && seedJob.JenkinsCredentialType == v1alpha2.NoJenkinsCredentialCredentialType { - logger.Info("Jenkins credential must be set while using ssh repository url") - valid = false + messages = append(messages, "Jenkins credential must be set while using ssh repository url") } if seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType || seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType { @@ -71,56 +64,56 @@ func (s *SeedJobs) ValidateSeedJobs(jenkins v1alpha2.Jenkins) (bool, error) { namespaceName := types.NamespacedName{Namespace: jenkins.Namespace, Name: seedJob.CredentialID} err := s.k8sClient.Get(context.TODO(), namespaceName, secret) if err != nil && apierrors.IsNotFound(err) { - logger.Info(fmt.Sprintf("required secret '%s' with Jenkins credential not found", seedJob.CredentialID)) - return false, nil + messages = append(messages, fmt.Sprintf("required secret '%s' with Jenkins credential not found", seedJob.CredentialID)) } else if err != nil { - return false, stackerr.WithStack(err) + return nil, stackerr.WithStack(err) } if seedJob.JenkinsCredentialType == v1alpha2.BasicSSHCredentialType { - if ok := validateBasicSSHSecret(logger, *secret); !ok { - valid = false + if msg := validateBasicSSHSecret(logger, *secret); msg != nil { + messages = append(messages, msg...) } } if seedJob.JenkinsCredentialType == v1alpha2.UsernamePasswordCredentialType { - if ok := validateUsernamePasswordSecret(logger, *secret); !ok { - valid = false + if msg := validateUsernamePasswordSecret(logger, *secret); msg != nil { + messages = append(messages, msg...) } } } if len(seedJob.BuildPeriodically) > 0 { - if !s.validateSchedule(seedJob, seedJob.BuildPeriodically, "buildPeriodically") { - valid = false + if msg := s.validateSchedule(seedJob, seedJob.BuildPeriodically, "buildPeriodically"); msg != nil { + messages = append(messages, msg...) } } if len(seedJob.PollSCM) > 0 { - if !s.validateSchedule(seedJob, seedJob.PollSCM, "pollSCM") { - valid = false + if msg := s.validateSchedule(seedJob, seedJob.PollSCM, "pollSCM"); msg != nil { + messages = append(messages, msg...) } } if seedJob.GitHubPushTrigger { - if !s.validateGitHubPushTrigger(jenkins) { - valid = false + if msg := s.validateGitHubPushTrigger(jenkins); msg != nil { + messages = append(messages, msg...) } } } - return valid, nil + return messages, nil } -func (s *SeedJobs) validateSchedule(job v1alpha2.SeedJob, str string, key string) bool { +func (s *SeedJobs) validateSchedule(job v1alpha2.SeedJob, str string, key string) []string { + var messages []string _, err := cron.Parse(str) if err != nil { - s.logger.V(log.VWarn).Info(fmt.Sprintf("`%s` schedule '%s' is invalid cron spec in `%s`", key, str, job.ID)) - return false + messages = append(messages, fmt.Sprintf("`%s` schedule '%s' is invalid cron spec in `%s`", key, str, job.ID)) } - return true + return messages } -func (s *SeedJobs) validateGitHubPushTrigger(jenkins v1alpha2.Jenkins) bool { +func (s *SeedJobs) validateGitHubPushTrigger(jenkins v1alpha2.Jenkins) []string { + var messages []string exists := false for _, plugin := range jenkins.Spec.Master.BasePlugins { if plugin.Name == "github" { @@ -136,75 +129,65 @@ func (s *SeedJobs) validateGitHubPushTrigger(jenkins v1alpha2.Jenkins) bool { } if !exists && !userExists { - s.logger.V(log.VWarn).Info("githubPushTrigger is set. This function requires `github` plugin installed in .Spec.Master.Plugins because seed jobs Push Trigger function needs it") - return false + messages = append(messages, "githubPushTrigger is set. This function requires `github` plugin installed in .Spec.Master.Plugins because seed jobs Push Trigger function needs it") } - return true + return messages } -func (s *SeedJobs) validateIfIDIsUnique(seedJobs []v1alpha2.SeedJob) bool { +func (s *SeedJobs) validateIfIDIsUnique(seedJobs []v1alpha2.SeedJob) []string { + var messages []string ids := map[string]bool{} for _, seedJob := range seedJobs { if _, found := ids[seedJob.ID]; found { - s.logger.V(log.VWarn).Info(fmt.Sprintf("'%s' seed job ID is not unique", seedJob.ID)) - return false + messages = append(messages, fmt.Sprintf("'%s' seed job ID is not unique", seedJob.ID)) } ids[seedJob.ID] = true } - return true + return messages } -func validateBasicSSHSecret(logger logr.InfoLogger, secret v1.Secret) bool { - valid := true +func validateBasicSSHSecret(logger logr.InfoLogger, secret v1.Secret) []string { + var messages []string username, exists := secret.Data[UsernameSecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } if len(username) == 0 { - logger.Info(fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } privateKey, exists := secret.Data[PrivateKeySecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) } if len(string(privateKey)) == 0 { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) - return false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", PrivateKeySecretKey, secret.ObjectMeta.Name)) } if err := validatePrivateKey(string(privateKey)); err != nil { - logger.Info(fmt.Sprintf("private key '%s' invalid in secret '%s': %s", PrivateKeySecretKey, secret.ObjectMeta.Name, err)) - valid = false + messages = append(messages, fmt.Sprintf("private key '%s' invalid in secret '%s': %s", PrivateKeySecretKey, secret.ObjectMeta.Name, err)) } - return valid + return messages } -func validateUsernamePasswordSecret(logger logr.InfoLogger, secret v1.Secret) bool { - valid := true +func validateUsernamePasswordSecret(logger logr.InfoLogger, secret v1.Secret) []string { + var messages []string username, exists := secret.Data[UsernameSecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } if len(username) == 0 { - logger.Info(fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' is empty in secret '%s'", UsernameSecretKey, secret.ObjectMeta.Name)) } password, exists := secret.Data[PasswordSecretKey] if !exists { - logger.Info(fmt.Sprintf("required data '%s' not found in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' not found in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) } if len(password) == 0 { - logger.Info(fmt.Sprintf("required data '%s' is empty in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) - valid = false + messages = append(messages, fmt.Sprintf("required data '%s' is empty in secret '%s'", PasswordSecretKey, secret.ObjectMeta.Name)) } - return valid + return messages } func validatePrivateKey(privateKey string) error { diff --git a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go index 864cafe9..707db9a8 100644 --- a/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go +++ b/pkg/controller/jenkins/configuration/user/seedjobs/validate_test.go @@ -76,7 +76,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, true, result) + assert.Nil(t, result) }) t.Run("Invalid without id", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -96,7 +96,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Valid with private key and secret", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -129,7 +129,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, true, result) + assert.Nil(t, result) }) t.Run("Invalid private key in secret", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -162,7 +162,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with PrivateKey and empty Secret data", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -195,7 +195,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with ssh RepositoryURL and empty PrivateKey", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -217,7 +217,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without targets", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -237,7 +237,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without repository URL", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -257,7 +257,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without repository branch", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -277,7 +277,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Valid with username and password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -310,7 +310,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, true, result) + assert.Nil(t, result) }) t.Run("Invalid with empty username", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -343,7 +343,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with empty password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -376,7 +376,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without username", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -408,7 +408,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid without password", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -440,7 +440,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.Equal(t, false, result) + assert.NotNil(t, result) }) t.Run("Invalid with wrong cron spec", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -463,7 +463,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.False(t, result) + assert.NotNil(t, result) }) t.Run("Valid with good cron spec", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -487,7 +487,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.True(t, result) + assert.Nil(t, result) }) t.Run("Invalid with set githubPushTrigger and not installed github plugin", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -510,7 +510,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.False(t, result) + assert.NotNil(t, result) }) t.Run("Invalid with set githubPushTrigger and not installed github plugin", func(t *testing.T) { jenkins := v1alpha2.Jenkins{ @@ -538,7 +538,7 @@ func TestValidateSeedJobs(t *testing.T) { result, err := seedJobs.ValidateSeedJobs(jenkins) assert.NoError(t, err) - assert.True(t, result) + assert.Nil(t, result) }) } @@ -549,7 +549,7 @@ func TestValidateIfIDIsUnique(t *testing.T) { } ctrl := New(nil, nil, logf.ZapLogger(false)) got := ctrl.validateIfIDIsUnique(seedJobs) - assert.Equal(t, true, got) + assert.Nil(t, got) }) t.Run("duplicated ids", func(t *testing.T) { seedJobs := []v1alpha2.SeedJob{ @@ -557,6 +557,6 @@ func TestValidateIfIDIsUnique(t *testing.T) { } ctrl := New(nil, nil, logf.ZapLogger(false)) got := ctrl.validateIfIDIsUnique(seedJobs) - assert.Equal(t, false, got) + assert.NotNil(t, got) }) } diff --git a/pkg/controller/jenkins/configuration/user/validate.go b/pkg/controller/jenkins/configuration/user/validate.go index ec86203a..56780bb6 100644 --- a/pkg/controller/jenkins/configuration/user/validate.go +++ b/pkg/controller/jenkins/configuration/user/validate.go @@ -7,10 +7,10 @@ import ( ) // Validate validates Jenkins CR Spec section -func (r *ReconcileUserConfiguration) Validate(jenkins *v1alpha2.Jenkins) (bool, error) { +func (r *ReconcileUserConfiguration) Validate(jenkins *v1alpha2.Jenkins) ([]string, error) { backupAndRestore := backuprestore.New(r.k8sClient, r.clientSet, r.logger, r.jenkins, r.config) - if ok := backupAndRestore.Validate(); !ok { - return false, nil + if msg := backupAndRestore.Validate(); msg != nil { + return msg, nil } seedJobs := seedjobs.New(r.jenkinsClient, r.k8sClient, r.logger) diff --git a/pkg/controller/jenkins/jenkins_controller.go b/pkg/controller/jenkins/jenkins_controller.go index 7cf371cb..528fb6d0 100644 --- a/pkg/controller/jenkins/jenkins_controller.go +++ b/pkg/controller/jenkins/jenkins_controller.go @@ -3,7 +3,7 @@ package jenkins import ( "context" "fmt" - "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" + "reflect" "github.com/jenkinsci/kubernetes-operator/pkg/apis/jenkins/v1alpha2" @@ -12,8 +12,8 @@ import ( "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/base/resources" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/configuration/user" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/constants" + "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/notifications" "github.com/jenkinsci/kubernetes-operator/pkg/controller/jenkins/plugins" - "github.com/jenkinsci/kubernetes-operator/pkg/event" "github.com/jenkinsci/kubernetes-operator/pkg/log" "github.com/jenkinsci/kubernetes-operator/version" @@ -35,15 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" ) -const ( - // reasonBaseConfigurationSuccess is the event which informs base configuration has been completed successfully - reasonBaseConfigurationSuccess event.Reason = "BaseConfigurationSuccess" - // reasonUserConfigurationSuccess is the event which informs user configuration has been completed successfully - reasonUserConfigurationSuccess event.Reason = "BaseConfigurationFailure" - // reasonCRValidationFailure is the event which informs user has provided invalid configuration in Jenkins CR - reasonCRValidationFailure event.Reason = "CRValidationFailure" -) - type reconcileError struct { err error counter uint64 @@ -53,18 +44,17 @@ var reconcileErrors = map[string]reconcileError{} // Add creates a new Jenkins Controller and adds it to the Manager. The Manager will set fields on the Controller // and Start it when the Manager is Started. -func Add(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) error { - return add(mgr, newReconciler(mgr, local, minikube, events, clientSet, config, notificationEvents)) +func Add(mgr manager.Manager, local, minikube bool, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) error { + return add(mgr, newReconciler(mgr, local, minikube, clientSet, config, notificationEvents)) } // newReconciler returns a new reconcile.Reconciler -func newReconciler(mgr manager.Manager, local, minikube bool, events event.Recorder, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) reconcile.Reconciler { +func newReconciler(mgr manager.Manager, local, minikube bool, clientSet kubernetes.Clientset, config rest.Config, notificationEvents *chan notifications.Event) reconcile.Reconciler { return &ReconcileJenkins{ client: mgr.GetClient(), scheme: mgr.GetScheme(), local: local, minikube: minikube, - events: events, clientSet: clientSet, config: config, notificationEvents: notificationEvents, @@ -124,7 +114,6 @@ type ReconcileJenkins struct { client client.Client scheme *runtime.Scheme local, minikube bool - events event.Recorder clientSet kubernetes.Clientset config rest.Config notificationEvents *chan notifications.Event @@ -136,19 +125,6 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul logger := r.buildLogger(request.Name) logger.V(log.VDebug).Info("Reconciling Jenkins") - jenkins := &v1alpha2.Jenkins{} - err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) - if err != nil { - if apierrors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. - // Return and don't requeue - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, errors.WithStack(err) - } - result, err := r.reconcile(request, logger) if err != nil && apierrors.IsConflict(err) { return reconcile.Result{Requeue: true}, nil @@ -175,12 +151,25 @@ func (r *ReconcileJenkins) Reconcile(request reconcile.Request) (reconcile.Resul logger.V(log.VWarn).Info(fmt.Sprintf("Reconcile loop failed %d times with the same error, giving up: %s", reconcileFailLimit, err)) } + jenkins := &v1alpha2.Jenkins{} + err := r.client.Get(context.TODO(), request.NamespacedName, jenkins) + if err != nil { + if apierrors.IsNotFound(err) { + // Request object not found, could have been deleted after reconcile request. + // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. + // Return and don't requeue + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + return reconcile.Result{}, errors.WithStack(err) + } + *r.notificationEvents <- notifications.Event{ Jenkins: *jenkins, ConfigurationType: notifications.ConfigurationTypeUnknown, LogLevel: v1alpha2.NotificationLogLevelWarning, Message: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %s", err), - MessageVerbose: fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err), + MessagesVerbose: []string{fmt.Sprintf("Reconcile loop failed ten times with the same error, giving up: %+v", err)}, } return reconcile.Result{Requeue: false}, nil } @@ -219,25 +208,26 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg if err != nil { return reconcile.Result{}, err } - // Reconcile base configuration baseConfiguration := base.New(r.client, r.scheme, logger, jenkins, r.local, r.minikube, &r.clientSet, &r.config) - valid, err := baseConfiguration.Validate(jenkins) + messages, err := baseConfiguration.Validate(jenkins) if err != nil { return reconcile.Result{}, err } - if !valid { - message := "Validation of base configuration failed, please correct Jenkins CR" + if messages != nil { + message := "Validation of base configuration failed, please correct Jenkins CR." *r.notificationEvents <- notifications.Event{ Jenkins: *jenkins, ConfigurationType: notifications.ConfigurationTypeBase, LogLevel: v1alpha2.NotificationLogLevelWarning, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } - r.events.Emit(jenkins, event.TypeWarning, reasonCRValidationFailure, "Base CR validation failed") logger.V(log.VWarn).Info(message) + for _, msg := range messages { + logger.V(log.VDebug).Info(msg) + } return reconcile.Result{}, nil // don't requeue } @@ -267,29 +257,31 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg ConfigurationType: notifications.ConfigurationTypeBase, LogLevel: v1alpha2.NotificationLogLevelInfo, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } logger.Info(message) - r.events.Emit(jenkins, event.TypeNormal, reasonBaseConfigurationSuccess, "Base configuration completed") } // Reconcile user configuration userConfiguration := user.New(r.client, jenkinsClient, logger, jenkins, r.clientSet, r.config) - valid, err = userConfiguration.Validate(jenkins) + messages, err = userConfiguration.Validate(jenkins) if err != nil { return reconcile.Result{}, err } - if !valid { + if messages != nil { message := fmt.Sprintf("Validation of user configuration failed, please correct Jenkins CR") *r.notificationEvents <- notifications.Event{ Jenkins: *jenkins, ConfigurationType: notifications.ConfigurationTypeUser, LogLevel: v1alpha2.NotificationLogLevelWarning, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } + logger.V(log.VWarn).Info(message) - r.events.Emit(jenkins, event.TypeWarning, reasonCRValidationFailure, "User CR validation failed") + for _, msg := range messages { + logger.V(log.VDebug).Info(msg) + } return reconcile.Result{}, nil // don't requeue } @@ -315,10 +307,9 @@ func (r *ReconcileJenkins) reconcile(request reconcile.Request, logger logr.Logg ConfigurationType: notifications.ConfigurationTypeUser, LogLevel: v1alpha2.NotificationLogLevelInfo, Message: message, - MessageVerbose: message, + MessagesVerbose: messages, } logger.Info(message) - r.events.Emit(jenkins, event.TypeNormal, reasonUserConfigurationSuccess, "User configuration completed") } return reconcile.Result{}, nil diff --git a/pkg/controller/jenkins/notifications/mailgun.go b/pkg/controller/jenkins/notifications/mailgun.go index 8e705c56..8fd3dab3 100644 --- a/pkg/controller/jenkins/notifications/mailgun.go +++ b/pkg/controller/jenkins/notifications/mailgun.go @@ -73,12 +73,17 @@ func (m MailGun) Send(event Event, config v1alpha2.Notification) error { var statusMessage string if config.Verbose { - statusMessage = event.MessageVerbose + message := event.Message + "