From 71d86097f8703a354c8ef67803fd83c668a28e7b Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Wed, 13 Oct 2021 12:49:22 -0400 Subject: [PATCH] Fix reconcile and validation failure issue for Knative service --- pkg/reconciler/eventlistener/eventlistener.go | 37 ++- .../eventlistener/eventlistener_test.go | 3 + .../eventlistener/resources/custom.go | 96 +++++++ .../eventlistener/resources/custom_test.go | 236 ++++++++++++++++++ 4 files changed, 367 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/eventlistener/eventlistener.go b/pkg/reconciler/eventlistener/eventlistener.go index 97bb8640f..de073c582 100644 --- a/pkg/reconciler/eventlistener/eventlistener.go +++ b/pkg/reconciler/eventlistener/eventlistener.go @@ -279,19 +279,46 @@ func (r *Reconciler) reconcileCustomObject(ctx context.Context, el *v1beta1.Even } } + var updated bool + // Preserve any externally added annotations + data.SetAnnotations(kmeta.UnionMaps(data.GetAnnotations(), existingCustomObject.GetAnnotations())) + if !equality.Semantic.DeepEqual(data.GetLabels(), existingCustomObject.GetLabels()) || - !equality.Semantic.DeepEqual(data.GetAnnotations(), existingCustomObject.GetAnnotations()) || - !equality.Semantic.DeepEqual(data.Object["spec"], existingCustomObject.Object["spec"]) { + !equality.Semantic.DeepEqual(data.GetAnnotations(), existingCustomObject.GetAnnotations()) { // Don't modify informer copy existingCustomObject = existingCustomObject.DeepCopy() existingCustomObject.SetLabels(data.GetLabels()) existingCustomObject.SetAnnotations(data.GetAnnotations()) - existingCustomObject.Object["spec"] = data.Object["spec"] - if updated, err := r.DynamicClientSet.Resource(gvr).Namespace(data.GetNamespace()).Update(ctx, existingCustomObject, metav1.UpdateOptions{}); err != nil { + updated = true + } + + if !equality.Semantic.DeepEqual(data.Object["spec"], existingCustomObject.Object["spec"]) { + diffExist, existingObject, err := resources.UpdateCustomObject(data, existingCustomObject) + if err != nil { + return err + } + // To avoid un necessary marshalling when there is no updates. + if diffExist { + existingMarshaledData, err := json.Marshal(existingObject) + if err != nil { + logging.FromContext(ctx).Errorf("failed to marshal custom object: %v", err) + return err + } + existingCustomObject = new(unstructured.Unstructured) + if err := existingCustomObject.UnmarshalJSON(existingMarshaledData); err != nil { + logging.FromContext(ctx).Errorf("failed to unmarshal to unstructured object: %v", err) + return err + } + updated = diffExist + } + } + if updated { + updatedData, err := r.DynamicClientSet.Resource(gvr).Namespace(data.GetNamespace()).Update(ctx, existingCustomObject, metav1.UpdateOptions{}) + if err != nil { logging.FromContext(ctx).Errorf("error updating to eventListener custom object: %v", err) return err - } else if data.GetResourceVersion() != updated.GetResourceVersion() { + } else if data.GetResourceVersion() != updatedData.GetResourceVersion() { logging.FromContext(ctx).Infof("Updated EventListener Custom Object %s in Namespace %s", data.GetName(), el.Namespace) } } diff --git a/pkg/reconciler/eventlistener/eventlistener_test.go b/pkg/reconciler/eventlistener/eventlistener_test.go index 293c8ca9c..32066cbab 100644 --- a/pkg/reconciler/eventlistener/eventlistener_test.go +++ b/pkg/reconciler/eventlistener/eventlistener_test.go @@ -1326,6 +1326,7 @@ func TestReconcile(t *testing.T) { startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, EventListeners: []*v1beta1.EventListener{elWithCustomResourceForEnv}, + WithPod: []*duckv1.WithPod{envForCustomResource}, }, endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, @@ -1351,6 +1352,7 @@ func TestReconcile(t *testing.T) { startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, EventListeners: []*v1beta1.EventListener{elWithCustomResourceForArgs}, + WithPod: []*duckv1.WithPod{argsForCustomResource}, }, endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, @@ -1363,6 +1365,7 @@ func TestReconcile(t *testing.T) { startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, EventListeners: []*v1beta1.EventListener{elWithCustomResourceForImage}, + WithPod: []*duckv1.WithPod{imageForCustomResource}, }, endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, diff --git a/pkg/reconciler/eventlistener/resources/custom.go b/pkg/reconciler/eventlistener/resources/custom.go index 11127c94f..db51f2c15 100644 --- a/pkg/reconciler/eventlistener/resources/custom.go +++ b/pkg/reconciler/eventlistener/resources/custom.go @@ -21,6 +21,7 @@ import ( "context" "encoding/json" "os" + "reflect" "github.com/tektoncd/triggers/pkg/apis/triggers/v1beta1" corev1 "k8s.io/api/core/v1" @@ -112,3 +113,98 @@ func MakeCustomObject(ctx context.Context, el *v1beta1.EventListener, configAcc return data, nil } + +func UpdateCustomObject(originalData, updatedCustomObject *unstructured.Unstructured) (bool, *duckv1.WithPod, error) { + updated := false + originalObject := &duckv1.WithPod{} + existingObject := &duckv1.WithPod{} + data, e := originalData.MarshalJSON() + if e != nil { + return false, nil, e + } + if e := json.Unmarshal(data, &originalObject); e != nil { + return false, nil, e + } + updatedData, e := updatedCustomObject.MarshalJSON() + if e != nil { + return false, nil, e + } + if e := json.Unmarshal(updatedData, &existingObject); e != nil { + return false, nil, e + } + + // custom resource except few spec fields from user + // added below checks in order to avoid unwanted updates on all spec changes. + if !reflect.DeepEqual(existingObject.Spec.Template.Name, originalObject.Spec.Template.Name) { + existingObject.Spec.Template.Name = originalObject.Spec.Template.Name + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Labels, originalObject.Spec.Template.Labels) { + existingObject.Spec.Template.Labels = originalObject.Spec.Template.Labels + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Annotations, originalObject.Spec.Template.Annotations) { + existingObject.Spec.Template.Annotations = originalObject.Spec.Template.Annotations + updated = true + } + if existingObject.Spec.Template.Spec.ServiceAccountName != originalObject.Spec.Template.Spec.ServiceAccountName { + existingObject.Spec.Template.Spec.ServiceAccountName = originalObject.Spec.Template.Spec.ServiceAccountName + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Tolerations, originalObject.Spec.Template.Spec.Tolerations) { + existingObject.Spec.Template.Spec.Tolerations = originalObject.Spec.Template.Spec.Tolerations + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.NodeSelector, originalObject.Spec.Template.Spec.NodeSelector) { + existingObject.Spec.Template.Spec.NodeSelector = originalObject.Spec.Template.Spec.NodeSelector + updated = true + } + if len(existingObject.Spec.Template.Spec.Containers) == 0 || + len(existingObject.Spec.Template.Spec.Containers) > 1 { + existingObject.Spec.Template.Spec.Containers = originalObject.Spec.Template.Spec.Containers + updated = true + } else { + if existingObject.Spec.Template.Spec.Containers[0].Name != originalObject.Spec.Template.Spec.Containers[0].Name { + existingObject.Spec.Template.Spec.Containers[0].Name = originalObject.Spec.Template.Spec.Containers[0].Name + updated = true + } + if existingObject.Spec.Template.Spec.Containers[0].Image != originalObject.Spec.Template.Spec.Containers[0].Image { + existingObject.Spec.Template.Spec.Containers[0].Image = originalObject.Spec.Template.Spec.Containers[0].Image + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Containers[0].Ports, originalObject.Spec.Template.Spec.Containers[0].Ports) { + existingObject.Spec.Template.Spec.Containers[0].Ports = originalObject.Spec.Template.Spec.Containers[0].Ports + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Containers[0].Args, originalObject.Spec.Template.Spec.Containers[0].Args) { + existingObject.Spec.Template.Spec.Containers[0].Args = originalObject.Spec.Template.Spec.Containers[0].Args + updated = true + } + if existingObject.Spec.Template.Spec.Containers[0].Command != nil { + existingObject.Spec.Template.Spec.Containers[0].Command = nil + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Containers[0].Resources, originalObject.Spec.Template.Spec.Containers[0].Resources) { + existingObject.Spec.Template.Spec.Containers[0].Resources = originalObject.Spec.Template.Spec.Containers[0].Resources + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Containers[0].Env, originalObject.Spec.Template.Spec.Containers[0].Env) { + existingObject.Spec.Template.Spec.Containers[0].Env = originalObject.Spec.Template.Spec.Containers[0].Env + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Containers[0].ReadinessProbe, originalObject.Spec.Template.Spec.Containers[0].ReadinessProbe) { + existingObject.Spec.Template.Spec.Containers[0].ReadinessProbe = originalObject.Spec.Template.Spec.Containers[0].ReadinessProbe + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Containers[0].VolumeMounts, originalObject.Spec.Template.Spec.Containers[0].VolumeMounts) { + existingObject.Spec.Template.Spec.Containers[0].VolumeMounts = originalObject.Spec.Template.Spec.Containers[0].VolumeMounts + updated = true + } + if !reflect.DeepEqual(existingObject.Spec.Template.Spec.Volumes, originalObject.Spec.Template.Spec.Volumes) { + existingObject.Spec.Template.Spec.Volumes = originalObject.Spec.Template.Spec.Volumes + updated = true + } + } + + return updated, existingObject, nil +} diff --git a/pkg/reconciler/eventlistener/resources/custom_test.go b/pkg/reconciler/eventlistener/resources/custom_test.go index 132506458..d001e939e 100644 --- a/pkg/reconciler/eventlistener/resources/custom_test.go +++ b/pkg/reconciler/eventlistener/resources/custom_test.go @@ -331,3 +331,239 @@ func TestCustomObjectError(t *testing.T) { t.Fatalf("MakeCustomObject() = %v, wanted error", got) } } + +func TestUpdateCustomObject(t *testing.T) { + originalMetadata := map[string]interface{}{ + "name": eventListenerName, + "labels": map[string]interface{}{ + "app.kubernetes.io/managed-by": "EventListener", + }, + "annotations": map[string]interface{}{ + "key": "value", + }, + "ownerReferences": []interface{}{ + map[string]interface{}{ + "apiVersion": "triggers.tekton.dev/v1beta1", + "blockOwnerDeletion": true, + "controller": true, + "kind": "EventListener", + "name": eventListenerName, + "uid": "", + }, + }, + } + updatedMetadata := map[string]interface{}{ + "name": "updatedname", + "labels": map[string]interface{}{ + "app.kubernetes.io/part-of": "Triggers", + }, + "annotations": map[string]interface{}{ + "creator": "tekton", + }, + "ownerReferences": []interface{}{ + map[string]interface{}{ + "apiVersion": "triggers.tekton.dev/v1beta1", + "blockOwnerDeletion": true, + "controller": true, + "kind": "EventListener", + "name": eventListenerName, + "uid": "asbdhfjfk", + }, + }, + } + + tests := []struct { + name string + originalData *unstructured.Unstructured + updatedData *unstructured.Unstructured + updated bool + }{{ + name: "entire object update with single container", + originalData: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "serving.knative.dev/v1", + "kind": "Service", + "metadata": originalMetadata, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": originalMetadata, + "spec": map[string]interface{}{ + "serviceAccountName": "default", + "tolerations": []interface{}{ + map[string]interface{}{ + "key": "key", + "value": "value", + "operator": "Equal", + "effect": "NoSchedule", + }, + }, + "nodeSelector": map[string]interface{}{ + "app": "test", + }, + "volumes": []interface{}{ + map[string]interface{}{ + "name": "volume", + }, + }, + "containers": []interface{}{ + map[string]interface{}{ + "name": "event-listener", + "image": DefaultImage, + "args": []interface{}{ + "--writetimeout=" + strconv.FormatInt(DefaultWriteTimeout, 10), + }, + "env": []interface{}{ + map[string]interface{}{ + "name": "K_LOGGING_CONFIG", + }, + }, + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(8080), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{ + "limits": map[string]interface{}{ + "cpu": "101m", + }, + }, + "readinessProbe": map[string]interface{}{ + "httpGet": map[string]interface{}{ + "path": "/live", + "port": int64(0), + "scheme": "HTTP", + }, + "successThreshold": int64(1), + }, + "command": []interface{}{ + "bin/bash", + }, + "volumeMounts": []interface{}{ + map[string]interface{}{ + "name": "vm", + "mountPath": "/tmp/test", + }, + }, + }, + }, + }, + }, + }, + }, + }, + updatedData: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "serving.knative.dev/v1", + "kind": "Service", + "metadata": updatedMetadata, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": updatedMetadata, + "spec": map[string]interface{}{ + "serviceAccountName": "default1", + "tolerations": []interface{}{ + map[string]interface{}{ + "key": "key1", + "value": "value1", + "operator": "NotEqual", + "effect": "Schedule", + }, + }, + "nodeSelector": map[string]interface{}{ + "app1": "test1", + }, + "volumes": []interface{}{ + map[string]interface{}{ + "name1": "volume1", + }, + }, + "containers": []interface{}{ + map[string]interface{}{ + "name": "event-listener1", + "image": "image2", + "args": []interface{}{ + "--readtimeout=" + strconv.FormatInt(DefaultReadTimeout, 10), + }, + "env": []interface{}{ + map[string]interface{}{ + "name": "K_METRICS_CONFIG", + }, + }, + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(8888), + "protocol": "UDP", + }, + }, + "resources": map[string]interface{}{ + "limits": map[string]interface{}{ + "memory": "128", + }, + }, + "readinessProbe": map[string]interface{}{ + "httpGet": map[string]interface{}{ + "path": "/ready", + "port": int64(0), + "scheme": "HTTP", + }, + "successThreshold": int64(1), + }, + "command": []interface{}{ + "ls -lrt", + }, + "volumeMounts": []interface{}{ + map[string]interface{}{ + "name": "vm", + "mountPath": "/tmp/test1", + }, + }, + }, + }, + }, + }, + }, + }, + }, + updated: true, + }, { + name: "entire object update without container", + originalData: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "serving.knative.dev/v1", + "kind": "Service", + "metadata": originalMetadata, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": originalMetadata, + }, + }, + }, + }, + updatedData: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "serving.knative.dev/v1", + "kind": "Service", + "metadata": updatedMetadata, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": updatedMetadata, + }, + }, + }, + }, + updated: true, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, _, err := UpdateCustomObject(tt.originalData, tt.updatedData) + if err != nil { + t.Fatalf("UpdateCustomObject() = %v", err) + } + if diff := cmp.Diff(tt.updated, got); diff != "" { + t.Errorf("UpdateCustomObject() did not return expected. -want, +got: %s", diff) + } + }) + } +}