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 service account token secret #160

Merged
merged 1 commit into from
May 1, 2024
Merged

Add service account token secret #160

merged 1 commit into from
May 1, 2024

Conversation

usimd
Copy link
Contributor

@usimd usimd commented Apr 23, 2024

Adds a new secret to be compatible with EKS >= 1.24 (see https://aws.github.io/aws-eks-best-practices/security/docs/iam/).

This closes #143 and closes #140.

Open design questions:

  • put token secret in dedicated file?
  • make secret name configurable?
  • add token conditionally (for example only for versions later than 1.24 based on KubeVersion?
  • add specific flag to toggle token secret creation?

Let me know what you think 😃

@@ -14,3 +14,11 @@ stringData:
s3BucketName: {{ .Values.daemonset.s3BucketName }}
s3Region: {{ .Values.daemonset.s3Region }}
{{- end }}
---
apiVersion: v1
Copy link
Collaborator

@No9 No9 Apr 26, 2024

Choose a reason for hiding this comment

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

Can this be put behind a flag such as

{{- if .Values.daemonset.useServiceAccountToken }}
apiVersion: v1
.....

With the value set to false in the values.yaml
https://github.com/IBM/core-dump-handler/blob/main/charts/core-dump-handler/values.yaml#L60

useServiceAccountToken: false

If this is mandatory now for all AWS instances can you set the value set to true in aws.values.yaml?

Copy link
Contributor Author

@usimd usimd Apr 28, 2024

Choose a reason for hiding this comment

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

Thanks for your feedback! I've moved the secret to a dedicated file and added the flag, but to the serviceAccount section, seemed more intuitive to me.

Also want to outline, that this does not seem to be limited to AWS (https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#no-really-you-must-read-this-before-you-upgrade-1). I can't test/verify this for any other environment, have no access to any. But if it turns out it is required, I could add the respective config to the other values files.

Signed-off-by: usimd <11619247+usimd@users.noreply.github.com>
@No9 No9 merged commit b8ea795 into IBM:main May 1, 2024
3 checks passed
@No9
Copy link
Collaborator

No9 commented May 1, 2024

@usimd Thank you very much for this work.

@usimd
Copy link
Contributor Author

usimd commented May 2, 2024

Thank you for sharing this work, @No9! Much appreciated 👍

Would it be possible to create a new release with this PR in place? Or are there other changes you're waiting for?

@No9
Copy link
Collaborator

No9 commented May 2, 2024

Yes I want to try and land this before the next release
#157

@usimd
Copy link
Contributor Author

usimd commented Sep 10, 2024

Hey @No9 👋
Since the other PR (and a few more) have been merged, I was wondering whether there was a bug in the release process that's preventing a new version. Or are you waiting for something else? It's been awhile since v8.10.0.

@No9
Copy link
Collaborator

No9 commented Sep 16, 2024

Hey @usimd
I'm crushed for time right now but I plan to get this out by the end of the month.

@sorind-broadsign
Copy link

Hello guys, sorry for the nudge. I understand @No9 you are busy but do you have an estimate when the next release is out. I'm waiting for the fix in this PR.
I can try to build the chart myself. Should I build the docker image as well ? Or can I use the 8.10.0 image ?

@sorind-broadsign
Copy link

Much appreciated @No9 , cheers to everyone working on this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment error fails to EKS Kubernetes 1.23+ does not mount service account token anymore
3 participants