✨ (go/v3) create and bind to a non-default service account #2070

estroz · 2021-03-09T00:01:04Z

As of now, all managers are deployed under the same service account in a namespace. Although this namespace is unique to the manager initially, it is a common admin pattern to deploy multiple managers under the same namespace for security reasons. This pattern becomes insecure given the current one-service-account-for-all approach, since RBAC will conflict.

This PR adds a ServiceAccount (config/rbac/service_account.yaml) and changes the default service account name from default to controller-manager such that a user can specify which service account their manager should be created in. They may change this value by updating files referencing the metadata.name value in service_account.yaml.

pkg/plugins/golang/v3: update all scaffold referencing the default service account with references to the "controller-manager" service account, specified in service_account.yaml.

Signed-off-by: Eric Stroczynski ericstroczynski@gmail.com

/kind feature

k8s-ci-robot · 2021-03-09T00:01:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: estroz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [estroz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86 · 2021-03-09T01:20:27Z

pkg/plugins/golang/v3/scaffolds/internal/templates/config/rbac/auth_proxy_role_binding.go

@@ -50,6 +50,6 @@ roleRef:
  name: proxy-role
 subjects:
 - kind: ServiceAccount
-  name: default
+  name: controller-manager


Why do we need to change the default value? Can we not only address the change without change the names of what is scaffold currently?

If the manager's namespace is changed to a one shared by more than one access-controlled object, but the service account remains as "default", RBAC already bound to "default" will be applied to the manager, on top of the manager's RBAC. This is insecure from an admin perspective. A more secure default is to create a manager-specific service account so RBAC is never polluted.

test/e2e/v3/plugin_cluster_test.go

estroz · 2021-03-09T03:46:07Z

/test pull-kubebuilder-e2e-k8s-1-15-12

camilamacedo86 · 2021-03-09T10:01:52Z

test/e2e/v3/plugin_cluster_test.go

@@ -166,7 +171,7 @@ func Run(kbc *utils.TestContext) {
 	_, err = kbc.Kubectl.Command(
 		"create", "clusterrolebinding", fmt.Sprintf("metrics-%s", kbc.TestSuffix),
 		fmt.Sprintf("--clusterrole=e2e-%s-metrics-reader", kbc.TestSuffix),
-		fmt.Sprintf("--serviceaccount=%s:default", kbc.Kubectl.Namespace))
+		fmt.Sprintf("--serviceaccount=%s:%s", kbc.Kubectl.Namespace, kbc.Kubectl.ServiceAccount))


This is the kind of PR that will require changes in the SDK side such as:

In the go e2e tests

Apply the same feature for ansible/helm

Change the e2e test for ansible and helm.

So, instead of manually change ansible and helm regards the changes in the config then, I'd like to see if we can first:

merge 1.5 ⚠️ Plugin phase 1.5 implementation #2060 / sdk alignment deps: bump kubebuilder to v3.0.0-beta.1 operator-framework/operator-sdk#4581.

then push a pr in kb to centralize the code Extract config/base plugin valid for any language/type from Golang #2015 and bump SDK with where Ansible/Helm/GO will use all the same code and let us know to close the issue: Align Helm/Ansible plugins with the changes made for Golang ( go/v3 ) operator-framework/operator-sdk#4542

For them, we get this merged. Since it will mean change the e2e test for all types on the SDK side as well.

See that how much changes like this one we merge before that, harder will be for us to align and keep maintained the SDK side.

c/c @Adirio @jmrodri

The addition of a service account is straightforward, improves default security, and is an often requested feature by users. The PR’s/steps you linked are unrelated and need a lot of review. Other PR’s aren’t being blocked by them so the argument that this one should doesn’t hold water.

So, are you planning to get it merged and as follow up;

bump kb in sdk

then, to do the config of helm and ansible plugin

then, to do changes in the tests for both + go/v2 and go/v3?

I would interpret this as: In order to apply all these changes downstream, there is a lot of work involved. Plugins phase 1.5 would enable us to reduce the wwork needed downstream to pinning to a kubebuilder version that contains a feature like this.
More like this is one of the PRs that would greatly benefit from having plugins phase 1.5 than this should be blocked by it.

Is that true though? Why would merging this now prevent ansible/helm plugins from receiving the same changes later through phase 1.5 or some other machination mentioned above? Wouldn't they just get this feature for free then?

It would definetely work the way you mention. I just meant that once phase 1.5 is implemented, the process will be much easier as changing them in one place will update all (after a kubebuilder bump). I think the point of this conversation was just to show how right now they need to be updated in 3 or 4 different places while with 1.5 would only require to be updated in one place. But by no means does that mean that it is blocked, just that it requires more work as it is right now.

Agreed. I guess my point that this PR adds a feature that doesn’t necessarily need to be added to other plugins right now wasn’t clear. For example, neither of the operator-sdk’s own plugins support component configs yet the Go plugin does, and that disparity was acceptable at the time.

Also, a PR here shouldn’t be blocked by downstream stuff.

My comment here was we will spend a lot of effort which shows unnecessary if we agree to merge the open prs. Then, if it is not urgent(can wait until Tue) why spend all this effort at all?

However, if it gets merged before that and we do not apply these changes for helm/ansible in sdk the, it means that it also needs to remember to do that as well to address the pr to sync #2015 with sdk.

Otherwise, it is fine. I am ok with the changes.

/lgtm

/hold
to let you get the chance to merge when you wish.

I hope that clarifies my comments and motivations.

If I'm not mistaken this should be merged after #2071, right? It seems to have the same changes also inside here.

By the way this also has my LGTM. Even if phase 1.5 was implemented, we would have to do this here, so that doesn't make a difference.

Adirio · 2021-03-09T13:58:27Z

pkg/plugins/golang/v3/scaffolds/internal/templates/config/manager/config.go

@@ -100,5 +100,6 @@ spec:
          requests:
            cpu: 100m
            memory: 20Mi
+      serviceAccountName: controller-manager


What about using a prefix/suffix with the project name so that multiple managers created in different projects don't conflict?

When you kustomize build config/default, the namePrefix is applied to the ServiceAccount’s name and all references to it since that name is a builtin nameReference.

estroz · 2021-03-09T22:33:19Z

/hold cancel

and changes the default service account name from default to controller-manager such that a user can specify which service account their manager should be created in. They may change this value by updating files referencing the metadata.name value in service_account.yaml. pkg/plugins/golang/v3: update all scaffold referencing the default service account with references to the "controller-manager" service account, specified in service_account.yaml. Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>

estroz · 2021-03-09T22:35:51Z

@camilamacedo86 needs another /lgtm when you get a chance.

Adirio · 2021-03-09T22:55:54Z

/lgtm

Adirio · 2021-03-09T22:56:44Z

@pwittrock I think you may want to consider this for config-gen

config-gen support for the capability introduced in kubernetes-sigs#2070

pwittrock · 2021-03-13T23:45:01Z

@Adirio

@pwittrock I think you may want to consider this for config-gen

Thx! #2085

config-gen support for the capability introduced in kubernetes-sigs#2070

**Description of the change:** - create and bind to a non-default service account - follow up : kubernetes-sigs/kubebuilder#2070 and #4626 **Motivation for the change:** - fix: Scorecard docs instructions are wrong now for Ansible/Helm - rfe: apply the same go behaviour of Go default config for Ansible/Helm - Reduce the complexities for we address kubernetes-sigs/kubebuilder#2015 - Keep Ansible/Helm/Go aligned (Align Helm/Ansible plugins with the changes made for Golang ( go/v3 )) Closes: #4644

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 9, 2021

k8s-ci-robot requested review from DirectXMan12 and mengqiy March 9, 2021 00:01

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

estroz mentioned this pull request Mar 9, 2021

Operator's Service account placement in versions 1.3.0 operator-framework/operator-sdk#4468

Closed

estroz force-pushed the feature/service-account branch from 472f314 to d406df9 Compare March 9, 2021 00:26

camilamacedo86 reviewed Mar 9, 2021

View reviewed changes

test/e2e/v3/plugin_cluster_test.go Outdated Show resolved Hide resolved

estroz force-pushed the feature/service-account branch 2 times, most recently from 8dffb8e to c6caf4d Compare March 9, 2021 03:19

estroz changed the title ~~✨ (go/v3) non-default service account~~ ✨ (go/v3) create and bind to a non-default service account Mar 9, 2021

estroz requested a review from camilamacedo86 March 9, 2021 03:25

estroz force-pushed the feature/service-account branch 2 times, most recently from 79b885b to bb756d6 Compare March 9, 2021 03:43

camilamacedo86 reviewed Mar 9, 2021

View reviewed changes

Adirio reviewed Mar 9, 2021

View reviewed changes

estroz requested review from Adirio and camilamacedo86 March 9, 2021 17:28

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2021

k8s-ci-robot assigned camilamacedo86 Mar 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2021

estroz force-pushed the feature/service-account branch from bb756d6 to c748ebd Compare March 9, 2021 22:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021

k8s-ci-robot assigned Adirio Mar 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021

k8s-ci-robot merged commit dd3942c into kubernetes-sigs:master Mar 9, 2021

This was referenced Mar 12, 2021

Ansible/Helm should also provide the specific Service Account operator-framework/operator-sdk#4644

Closed

ansible/helm - apply the service account operator-framework/operator-sdk#4653

Merged

pwittrock added a commit to pwittrock/kubebuilder that referenced this pull request Mar 13, 2021

config-gen create and bind to a non-default service account

a9164bb

config-gen support for the capability introduced in kubernetes-sigs#2070

pwittrock mentioned this pull request Mar 13, 2021

✨ config-gen create and bind to a non-default service account #2085

Closed

pwittrock added a commit to pwittrock/kubebuilder that referenced this pull request Mar 13, 2021

config-gen create and bind to a non-default service account

6748c1f

config-gen support for the capability introduced in kubernetes-sigs#2070

pwittrock added a commit to pwittrock/kubebuilder that referenced this pull request Mar 14, 2021

config-gen create and bind to a non-default service account

405d67e

config-gen support for the capability introduced in kubernetes-sigs#2070

pwittrock added a commit to pwittrock/kubebuilder that referenced this pull request Mar 14, 2021

config-gen create and bind to a non-default service account

6534249

config-gen support for the capability introduced in kubernetes-sigs#2070

estroz deleted the feature/service-account branch March 18, 2021 16:03

komish mentioned this pull request May 12, 2021

Add support for service-specific service accounts in place of default in generated kustomize manifests aws-controllers-k8s/community#777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ (go/v3) create and bind to a non-default service account #2070

✨ (go/v3) create and bind to a non-default service account #2070

estroz commented Mar 9, 2021 •

edited

Loading

k8s-ci-robot commented Mar 9, 2021

camilamacedo86 Mar 9, 2021

estroz Mar 9, 2021 •

edited

Loading

estroz commented Mar 9, 2021

camilamacedo86 Mar 9, 2021

estroz Mar 9, 2021

camilamacedo86 Mar 9, 2021

estroz Mar 9, 2021

Adirio Mar 9, 2021

estroz Mar 9, 2021

Adirio Mar 9, 2021

estroz Mar 9, 2021

camilamacedo86 Mar 9, 2021 •

edited

Loading

Adirio Mar 9, 2021

Adirio Mar 9, 2021

estroz Mar 9, 2021

estroz commented Mar 9, 2021

estroz commented Mar 9, 2021

Adirio commented Mar 9, 2021

Adirio commented Mar 9, 2021

pwittrock commented Mar 13, 2021

✨ (go/v3) create and bind to a non-default service account #2070

✨ (go/v3) create and bind to a non-default service account #2070

Conversation

estroz commented Mar 9, 2021 • edited Loading

k8s-ci-robot commented Mar 9, 2021

Choose a reason for hiding this comment

estroz Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

estroz commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Mar 9, 2021

estroz commented Mar 9, 2021

Adirio commented Mar 9, 2021

Adirio commented Mar 9, 2021

pwittrock commented Mar 13, 2021

estroz commented Mar 9, 2021 •

edited

Loading

estroz Mar 9, 2021 •

edited

Loading

camilamacedo86 Mar 9, 2021 •

edited

Loading