Skip to content

Commit

Permalink
Fix reconcile and validation failure issue for Knative service
Browse files Browse the repository at this point in the history
dibyom authored and tekton-robot committed Oct 18, 2021
1 parent 5dcf778 commit 71d8609
Showing 4 changed files with 367 additions and 5 deletions.
37 changes: 32 additions & 5 deletions pkg/reconciler/eventlistener/eventlistener.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
3 changes: 3 additions & 0 deletions pkg/reconciler/eventlistener/eventlistener_test.go
Original file line number Diff line number Diff line change
@@ -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},
96 changes: 96 additions & 0 deletions pkg/reconciler/eventlistener/resources/custom.go
Original file line number Diff line number Diff line change
@@ -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
}
236 changes: 236 additions & 0 deletions pkg/reconciler/eventlistener/resources/custom_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}

0 comments on commit 71d8609

Please sign in to comment.