Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

pwittrock
Copy link
Contributor

@pwittrock pwittrock commented Mar 13, 2021

kubebuilder alpha config-gen support for non-default service accounts -- originally introduced in #2070

From #2070 description:

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pwittrock

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 13, 2021
pkg/cli/alpha/config-gen/cmd.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 14, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 14, 2021
@pwittrock
Copy link
Contributor Author

PTAL

Copy link
Contributor

@Adirio Adirio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, we are using a fixed name for the ServiceAccount and then prefixing it with a unique prefix per manager. This approach is a bit different, unsing a user-provided name and defaulting to the KubebuilderConfigGen name.

I don't have a strong opinion on which approach is better. @estroz?

Comment on lines 183 to 186
# configure the service account used for RBAC
name: foo

# generate the service account resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found at least these 4 tabs

Suggested change
# configure the service account used for RBAC
name: foo
# generate the service account resource
# configure the service account used for RBAC
name: foo
# generate the service account resource

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2021
Comment on lines 175 to 181
serviceAccount:
# configure the service account used for RBAC
name: foo

# generate the service account resource
generate: true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate?

@estroz
Copy link
Contributor

estroz commented Mar 16, 2021

@Adirio correct. @pwittrock it seems some resources are generated with their final name, and some do not have a name prefix such that they can be kustomize edit set nameprefix'd. Is this intentional?

@pwittrock
Copy link
Contributor Author

@Adirio correct. @pwittrock it seems some resources are generated with their final name, and some do not have a name prefix such that they can be kustomize edit set nameprefix'd

What is the issue that keeps nameprefix from working? This could be an issue with the transformer plugin ordering -- does it work if you use the plugin as a base for the kustomization that does the prefix?

@pwittrock
Copy link
Contributor Author

IIRC, we are using a fixed name for the ServiceAccount and then prefixing it with a unique prefix per manager. This approach is a bit different, unsing a user-provided name and defaulting to the KubebuilderConfigGen name.

I suspect this is in part because of kustomize's inability to finely configure name transformations -- e.g. Kustomize doesn't allow a user to provide a name and then generate names from that, it only supports adding either a prefix or suffix.

@estroz
Copy link
Contributor

estroz commented Mar 18, 2021

What is the issue that keeps nameprefix from working?

@pwittrock nothing necessarily, I'm just wondering why some templates are generated with their final name and some are not such that kustomize can prefix their name. Shouldn't they all be prefixed by kustomize, or all templates have the final name, ex. {{ .Metadata.Name }}-controller-manager?

This could be an issue with the transformer plugin ordering -- does it work if you use the plugin as a base for the kustomization that does the prefix?

It seems kustomize does not apply builtin functions to resources post transformation within the same kustomize dir. In #2099 I place the transformer in config/configgen/kustomization.yaml then reference that kustomize dir from config/default/kustomization.yaml as a resource; the latter contains a name prefix and applies it successfully.

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 4, 2021
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 4, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 4, 2021
@k8s-ci-robot
Copy link
Contributor

@pwittrock: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubebuilder-test f17d444 link /test pull-kubebuilder-test
pull-kubebuilder-e2e-k8s-1-16-15 f17d444 link /test pull-kubebuilder-e2e-k8s-1-16-15
pull-kubebuilder-e2e-k8s-1-18-8 f17d444 link /test pull-kubebuilder-e2e-k8s-1-18-8
pull-kubebuilder-e2e-k8s-1-19-4 f17d444 link /test pull-kubebuilder-e2e-k8s-1-19-4
pull-kubebuilder-e2e-k8s-1-17-11 f17d444 link /test pull-kubebuilder-e2e-k8s-1-17-11
pull-kubebuilder-e2e-k8s-1-14-10 f17d444 link /test pull-kubebuilder-e2e-k8s-1-14-10
pull-kubebuilder-e2e-k8s-1-15-12 f17d444 link /test pull-kubebuilder-e2e-k8s-1-15-12

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@pwittrock pwittrock closed this Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants