From 359bd170667983201b27422e539abcb3fd46fdac Mon Sep 17 00:00:00 2001 From: Quinton Hoole Date: Thu, 22 Sep 2016 15:11:55 -0700 Subject: [PATCH] Don't try to write the wrong UID, version on Federated Ingress updates. Fixes #33135. --- .../ingress/ingress_controller.go | 60 +++++++++++-------- .../ingress/ingress_controller_test.go | 21 ++++--- .../pkg/federation-controller/util/meta.go | 24 +++++++- 3 files changed, 69 insertions(+), 36 deletions(-) diff --git a/federation/pkg/federation-controller/ingress/ingress_controller.go b/federation/pkg/federation-controller/ingress/ingress_controller.go index b5dc69cac6f..131d9d3a808 100644 --- a/federation/pkg/federation-controller/ingress/ingress_controller.go +++ b/federation/pkg/federation-controller/ingress/ingress_controller.go @@ -624,10 +624,7 @@ func (ic *IngressController) reconcileIngress(ingress types.NamespacedName) { ic.deliverIngress(ingress, 0, true) return } - desiredIngress := &extensions_v1beta1.Ingress{ - ObjectMeta: baseIngress.ObjectMeta, - Spec: baseIngress.Spec, - } + desiredIngress := &extensions_v1beta1.Ingress{} objMeta, err := conversion.NewCloner().DeepCopy(baseIngress.ObjectMeta) if err != nil { glog.Errorf("Error deep copying ObjectMeta: %v", err) @@ -649,8 +646,7 @@ func (ic *IngressController) reconcileIngress(ingress types.NamespacedName) { if !clusterIngressFound { glog.V(4).Infof("No existing Ingress %s in cluster %s - checking if appropriate to queue a create operation", ingress, cluster.Name) // We can't supply server-created fields when creating a new object. - desiredIngress.ObjectMeta.ResourceVersion = "" - desiredIngress.ObjectMeta.UID = "" + desiredIngress.ObjectMeta = util.DeepCopyObjectMeta(baseIngress.ObjectMeta) ic.eventRecorder.Eventf(baseIngress, api.EventTypeNormal, "CreateInCluster", "Creating ingress in cluster %s", cluster.Name) @@ -707,29 +703,43 @@ func (ic *IngressController) reconcileIngress(ingress types.NamespacedName) { glog.V(4).Infof(logStr, "Not transferring") } // Update existing cluster ingress, if needed. - if util.ObjectMetaEquivalent(desiredIngress.ObjectMeta, clusterIngress.ObjectMeta) && - reflect.DeepEqual(desiredIngress.Spec, clusterIngress.Spec) && - reflect.DeepEqual(baseIngress.Status.LoadBalancer.Ingress, clusterIngress.Status.LoadBalancer.Ingress) { + if util.ObjectMetaEquivalent(baseIngress.ObjectMeta, clusterIngress.ObjectMeta) && + reflect.DeepEqual(baseIngress.Spec, clusterIngress.Spec) { glog.V(4).Infof("Ingress %q in cluster %q does not need an update: cluster ingress is equivalent to federated ingress", ingress, cluster.Name) } else { glog.V(4).Infof("Ingress %s in cluster %s needs an update: cluster ingress %v is not equivalent to federated ingress %v", ingress, cluster.Name, clusterIngress, desiredIngress) - if !util.ObjectMetaEquivalent(desiredIngress.ObjectMeta, clusterIngress.ObjectMeta) { - // Merge any annotations on the federated ingress onto the underlying cluster ingress, - // overwriting duplicates. - for key, val := range baseIngress.ObjectMeta.Annotations { - desiredIngress.ObjectMeta.Annotations[key] = val - } - ic.eventRecorder.Eventf(baseIngress, api.EventTypeNormal, "UpdateInCluster", - "Updating ingress in cluster %s", cluster.Name) - - operations = append(operations, util.FederatedOperation{ - Type: util.OperationTypeUpdate, - Obj: desiredIngress, - ClusterName: cluster.Name, - }) - // TODO: Transfer any readonly (target-proxy, url-map etc) annotations from the master cluster to the federation, if this is the master cluster. - // This is only for consistency, so that the federation ingress metadata matches the underlying clusters. It's not actually required. + objMeta, err := conversion.NewCloner().DeepCopy(clusterIngress.ObjectMeta) + if err != nil { + glog.Errorf("Error deep copying ObjectMeta: %v", err) } + desiredIngress.ObjectMeta, ok = objMeta.(v1.ObjectMeta) + if !ok { + glog.Errorf("Internal error: Failed to cast to v1.ObjectMeta: %v", objMeta) + } + // Merge any annotations and labels on the federated ingress onto the underlying cluster ingress, + // overwriting duplicates. + if desiredIngress.ObjectMeta.Annotations == nil { + desiredIngress.ObjectMeta.Annotations = make(map[string]string) + } + for key, val := range baseIngress.ObjectMeta.Annotations { + desiredIngress.ObjectMeta.Annotations[key] = val + } + if desiredIngress.ObjectMeta.Labels == nil { + desiredIngress.ObjectMeta.Labels = make(map[string]string) + } + for key, val := range baseIngress.ObjectMeta.Labels { + desiredIngress.ObjectMeta.Labels[key] = val + } + ic.eventRecorder.Eventf(baseIngress, api.EventTypeNormal, "UpdateInCluster", + "Updating ingress in cluster %s", cluster.Name) + + operations = append(operations, util.FederatedOperation{ + Type: util.OperationTypeUpdate, + Obj: desiredIngress, + ClusterName: cluster.Name, + }) + // TODO: Transfer any readonly (target-proxy, url-map etc) annotations from the master cluster to the federation, if this is the master cluster. + // This is only for consistency, so that the federation ingress metadata matches the underlying clusters. It's not actually required } } } } diff --git a/federation/pkg/federation-controller/ingress/ingress_controller_test.go b/federation/pkg/federation-controller/ingress/ingress_controller_test.go index c183abeb2af..bbaa90067e4 100644 --- a/federation/pkg/federation-controller/ingress/ingress_controller_test.go +++ b/federation/pkg/federation-controller/ingress/ingress_controller_test.go @@ -96,10 +96,10 @@ func TestIngressController(t *testing.T) { ing1 := extensions_v1beta1.Ingress{ ObjectMeta: api_v1.ObjectMeta{ - Name: "test-ingress", - Namespace: "mynamespace", - SelfLink: "/api/v1/namespaces/mynamespace/ingress/test-ingress", - Annotations: map[string]string{}, + Name: "test-ingress", + Namespace: "mynamespace", + SelfLink: "/api/v1/namespaces/mynamespace/ingress/test-ingress", + // TODO: Remove: Annotations: map[string]string{}, }, Status: extensions_v1beta1.IngressStatus{ LoadBalancer: api_v1.LoadBalancerStatus{ @@ -139,6 +139,9 @@ func TestIngressController(t *testing.T) { } // Test update federated ingress. + if updatedIngress.ObjectMeta.Annotations == nil { + updatedIngress.ObjectMeta.Annotations = make(map[string]string) + } updatedIngress.ObjectMeta.Annotations["A"] = "B" t.Log("Modifying Federated Ingress") fedIngressWatch.Modify(updatedIngress) @@ -146,7 +149,7 @@ func TestIngressController(t *testing.T) { updatedIngress2 := GetIngressFromChan(t, cluster1IngressUpdateChan) assert.NotNil(t, updatedIngress2) assert.True(t, reflect.DeepEqual(updatedIngress2.Spec, updatedIngress.Spec), "Spec of updated ingress is not equal") - assert.True(t, util.ObjectMetaEquivalent(updatedIngress2.ObjectMeta, updatedIngress.ObjectMeta), "Metadata of updated object is not equivalent") + assert.Equal(t, updatedIngress2.ObjectMeta.Annotations["A"], updatedIngress.ObjectMeta.Annotations["A"], "Updated annotation not transferred from federated to cluster ingress.") // Test add cluster t.Log("Adding a second cluster") ing1.Annotations[staticIPNameKeyWritable] = "foo" // Make sure that the base object has a static IP name first. @@ -194,10 +197,10 @@ func GetClusterFromChan(c chan runtime.Object) *federation_api.Cluster { func NewConfigMap(uid string) *api_v1.ConfigMap { return &api_v1.ConfigMap{ ObjectMeta: api_v1.ObjectMeta{ - Name: uidConfigMapName, - Namespace: uidConfigMapNamespace, - SelfLink: "/api/v1/namespaces/" + uidConfigMapNamespace + "/configmap/" + uidConfigMapName, - Annotations: map[string]string{}, + Name: uidConfigMapName, + Namespace: uidConfigMapNamespace, + SelfLink: "/api/v1/namespaces/" + uidConfigMapNamespace + "/configmap/" + uidConfigMapName, + // TODO: Remove: Annotations: map[string]string{}, }, Data: map[string]string{ uidKey: uid, diff --git a/federation/pkg/federation-controller/util/meta.go b/federation/pkg/federation-controller/util/meta.go index 1d4e84cc59f..8ac6eeee94f 100644 --- a/federation/pkg/federation-controller/util/meta.go +++ b/federation/pkg/federation-controller/util/meta.go @@ -23,7 +23,7 @@ import ( ) // Copies cluster-independent, user provided data from the given ObjectMeta struct. If in -// the future the ObjectMeta structure is expanded then any field that is not populared +// the future the ObjectMeta structure is expanded then any field that is not populated // by the api server should be included here. func CopyObjectMeta(obj api_v1.ObjectMeta) api_v1.ObjectMeta { return api_v1.ObjectMeta{ @@ -34,8 +34,28 @@ func CopyObjectMeta(obj api_v1.ObjectMeta) api_v1.ObjectMeta { } } +// Deep copies cluster-independent, user provided data from the given ObjectMeta struct. If in +// the future the ObjectMeta structure is expanded then any field that is not populated +// by the api server should be included here. +func DeepCopyObjectMeta(obj api_v1.ObjectMeta) api_v1.ObjectMeta { + copyMeta := CopyObjectMeta(obj) + if obj.Labels != nil { + copyMeta.Labels = make(map[string]string) + for key, val := range obj.Labels { + copyMeta.Labels[key] = val + } + } + if obj.Annotations != nil { + copyMeta.Annotations = make(map[string]string) + for key, val := range obj.Annotations { + copyMeta.Annotations[key] = val + } + } + return copyMeta +} + // Checks if cluster-independed, user provided data in two given ObjectMeta are eqaul. If in -// the future the ObjectMeta structure is expanded then any field that is not populared +// the future the ObjectMeta structure is expanded then any field that is not populated // by the api server should be included here. func ObjectMetaEquivalent(a, b api_v1.ObjectMeta) bool { if a.Name != b.Name {