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

Making helm charts configurable #116

Merged
merged 4 commits into from
Jan 3, 2024
Merged

Making helm charts configurable #116

merged 4 commits into from
Jan 3, 2024

Conversation

dlakhaws
Copy link
Contributor

Issue #, if available:

Description of changes:

  • Make helm chart configurable

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

@@ -39,11 +39,11 @@ const (
defaultRegionEnv = "AWS_DEFAULT_REGION"
stsEndpointsEnv = "AWS_STS_REGIONAL_ENDPOINTS"
MountS3PathEnv = "MOUNT_S3_PATH"
hostTokenPath = "HOST_TOKEN_PATH"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. I would expect another code change that reads the environment variable. Unless mountpoint is reading the env var for us. Either way make sure this works on a clean cluster with service account auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

latest commit wasn't pushed, should be there now!

if hostTokenPath == "" {
// set the default in case the env variable isn't found
hostTokenPath = "/var/lib/kubelet/plugins/s3.csi.aws.com/token"
}
if webIdentityFile != "" {

Choose a reason for hiding this comment

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

should this be == in place of != ? if webIdentityFile is set should we not use it?

Copy link
Contributor Author

@dlakhaws dlakhaws Jan 3, 2024

Choose a reason for hiding this comment

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

not quite, we're trying to set the env variable here based on the driver env (if you look at webIdentityFileEnv it comes from driver.go, this isn't something set by the customer) so if the driver is running with the web token, that should be passed here as well (but with different paths)

Copy link

@iostreamdoth iostreamdoth Jan 3, 2024

Choose a reason for hiding this comment

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

I see, so this line.
https://github.com/awslabs/mountpoint-s3-csi-driver/blob/main/pkg/driver/driver.go#L78C4-L82
I was little confused as it was pointing to /csi/token as path in driver. I assumed this might be some mechanism to override it. Thanks for clarification.

@dlakhaws dlakhaws merged commit b25d20d into main Jan 3, 2024
7 checks passed
@dlakhaws dlakhaws deleted the dl-helm-config branch January 3, 2024 18:16
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.

3 participants