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

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
119 changes: 119 additions & 0 deletions pkg/mountpoint/args.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package mountpoint

import (
"strings"

"k8s.io/apimachinery/pkg/util/sets"
)

const (
ArgForeground = "--foreground"
ArgReadOnly = "--read-only"
ArgAllowOther = "--allow-other"
ArgAllowRoot = "--allow-root"
ArgRegion = "--region"
ArgCache = "--cache"
ArgUserAgentPrefix = "--user-agent-prefix"
ArgAWSMaxAttempts = "--aws-max-attempts"
)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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]]]

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Address this in a separate PR: #349

}

// ParseArgs parses given list of unnormalized and returns a normalized [Args].
func ParseArgs(passedArgs []string) Args {
args := sets.New[string]()

for _, arg := range passedArgs {
// trim left and right spaces
// trim spaces in between from multiple spaces to just one i.e. uid 1001 would turn into uid 1001
// if there is a space between, replace it with an = sign
arg = strings.Replace(strings.Join(strings.Fields(strings.Trim(arg, " ")), " "), " ", "=", -1)
// prepend -- if it's not already there
if !strings.HasPrefix(arg, "-") {
arg = "--" + arg
}

// disallow options that don't make sense in CSI
switch arg {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

case "--foreground", "-f", "--help", "-h", "--version", "-v":
continue
}

args.Insert(arg)
}

return Args{args}
}

// ParsedArgs creates [Args] from already parsed arguments by [ParseArgs].
func ParsedArgs(parsedArgs []string) Args {
return Args{args: sets.New(parsedArgs...)}
}

// Insert inserts given normalized argument to [Args] if not exists.
func (a *Args) Insert(arg string) {
a.args.Insert(arg)
}

// Value extracts value of given key, it returns extracted value and whether the key was found.
func (a *Args) Value(key string) (string, bool) {
_, val, exists := a.find(key)
return val, exists
}

// Has returns whether given key exists in [Args].
func (a *Args) Has(key string) bool {
_, _, exists := a.find(key)
return exists
}

// Remove removes given key, it returns the key's value and whether the key was found.
func (a *Args) Remove(key string) (string, bool) {
entry, val, exists := a.find(key)
if exists {
a.args.Delete(entry)
}
return val, exists
}

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

Choose a reason for hiding this comment

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

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?

}

// find tries to find given key from [Args], and returns whole entry, value and whether the key was found.
func (a *Args) find(key string) (string, string, bool) {
key, prefix := a.keysForSearch(key)

for _, arg := range a.args.UnsortedList() {
if key == arg {
return key, "", true
}

if strings.HasPrefix(arg, prefix) {
val := strings.SplitN(arg, "=", 2)[1]
return arg, val, true
}
}

return "", "", false
}

// keysForSearch returns whole key and a prefix to search for given key in [Args].
// First one is the whole key to look for without `=` at the end for option-like arguments without any value.
// Second one is a prefix with `=` at the end for arguments with value.
//
// Arguments are normalized to `--key[=value]` in [ParseArgs], here this function also makes sure
// the returned prefixes have the same prefix format for the given key.
func (a *Args) keysForSearch(key string) (string, string) {
prefix := strings.TrimSuffix(key, "=")

if !strings.HasPrefix(key, "-") {
prefix = "--" + prefix
}

return prefix, prefix + "="
}
Loading