Fixes in cluster credentials for juju upgrades.

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. Fixes LP1930798
sdg0919 · Jun 22, 2021 · 166f8ab · 166f8ab
1 parent e9fd5a1
commit 166f8ab
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)
+}