Fixes Job Equality Comparison (#47)

Signed-off-by: Daneyon Hansen <[email protected]>

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.

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{

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},
  }
 }

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)

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
 }

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)
  }
  }