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

Add automount_service_account_token to podSpec. #57

Closed
wants to merge 2 commits into from

Conversation

sl1pm4t
Copy link
Contributor

@sl1pm4t sl1pm4t commented Aug 22, 2017

The automountServiceAccountToken attribute was added to the Kubernetes
podSpec in 1.6+ and is intended as a way to opt out of the default pre
1.6 behaviour of always mounting the service account token.
For this reason, I’ve set the default to True.

Additional Info
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/

The automountServiceAccountToken attribute was added to the Kubernetes
podSpec in 1.6+ and is intended as a way to opt out of the default pre
1.6 behaviour of always mounting the service account token.
For this reason, I’ve set the default to True.

Additional Info
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/
@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 22, 2017

BTW - the equivalent attribute should be added to the kubernetes_service_account resource.

@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 22, 2017

Oops I missed merging some of the code. This doesn't work.

@radeksimko
Copy link
Member

radeksimko commented Aug 22, 2017

Hi @sl1pm4t
thanks for the effort. We intentionally don't allow users to set this field. See full explanation at #38 (comment)

The main reason your acceptance test is passing is because you didn't change anything in the CRUD
https://github.com/terraform-providers/terraform-provider-kubernetes/blob/d1b1856337db8165cb3a23f2b3f8a42d0588f8a3/kubernetes/resource_kubernetes_pod.go#L50 or https://github.com/terraform-providers/terraform-provider-kubernetes/blob/d1b1856337db8165cb3a23f2b3f8a42d0588f8a3/kubernetes/resource_kubernetes_replication_controller.go#L85 or https://github.com/terraform-providers/terraform-provider-kubernetes/blob/004ed6b151f84eb38bc4ae9d15134d08fcca5d1c/kubernetes/resource_kubernetes_service_account.go#L72 so the argument is basically ignored despite being set in the config.

If you do modify either of these fields to true it will intermittently start failing (depending on how fast kubelet is in assigning volumes w/ tokens and/or secret after creation).

I admit I could've done much better job documenting these reasons in the code and possibly in the Readme.

Let me know if the explanation makes sense.

- Fix ACC Tests
- Handle diffs generated on volumes / volumeMounts
@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 22, 2017

@radeksimko I've fixed the implementation, and acc tests etc.

I added code to prevent intermittent diffs caused by volume and volumeMount that are added server side when the token is auto mounted. There's a small risk the check could be erroneously triggered if a user created a volume or volume_mount called default-token-* but I think the risk is small.

As for your comments from #38

From user's point of view I don't believe there's much downside to
AutomountServiceAccountToken being set to false - the user just has to be explicit. Maybe the
only downside is expectations of default behaviour when coming from kubectl and/or other tools.

What could the user do to be "explicit" - create the secret, volume and volumeMount?
That's a lot of work just to achieve what is available for free out of the box.

The user would also have to know to mount the token at the exact path that Kubernetes would auto mount it, otherwise some apps will fail randomly.
For example, I hit this when trying to use Mongo in Kubernetes following this tutorial.
The Mongo sidecar was failing to do it's work of creating the Mongo replica set, because the token was not available at the exact path Kubernetes would auto-mount it:
https://github.com/cvallance/mongo-k8s-sidecar/blob/d0035585f0c1841a05b537d1f941782e5dc37693/src/lib/k8s.js#L7

@radeksimko
Copy link
Member

radeksimko commented Aug 22, 2017

There's a small risk the check could be erroneously triggered if a user created a volume or volume_mount called default-token-* but I think the risk is small.

I did consider this, but I don't think name string-matching is a good approach. We don't want to prescribe what names should or shouldn't users use for their volumes or secrets.

That's a lot of work just to achieve what is available for free out of the box.

I agree, it's a lot of work and thanks for providing examples.

I think the best way forward would be to reach out to the upstream K8S community and propose an API field that would mark the volume and/or secret as default, so we have a reliable and clean way to detect such secrets and/or volumes.

What do you think?

@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 22, 2017

I agree an upstream improvement is the best option.
However I know from experience, not having the token auto mounting, as per expected default behaviour, creates problems that are hard to troubleshoot and isolate.

@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 29, 2017

Closing until we have an upstream enhancement.

@sl1pm4t sl1pm4t closed this Aug 29, 2017
@sl1pm4t sl1pm4t deleted the automount-token branch May 3, 2018 21:52
@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants