Split the functionality in `node/mounter` into smaller packages #328

unexge · 2024-12-27T18:34:32Z

Currently, node/mounter package contains lots of logic on top of mounting, and it makes harder to change/add new mounting logic for #279. This PR tries to address that by splitting the functionality in node/mounter into smaller packages.

Changes:

Create mountpoint.Args for parsing and accessing Mountpoint args - Previously, accessing and manipulating of Mountpoint arguments was spread over to the codebase. This new mountpoint package contains all the logic related to Mountpoint arguments.
Add envprovider package for accessing environment variables - This package contains all the logic related to accessing environment variables to pass Mountpoint.
Move STS region detection into regionprovider package - This package contains all the logic related to detecting regions, which is currently just for STS to use in Pod-level identity.
Create credentialprovider package for getting AWS credentials - This package contains all the logic related to providing AWS credentials for Mountpoint. One other change with this refactor is that, now this package does not assume any path to write credentials to, and instead accepts writePath and envPath in Credentials.Dump method. This is needed as the path to write credentials will be different with SystemdMounter and upcoming PodMounter for containerization. Another change is that, previously we were writing service account tokens for IRSA to /var/lib/kubelet/plugins/s3.csi.aws.com/, now we're writing them to /var/lib/kubelet/pods/<pod-uuid>/volumes/kubernetes.io~csi/<volume-id>/credentials – which is unique for each mount and makes cleaning credentials easier. This new logic will be also required in PodMounter as Mountpoint Pods won't have hostPath mount and instead each will have its emptyDir to write credentials to.
Use newly created packages in SystemdMounter - This change basically applies all previous changes to SystemdMounter.
Ensure to link documentations if some configuration is missing
Ignore nil errors in SystemdMounter.ensureCredentialsDirExists

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

unexge · 2024-12-27T18:45:55Z

pkg/driver/driver.go

-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-	tokenFile := os.Getenv(webIdentityTokenEnv)
-	if tokenFile != "" {
-		klog.Infof("Found AWS_WEB_IDENTITY_TOKEN_FILE, syncing token")
-		go tokenFileTender(ctx, tokenFile, "/csi/token")
-	}


This logic has been moved to credentials_sts_web_identity.go. Since we enabled requiresRepublish as part of Pod-level identity support, Kubernetes will call NodePublishVolume periodically to update existing service account tokens before they expire, and the credential provider will be called as part of this method. Another reason for this change is that, this assumes a single location for service account tokens for Driver-level identity, but with containerization this won't be the case. See the note regarding credential provider in the original PR description for more context.

unexge · 2024-12-27T18:48:24Z

pkg/driver/node/credentialprovider/awsprofile/aws_profile.go

-	// If the given file exists, `os.WriteFile` just truncates it without changing it's permissions,
-	// so we need to ensure it always has the correct permissions.
-	return os.Chmod(path, awsProfileFilePerm)


renameio.WriteFile does this for us.

unexge · 2024-12-27T18:52:02Z

pkg/driver/node/mounter/systemd_mounter.go

-// Moves a parameter optionName from the options list to MP's environment variable list. We need this as options are
-// passed to the driver in a single field, but MP sometimes only supports config from environment variables.
-// Returns an updated options and environment.
-func moveOptionToEnvironmentVariables(optionName string, envName string, options []string, env []string) ([]string, []string) {
-	optionIdx := -1
-	for i, o := range options {
-		if strings.HasPrefix(o, optionName) {
-			optionIdx = i
-			break
-		}
-	}
-	if optionIdx != -1 {
-		// We can do replace here as we've just verified it has the right prefix
-		env = append(env, strings.Replace(options[optionIdx], optionName, envName, 1))
-		options = append(options[:optionIdx], options[optionIdx+1:]...)
-	}
-	return options, env
-}
-
-// method to add the user agent prefix to the Mountpoint headers
-// https://github.com/awslabs/mountpoint-s3/pull/548
-func addUserAgentToOptions(options []string, userAgent string) []string {
-	// first remove it from the options in case it's in there
-	for i := len(options) - 1; i >= 0; i-- {
-		if strings.Contains(options[i], userAgentPrefix) {
-			options = append(options[:i], options[i+1:]...)
-		}
-	}
-	// add the hard coded S3 CSI driver user agent string
-	return append(options, userAgentPrefix+"="+userAgent)
-}


The logic for moving --aws-max-attempts to env and adding user-agent to arguments has been moved to node.go as it's unrelated to mount mechanism and needs to be there for both SystemdMounter and PodMounter.

unexge · 2024-12-27T18:54:00Z

pkg/driver/node/node.go

-	volumeContext := req.GetVolumeContext()
-	if volumeContext[mounter.VolumeCtxAuthenticationSource] == mounter.AuthenticationSourcePod {
-		podID := volumeContext[mounter.VolumeCtxPodUID]
-		volumeID := req.GetVolumeId()
-		if podID != "" && volumeID != "" {
-			err := ns.credentialProvider.CleanupToken(volumeID, podID)
-			if err != nil {
-				klog.V(4).Infof("NodeStageVolume: Failed to cleanup token for pod/volume %s/%s: %v", podID, volumeID, err)
-			}
-		}
-	}


This NodeStageVolume wasn't enabled at all – we need to add STAGE_UNSTAGE_VOLUME node capability to enable – and also this cleaning of tokens has been moved to Unmount method of mounter.

unexge · 2024-12-27T18:54:42Z

pkg/driver/node/node.go

-	targetPath, err := ParseTargetPath(target)
-	if err == nil {
-		if targetPath.VolumeID != volumeID {
-			klog.V(4).Infof("NodeUnpublishVolume: Volume ID from parsed target path differs from Volume ID passed: %s (parsed) != %s (passed)", targetPath.VolumeID, volumeID)
-		} else {
-			err := ns.credentialProvider.CleanupToken(targetPath.VolumeID, targetPath.PodID)
-			if err != nil {
-				klog.V(4).Infof("NodeUnpublishVolume: Failed to cleanup token for pod/volume %s/%s: %v", targetPath.PodID, volumeID, err)
-			}
-		}
-	} else {
-		klog.V(4).Infof("NodeUnpublishVolume: Failed to parse target path %s: %v", target, err)
-	}


This clean up logic has been moved to Unmount method of mounter.

pkg/driver/node/credentialprovider/awsprofile/awsprofiletest/aws_profile.go

muddyfish · 2025-01-06T11:15:07Z

Is it possible to split this refactor our into many smaller PRs? It's quite hard to understand the context here with so many changes

unexge · 2025-01-06T11:28:17Z

Since each commit builds on top of each other, it would be hard to create lots of PRs and that's why I split them into commits and added description for each. Would it be possible to review it commit-by-commit?

Splitted out of #328. --- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. Signed-off-by: Burak Varlı <[email protected]>

…335) Splitted out of #328. --- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. Signed-off-by: Burak Varlı <[email protected]>

Splitted out of #328. This might not make sense since it's moved out of its original context, but the original motivation was that these constants was spread out to different packages and that was causing circular dependencies. For example, if you need to use `VolumeCtxBucketName` in `pkg/driver/node/mounter` package, you'd need to import `pkg/driver/node` package which would cause a circular dependency as `pkg/driver/node` imports `pkg/driver/node/mounter`. In situations like that, it's best to extract common things into a leaf package that doesn't import anything, which is what this PR does with `pkg/driver/node/volumecontext` package. It also makes finding these volume context keys easier by placing them into a singular place. --- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. --------- Signed-off-by: Burak Varlı <[email protected]>

Signed-off-by: Burak Varlı <[email protected]>

muddyfish · 2025-01-16T13:10:21Z

pkg/mountpoint/args.go

+		}
+
+		// disallow options that don't make sense in CSI
+		switch arg {


Ideally this would be a function, but fine to keep as this hasn't changed

muddyfish · 2025-01-16T13:11:56Z

pkg/mountpoint/args.go

+
+// SortedList returns ordered list of normalized arguments.
+func (a *Args) SortedList() []string {
+	return sets.List(a.args)


Can we link to the docs here (https://pkg.go.dev/k8s.io/apimachinery/pkg/util/sets#List) to show this satisfies the 'is sorted' criteria?

muddyfish · 2025-01-16T14:36:01Z

pkg/mountpoint/args.go

+
+// An Args represents arguments to be passed to Mountpoint during mount.
+type Args struct {
+	args sets.Set[string]


I feel a set is the wrong data structure here. Why aren't we using a dictionary/hashmap? Then we don't have to iterate over the entire data structure to see presence, and we don't have to do as much messing around with formatting

Not sure if Mountpoint currently uses such an argument but - dict/map is not a correct data structure to represent command line arguments IMO as you could have multiple values for same option, and also order of arguments might matter.

Like this find usage:

$ find . -name "*file*" -type f -o \ -name "*folder*" -type d

But that's being said the current implementation does not preserve original ordering and not sure if we need this behavior.

I didn't want to change original parsing function as that might cause some breaking changes if we do unintentional changes, but I think I added enough test cases now, and we should have enough confidence to make more refactor.

I think ideal data structure for this is: List[Tuple[Key, Optional[Value]]]

Either an ordered map or what you proposed sounds ideal to me as I don't think MP currently supports multiple arguments with the same name.

Address this in a separate PR: #349

muddyfish · 2025-01-16T14:41:37Z

pkg/driver/node/envprovider/provider.go

+)
+
+// An Environment represents a list of environment variables.
+type Environment = []string


This should probably be a dict or list of tuples internally

Yes I think this should be map[string]string. Making it a map would cause lots of diffs in the tests though, they were already []string and due to way type aliases work in Go, we can just use []string in place of Environment. But if we make it a map, then we would need to update all call sites to use a proper type, which then would make this PR even bigger. I think it's best to do that as a follow-up PR.

Sounds good to me

muddyfish · 2025-01-16T14:43:05Z

pkg/driver/node/regionprovider/imds.go

@@ -0,0 +1,34 @@
+package regionprovider


Has any code here changed? The removed code isn't in this commit

There is no code change (other than function name) in this package (regionprovider/{imds, provider, provider_test}.go). It just moved out of now removed pkg/driver/node/mounter/credential_provider.go

muddyfish · 2025-01-16T14:44:17Z

pkg/driver/node/regionprovider/provider.go

+//  4. Calling IMDS to detect region
+//
+// It returns [ErrUnknownRegion] if all of them fails.
+func (p *Provider) SecurityTokenService(volumeContext map[string]string, args mountpoint.Args) (string, error) {


The name here doesn't describe the functionality. From what I understand, it either returns the region to use for STS or returns an error. Perhaps getSTSRegion?

So the usage of this function is like: regionProvider.SecurityTokenService(volumeCtx, args). That's why I didn't add region to name as we'd duplicate the information, and not sure if get adds any value.

I think get adds value as otherwise it's not obvious what we're actually doing with STS. I'd be happy with regionProvider.getSTS(...)

) Previously, accessing and manipulating of Mountpoint arguments was spread over to the codebase. This new mountpoint package contains all the logic related to Mountpoint arguments. Splitted out of #328. --- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. --------- Signed-off-by: Burak Varlı <[email protected]>

unexge commented Dec 27, 2024

View reviewed changes

jiaeenie reviewed Jan 2, 2025

View reviewed changes

pkg/driver/node/credentialprovider/awsprofile/awsprofiletest/aws_profile.go Outdated Show resolved Hide resolved

This was referenced Jan 13, 2025

Move ReplaceFile function to util package from driver package #335

Merged

Move TargetPath into its own package #336

Merged

unexge mentioned this pull request Jan 14, 2025

Add volumecontext package for accessing volume context from CSI #340

Merged

unexge added 8 commits January 15, 2025 14:48

Create mountpoint.Args for parsing and accessing Mountpoint args

871f52f

Signed-off-by: Burak Varlı <[email protected]>

Add envprovider package for accessing environment variables

a2ab962

Signed-off-by: Burak Varlı <[email protected]>

Move STS region detection into regionprovider package

42b9a81

Signed-off-by: Burak Varlı <[email protected]>

Create credentialprovider package for getting AWS credentials

bb9b559

Signed-off-by: Burak Varlı <[email protected]>

Use newly created packages in SystemdMounter

a90ac16

Signed-off-by: Burak Varlı <[email protected]>

Ensure to link documentations if some configuration is missing

2136441

Signed-off-by: Burak Varlı <[email protected]>

Ignore nil errors in SystemdMounter.ensureCredentialsDirExists

bb2c4fa

Signed-off-by: Burak Varlı <[email protected]>

Remove leftover credential_provider.go

0e360f8

Signed-off-by: Burak Varlı <[email protected]>

unexge force-pushed the unexge/podmounter branch from b7e02ac to 0e360f8 Compare January 15, 2025 15:38

unexge temporarily deployed to trusted January 15, 2025 15:38 — with GitHub Actions Inactive

unexge temporarily deployed to trusted January 15, 2025 15:43 — with GitHub Actions Inactive

unexge temporarily deployed to trusted January 15, 2025 16:35 — with GitHub Actions Inactive

muddyfish reviewed Jan 16, 2025

View reviewed changes

unexge mentioned this pull request Jan 20, 2025

Create mountpoint.Args for parsing and accessing Mountpoint args #349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the functionality in `node/mounter` into smaller packages #328

Split the functionality in `node/mounter` into smaller packages #328

unexge commented Dec 27, 2024 •

edited

Loading

unexge Dec 27, 2024

unexge Dec 27, 2024

unexge Dec 27, 2024 •

edited

Loading

unexge Dec 27, 2024

unexge Dec 27, 2024

muddyfish commented Jan 6, 2025

unexge commented Jan 6, 2025

muddyfish Jan 16, 2025

muddyfish Jan 16, 2025

muddyfish Jan 16, 2025

unexge Jan 16, 2025

muddyfish Jan 17, 2025

unexge Jan 20, 2025

muddyfish Jan 16, 2025

unexge Jan 16, 2025

muddyfish Jan 17, 2025

muddyfish Jan 16, 2025

unexge Jan 16, 2025

muddyfish Jan 16, 2025

unexge Jan 16, 2025

muddyfish Jan 17, 2025

Split the functionality in node/mounter into smaller packages #328

Are you sure you want to change the base?

Split the functionality in node/mounter into smaller packages #328

Conversation

unexge commented Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unexge Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muddyfish commented Jan 6, 2025

unexge commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Split the functionality in `node/mounter` into smaller packages #328

Split the functionality in `node/mounter` into smaller packages #328

unexge commented Dec 27, 2024 •

edited

Loading

unexge Dec 27, 2024 •

edited

Loading