diff --git a/api/v1alpha1/contour_types.go b/api/v1alpha1/contour_types.go index 59a5c653..0c161aa0 100644 --- a/api/v1alpha1/contour_types.go +++ b/api/v1alpha1/contour_types.go @@ -19,6 +19,12 @@ import ( // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. +const ( + // OwningContourLabel is the owner reference label used for a Contour + // created by the operator. + OwningContourLabel = "contour.operator.projectcontour.io/owning-contour" +) + // +kubebuilder:object:root=true // Contour is the Schema for the contours API. diff --git a/controller/contour/configmap.go b/controller/contour/configmap.go index 13691a62..fa6d9ff9 100644 --- a/controller/contour/configmap.go +++ b/controller/contour/configmap.go @@ -187,7 +187,7 @@ func desiredConfigMap(contour *operatorv1alpha1.Contour) (*corev1.ConfigMap, err Name: contour.Name, Namespace: contour.Spec.Namespace.Name, Labels: map[string]string{ - owningContourLabel: contour.Name, + operatorv1alpha1.OwningContourLabel: contour.Name, }, }, Data: map[string]string{ diff --git a/controller/contour/controller.go b/controller/contour/controller.go index e7f500e0..e5a392c1 100644 --- a/controller/contour/controller.go +++ b/controller/contour/controller.go @@ -28,12 +28,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -const ( - // owningContourLabel is the owner reference label used for objects - // created by the operator. - owningContourLabel = "contour.operator.projectcontour.io/owning-contour" -) - // Config holds all the things necessary for the controller to run. type Config struct { // Image is the name of the Contour container image. @@ -193,6 +187,6 @@ func (r *Reconciler) otherContoursExistInSpecNs(ctx context.Context, contour *op // contourOwningSelector returns a label selector based on the provided contour. func contourOwningSelector(contour *operatorv1alpha1.Contour) *metav1.LabelSelector { return &metav1.LabelSelector{ - MatchLabels: map[string]string{owningContourLabel: contour.Name}, + MatchLabels: map[string]string{operatorv1alpha1.OwningContourLabel: contour.Name}, } } diff --git a/controller/contour/job.go b/controller/contour/job.go index 9844070e..a3fc67c5 100644 --- a/controller/contour/job.go +++ b/controller/contour/job.go @@ -130,16 +130,19 @@ func DesiredJob(contour *operatorv1alpha1.Contour, image string) (*batchv1.Job, fmt.Sprintf("--namespace=$(%s)", jobNsEnvVar), }, Env: []corev1.EnvVar{env}, - Resources: corev1.ResourceRequirements{}, TerminationMessagePath: "/dev/termination-log", TerminationMessagePolicy: "File", } spec := corev1.PodSpec{ - Containers: []corev1.Container{container}, - ServiceAccountName: defaultCertGenRbacName, - SecurityContext: oputil.NewUnprivilegedPodSecurity(), - RestartPolicy: corev1.RestartPolicyNever, + Containers: []corev1.Container{container}, + DeprecatedServiceAccount: defaultCertGenRbacName, + ServiceAccountName: defaultCertGenRbacName, + SecurityContext: oputil.NewUnprivilegedPodSecurity(), + RestartPolicy: corev1.RestartPolicyNever, + DNSPolicy: corev1.DNSClusterFirst, + SchedulerName: "default-scheduler", + TerminationGracePeriodSeconds: pointer.Int64Ptr(int64(30)), } // TODO [danehans] certgen needs to be updated to match these labels. @@ -151,7 +154,7 @@ func DesiredJob(contour *operatorv1alpha1.Contour, image string) (*batchv1.Job, "app.kubernetes.io/part-of": "project-contour", "app.kubernetes.io/managed-by": "contour-operator", // associate the job with the provided contour. - owningContourLabel: contour.Name, + operatorv1alpha1.OwningContourLabel: contour.Name, } job := &batchv1.Job{ @@ -180,7 +183,7 @@ func DesiredJob(contour *operatorv1alpha1.Contour, image string) (*batchv1.Job, // recreateJobIfNeeded recreates a Job if current doesn't match desired. func (r *Reconciler) recreateJobIfNeeded(ctx context.Context, current, desired *batchv1.Job) error { - changed, updated := utilequality.JobConfigChanged(current, desired) + updated, changed := utilequality.JobConfigChanged(current, desired) if !changed { r.Log.Info("current job matches desired state; skipped updating", "namespace", current.Namespace, "name", current.Name) diff --git a/util/equality/equality.go b/util/equality/equality.go index 6c2414d0..fa43c261 100644 --- a/util/equality/equality.go +++ b/util/equality/equality.go @@ -14,39 +14,50 @@ package equality import ( + operatorv1alpha1 "github.com/projectcontour/contour-operator/api/v1alpha1" + batchv1 "k8s.io/api/batch/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" ) // JobConfigChanged checks if the current and expected Job match and if not, // returns true and the expected job. -func JobConfigChanged(current, expected *batchv1.Job) (bool, *batchv1.Job) { +func JobConfigChanged(current, expected *batchv1.Job) (*batchv1.Job, bool) { changed := false updated := current.DeepCopy() - if !apiequality.Semantic.DeepEqual(current.Name, expected.Name) { + if !apiequality.Semantic.DeepEqual(current.Labels, expected.Labels) { updated = expected changed = true } - if !apiequality.Semantic.DeepEqual(current.Namespace, expected.Namespace) { + if !apiequality.Semantic.DeepEqual(current.Spec.Parallelism, expected.Spec.Parallelism) { updated = expected changed = true } - if !apiequality.Semantic.DeepEqual(current.Labels, expected.Labels) { + if !apiequality.Semantic.DeepEqual(current.Spec.BackoffLimit, expected.Spec.BackoffLimit) { updated = expected changed = true } - if !apiequality.Semantic.DeepEqual(current.Spec, expected.Spec) { + // The completions field is immutable, so no need to compare. Ignore job-generated + // labels and only check the presence of the contour owning label. + if current.Spec.Template.Labels != nil { + if _, ok := current.Spec.Template.Labels[operatorv1alpha1.OwningContourLabel]; !ok { + updated = expected + changed = true + } + } + + if !apiequality.Semantic.DeepEqual(current.Spec.Template.Spec, expected.Spec.Template.Spec) { updated = expected changed = true } if !changed { - return false, nil + return nil, false } - return true, updated + return updated, true } diff --git a/util/equality/equality_test.go b/util/equality/equality_test.go index 05a3aec2..95d470a1 100644 --- a/util/equality/equality_test.go +++ b/util/equality/equality_test.go @@ -41,26 +41,34 @@ func TestJobConfigChanged(t *testing.T) { expect: false, }, { - description: "if the container image is changed", + description: "if the job labels are removed", mutate: func(job *batchv1.Job) { - job.Spec.Template.Spec.Containers[0].Image = "foo:latest" + job.Labels = map[string]string{} }, expect: true, }, { - description: "if parallelism is changed", + description: "if the contour owning label is removed", mutate: func(job *batchv1.Job) { - job.Spec.Parallelism = &zero + delete(job.Spec.Template.Labels, operatorv1alpha1.OwningContourLabel) }, expect: true, }, { - description: "if completions is changed", + description: "if the container image is changed", mutate: func(job *batchv1.Job) { - job.Spec.Completions = &zero + job.Spec.Template.Spec.Containers[0].Image = "foo:latest" + }, + expect: true, + }, + { + description: "if parallelism is changed", + mutate: func(job *batchv1.Job) { + job.Spec.Parallelism = &zero }, expect: true, }, + // Completions is immutable, so performing an equality comparison is unneeded. { description: "if backoffLimit is changed", mutate: func(job *batchv1.Job) { @@ -124,10 +132,10 @@ func TestJobConfigChanged(t *testing.T) { mutated := expected.DeepCopy() tc.mutate(mutated) - if changed, updated := utilequality.JobConfigChanged(mutated, expected); changed != tc.expect { + if updated, changed := utilequality.JobConfigChanged(mutated, expected); changed != tc.expect { t.Errorf("%s, expect jobConfigChanged to be %t, got %t", tc.description, tc.expect, changed) } else if changed { - if changedAgain, _ := utilequality.JobConfigChanged(updated, expected); changedAgain { + if _, changedAgain := utilequality.JobConfigChanged(updated, expected); changedAgain { t.Errorf("%s, jobConfigChanged does not behave as a fixed point function", tc.description) } }