Merge pull request #33339 from quinton-hoole/2016-09-22-fix-uid-failure-updating-ingress

Automatic merge from submit-queue

Don't try to write the wrong UID, version on Federated Ingress updates.

Fixes #33135.

This looks more complicated than it really is. 
Essentially, use the cluster object's metadata, rather than the federated objects's metadata when updating cluster Ingress objects.  The deepcopy stuff is mainly to get around shortcomings in the Kubernetes fake test infrastructure, which ends up with crossed pointers if we don't deep copy.

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{
+		desiredIngress := &extensions_v1beta1.Ingress{}
-			ObjectMeta: baseIngress.ObjectMeta,
-			Spec:       baseIngress.Spec,
-		}
 		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 = util.DeepCopyObjectMeta(baseIngress.ObjectMeta)
-			desiredIngress.ObjectMeta.UID = ""
 			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) &&
+			if util.ObjectMetaEquivalent(baseIngress.ObjectMeta, clusterIngress.ObjectMeta) &&
-				reflect.DeepEqual(desiredIngress.Spec, clusterIngress.Spec) &&
+				reflect.DeepEqual(baseIngress.Spec, clusterIngress.Spec) {
-				reflect.DeepEqual(baseIngress.Status.LoadBalancer.Ingress, clusterIngress.Status.LoadBalancer.Ingress) {
 				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) {
+				objMeta, err := conversion.NewCloner().DeepCopy(clusterIngress.ObjectMeta)
-					// Merge any annotations on the federated ingress onto the underlying cluster ingress,
+				if err != nil {
-					// overwriting duplicates.
+					glog.Errorf("Error deep copying ObjectMeta: %v", err)
-					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.
 				}
+				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				}
 			}
 		}
 	}

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",
+			Name:      "test-ingress",
-			Namespace:   "mynamespace",
+			Namespace: "mynamespace",
-			SelfLink:    "/api/v1/namespaces/mynamespace/ingress/test-ingress",
+			SelfLink:  "/api/v1/namespaces/mynamespace/ingress/test-ingress",
-			Annotations: map[string]string{},
+			// 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,
+			Name:      uidConfigMapName,
-			Namespace:   uidConfigMapNamespace,
+			Namespace: uidConfigMapNamespace,
-			SelfLink:    "/api/v1/namespaces/" + uidConfigMapNamespace + "/configmap/" + uidConfigMapName,
+			SelfLink:  "/api/v1/namespaces/" + uidConfigMapNamespace + "/configmap/" + uidConfigMapName,
-			Annotations: map[string]string{},
+			// TODO: Remove: Annotations: map[string]string{},
 		},
 		Data: map[string]string{
 			uidKey: uid,

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 {