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.

Sign up for GitHub

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

Jump to bottom

Subdir support #40

Merged
merged 5 commits into from
Mar 27, 2018
Merged

Subdir support #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
93c1241
subdir support
trajano Mar 27, 2018
83809c7
Additional tests
trajano Mar 27, 2018
6022eca
Fix error
trajano Mar 27, 2018
4d26126
Update tools_test.go
trajano Mar 27, 2018
4f227e9
Changed expectation
trajano Mar 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions gluster/driver/tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,26 @@ import (
)

const (
validHostnameRegex = `(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])`
validVolUriRegex = `([^:]+?):\/?([^\/]+)(/.+)?`
Copy link
Author

Choose a reason for hiding this comment

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

I am opting not to make the regex too strict, if the user decides to put in wrong input then it will just fail anyway. So I simplified it here

Copy link
Author

Choose a reason for hiding this comment

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

It will also allow special internationalized characters https://en.wikipedia.org/wiki/Internationalized_domain_name

Copy link
Owner

@sapk sapk Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It is better to fail at creation of volume and not mount and be strict to validate that.
It mostly important (at least for me) when using distributed docker like swarm because it will fail at creation of service and not try to mount (and remount and remount and ...) a false volume.
It allow for better error handling also otherwise it will be another case behind mount failling with just exit status 1. https://github.com/sapk/docker-volume-gluster/issues?utf8=%E2%9C%93&q=is%3Aissue+exit+status+1

I will fix this if you prefer for the rest thank you very much.

)

func isValidURI(volURI string) bool {
re := regexp.MustCompile(validHostnameRegex + ":.+")
re := regexp.MustCompile(validVolUriRegex)
return re.MatchString(volURI)
}

func parseVolURI(volURI string) string {
volParts := strings.Split(volURI, ":")
volServers := strings.Split(volParts[0], ",")
return fmt.Sprintf("--volfile-id='%s' -s '%s'", volParts[1], strings.Join(volServers, "' -s '"))
re := regexp.MustCompile(validVolUriRegex)
res := re.FindAllStringSubmatch(volURI, -1)
volServers := strings.Split(res[0][1], ",")
volumeId := res[0][2]
subDir := res[0][3]

if (subDir == "") {
return fmt.Sprintf("--volfile-id='%s' -s '%s'", volumeId, strings.Join(volServers, "' -s '"))
} else {
return fmt.Sprintf("--volfile-id='%s' --subdir-mount='%s' -s '%s'", volumeId, subDir, strings.Join(volServers, "' -s '"))
}
}

//GetMountName get moint point base on request and driver config (mountUniqName)
Expand Down
9 changes: 8 additions & 1 deletion gluster/driver/tools_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ func TestIsValidURI(t *testing.T) {
{"test,volume", false},
{"test,test2:volume", true},
{"192.168.1.1:volume", true},
{"192.168.1.:volume", false},
{"192.168.1.:volume", true},
Copy link
Author

Choose a reason for hiding this comment

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

this needed to change because I made the regex looser.

Copy link
Owner

@sapk sapk Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

From previous comment, this should fail

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to update the PR. However, I still think making it loose will prevent future cases when some people have atypical but valid host names or volume names. Is there a way of extracting the error message so it can be displayed rather than status code is 1?

Copy link
Author

Choose a reason for hiding this comment

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

Hopefully we'd get a release I can try so I don't have to spend the effort of creating extra volumes.

{"192.168.1.1,10.8.0.1:volume", true},
{"192.168.1.1,test2:volume", true},
{"192.168.1.1,test2:volume/subdir", true},
{"192.168.1.1,test2:/volume/subdir", true},
{"192.168.1.1,test2://volume/subdir", false},
Copy link
Author

Choose a reason for hiding this comment

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

double slashes in the beginning is not allowed from what I can tell.

{"192.168.1.1,test2:/volume", true},
}

for _, test := range tt {
Expand All @@ -36,6 +40,9 @@ func TestParseVolURI(t *testing.T) {
result string
}{
{"test:volume", "--volfile-id='volume' -s 'test'"},
{"test:/volume", "--volfile-id='volume' -s 'test'"},
{"test:/volume/subdir", "--volfile-id='volume' --subdir-mount='/subdir' -s 'test'"},
{"test:/volume/subdir/dir", "--volfile-id='volume' --subdir-mount='/subdir/dir' -s 'test'"},
{"test,test2:volume", "--volfile-id='volume' -s 'test' -s 'test2'"},
{"192.168.1.1:volume", "--volfile-id='volume' -s '192.168.1.1'"},
{"192.168.1.1,10.8.0.1:volume", "--volfile-id='volume' -s '192.168.1.1' -s '10.8.0.1'"},
Expand Down