Skip to content

Commit

Permalink
Merge annotations on created resources
Browse files Browse the repository at this point in the history 
In #712, we added a feature to propagate annotations added to the EL to its
underlying resources. However, this resulted in infinite reconcile loops since
the deployment controller will add a standard revision annotation that the EL
controller will keep trying to overwrite. To fix this, this commit switches the
annotation propagation to merge any annotations set on the underlying resources
before adding any extra annotations from the EL.

Fixes #752

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
  • Loading branch information
@dibyom @tekton-robot
dibyom authored and tekton-robot committed Sep 11, 2020
1 parent c05c2eb commit 7622d51
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 54 deletions.
25 changes: 14 additions & 11 deletions pkg/reconciler/v1alpha1/eventlistener/eventlistener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,18 +153,21 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, el *v1alpha1.EventListene
return nil
}

func reconcileObjectMeta(oldMeta *metav1.ObjectMeta, newMeta metav1.ObjectMeta) (updated bool) {
if !reflect.DeepEqual(oldMeta.Labels, newMeta.Labels) {
func reconcileObjectMeta(existing *metav1.ObjectMeta, desired metav1.ObjectMeta) (updated bool) {
if !reflect.DeepEqual(existing.Labels, desired.Labels) {
updated = true
oldMeta.Labels = newMeta.Labels
existing.Labels = desired.Labels
}
if !reflect.DeepEqual(oldMeta.Annotations, newMeta.Annotations) {

// TODO(dibyom): We should exclude propagation of some annotations such as `kubernetes.io/last-applied-revision`
if !reflect.DeepEqual(existing.Annotations, mergeMaps(existing.Annotations, desired.Annotations)) {
updated = true
oldMeta.Annotations = newMeta.Annotations
existing.Annotations = desired.Annotations
}
if !reflect.DeepEqual(oldMeta.OwnerReferences, newMeta.OwnerReferences) {

if !reflect.DeepEqual(existing.OwnerReferences, desired.OwnerReferences) {
updated = true
oldMeta.OwnerReferences = newMeta.OwnerReferences
existing.OwnerReferences = desired.OwnerReferences
}
return
}
Expand Down Expand Up @@ -252,7 +255,7 @@ func (r *Reconciler) reconcileDeployment(logger *zap.SugaredLogger, el *v1alpha1
return err
}

labels := mergeLabels(el.Labels, GenerateResourceLabels(el.Name))
labels := mergeMaps(el.Labels, GenerateResourceLabels(el.Name))
var replicas = ptr.Int32(1)
if el.Spec.Replicas != nil {
replicas = el.Spec.Replicas
Expand Down Expand Up @@ -445,14 +448,14 @@ func generateObjectMeta(el *v1alpha1.EventListener) metav1.ObjectMeta {
Namespace: el.Namespace,
Name: el.Status.Configuration.GeneratedResourceName,
OwnerReferences: []metav1.OwnerReference{*el.GetOwnerReference()},
Labels: mergeLabels(el.Labels, GenerateResourceLabels(el.Name)),
Labels: mergeMaps(el.Labels, GenerateResourceLabels(el.Name)),
Annotations: el.Annotations,
}
}

// mergeLabels merges the values in the passed maps into a new map.
// mergeMaps merges the values in the passed maps into a new map.
// Values within m2 potentially clobber m1 values.
func mergeLabels(m1, m2 map[string]string) map[string]string {
func mergeMaps(m1, m2 map[string]string) map[string]string {
merged := make(map[string]string, len(m1)+len(m2))
for k, v := range m1 {
merged[k] = v
Expand Down
68 changes: 25 additions & 43 deletions pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ func getEventListenerTestAssets(t *testing.T, r test.Resources) (test.Assets, co
// If no ops are specified, it generates a base EventListener with no triggers and no Status
func makeEL(ops ...func(el *v1alpha1.EventListener)) *v1alpha1.EventListener {
e := bldr.EventListener(eventListenerName, namespace,

bldr.EventListenerSpec(
bldr.EventListenerServiceAccount("sa"),
),
Expand Down Expand Up @@ -335,10 +334,7 @@ func TestReconcile(t *testing.T) {
t.Fatal(err)
}

elPreReoncile := makeEL()
elWithStatus := makeEL(withStatus)
elWithLabels := makeEL(withStatus, withAddedLabels)
elWithAnnotations := makeEL(withStatus, withAddedAnnotations)

elWithUpdatedSA := makeEL(withStatus, func(el *v1alpha1.EventListener) {
el.Spec.ServiceAccountName = updatedSa
Expand Down Expand Up @@ -368,9 +364,9 @@ func TestReconcile(t *testing.T) {

elDeployment := makeDeployment()
elDeploymentWithLabels := makeDeployment(func(d *appsv1.Deployment) {
d.Labels = mergeLabels(updateLabel, generatedLabels)
d.Labels = mergeMaps(updateLabel, generatedLabels)
d.Spec.Selector.MatchLabels = generatedLabels
d.Spec.Template.Labels = mergeLabels(updateLabel, generatedLabels)
d.Spec.Template.Labels = mergeMaps(updateLabel, generatedLabels)
})

elDeploymentWithAnnotations := makeDeployment(func(d *appsv1.Deployment) {
Expand Down Expand Up @@ -405,7 +401,7 @@ func TestReconcile(t *testing.T) {
elService := makeService()

elServiceWithLabels := makeService(func(s *corev1.Service) {
s.Labels = mergeLabels(updateLabel, generatedLabels)
s.Labels = mergeMaps(updateLabel, generatedLabels)
s.Spec.Selector = generatedLabels
})

Expand Down Expand Up @@ -437,13 +433,13 @@ func TestReconcile(t *testing.T) {
key: reconcileKey,
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elPreReoncile},
EventListeners: []*v1alpha1.EventListener{makeEL()},
},
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
Deployments: []*appsv1.Deployment{elDeployment},
Services: []*corev1.Service{elService},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus)},
Deployments: []*appsv1.Deployment{makeDeployment()},
Services: []*corev1.Service{makeService()},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
}, {
Expand All @@ -452,16 +448,16 @@ func TestReconcile(t *testing.T) {
// Resources before reconcile starts: EL has extra label that deployment/svc does not
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithLabels},
Deployments: []*appsv1.Deployment{elDeployment},
Services: []*corev1.Service{elService},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedLabels)},
Deployments: []*appsv1.Deployment{makeDeployment()},
Services: []*corev1.Service{makeService()},
},
// We expect the deployment and services to propagate the extra label
// but the selectors in both Service and deployment should have the same
// label
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithLabels},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedLabels)},
Deployments: []*appsv1.Deployment{elDeploymentWithLabels},
Services: []*corev1.Service{elServiceWithLabels},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
Expand All @@ -472,14 +468,14 @@ func TestReconcile(t *testing.T) {
// Resources before reconcile starts: EL has annotation that deployment/svc does not
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithAnnotations},
Deployments: []*appsv1.Deployment{elDeployment},
Services: []*corev1.Service{elService},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedAnnotations)},
Deployments: []*appsv1.Deployment{makeDeployment()},
Services: []*corev1.Service{makeService()},
},
// We expect the deployment and services to propagate the annotations
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithAnnotations},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedAnnotations)},
Deployments: []*appsv1.Deployment{elDeploymentWithAnnotations},
Services: []*corev1.Service{elServiceWithAnnotation},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
Expand Down Expand Up @@ -566,7 +562,7 @@ func TestReconcile(t *testing.T) {
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
}, {
// Check that if a user manually updates the annotations for a service, we revert the change.
// Check that if a user manually updates the annotations for a service, we do not revert the change.
name: "update-el-service-annotations",
key: reconcileKey,
startResources: test.Resources{
Expand All @@ -578,7 +574,7 @@ func TestReconcile(t *testing.T) {
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
Services: []*corev1.Service{elService}, // We expect the service to drop the user added annotations
Services: []*corev1.Service{elServiceWithAnnotation},
Deployments: []*appsv1.Deployment{elDeployment},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
Expand Down Expand Up @@ -616,6 +612,7 @@ func TestReconcile(t *testing.T) {
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
}, {
// Check that if a user manually updates the annotations for a deployment, we do not revert the change.
name: "deployment-annotation-update",
key: reconcileKey,
startResources: test.Resources{
Expand All @@ -627,7 +624,7 @@ func TestReconcile(t *testing.T) {
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
Deployments: []*appsv1.Deployment{elDeployment},
Deployments: []*appsv1.Deployment{elDeploymentWithAnnotations},
Services: []*corev1.Service{elService},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
Expand Down Expand Up @@ -669,27 +666,12 @@ func TestReconcile(t *testing.T) {
key: reconcileKey,
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
Deployments: []*appsv1.Deployment{deploymentMissingVolumes},
},
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
Deployments: []*appsv1.Deployment{elDeployment},
Services: []*corev1.Service{elService},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
}, {
name: "eventlistener-config-volume-mount-update",
key: reconcileKey,
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus)},
Deployments: []*appsv1.Deployment{deploymentMissingVolumes},
},
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1alpha1.EventListener{elWithStatus},
EventListeners: []*v1alpha1.EventListener{makeEL(withStatus)},
Deployments: []*appsv1.Deployment{elDeployment},
Services: []*corev1.Service{elService},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
Expand Down Expand Up @@ -867,7 +849,7 @@ func Test_wrapError(t *testing.T) {
}
}

func Test_mergeLabels(t *testing.T) {
func Test_mergeMaps(t *testing.T) {
tests := []struct {
name string
l1, l2 map[string]string
Expand Down Expand Up @@ -900,7 +882,7 @@ func Test_mergeLabels(t *testing.T) {
}}
for i := range tests {
t.Run(tests[i].name, func(t *testing.T) {
actualLabels := mergeLabels(tests[i].l1, tests[i].l2)
actualLabels := mergeMaps(tests[i].l1, tests[i].l2)
if diff := cmp.Diff(tests[i].expectedLabels, actualLabels); diff != "" {
t.Errorf("mergeLabels() did not return expected. -want, +got: %s", diff)
}
Expand All @@ -909,7 +891,7 @@ func Test_mergeLabels(t *testing.T) {
}

func TestGenerateResourceLabels(t *testing.T) {
expectedLabels := mergeLabels(StaticResourceLabels, map[string]string{"eventlistener": eventListenerName})
expectedLabels := mergeMaps(StaticResourceLabels, map[string]string{"eventlistener": eventListenerName})
actualLabels := GenerateResourceLabels(eventListenerName)
if diff := cmp.Diff(expectedLabels, actualLabels); diff != "" {
t.Errorf("mergeLabels() did not return expected. -want, +got: %s", diff)
Expand Down Expand Up @@ -977,7 +959,7 @@ func Test_generateObjectMeta(t *testing.T) {
Controller: &isController,
BlockOwnerDeletion: &blockOwnerDeletion,
}},
Labels: mergeLabels(map[string]string{"k": "v"}, generatedLabels),
Labels: mergeMaps(map[string]string{"k": "v"}, generatedLabels),
},
}, {
name: "EventListener with Annotation",
Expand Down

0 comments on commit 7622d51

Please sign in to comment.