Merge pull request #31028 from mwielgus/secret-fix

Automatic merge from submit-queue

Handle secret namespace and data properly in federated secret controller

This PR fixes what was missed in #30669 review. It uses both namespace and secret name for keying and propagates secret data to underlying clusters.

cc: @quinton-hoole @kshafiee @kubernetes/sig-cluster-federation

federation/pkg/federation-controller/secret/secret_controller.go

@@ -17,6 +17,7 @@ limitations under the License.
 package secret
 
 import (
+	"fmt"
 	"reflect"
 	"time"
 
@@ -159,8 +160,8 @@ func (secretcontroller *SecretController) Run(stopChan <-chan struct{}) {
 		secretcontroller.secretFederatedInformer.Stop()
 	}()
 	secretcontroller.secretDeliverer.StartWithHandler(func(item *util.DelayingDelivererItem) {
-		secret := item.Value.(string)
+		secret := item.Value.(*secretItem)
-		secretcontroller.reconcileSecret(secret)
+		secretcontroller.reconcileSecret(secret.namespace, secret.name)
 	})
 	secretcontroller.clusterDeliverer.StartWithHandler(func(_ *util.DelayingDelivererItem) {
 		secretcontroller.reconcileSecretsOnClusterChange()
@@ -175,20 +176,32 @@ func (secretcontroller *SecretController) Run(stopChan <-chan struct{}) {
 	}()
 }
 
+func getSecretKey(namespace, name string) string {
+	return fmt.Sprintf("%s/%s", namespace, name)
+}
+
+// Internal structure for data in delaying deliverer.
+type secretItem struct {
+	namespace string
+	name      string
+}
+
 func (secretcontroller *SecretController) deliverSecretObj(obj interface{}, delay time.Duration, failed bool) {
 	secret := obj.(*api_v1.Secret)
-	secretcontroller.deliverSecret(secret.Name, delay, failed)
+	secretcontroller.deliverSecret(secret.Namespace, secret.Name, delay, failed)
 }
 
 // Adds backoff to delay if this delivery is related to some failure. Resets backoff if there was no failure.
-func (secretcontroller *SecretController) deliverSecret(secret string, delay time.Duration, failed bool) {
+func (secretcontroller *SecretController) deliverSecret(namespace string, name string, delay time.Duration, failed bool) {
+	key := getSecretKey(namespace, name)
 	if failed {
-		secretcontroller.secretBackoff.Next(secret, time.Now())
+		secretcontroller.secretBackoff.Next(key, time.Now())
-		delay = delay + secretcontroller.secretBackoff.Get(secret)
+		delay = delay + secretcontroller.secretBackoff.Get(key)
 	} else {
-		secretcontroller.secretBackoff.Reset(secret)
+		secretcontroller.secretBackoff.Reset(key)
 	}
-	secretcontroller.secretDeliverer.DeliverAfter(secret, secret, delay)
+	secretcontroller.secretDeliverer.DeliverAfter(key,
+		&secretItem{namespace: namespace, name: name}, delay)
 }
 
 // Check whether all data stores are in sync. False is returned if any of the informer/stores is not yet
@@ -216,20 +229,22 @@ func (secretcontroller *SecretController) reconcileSecretsOnClusterChange() {
 	}
 	for _, obj := range secretcontroller.secretInformerStore.List() {
 		secret := obj.(*api_v1.Secret)
-		secretcontroller.deliverSecret(secret.Name, secretcontroller.smallDelay, false)
+		secretcontroller.deliverSecret(secret.Namespace, secret.Name, secretcontroller.smallDelay, false)
 	}
 }
 
-func (secretcontroller *SecretController) reconcileSecret(secret string) {
+func (secretcontroller *SecretController) reconcileSecret(namespace string, secretName string) {
+
 	if !secretcontroller.isSynced() {
-		secretcontroller.deliverSecret(secret, secretcontroller.clusterAvailableDelay, false)
+		secretcontroller.deliverSecret(namespace, secretName, secretcontroller.clusterAvailableDelay, false)
 		return
 	}
 
-	baseSecretObj, exist, err := secretcontroller.secretInformerStore.GetByKey(secret)
+	key := getSecretKey(namespace, secretName)
+	baseSecretObj, exist, err := secretcontroller.secretInformerStore.GetByKey(key)
 	if err != nil {
-		glog.Errorf("Failed to query main secret store for %v: %v", secret, err)
+		glog.Errorf("Failed to query main secret store for %v: %v", key, err)
-		secretcontroller.deliverSecret(secret, 0, true)
+		secretcontroller.deliverSecret(namespace, secretName, 0, true)
 		return
 	}
 
@@ -242,21 +257,23 @@ func (secretcontroller *SecretController) reconcileSecret(secret string) {
 	clusters, err := secretcontroller.secretFederatedInformer.GetReadyClusters()
 	if err != nil {
 		glog.Errorf("Failed to get cluster list: %v", err)
-		secretcontroller.deliverSecret(secret, secretcontroller.clusterAvailableDelay, false)
+		secretcontroller.deliverSecret(namespace, secretName, secretcontroller.clusterAvailableDelay, false)
 		return
 	}
 
 	operations := make([]util.FederatedOperation, 0)
 	for _, cluster := range clusters {
-		clusterSecretObj, found, err := secretcontroller.secretFederatedInformer.GetTargetStore().GetByKey(cluster.Name, secret)
+		clusterSecretObj, found, err := secretcontroller.secretFederatedInformer.GetTargetStore().GetByKey(cluster.Name, key)
 		if err != nil {
-			glog.Errorf("Failed to get %s from %s: %v", secret, cluster.Name, err)
+			glog.Errorf("Failed to get %s from %s: %v", key, cluster.Name, err)
-			secretcontroller.deliverSecret(secret, 0, true)
+			secretcontroller.deliverSecret(namespace, secretName, 0, true)
 			return
 		}
 
 		desiredSecret := &api_v1.Secret{
 			ObjectMeta: baseSecret.ObjectMeta,
+			Data:       baseSecret.Data,
+			Type:       baseSecret.Type,
 		}
 
 		if !found {
@@ -285,11 +302,11 @@ func (secretcontroller *SecretController) reconcileSecret(secret string) {
 	}
 	err = secretcontroller.federatedUpdater.Update(operations, secretcontroller.updateTimeout)
 	if err != nil {
-		glog.Errorf("Failed to execute updates for %s: %v", secret, err)
+		glog.Errorf("Failed to execute updates for %s: %v", key, err)
-		secretcontroller.deliverSecret(secret, 0, true)
+		secretcontroller.deliverSecret(namespace, secretName, 0, true)
 		return
 	}
 
 	// Evertyhing is in order but lets be double sure
-	secretcontroller.deliverSecret(secret, secretcontroller.secretReviewDelay, false)
+	secretcontroller.deliverSecret(namespace, secretName, secretcontroller.secretReviewDelay, false)
 }

federation/pkg/federation-controller/secret/secret_controller_test.go

@@ -18,6 +18,7 @@ package secret
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
 	"time"
 
@@ -77,15 +78,23 @@ func TestSecretController(t *testing.T) {
 
 	secret1 := api_v1.Secret{
 		ObjectMeta: api_v1.ObjectMeta{
-			Name: "test-secret",
+			Name:      "test-secret",
+			Namespace: "mynamespace",
 		},
+		Data: map[string][]byte{
+			"A": []byte("ala ma kota"),
+			"B": []byte("quick brown fox"),
+		},
+		Type: api_v1.SecretTypeOpaque,
 	}
 
 	// Test add federated secret.
 	secretWatch.Add(&secret1)
 	createdSecret := GetSecretFromChan(cluster1CreateChan)
 	assert.NotNil(t, createdSecret)
+	assert.Equal(t, secret1.Namespace, createdSecret.Namespace)
 	assert.Equal(t, secret1.Name, createdSecret.Name)
+	assert.True(t, reflect.DeepEqual(&secret1, createdSecret))
 
 	// Test update federated secret.
 	secret1.Annotations = map[string]string{
@@ -95,12 +104,16 @@ func TestSecretController(t *testing.T) {
 	updatedSecret := GetSecretFromChan(cluster1UpdateChan)
 	assert.NotNil(t, updatedSecret)
 	assert.Equal(t, secret1.Name, updatedSecret.Name)
+	assert.Equal(t, secret1.Namespace, updatedSecret.Namespace)
+	assert.True(t, reflect.DeepEqual(&secret1, updatedSecret))
 
 	// Test add cluster
 	clusterWatch.Add(cluster2)
 	createdSecret2 := GetSecretFromChan(cluster2CreateChan)
 	assert.NotNil(t, createdSecret2)
 	assert.Equal(t, secret1.Name, createdSecret2.Name)
+	assert.Equal(t, secret1.Namespace, createdSecret2.Namespace)
+	assert.True(t, reflect.DeepEqual(&secret1, createdSecret2))
 
 	close(stop)
 }