Merge pull request juju#13073 from tlm/LP1930798-juju-2.9-k8s-upgrade-2

juju#13073 In juju 2.9 we migrated the controller pod to using in cluster credentials when talking to Kubernetes. This change did not apply to upgrade controllers. This commit changes that with upgrade steps to the controller. ## Checklist - [x] Requires a [pylibjuju](https://github.com/juju/python-libjuju) change - [x] Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR - [x] Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed - [x] Comments answer the question of why design decisions were made ## QA steps See bug. We want to do an upgrade from 2.8 to 2.9 and check that the statefulset for the controller gets it's service account attached and that the controller logs report no provider errors for credentials. ## Bug reference https://bugs.launchpad.net/juju/+bug/1930798
sdg0919 · Jun 22, 2021 · 4da92bf · 4da92bf
2 parents c279693 + 166f8ab
commit 4da92bf
Show file tree

Hide file tree

Showing 14 changed files with 359 additions and 46 deletions.
diff --git a/caas/kubernetes/provider/bootstrap.go b/caas/kubernetes/provider/bootstrap.go
@@ -453,7 +453,20 @@ func (c *controllerStack) Deploy() (err error) {
 	}
 
 	// create service account for local cluster/provider connections.
-	if err = c.ensureControllerServiceAccount(); err != nil {
+	_, saCleanUps, err := ensureControllerServiceAccount(
+		c.ctx.Context(),
+		c.broker.client(),
+		c.broker.GetCurrentNamespace(),
+		c.stackLabels,
+		c.stackAnnotations,
+	)
+	c.addCleanUp(func() {
+		logger.Debugf("delete controller service accounts")
+		for _, v := range saCleanUps {
+			v()
+		}
+	})
+	if err != nil {
 		return errors.Annotate(err, "creating service account for controller")
 	}
 	if isDone() {
@@ -721,39 +734,39 @@ func (c *controllerStack) ensureControllerConfigmapAgentConf() error {
 	return errors.Trace(err)
 }
 
-func (c *controllerStack) ensureControllerServiceAccount() error {
-	sa := &core.ServiceAccount{
+// ensureControllerServiceAccount is responsible for making sure the in cluster
+// service account for the controller exists and is upto date. Returns the name
+// of the service account create, cleanup functions and any errors.
+func ensureControllerServiceAccount(
+	ctx context.Context,
+	client kubernetes.Interface,
+	namespace string,
+	labels map[string]string,
+	annotations map[string]string,
+) (string, []func(), error) {
+	sa := resources.NewServiceAccount("controller", namespace, &core.ServiceAccount{
 		ObjectMeta: v1.ObjectMeta{
-			Name:      "controller",
-			Namespace: c.broker.GetCurrentNamespace(),
 			Labels: providerutils.LabelsMerge(
-				c.stackLabels,
+				labels,
 				providerutils.LabelsJujuModelOperatorDisableWebhook,
 			),
-			Annotations: c.stackAnnotations,
+			Annotations: annotations,
 		},
 		AutomountServiceAccountToken: boolPtr(true),
-	}
-
-	logger.Debugf("ensuring controller service account: \n%+v", sa)
-	_, cleanUps, err := c.broker.ensureServiceAccount(sa)
-	c.addCleanUp(func() {
-		logger.Debugf("deleting %q service account", sa.Name)
-		for _, v := range cleanUps {
-			v()
-		}
 	})
+
+	cleanUps, err := sa.Ensure(context.TODO(), client)
 	if err != nil {
-		return errors.Trace(err)
+		return sa.Name, cleanUps, errors.Trace(err)
 	}
 
 	// name cluster role binding after the controller namespace.
-	clusterRoleBindingName := c.broker.GetCurrentNamespace()
+	clusterRoleBindingName := namespace
 	crb := resources.NewClusterRoleBinding(clusterRoleBindingName, &rbacv1.ClusterRoleBinding{
 		ObjectMeta: v1.ObjectMeta{
 			Name:        clusterRoleBindingName,
 			Labels:      providerutils.LabelsForModel("controller", false),
-			Annotations: c.stackAnnotations,
+			Annotations: annotations,
 		},
 		RoleRef: rbacv1.RoleRef{
 			APIGroup: "rbac.authorization.k8s.io",
@@ -763,18 +776,13 @@ func (c *controllerStack) ensureControllerServiceAccount() error {
 		Subjects: []rbacv1.Subject{{
 			Kind:      "ServiceAccount",
 			Name:      "controller",
-			Namespace: c.broker.GetCurrentNamespace(),
+			Namespace: namespace,
 		}},
 	})
 
-	crbCleanUps, err := crb.Ensure(c.ctx.Context(), c.broker.client())
-	c.addCleanUp(func() {
-		logger.Debugf("deleting %q cluster role binding", crb.Name)
-		for _, v := range crbCleanUps {
-			v()
-		}
-	})
-	return errors.Trace(err)
+	crbCleanUps, err := crb.Ensure(ctx, client)
+	cleanUps = append(cleanUps, crbCleanUps...)
+	return sa.Name, cleanUps, errors.Trace(err)
 }
 
 func (c *controllerStack) createControllerStatefulset() error {

diff --git a/caas/kubernetes/provider/bootstrap_test.go b/caas/kubernetes/provider/bootstrap_test.go
@@ -906,8 +906,6 @@ $JUJU_TOOLS_DIR/jujud machine --data-dir $JUJU_DATA_DIR --controller-id 0 --log-
 			Return(emptyConfigMap, nil),
 		s.mockConfigMaps.EXPECT().Create(gomock.Any(), configMapWithBootstrapParamsAdded, v1.CreateOptions{}).AnyTimes().
 			Return(nil, s.k8sAlreadyExistsError()),
-		s.mockConfigMaps.EXPECT().List(gomock.Any(), v1.ListOptions{LabelSelector: "app.kubernetes.io/managed-by=juju,app.kubernetes.io/name=juju-controller-test"}).
-			Return(&core.ConfigMapList{Items: []core.ConfigMap{*emptyConfigMap}}, nil),
 		s.mockConfigMaps.EXPECT().Update(gomock.Any(), configMapWithBootstrapParamsAdded, v1.UpdateOptions{}).AnyTimes().
 			Return(configMapWithBootstrapParamsAdded, nil),
 
@@ -916,12 +914,12 @@ $JUJU_TOOLS_DIR/jujud machine --data-dir $JUJU_DATA_DIR --controller-id 0 --log-
 			Return(configMapWithBootstrapParamsAdded, nil),
 		s.mockConfigMaps.EXPECT().Create(gomock.Any(), configMapWithAgentConfAdded, v1.CreateOptions{}).AnyTimes().
 			Return(nil, s.k8sAlreadyExistsError()),
-		s.mockConfigMaps.EXPECT().List(gomock.Any(), v1.ListOptions{LabelSelector: "app.kubernetes.io/managed-by=juju,app.kubernetes.io/name=juju-controller-test"}).
-			Return(&core.ConfigMapList{Items: []core.ConfigMap{*configMapWithBootstrapParamsAdded}}, nil),
 		s.mockConfigMaps.EXPECT().Update(gomock.Any(), configMapWithAgentConfAdded, v1.UpdateOptions{}).AnyTimes().
 			Return(configMapWithAgentConfAdded, nil),
 
-		s.mockServiceAccounts.EXPECT().Create(gomock.Any(), controllerServiceAccount, v1.CreateOptions{}).
+		s.mockServiceAccounts.EXPECT().Get(gomock.Any(), controllerServiceAccount.Name, gomock.Any()).
+			Return(controllerServiceAccount, nil),
+		s.mockServiceAccounts.EXPECT().Update(gomock.Any(), controllerServiceAccount, gomock.Any()).
 			Return(controllerServiceAccount, nil),
 		s.mockClusterRoleBindings.EXPECT().Get(gomock.Any(), controllerServiceCRB.Name, gomock.Any()).
 			Return(controllerServiceCRB, nil),

diff --git a/caas/kubernetes/provider/configmap.go b/caas/kubernetes/provider/configmap.go
@@ -74,14 +74,6 @@ func (k *kubernetesClient) ensureConfigMap(cm *core.ConfigMap) (func(), error) {
 	if !errors.IsAlreadyExists(err) {
 		return cleanUp, errors.Trace(err)
 	}
-	_, err = k.listConfigMaps(cm.GetLabels())
-	if err != nil {
-		if errors.IsNotFound(err) {
-			// configmap name is already used for an existing secret.
-			return cleanUp, errors.AlreadyExistsf("configmap %q", cm.GetName())
-		}
-		return cleanUp, errors.Trace(err)
-	}
 	err = k.updateConfigMap(cm)
 	logger.Debugf("updating configmap %q", cm.GetName())
 	return cleanUp, errors.Trace(err)

diff --git a/caas/kubernetes/provider/controller_upgrade.go b/caas/kubernetes/provider/controller_upgrade.go
@@ -4,9 +4,15 @@
 package provider
 
 import (
+	"context"
+
+	"github.com/juju/errors"
 	"github.com/juju/version/v2"
+	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/client-go/kubernetes"
 
+	"github.com/juju/juju/caas/kubernetes/provider/resources"
+	providerutils "github.com/juju/juju/caas/kubernetes/provider/utils"
 	"github.com/juju/juju/environs/bootstrap"
 )
 
@@ -59,3 +65,56 @@ func (k *kubernetesClient) upgradeController(vers version.Number) error {
 	}
 	return controllerUpgrade(bootstrap.ControllerModelName, vers, broker)
 }
+
+// InClusterCredentialUpgrade implements upgrades.upgradeKubernetesClusterCredential
+// used in the Juju 2.9.6 upgrade step
+func (k *kubernetesClient) InClusterCredentialUpgrade() error {
+	return inClusterCredentialUpgrade(
+		k.client(),
+		k.IsLegacyLabels(),
+		k.GetCurrentNamespace(),
+	)
+}
+
+func inClusterCredentialUpgrade(
+	client kubernetes.Interface,
+	legacyLabels bool,
+	namespace string,
+) error {
+	ctx := context.TODO()
+	labels := providerutils.LabelsForApp("controller", legacyLabels)
+
+	saName, cleanUps, err := ensureControllerServiceAccount(
+		ctx,
+		client,
+		namespace,
+		labels,
+		map[string]string{},
+	)
+
+	runCleanups := func() {
+		for _, v := range cleanUps {
+			v()
+		}
+	}
+
+	if err != nil {
+		runCleanups()
+		return errors.Trace(err)
+	}
+
+	ss := resources.NewStatefulSet("controller", namespace, &appsv1.StatefulSet{})
+	if err := ss.Get(ctx, client); err != nil {
+		runCleanups()
+		return errors.Annotate(err, "updating controller for in cluster credentials")
+	}
+
+	ss.Spec.Template.Spec.ServiceAccountName = saName
+	ss.Spec.Template.Spec.AutomountServiceAccountToken = boolPtr(true)
+	if err := ss.Apply(ctx, client); err != nil {
+		runCleanups()
+		return errors.Annotate(err, "updating controller for in cluster credentials")
+	}
+
+	return nil
+}
diff --git a/caas/kubernetes/provider/k8s_test.go b/caas/kubernetes/provider/k8s_test.go
@@ -2809,8 +2809,6 @@ password: shhhh`[1:],
 		// ensure configmaps.
 		s.mockConfigMaps.EXPECT().Create(gomock.Any(), cm, v1.CreateOptions{}).
 			Return(nil, s.k8sAlreadyExistsError()),
-		s.mockConfigMaps.EXPECT().List(gomock.Any(), v1.ListOptions{LabelSelector: "app.kubernetes.io/managed-by=juju,app.kubernetes.io/name=app-name"}).
-			Return(&core.ConfigMapList{Items: []core.ConfigMap{*cm}}, nil),
 		s.mockConfigMaps.EXPECT().Update(gomock.Any(), cm, v1.UpdateOptions{}).
 			Return(cm, nil),
 

diff --git a/caas/kubernetes/provider/resources/clusterrole.go b/caas/kubernetes/provider/resources/clusterrole.go
@@ -111,7 +111,7 @@ func (r *ClusterRole) Ensure(
 		return cleanups, errors.Annotatef(
 			err,
 			"checking for existing cluster role %q",
-			r.Name,
+			existing.Name,
 		)
 	}
 
@@ -154,7 +154,7 @@ func (r *ClusterRole) Update(ctx context.Context, client kubernetes.Interface) e
 		},
 	)
 	if k8serrors.IsNotFound(err) {
-		return errors.Annotatef(err, "updating cluster role %q", r.Name)
+		return errors.NewNotFound(err, "updating cluster role")
 	} else if err != nil {
 		return errors.Trace(err)
 	}

diff --git a/caas/kubernetes/provider/resources/serviceaccount.go b/caas/kubernetes/provider/resources/serviceaccount.go
@@ -90,6 +90,49 @@ func (sa *ServiceAccount) Delete(ctx context.Context, client kubernetes.Interfac
 	return nil
 }
 
+func (sa *ServiceAccount) Ensure(
+	ctx context.Context,
+	client kubernetes.Interface,
+	claims ...Claim,
+) ([]func(), error) {
+	alreadyExists := false
+	cleanups := []func(){}
+	hasClaim := true
+
+	existing := ServiceAccount{sa.ServiceAccount}
+	err := existing.Get(ctx, client)
+	if err != nil && !errors.IsNotFound(err) {
+		return cleanups, errors.Annotatef(
+			err,
+			"checking for existing service account %q",
+			existing.Name,
+		)
+	}
+	if err == nil {
+		alreadyExists = true
+		hasClaim, err = RunClaims(claims...).Assert(&existing.ServiceAccount)
+		if err != nil {
+			return cleanups, errors.Annotatef(
+				err,
+				"checking claims for service account %q",
+				existing.Name,
+			)
+		}
+	}
+
+	if !hasClaim {
+		return cleanups, errors.AlreadyExistsf(
+			"service account %q not controlled by juju", sa.Name)
+	}
+
+	cleanups = append(cleanups, func() { _ = sa.Delete(ctx, client) })
+	if !alreadyExists {
+		return cleanups, sa.Apply(ctx, client)
+	}
+
+	return cleanups, errors.Trace(sa.Update(ctx, client))
+}
+
 // Events emitted by the resource.
 func (sa *ServiceAccount) Events(ctx context.Context, client kubernetes.Interface) ([]corev1.Event, error) {
 	return ListEventsForObject(ctx, client, sa.Namespace, sa.Name, "ServiceAccount")
@@ -102,3 +145,23 @@ func (sa *ServiceAccount) ComputeStatus(_ context.Context, _ kubernetes.Interfac
 	}
 	return "", status.Active, now, nil
 }
+
+func (sa *ServiceAccount) Update(
+	ctx context.Context,
+	client kubernetes.Interface,
+) error {
+	out, err := client.CoreV1().ServiceAccounts(sa.Namespace).Update(
+		ctx,
+		&sa.ServiceAccount,
+		metav1.UpdateOptions{
+			FieldManager: JujuFieldManager,
+		},
+	)
+	if k8serrors.IsNotFound(err) {
+		return errors.NewNotFound(err, "updating service account")
+	} else if err != nil {
+		return errors.Trace(err)
+	}
+	sa.ServiceAccount = *out
+	return nil
+}
diff --git a/caas/kubernetes/provider/resources/serviceaccount_test.go b/caas/kubernetes/provider/resources/serviceaccount_test.go
@@ -93,3 +93,81 @@ func (s *serviceAccountSuite) TestDelete(c *gc.C) {
 	_, err = s.client.CoreV1().ServiceAccounts("test").Get(context.TODO(), "sa1", metav1.GetOptions{})
 	c.Assert(err, jc.Satisfies, k8serrors.IsNotFound)
 }
+
+func (s *serviceAccountSuite) TestUpdate(c *gc.C) {
+	sa := corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "sa1",
+			Namespace: "test",
+		},
+	}
+	_, err := s.client.CoreV1().ServiceAccounts("test").Create(
+		context.TODO(),
+		&sa,
+		metav1.CreateOptions{},
+	)
+	c.Assert(err, jc.ErrorIsNil)
+
+	sa.ObjectMeta.Labels = map[string]string{
+		"test": "label",
+	}
+
+	saResource := resources.NewServiceAccount("sa1", "test", &sa)
+	err = saResource.Update(context.TODO(), s.client)
+	c.Assert(err, jc.ErrorIsNil)
+
+	rsa, err := s.client.CoreV1().ServiceAccounts("test").Get(
+		context.TODO(),
+		"sa1",
+		metav1.GetOptions{},
+	)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(rsa, jc.DeepEquals, &saResource.ServiceAccount)
+}
+
+func (s *serviceAccountSuite) TestEnsureCreatesNew(c *gc.C) {
+	sa := resources.NewServiceAccount("sa1", "test", &corev1.ServiceAccount{})
+	cleanups, err := sa.Ensure(context.TODO(), s.client)
+	c.Assert(err, jc.ErrorIsNil)
+
+	obj, err := s.client.CoreV1().ServiceAccounts("test").Get(
+		context.TODO(), "sa1", metav1.GetOptions{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(&sa.ServiceAccount, jc.DeepEquals, obj)
+
+	for _, v := range cleanups {
+		v()
+	}
+
+	// Test cleanup removes service account
+	_, err = s.client.CoreV1().ServiceAccounts("test").Get(
+		context.TODO(), "sa1", metav1.GetOptions{})
+	c.Assert(k8serrors.IsNotFound(err), jc.IsTrue)
+}
+
+func (s *serviceAccountSuite) TestEnsureUpdates(c *gc.C) {
+	sa := &corev1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "sa2",
+			Namespace: "testing",
+		},
+	}
+
+	_, err := s.client.CoreV1().ServiceAccounts("testing").Create(
+		context.TODO(), sa, metav1.CreateOptions{})
+	c.Assert(err, jc.ErrorIsNil)
+
+	sa.ObjectMeta.Labels = map[string]string{
+		"test": "case",
+	}
+
+	resource := resources.NewServiceAccount("sa2", "testing", sa)
+	_, err = resource.Ensure(context.TODO(), s.client)
+	c.Assert(err, jc.ErrorIsNil)
+
+	obj, err := s.client.CoreV1().ServiceAccounts("testing").Get(
+		context.TODO(), sa.Name, metav1.GetOptions{})
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(obj, jc.DeepEquals, &resource.ServiceAccount)
+}