Skip to content

Commit

Permalink
Merge pull request kubernetes-sigs#1515 from aartij17/fix-secret-lookup
Browse files Browse the repository at this point in the history 
🐛 Fixes logic to fetch credentials of remote cluster
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed May 9, 2022
2 parents 0404ba7 + 46ca481 commit 89338e0
Show file tree
Hide file tree
Showing 13 changed files with 377 additions and 332 deletions.
102 changes: 63 additions & 39 deletions controllers/serviceaccount_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"
"reflect"
"strconv"
"strings"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand All @@ -31,6 +32,7 @@ import (
"k8s.io/apimachinery/pkg/types"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/cluster-api/controllers/remote"
clusterutilv1 "sigs.k8s.io/cluster-api/util"
"sigs.k8s.io/cluster-api/util/conditions"
"sigs.k8s.io/cluster-api/util/patch"
ctrl "sigs.k8s.io/controller-runtime"
Expand All @@ -55,16 +57,19 @@ import (
// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=get;list;watch;create;update;patch;delete

const (
controllerName = "provider-serviceaccount-controller"
kindProviderServiceAccount = "ProviderServiceAccount"
systemServiceAccountPrefix = "system.serviceaccount"
// ProviderServiceAccountControllerName defines the controller used when creating clients.
ProviderServiceAccountControllerName = "provider-serviceaccount-controller"
kindProviderServiceAccount = "ProviderServiceAccount"
systemServiceAccountPrefix = "system.serviceaccount"
)

// AddServiceAccountProviderControllerToManager adds this controller to the provided manager.
func AddServiceAccountProviderControllerToManager(ctx *context.ControllerManagerContext, mgr manager.Manager) error {
var (
controlledType = &vmwarev1.ProviderServiceAccount{}
controllerNameShort = reflect.TypeOf(controlledType).Elem().Name()
controlledType = &vmwarev1.ProviderServiceAccount{}
controlledTypeName = reflect.TypeOf(controlledType).Elem().Name()

controllerNameShort = fmt.Sprintf("%s-controller", strings.ToLower(controlledTypeName))
controllerNameLong = fmt.Sprintf("%s/%s/%s", ctx.Namespace, ctx.Name, controllerNameShort)
)

Expand All @@ -75,56 +80,31 @@ func AddServiceAccountProviderControllerToManager(ctx *context.ControllerManager
Logger: ctx.Logger.WithName(controllerNameShort),
}
r := ServiceAccountReconciler{
ControllerContext: controllerContext,
ControllerContext: controllerContext,
remoteClientGetter: remote.NewClusterClient,
}

return ctrl.NewControllerManagedBy(mgr).For(controlledType).
// Watch a ProviderServiceAccount
Watches(
&source.Kind{Type: &vmwarev1.ProviderServiceAccount{}}, &handler.EnqueueRequestForObject{}).
&source.Kind{Type: &vmwarev1.ProviderServiceAccount{}},
handler.EnqueueRequestsFromMapFunc(r.providerServiceAccountToVSphereCluster),
).
Watches(
&source.Kind{Type: &corev1.ServiceAccount{}},
handler.EnqueueRequestsFromMapFunc(requestMapper{ctx}.Map),
handler.EnqueueRequestsFromMapFunc(r.serviceAccountToVSphereCluster),
).
Complete(r)
}

type requestMapper struct {
ctx *context.ControllerManagerContext
}

func (d requestMapper) Map(o client.Object) []reconcile.Request {
// If the watched object is owned by this providerserviceaccount controller, then
// lookup the vsphere cluster that owns the providerserviceaccount object that needs to be queued. We do this because
// this controller is effectively a vsphere controller which reconciles it's dependent providerserviceaccounts.
ownerRef := metav1.GetControllerOf(o)
if ownerRef != nil && ownerRef.Kind == kindProviderServiceAccount {
key := types.NamespacedName{Namespace: o.GetNamespace(), Name: ownerRef.Name}
return getVSphereCluster(d.ctx, key)
}
return nil
}

func getVSphereCluster(ctx *context.ControllerManagerContext, pSvcAccountKey types.NamespacedName) []reconcile.Request {
pSvcAccount := &vmwarev1.ProviderServiceAccount{}
if err := ctx.Client.Get(ctx, pSvcAccountKey, pSvcAccount); err != nil {
return nil
}

vsphereClusterRef := pSvcAccount.Spec.Ref
if vsphereClusterRef == nil || vsphereClusterRef.Name == "" {
return nil
}
key := client.ObjectKey{Namespace: pSvcAccount.Namespace, Name: vsphereClusterRef.Name}
return []reconcile.Request{{NamespacedName: key}}
}

func NewServiceAccountReconciler() builder.Reconciler {
return ServiceAccountReconciler{}
}

type ServiceAccountReconciler struct {
*context.ControllerContext

remoteClientGetter remote.ClusterClientGetter
}

func (r ServiceAccountReconciler) Reconcile(ctx goctx.Context, req reconcile.Request) (_ reconcile.Result, reterr error) {
Expand Down Expand Up @@ -175,11 +155,17 @@ func (r ServiceAccountReconciler) Reconcile(ctx goctx.Context, req reconcile.Req
return r.ReconcileDelete(clusterContext)
}

cluster, err := clusterutilv1.GetClusterFromMetadata(r, r.Client, vsphereCluster.ObjectMeta)
if err != nil {
r.Logger.Info("unable to get capi cluster from vsphereCluster", "err", err)
return reconcile.Result{}, nil
}

// We cannot proceed until we are able to access the target cluster. Until
// then just return a no-op and wait for the next sync. This will occur when
// the Cluster's status is updated with a reference to the secret that has
// the Kubeconfig data used to access the target cluster.
guestClient, err := remote.NewClusterClient(clusterContext, controllerName, clusterContext.Client, clusterKey)
guestClient, err := r.remoteClientGetter(clusterContext, ProviderServiceAccountControllerName, clusterContext.Client, client.ObjectKeyFromObject(cluster))
if err != nil {
clusterContext.Logger.Info("The control plane is not ready yet", "err", err)
return reconcile.Result{RequeueAfter: clusterNotReadyRequeueTime}, nil
Expand Down Expand Up @@ -502,3 +488,41 @@ func GetCMNamespaceName() types.NamespacedName {
Name: os.Getenv("SERVICE_ACCOUNTS_CM_NAME"),
}
}

// serviceAccountToVSphereCluster is a mapper function used to enqueue reconcile.Request objects.
// From the watched object owned by this controller, it creates reconcile.Request object
// for the vmwarev1.VSphereCluster object that owns the watched object.
func (r ServiceAccountReconciler) serviceAccountToVSphereCluster(o client.Object) []reconcile.Request {
// We do this because this controller is effectively a vSphereCluster controller that reconciles its
// dependent ProviderServiceAccount objects.
ownerRef := metav1.GetControllerOf(o)
if ownerRef != nil && ownerRef.Kind == kindProviderServiceAccount {
key := types.NamespacedName{Namespace: o.GetNamespace(), Name: ownerRef.Name}
pSvcAccount := &vmwarev1.ProviderServiceAccount{}
if err := r.Client.Get(r.Context, key, pSvcAccount); err != nil {
return nil
}
return toVSphereClusterRequest(pSvcAccount)
}
return nil
}

// providerServiceAccountToVSphereCluster is a mapper function used to enqueue reconcile.Request objects.
func (r ServiceAccountReconciler) providerServiceAccountToVSphereCluster(o client.Object) []reconcile.Request {
providerServiceAccount, ok := o.(*vmwarev1.ProviderServiceAccount)
if !ok {
return nil
}

return toVSphereClusterRequest(providerServiceAccount)
}

func toVSphereClusterRequest(providerServiceAccount *vmwarev1.ProviderServiceAccount) []reconcile.Request {
vsphereClusterRef := providerServiceAccount.Spec.Ref
if vsphereClusterRef == nil || vsphereClusterRef.Name == "" {
return nil
}
return []reconcile.Request{
{NamespacedName: client.ObjectKey{Namespace: providerServiceAccount.Namespace, Name: vsphereClusterRef.Name}},
}
}
109 changes: 94 additions & 15 deletions controllers/serviceaccount_controller_intg_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,30 @@ limitations under the License.
package controllers

import (
"context"
"fmt"
"os"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/cluster-api/util/conditions"
"sigs.k8s.io/controller-runtime/pkg/client"

vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1"
"sigs.k8s.io/cluster-api-provider-vsphere/pkg/builder"
)

var _ = Describe("ServiceAccount controller integration tests", func() {
var _ = Describe("ProviderServiceAccount controller integration tests", func() {
var (
intCtx *builder.IntegrationTestContext
)

BeforeEach(func() {
ServiceAccountProviderTestsuite.SetIntegrationTestClient(testEnv.Manager.GetClient())
intCtx = ServiceAccountProviderTestsuite.NewIntegrationTestContextWithClusters(context.Background(), testEnv.Manager.GetClient(), true)
intCtx = ServiceAccountProviderTestsuite.NewIntegrationTestContextWithClusters(ctx, testEnv.Manager.GetClient())
testSystemSvcAcctCM := "test-system-svc-acct-cm"
cfgMap := getSystemServiceAccountsConfigMap(intCtx.VSphereCluster.Namespace, testSystemSvcAcctCM)
Expect(intCtx.Client.Create(intCtx, cfgMap)).To(Succeed())
Expand All @@ -46,7 +50,6 @@ var _ = Describe("ServiceAccount controller integration tests", func() {

AfterEach(func() {
intCtx.AfterEach()
intCtx = nil
})

Describe("When the ProviderServiceAccount is created", func() {
Expand All @@ -55,13 +58,13 @@ var _ = Describe("ServiceAccount controller integration tests", func() {
targetNSObj *corev1.Namespace
)
BeforeEach(func() {
pSvcAccount = getTestProviderServiceAccount(intCtx.Namespace, testProviderSvcAccountName, intCtx.VSphereCluster)
pSvcAccount = getTestProviderServiceAccount(intCtx.Namespace, intCtx.VSphereCluster)
createTestResource(intCtx, intCtx.Client, pSvcAccount)
assertEventuallyExistsInNamespace(intCtx, intCtx.Client, intCtx.Namespace, testProviderSvcAccountName, pSvcAccount)
assertEventuallyExistsInNamespace(intCtx, intCtx.Client, intCtx.Namespace, pSvcAccount.GetName(), pSvcAccount)
})
AfterEach(func() {
// Deleting the provider service account is not strictly required as the context itself gets teared down but
// keeping it for clarity.
// Deleting the provider service account is not strictly required as the context itself
// gets teared down but keeping it for clarity.
deleteTestResource(intCtx, intCtx.Client, pSvcAccount)
})

Expand All @@ -71,31 +74,107 @@ var _ = Describe("ServiceAccount controller integration tests", func() {
// to create a secret containing the bearer token, cert etc for a service account. We need to
// simulate the job of the token controller by waiting for the service account creation and then updating it
// with a prototype secret.
assertServiceAccountAndUpdateSecret(intCtx, intCtx.Client, intCtx.Namespace, testProviderSvcAccountName)
assertServiceAccountAndUpdateSecret(intCtx, intCtx.Client, intCtx.Namespace, pSvcAccount.GetName())
})

It("should create the role and role binding", func() {
Eventually(func() error {
role := &rbacv1.Role{}
key := client.ObjectKeyFromObject(pSvcAccount)
return intCtx.Client.Get(ctx, key, role)
}).Should(Succeed())

Eventually(func() error {
roleBinding := &rbacv1.RoleBinding{}
key := client.ObjectKeyFromObject(pSvcAccount)
if err := intCtx.Client.Get(ctx, key, roleBinding); err != nil {
return err
}
if roleBinding.RoleRef.Name != pSvcAccount.GetName() || len(roleBinding.Subjects) != 1 {
return errors.Errorf("roleBinding %s/%s is incorrect", roleBinding.GetNamespace(), roleBinding.GetName())
}
return nil
}).Should(Succeed())
})

It("Should reconcile", func() {
By("Creating the target secret in the target namespace")
assertTargetSecret(intCtx, intCtx.GuestClient, testTargetNS, testTargetSecret)
assertTargetSecret(intCtx, intCtx.GuestClient, pSvcAccount.Spec.TargetNamespace, testTargetSecret)
})
})

Context("When serviceaccount secret is rotated", func() {
BeforeEach(func() {
targetNSObj = &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: testTargetNS,
Name: pSvcAccount.Spec.TargetNamespace,
},
}
Expect(intCtx.GuestClient.Create(intCtx, targetNSObj)).To(Succeed())
createTargetSecretWithInvalidToken(intCtx, intCtx.GuestClient, testTargetNS)
assertServiceAccountAndUpdateSecret(intCtx, intCtx.Client, intCtx.Namespace, testSvcAccountName)
createTargetSecretWithInvalidToken(intCtx, intCtx.GuestClient, pSvcAccount.Spec.TargetNamespace)
assertServiceAccountAndUpdateSecret(intCtx, intCtx.Client, intCtx.Namespace, pSvcAccount.GetName())
})
AfterEach(func() {
deleteTestResource(intCtx, intCtx.GuestClient, targetNSObj)
})
It("Should reconcile", func() {
By("Updating the target secret in the target namespace")
assertTargetSecret(intCtx, intCtx.GuestClient, testTargetNS, testTargetSecret)
assertTargetSecret(intCtx, intCtx.GuestClient, pSvcAccount.Spec.TargetNamespace, testTargetSecret)
})
})
})

Context("With non-existent Cluster object", func() {
It("cannot reconcile the ProviderServiceAccount object", func() {
By("Deleting the CAPI cluster object", func() {
clusterName, ok := intCtx.VSphereCluster.GetLabels()[clusterv1.ClusterLabelName]
Expect(ok).To(BeTrue())
cluster := &clusterv1.Cluster{}
key := client.ObjectKey{Namespace: intCtx.Namespace, Name: clusterName}
Expect(intCtx.Client.Get(intCtx, key, cluster)).To(Succeed())
Expect(intCtx.Client.Delete(intCtx, cluster)).To(Succeed())
})

By("Creating the ProviderServiceAccount", func() {
pSvcAccount := getTestProviderServiceAccount(intCtx.Namespace, intCtx.VSphereCluster)
createTestResource(intCtx, intCtx.Client, pSvcAccount)
assertEventuallyExistsInNamespace(intCtx, intCtx.Client, intCtx.Namespace, pSvcAccount.GetName(), pSvcAccount)
})

By("ProviderServiceAccountsReady Condition is not set", func() {
vsphereCluster := &vmwarev1.VSphereCluster{}
key := client.ObjectKey{Namespace: intCtx.Namespace, Name: intCtx.VSphereCluster.GetName()}
Expect(intCtx.Client.Get(intCtx, key, vsphereCluster)).To(Succeed())
Expect(conditions.Has(vsphereCluster, vmwarev1.ProviderServiceAccountsReadyCondition)).To(BeFalse())
})
})
})

Context("With non-existent Cluster credentials secret", func() {
It("cannot reconcile the ProviderServiceAccount object", func() {
By("Deleting the CAPI kubeconfig secret object", func() {
clusterName, ok := intCtx.VSphereCluster.GetLabels()[clusterv1.ClusterLabelName]
Expect(ok).To(BeTrue())
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: intCtx.Namespace,
Name: fmt.Sprintf("%s-kubeconfig", clusterName),
},
}
Expect(intCtx.Client.Delete(intCtx, secret)).To(Succeed())
})

By("Creating the ProviderServiceAccount", func() {
pSvcAccount := getTestProviderServiceAccount(intCtx.Namespace, intCtx.VSphereCluster)
createTestResource(intCtx, intCtx.Client, pSvcAccount)
assertEventuallyExistsInNamespace(intCtx, intCtx.Client, intCtx.Namespace, pSvcAccount.GetName(), pSvcAccount)
})

By("ProviderServiceAccountsReady Condition is not set", func() {
vsphereCluster := &vmwarev1.VSphereCluster{}
key := client.ObjectKey{Namespace: intCtx.Namespace, Name: intCtx.VSphereCluster.GetName()}
Expect(intCtx.Client.Get(intCtx, key, vsphereCluster)).To(Succeed())
Expect(conditions.Has(vsphereCluster, vmwarev1.ProviderServiceAccountsReadyCondition)).To(BeFalse())
})
})
})
Expand Down
23 changes: 12 additions & 11 deletions controllers/serviceaccount_controller_suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,11 @@ import (
var ServiceAccountProviderTestsuite = builder.NewTestSuiteForController(NewServiceAccountReconciler)

const (
testNS = "test-pvcsi-system"
testProviderSvcAccountName = "test-pvcsi"

testTargetNS = "test-pvcsi-system"
testTargetSecret = "test-pvcsi-secret" // nolint:gosec
testSvcAccountName = testProviderSvcAccountName
testSvcAccountSecretName = testProviderSvcAccountName + "-token-abcdef"
testRoleName = testProviderSvcAccountName
testRoleBindingName = testProviderSvcAccountName
testSystemSvcAcctNs = "test-system-svc-acct-namespace"
testSystemSvcAcctCM = "test-system-svc-acct-cm"

Expand Down Expand Up @@ -161,7 +157,7 @@ func assertRoleBinding(_ *builder.UnitTestContextForController, ctrlClient clien
Expect(len(roleBindingList.Items)).To(Equal(1))
Expect(roleBindingList.Items[0].Name).To(Equal(name))
Expect(roleBindingList.Items[0].RoleRef).To(Equal(rbacv1.RoleRef{
Name: testRoleName,
Name: name,
Kind: "Role",
APIGroup: rbacv1.GroupName,
}))
Expand Down Expand Up @@ -200,12 +196,17 @@ func getTestTargetSecretWithValidToken(namespace string) *corev1.Secret {
}
}

func getTestProviderServiceAccount(namespace, name string, vSphereCluster *vmwarev1.VSphereCluster) *vmwarev1.ProviderServiceAccount {
func getTestProviderServiceAccount(namespace string, vSphereCluster *vmwarev1.VSphereCluster, randomize ...bool) *vmwarev1.ProviderServiceAccount {
objectMeta := metav1.ObjectMeta{
Namespace: namespace,
}
if len(randomize) > 0 && !randomize[0] {
objectMeta.Name = vSphereCluster.GetName()
} else {
objectMeta.GenerateName = vSphereCluster.GetName()
}
pSvcAccount := &vmwarev1.ProviderServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
},
ObjectMeta: objectMeta,
Spec: vmwarev1.ProviderServiceAccountSpec{
Rules: []rbacv1.PolicyRule{
{
Expand Down Expand Up @@ -292,7 +293,7 @@ func getTestRoleBindingWithInvalidRoleRef(namespace, name string) *rbacv1.RoleBi
{
Kind: "ServiceAccount",
APIGroup: "",
Name: testProviderSvcAccountName,
Name: name,
Namespace: namespace},
},
}
Expand Down
Loading

0 comments on commit 89338e0

Please sign in to comment.