Merge pull request #2988 from fabriziopandini/fix-clusterctl-soft-ownership

🐛fix a clusterctl error when moving cluster with user provided certs

Author: k8s-ci-robot

Diff for: cmd/clusterctl/client/cluster/objectgraph.go

@@ -17,8 +17,6 @@ limitations under the License.
 package cluster
 
 import (
-	"strings"
-
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -29,6 +27,7 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
+	secretutil "sigs.k8s.io/cluster-api/util/secret"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -339,15 +338,15 @@ func (o *objectGraph) setSoftOwnership() {
 			continue
 		}
 
-		// If the secret name does not comply the naming convention {cluster-name}-{certificate-name/kubeconfig}, ignore it.
-		nameSplit := strings.Split(secret.identity.Name, "-")
-		if len(nameSplit) != 2 {
+		// If the secret name is not a valid cluster secret name, ignore it.
+		secretClusterName, _, err := secretutil.ParseSecretName(secret.identity.Name)
+		if err != nil {
 			continue
 		}
 
-		// If the secret is linked to a cluster by the naming convention, then add the cluster to the list of the secrets's softOwners.
+		// If the secret is linked to a cluster, then add the cluster to the list of the secrets's softOwners.
 		for _, cluster := range clusters {
-			if nameSplit[0] == cluster.identity.Name && secret.identity.Namespace == cluster.identity.Namespace {
+			if secretClusterName == cluster.identity.Name && secret.identity.Namespace == cluster.identity.Namespace {
 				secret.addSoftOwner(cluster)
 			}
 		}

Diff for: cmd/clusterctl/client/cluster/objectgraph_test.go

@@ -1078,6 +1078,18 @@ func Test_objectGraph_setSoftOwnership(t *testing.T) {
 				"/v1, Kind=Secret, ns1/foo-kubeconfig": {}, // the kubeconfig secret has explicit OwnerRef to the cluster, so it should NOT be identified as a soft ownership
 			},
 		},
+		{
+			name: "A cluster with a soft owned secret (cluster name with - in the middle)",
+			fields: fields{
+				objs: test.NewFakeCluster("ns1", "foo-bar").Objs(),
+			},
+			wantSecrets: map[string][]string{ // wantSecrets is a map[node UID] --> list of soft owner UIDs
+				"/v1, Kind=Secret, ns1/foo-bar-ca": { // the ca secret has no explicit OwnerRef to the cluster, so it should be identified as a soft ownership
+					"cluster.x-k8s.io/v1alpha3, Kind=Cluster, ns1/foo-bar",
+				},
+				"/v1, Kind=Secret, ns1/foo-bar-kubeconfig": {}, // the kubeconfig secret has explicit OwnerRef to the cluster, so it should NOT be identified as a soft ownership
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

Diff for: util/secret/consts.go

@@ -47,3 +47,8 @@ const (
 	// APIServerEtcdClient is the secret name of user-supplied secret containing the apiserver-etcd-client key/cert
 	APIServerEtcdClient Purpose = "apiserver-etcd-client"
 )
+
+var (
+	// allSecretPurposes defines a lists with all the secret suffix used by Cluster API
+	allSecretPurposes = []Purpose{Kubeconfig, ClusterCA, EtcdCA, ServiceAccount, FrontProxyCA, APIServerEtcdClient}
+)

Diff for: util/secret/secret.go

@@ -19,7 +19,9 @@ package secret
 import (
 	"context"
 	"fmt"
+	"strings"
 
+	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -50,3 +52,20 @@ func GetFromNamespacedName(ctx context.Context, c client.Client, clusterName cli
 func Name(cluster string, suffix Purpose) string {
 	return fmt.Sprintf("%s-%s", cluster, suffix)
 }
+
+// ParseSecretName return the cluster name and the suffix Purpose in name is a valid cluster secrets,
+// otherwise it return error.
+func ParseSecretName(name string) (string, Purpose, error) {
+	separatorPos := strings.LastIndex(name, "-")
+	if separatorPos == -1 {
+		return "", "", errors.Errorf("%q is not a valid cluster secret name. The purpose suffix is missing", name)
+	}
+	clusterName := name[:separatorPos]
+	purposeSuffix := Purpose(name[separatorPos+1:])
+	for _, purpose := range allSecretPurposes {
+		if purpose == purposeSuffix {
+			return clusterName, purposeSuffix, nil
+		}
+	}
+	return "", "", errors.Errorf("%q is not a valid cluster secret name. Invalid purpose suffix", name)
+}

Diff for: util/secret/secret_test.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package secret
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestParseSecretName(t *testing.T) {
+	type args struct {
+		name string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    string
+		want1   Purpose
+		wantErr bool
+	}{
+		{
+			name: "A secret for the test cluster",
+			args: args{
+				name: "test-kubeconfig",
+			},
+			want:    "test",
+			want1:   Kubeconfig,
+			wantErr: false,
+		},
+		{
+			name: "A secret for the test-capa cluster (cluster name with - in the middle)",
+			args: args{
+				name: "test-capa-ca",
+			},
+			want:    "test-capa",
+			want1:   ClusterCA,
+			wantErr: false,
+		},
+		{
+			name: "Not a Cluster API secret",
+			args: args{
+				name: "foo",
+			},
+			wantErr: true,
+		},
+		{
+			name: "Not a Cluster API secret (secret name with - in the middle)",
+			args: args{
+				name: "foo-bar",
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			got, got1, err := ParseSecretName(tt.args.name)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+			}
+			g.Expect(got).To(Equal(tt.want))
+			g.Expect(got1).To(Equal(tt.want1))
+		})
+	}
+}