aws / secrets-store-csi-driver-provider-aws

The AWS provider for the Secrets Store CSI Driver allows you to fetch secrets from AWS Secrets Manager and AWS Systems Manager Parameter Store, and mount them into Kubernetes pods.
Apache License 2.0
459 stars 130 forks source link

feat: add default toleration to tolerate all taints #215

Open vikram-katkar opened 1 year ago

vikram-katkar commented 1 year ago

Issue #, if available:

Description of changes: Adding default toleration to the daemonSet to tolerate any taints.

Ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/

An empty key with operator Exists matches all keys, values and effects which means this will tolerate everything.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (8d3e503) 53.15% compared to head (7ce584b) 53.15%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #215 +/- ## ======================================= Coverage 53.15% 53.15% ======================================= Files 8 8 Lines 730 730 ======================================= Hits 388 388 Misses 332 332 Partials 10 10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

msardana94 commented 8 months ago

This is open for a while. Do you know @simonmarty if this can be reviewed? (not sure who will it be but saw that you contributed to one file recently so tagged you 🙂 )

simonmarty commented 8 months ago

Could you explain your use case? Helm chart configuration options can be overriden based on your needs. Why does this need to be merged back into the default configuration?

msardana94 commented 8 months ago

Yes I was able to work around this by setting the tolerations but I wasn't sure if it should be set as default? I am still new to EKS and saw this issue (https://github.com/aws/secrets-store-csi-driver-provider-aws/issues/267). If we can't set that as default, may be we can add it in troubleshooting?

I followed the docs to use csi driver provider for secrets manager and ran into this issue.

vikram-katkar commented 8 months ago

@simonmarty Most folks would probably assume the Daemon set runs on all nodes by default. If not, users might end up dealing with issues and struggle with configurations, making it a bit of a hassle to give the tool a spin.

Setting it as the default would save users from this hassle. The pros can still tweak things if they want to by using overrides.

simonmarty commented 4 months ago

I'm good to merge this based on the above info and the fact that the Secrets Store CSI Driver does the same thing in their helm chart

toretore commented 4 months ago

Another consideration before making this the default is that it works the other way when running some workloads on Fargate. When tolerating the Fargate nodes' taint, a DeamonSet will try to schedule pods on said nodes, which does not work. I've had to remove the default "tolerate all taints" from a number of chart defaults because of this (and it can be a PITA to work out exactly which magic incantations to use to get the correct value through Terraform and then Helm with their template languages).

simonmarty commented 4 months ago

@toretore Would #247 address the issue you're raising?

toretore commented 4 months ago

@simonmarty Yes, most likely.