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

Allow opt-out of service account token automounting #3983

Merged
merged 5 commits into from
Oct 25, 2023
Merged

Allow opt-out of service account token automounting #3983

merged 5 commits into from
Oct 25, 2023

Conversation

gilles-gosuin
Copy link
Contributor

Description

Two new Helm values have been introduced: automountServiceAccountToken and serviceAccount.automountServiceAccountToken.

The values have been willingly left commented out in the default values file and the template won't output anything if the value is not explicitly set, so that existing manifests are left unchanged. Opting out of the feature (or explicitly enabling it) requires the chart user to add the values in their values file.

Fixes #3982

Checklist

  • [? ] Unit tests updated (are there any that are applicable to this change?)
  • [x ] End user documentation updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 11, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 11, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @gilles-gosuin!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @gilles-gosuin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 11, 2023
@mloiseleur
Copy link
Contributor

mloiseleur commented Oct 11, 2023

Hello @gilles-gosuin,

Thanks for taking the time to open this PR.

It's a good idea to disable automountServiceAccountToken on ServiceAccount.
And it should be the default, as recommended by many security scanners.

But, on Deployment, external-dns needs it.
Is there really a use case where it can be useful ?

@stevehipwell
Copy link
Contributor

I can't see a use for this as Metrics Server needs the service account token to function correctly.

@gilles-gosuin
Copy link
Contributor Author

gilles-gosuin commented Oct 12, 2023

Our use case is simple: we have corporate policies that mandate that the service account token automounting feature be disabled.

In order for external-dns to function properly, Helm users who disable automounting should manually mount the token in containers that need it using the extraVolumes and extraVolumeMounts values.

@stevehipwell
Copy link
Contributor

@gilles-gosuin are you referring to ServiceAccount token volume projection?

@gilles-gosuin
Copy link
Contributor Author

@gilles-gosuin are you referring to ServiceAccount token volume projection?

I am indeed

@stevehipwell
Copy link
Contributor

@gilles-gosuin I can see the value in this for the ServiceAccount but I'm not convinced we need to support it on the Deployment given that we expect SA to Deployment to be a 1-2-1 relationship.

Also the key checking pattern is overkill for a boolean where we expect a specific behaviour. I think the value can be set to true by default and support being overridden.

@gilles-gosuin
Copy link
Contributor Author

gilles-gosuin commented Oct 17, 2023

I can see the value in this for the ServiceAccount but I'm not convinced we need to support it on the Deployment given that we expect SA to Deployment to be a 1-2-1 relationship.

To put it all in context, I'm requesting this feature because Azure Defender checks for the flag on the Deployment and creates a Security Recommendation if it is not set to false. It does not check for the flag on the ServiceAccount that is assigned to the Pod. Having the possibility to set that flag in the Deployment would allow us to have a Defender-compliant external-dns simply by updating the Helm values.

Also the key checking pattern is overkill for a boolean where we expect a specific behaviour

That's the approach I initially took, then I realized it might not be backward-compatible in some "extreme" cases. You could have for instance, someone who's facing the same issue as I am (corporate policies mandating automounting to be turned off) and who embarked on writing a webhook to handle this on the fly. eg. if the flag is not set, set it to false and mount the token manually.

@stevehipwell
Copy link
Contributor

@gilles-gosuin fair enough on the reason to support it on the Deployment, although it does seem a lot like security theater.

On the implementation side, the value key should be present and I can't see a reason why it can't be explicitly set. Alternatively the value key could be left empty and a with block could be used.

@gilles-gosuin
Copy link
Contributor Author

gilles-gosuin commented Oct 17, 2023

@gilles-gosuin fair enough on the reason to support it on the Deployment, although it does seem a lot like security theater.

It sure does indeed (and it sure is...)

On the implementation side, the value key should be present and I can't see a reason why it can't be explicitly set. Alternatively the value key could be left empty and a with block could be used.

Unfortunately, the default assumed by k8s is true; a with block will not output anything when the value is empty or false and this is not what we want.

I explained the reason why it might be problematic if a default is set in the Chart's values.yaml:

You could have for instance, someone who's facing the same issue as I am (corporate policies mandating automounting to be turned off) and who embarked on writing a webhook to handle this on the fly. eg. if the flag is not set, set it to false and mount the token manually.

Forcing a default value (which could only be true or it will definitely break most users) will modify existing manifests. This is probably NOT a major hurdle but it might break some corner use cases, as I tried to illustrate above.

@stevehipwell
Copy link
Contributor

@gilles-gosuin good point about the with behaviour. I think I'd still like to see the key defined but empty for consistency; a string test would allow for testing between false and null. I agree that in general terms unset but true is different to explicitly true, although in this case I think there is a case for it to be explicitly true.

@gilles-gosuin
Copy link
Contributor Author

So if I understand correctly:

  • default value: empty
  • if the value is empty, nothing gets output to the manifests
  • if the value is true or false, it gets output to the manifests

Is that what you have in mind?

@mloiseleur
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 24, 2023
@gilles-gosuin
Copy link
Contributor Author

I added the default (empty) values for both flags and kept the "hasKey" checks in the templates, as I can't think of a more idiomatic way of doing what's required by the specs you described.

@mloiseleur
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023
@mloiseleur
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023
Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

@gilles-gosuin could you please add an entry to the CHANGELOG for this PR.

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 25, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023
@stevehipwell
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevehipwell

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 Oct 25, 2023
@k8s-ci-robot k8s-ci-robot merged commit 6abbef1 into kubernetes-sigs:master Oct 25, 2023
15 checks passed
@gilles-gosuin gilles-gosuin deleted the opt-out-service-account-token-automouting branch October 25, 2023 19:18
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: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for opting out of the service account token automounting
4 participants