Use serviceAccountName for Service Account creation #2880

micnncim · 2022-02-06T18:36:20Z

Description of your changes

Updates the name of a Service Account, which is to be created by a controller, to be specified by the .spec.serviceAccountName field of ControllerConfig if specified.

It's currently named the same name of a ProviderRevision even if the .spec.serviceAccountName is specified whereas the .spec.template.spec.serviceAccountName in a Deployment is specified by it.

The current approach wouldn't make a lot of sense because:

ClusterRolebindings to be created by a controller target only Service Accounts that are managed by the controller so necessary Roles aren't granted to a Service Account defined in a ControllerConfig by default.
Inconstant names aren't helpful in some cases. A notable example is Workload Identity in GCP, where a Kubernetes Service Account needs to specified in IAM binding. With the current implementation, whenever a new ProvierRevision is created, which means a Service Account is "renamed", IAM binding for Workload Identity also needs to be updated. Though Workload Identity isn't supported in provider-gcp yet, this would meet one of the requirements to support it.

On the other hand, there wouldn't be a lot of practical use cases where it's necessary to specify a custom Service Account to a Deployment and keep letting a controller create Service Accounts with different names.

Signed-off-by: micnncim micnncim@gmail.com

Fixes #2295

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

See #2880 (comment)

Updates the name of a Service Account, which is to be created by a controller, to be specified by the `.spec.serviceAccountName` field of `ControllerConfig` if specified. It's currently named the same name of a `ProviderRevision` even if the `.spec.serviceAccountName` is specified whereas the `.spec.template.spec.serviceAccountName` in a Deployment is specified by it. This approach wouldn't make a lot of sense because: - ClusterRolebindings to be created by a controller target only Service Accounts that are managed by the controller so necessary Roles aren't granted to a Service Account defined in a `ControllerConfig` by default. - Inconstant names aren't helpful in some cases. A notable example is Workload Identity in GCP, where a Kubernetes Service Account needs to specified in IAM binding. With the current implementation, whenever a new `ProvierRevision` is created, which means a Service Account is "renamed", IAM binding for Workload Identity also needs to be updated. Though Workload Identity isn't supported in `provider-gcp`, this would meet one of the requirements to support it. On the other hand, there wouldn't be a lot of practical use cases where it's necessary to specify a custom Service Account to a Deployment and keep letting a controller create Service Accounts with different names. Signed-off-by: micnncim <micnncim@gmail.com>

turkenh · 2022-02-08T06:46:19Z

internal/controller/pkg/revision/deployment.go

@@ -116,6 +116,9 @@ func buildProviderDeployment(provider *pkgmetav1.Provider, revision v1.PackageRe
 		s.Annotations = cc.Annotations
 		d.Labels = cc.Labels
 		d.Annotations = cc.Annotations
+		if cc.Spec.ServiceAccountName != nil {
+			s.Name = *cc.Spec.ServiceAccountName


I agree that this would simplify most cases where people pass a ServiceAccountName today.
I am a bit hesitant due to the following:

This could be a breaking change for existing setups where a ServiceAccount is created separately.

I don't feel much comfortable API-wise. So far, any configurations there (except labels/annotations, i.e. metadata) were only affecting the pod spec.

@hasheddan what do you think? Any previous discussion on this?

I can understand your concern. I have another idea as below to realize my intention without breaking the compatibility or affecting other resources than Pods:

Add a new label, for example crossplane.io/service-account, to be used in a user-managed ServiceAccount, optionally

Update the logic to reconcile ClusterRoleBindings to create a ClusterRoleBinding for a user-managed ServiceAccount alongside with controller-managed ServiceAccounts if it has the above label

In this approach, in addition to resolving your concern, users can choose to both keep the current behavior and have their ServiceAccount be granted a necessary ClusterRole.

stale · 2022-08-13T01:10:17Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wwentland · 2022-09-06T23:31:58Z

This PR should not have been closed and implements exactly the behaviour I would have expected.

Furthermore it is a required building block for being able to implement Crossplane deployments that utilise workload identity (e.g. EKS IRSA) grounded in K8s service accounts.

Please consider merging this!

micnncim · 2022-09-16T05:57:22Z

@turkenh Hi, any discussion about this you've had in the team or community? I still believe some people want some kind of this update.

turkenh · 2022-09-28T09:49:10Z

@micnncim sorry for the delay here.

https://github.com/crossplane/crossplane/blob/master/CONTRIBUTING.md#establishing-a-development-environment doens't work in M1 Mac machines so I haven't tested it locally.

I believe M1 Mac is supported now, could you do some local testing and update the PR description accordingly.

I would really appreciate if you could also test how it behaves on a setup where user has manually created that service account beforehand, would it fail or simply adapt it without any error to make sure whether this would be a breaking change or not?

wwentland · 2022-11-30T13:48:39Z

Thank you for the ping, @turkenh!

https://github.com/crossplane/crossplane/blob/master/CONTRIBUTING.md#establishing-a-development-environment doens't work in M1 Mac machines so I haven't tested it locally.

I believe M1 Mac is supported now, could you do some local testing and update the PR description accordingly.

I've rebased this PR against the current master branch:

❯ git log --pretty=oneline
f8f3d3fa199733a2ed2e3be7b4f63e8a53cdd93d (HEAD -> service-account) Use serviceAccountName for Service Account creation
22648219c9dbe54d5606e9b3c263fcb2c17ff755 (origin/master, origin/HEAD, master) Merge pull request #3470 from csantanapr/broken-link-started
c4fb2315311c4ccadd449357155f029332421c1a Merge pull request #3473 from vshn/fix-typos-in-design-doc
a2fdb3bff85a34c4c191ea4d8100c06115d9eae3 correct spelling in composition functions design doc
...

and created a local kind test cluster by following the instructions for establishing a development environment.

The deployed versions were:

❯ k get deployment -n crossplane-system -o wide
NAME                                   READY   UP-TO-DATE   AVAILABLE   AGE    CONTAINERS     IMAGES                                             SELECTOR
crossplane                             1/1     1            1           126m   crossplane     crossplane/crossplane:v1.11.0-rc.0.104.gf8f3d3fa   app=crossplane,release=crossplane
crossplane-rbac-manager                1/1     1            1           126m   crossplane     crossplane/crossplane:v1.11.0-rc.0.104.gf8f3d3fa   app=crossplane-rbac-manager,release=crossplane

New ServiceAccount

I've created the following ControllerConfig and Provider to test the behaviour when no corresponding ServiceAccount exists already:

---
apiVersion: pkg.crossplane.io/v1alpha1
kind: ControllerConfig
metadata:
  name: crossplane-provider-aws-config
  annotations:
    eks.amazonaws.com/role-arn: aws:iam::01234567890:role/crossplane-provider-aws
    some.other.annotation: foobar
spec:
  args:
  - --enable-external-secret-stores
  serviceAccountName: crossplane-provider-aws
---
apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
  name: crossplane-provider-aws
spec:
  package: crossplanecontrib/provider-aws:v0.34.0
  controllerConfigRef:
    name: crossplane-provider-aws-config

Once applied, a new ServiceAccount is created:

❯ k get provider
NAME                      INSTALLED   HEALTHY   PACKAGE                                  AGE
crossplane-provider-aws   True        True      crossplanecontrib/provider-aws:v0.34.0   45s

❯ k get serviceaccounts -n crossplane-system
NAME                      SECRETS   AGE
crossplane                0         10m
crossplane-provider-aws   0         6s
default                   0         10m
rbac-manager              0         10m

with the expected ClusterRoleBinding and annotations in place:

❯ k describe clusterrolebindings.rbac.authorization.k8s.io crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Name:         crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Labels:       <none>
Annotations:  <none>
Role:
  Kind:  ClusterRole
  Name:  crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Subjects:
  Kind            Name                     Namespace
  ----            ----                     ---------
  ServiceAccount  crossplane-provider-aws  crossplane-system

❯ k describe serviceaccounts -n crossplane-system crossplane-provider-aws
Name:                crossplane-provider-aws
Namespace:           crossplane-system
Labels:              <none>
Annotations:         eks.amazonaws.com/role-arn: aws:iam::01234567890:role/crossplane-provider-aws
                     some.other.annotation: foobar
...

Existing ServiceAccount

The behaviour is as desired when the specified ServiceAccount exists already.

❯ k get serviceaccounts -n crossplane-system
NAME           SECRETS   AGE
crossplane     0         139m
default        0         139m
rbac-manager   0         139m

❯ k create serviceaccount -n crossplane-system crossplane-provider-aws
serviceaccount/crossplane-provider-aws created

❯ k describe serviceaccounts -n crossplane-system crossplane-provider-aws
Name:                crossplane-provider-aws
Namespace:           crossplane-system
Labels:              <none>
Annotations:         <none>
...

Specifically, the corresponding ClusterRoleBinding does not exist:

❯ k get clusterrolebindings.rbac.authorization.k8s.io|grep cross
crossplane                                             ClusterRole/crossplane                                                             139m
crossplane-admin                                       ClusterRole/crossplane-admin                                                       139m
crossplane-rbac-manager                                ClusterRole/crossplane-rbac-manager                                                139m

❯ k describe clusterrolebindings.rbac.authorization.k8s.io crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Error from server (NotFound): clusterrolebindings.rbac.authorization.k8s.io "crossplane:provider:crossplane-provider-aws-6707d06fe75f:system" not found

Lets apply ControllerConfig and Provider:

❯ k apply -f configure/aws/provider.yaml
controllerconfig.pkg.crossplane.io/crossplane-provider-aws-config created
provider.pkg.crossplane.io/crossplane-provider-aws created

❯ k get provider
NAME                      INSTALLED   HEALTHY   PACKAGE                                  AGE
crossplane-provider-aws   True        Unknown   crossplanecontrib/provider-aws:v0.34.0   5s

We can see that the ClusterRoleBinding has been created, but does not (yet) specify our ServiceAccount:

❯ k describe clusterrolebindings.rbac.authorization.k8s.io crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Name:         crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Labels:       <none>
Annotations:  <none>
Role:
  Kind:  ClusterRole
  Name:  crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Subjects:
  Kind  Name  Namespace
  ----  ----  ---------

Which changes in a couple of reconciliation loops and when the provider becomes healthy:

❯ k get provider
NAME                      INSTALLED   HEALTHY   PACKAGE                                  AGE
crossplane-provider-aws   True        True      crossplanecontrib/provider-aws:v0.34.0   60s

❯ k describe clusterrolebindings.rbac.authorization.k8s.io crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Name:         crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Labels:       <none>
Annotations:  <none>
Role:
  Kind:  ClusterRole
  Name:  crossplane:provider:crossplane-provider-aws-6707d06fe75f:system
Subjects:
  Kind            Name                     Namespace
  ----            ----                     ---------
  ServiceAccount  crossplane-provider-aws  crossplane-system

and the ServiceAccount also has the annotations specified in the ControllerConfig:

❯ k describe serviceaccounts -n crossplane-system crossplane-provider-aws
Name:                crossplane-provider-aws
Namespace:           crossplane-system
Labels:              <none>
Annotations:         eks.amazonaws.com/role-arn: aws:iam::01234567890:role/crossplane-provider-aws
                     some.other.annotation: foobar
...

If the existing ServiceAccount has annotations already, then the annotations in the ControllerConfig are added to any pre-existing annotation:

❯ k annotate serviceaccounts -n crossplane-system crossplane-provider-aws some.existing.annotation=quux
serviceaccount/crossplane-provider-aws annotated

❯ k describe serviceaccounts -n crossplane-system crossplane-provider-aws
Name:                crossplane-provider-aws
Namespace:           crossplane-system
Labels:              <none>
Annotations:         some.existing.annotation: quux
...

❯ k apply -f configure/aws/provider.yaml
controllerconfig.pkg.crossplane.io/crossplane-provider-aws-config created
provider.pkg.crossplane.io/crossplane-provider-aws created

❯ k describe serviceaccounts -n crossplane-system crossplane-provider-aws
Name:                crossplane-provider-aws
Namespace:           crossplane-system
Labels:            <none>
Annotations:         eks.amazonaws.com/role-arn: aws:iam::01234567890:role/crossplane-provider-aws
                     some.existing.annotation: quux
                     some.other.annotation: foobar
...

It should be noted that the ServiceAccount is still deleted when we remove the ControllerConfig and Provider resources:

❯ k get serviceaccounts -n crossplane-system
NAME                      SECRETS   AGE
crossplane                0         12m
crossplane-provider-aws   0         7m33s
default                   0         12m
rbac-manager              0         12m

❯ k delete -f configure/aws/provider.yaml
controllerconfig.pkg.crossplane.io "crossplane-provider-aws-config" deleted
provider.pkg.crossplane.io "crossplane-provider-aws" deleted

❯ k get serviceaccounts -n crossplane-system
NAME           SECRETS   AGE
crossplane     0         12m
default        0         12m
rbac-manager   0         12m

Conclusion

In summary, we are seeing the desired behaviour:

Service accounts specified in spec.serviceAccountName are created if they do not exist
Cluster role bindings are using the appropriate service account as subject (i.e. the one from spec.serviceAccountName)
Annotations in the controller config metadata.annotations are copied to the service account (and do not overwrite existing ones if the service account has been created already)
Existing service accounts can be used and will have the required changes applied to them

The only slightly problematic aspect is that service accounts are deleted, regardless if they they were created by Crossplane or were in place already.

aquam8 · 2022-12-20T06:18:57Z

This would be great, it is a real issue otherwise to deploy when using Infra-as-code like Terraform when you cannot control the resources created as part of the provider like clusterrole, clusterrolebinding and serviceaccount 🥇

stevendborrelli · 2022-12-28T16:16:26Z

Another +1 for this effort. A lot of folks are moving towards managed identities that are tied to K8s service accounts in the provider's IAM system (IRSA, GCP Workload Identity, etc.)

micnncim · 2022-12-29T10:43:34Z

@wwentland Thanks a lot for the detailed testing!

@turkenh Can you take another look?

turkenh

@wwentland, thanks a lot for the detailed testing 🙌 All looks good to me!

and thanks, @micnncim, for proposing this change in the first place 🙌

micnncim mentioned this pull request Feb 6, 2022

Support Workload Identity crossplane-contrib/provider-gcp#414

Merged

2 tasks

turkenh reviewed Feb 8, 2022

View reviewed changes

turkenh mentioned this pull request Feb 24, 2022

Possibility to configure RBAC crossplane-contrib/provider-kubernetes#24

Closed

stale bot added the stale label Aug 13, 2022

github-actions bot closed this Aug 20, 2022

turkenh reopened this Sep 28, 2022

chobostar mentioned this pull request Nov 17, 2022

RBAC permissions issue in OpenShift #3443

Closed

turkenh approved these changes Dec 29, 2022

View reviewed changes

turkenh merged commit 8c77706 into crossplane:master Dec 29, 2022

micnncim deleted the service-account branch December 29, 2022 13:39

micnncim mentioned this pull request Jan 8, 2023

Add commentary for spec.serviceAccountName in ControllerConfig #3559

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use serviceAccountName for Service Account creation #2880

Use serviceAccountName for Service Account creation #2880

micnncim commented Feb 6, 2022 •

edited by turkenh

turkenh Feb 8, 2022

micnncim Feb 8, 2022 •

edited

stale bot commented Aug 13, 2022

wwentland commented Sep 6, 2022

micnncim commented Sep 16, 2022

turkenh commented Sep 28, 2022 •

edited

wwentland commented Nov 30, 2022

aquam8 commented Dec 20, 2022

stevendborrelli commented Dec 28, 2022

micnncim commented Dec 29, 2022

turkenh left a comment

Use serviceAccountName for Service Account creation #2880

Use serviceAccountName for Service Account creation #2880

Conversation

micnncim commented Feb 6, 2022 • edited by turkenh

Description of your changes

How has this code been tested

turkenh Feb 8, 2022

Choose a reason for hiding this comment

micnncim Feb 8, 2022 • edited

Choose a reason for hiding this comment

stale bot commented Aug 13, 2022

wwentland commented Sep 6, 2022

micnncim commented Sep 16, 2022

turkenh commented Sep 28, 2022 • edited

wwentland commented Nov 30, 2022

New ServiceAccount

Existing ServiceAccount

Conclusion

aquam8 commented Dec 20, 2022

stevendborrelli commented Dec 28, 2022

micnncim commented Dec 29, 2022

turkenh left a comment

Choose a reason for hiding this comment

micnncim commented Feb 6, 2022 •

edited by turkenh

micnncim Feb 8, 2022 •

edited

turkenh commented Sep 28, 2022 •

edited