Validate gateway arguments #4376

krishnasrinivas · 2017-05-19T21:17:44Z

Description

gateway endpoints validation.

Motivation and Context

fixes #4355

How Has This Been Tested?

manually

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-05-19T21:17:46Z

@krishnasrinivas, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balamurugana, @krisis and @donatello to be potential reviewers.

balamurugana · 2017-05-19T21:43:16Z

Do you mean when myhostname is the same server as the gateway is being run but bound to different interface? I think it is OK not to allow it because code will be complicated to allow it to run, but more importantly allowing such a setup would be an erroneous setup as there is no reason for a user to run it that way and would mean that user is doing a mistake.

As I mentioned in the issue, it is test setup, not some user is going to use it that way. I am OK if such kind of test setup is not allowed.

balamurugana · 2017-05-19T21:46:59Z

We'll allow endpoint to be localhost (so that it is easy for testing) but same port for both gateway and server is not allowed (fixed by anis)

minio gateway --address=localhost:9000 http://myhostname:9000 is allowed to work. Expected is it should not.

krishnasrinivas · 2017-05-19T22:37:28Z

minio gateway --address=localhost:9000 http://myhostname:9000 is allowed to work. Expected is it should not.

why @balamurugana ? I am unable to see what I am missing.

balamurugana · 2017-05-19T22:46:25Z

minio gateway --address=localhost:9000 http://myhostname:9000 is allowed to work. Expected is it should not.

why @balamurugana ? I am unable to see what I am missing.

the case is localhost and myhostname resolve to same local host but different IPs. This is not a valid case as per your comment in the description.

krishnasrinivas · 2017-05-19T22:51:20Z

the case is localhost and myhostname resolve to same local host but different IPs. This is not a valid case as per your comment in the description.

But if myhostname resolves to localhost then anis' patch already takes care of not allowing the setup.

balamurugana · 2017-05-19T23:19:59Z

But if myhostname resolves to localhost then anis' patch already takes care of not allowing the setup.

Looks like its doing, but no unit tests show that. That needs to be added. TestCreateEndpoints() uses a technique to find non-loop back IPs of local host.

harshavardhana · 2017-05-20T06:10:04Z

Can you fix the commit message typo @krishnasrinivas

vadmeste

Tested (on linux)

krisis

Please fix the typo in title of the PR.
PR's description looks more like a comment. You could record it as a comment here. The description should describe the changes and how it fixes the issue.

krisis · 2017-05-22T07:18:17Z

cmd/gateway-main.go

+	if runtime.GOOS == "darwin" {
+		host, port := mustSplitHostPort(serverAddr)
+		// On macOS, if a process already listens on LOCALIPADDR:PORT, net.Listen() falls back
+		// to IPv6 address ie minio will start listening on IPv6 address whereas another


krisis · 2017-05-22T07:18:38Z

cmd/gateway-main.go

+		// On macOS, if a process already listens on LOCALIPADDR:PORT, net.Listen() falls back
+		// to IPv6 address ie minio will start listening on IPv6 address whereas another
+		// (non-)minio process is listening on IPv4 of given port.
+		// To avoid this error sutiation we check for port availability only for macOS.


s/sutiation/situation

balamurugana

Needs to add unit test to address case
minio gateway --address=localhost:9000 http://mylocalhostname:9000 where localhost and mylocalhostname point different IPs of local hosts.

krishnasrinivas · 2017-05-22T21:30:04Z

PR's description looks more like a comment. You could record it as a comment here. The description should describe the changes and how it fixes the issue.

Oops, I don't know how it ended up in the description. It was intended to be a comment in the issue.

codecov-io · 2017-05-22T21:32:00Z

Codecov Report

Merging #4376 into master will increase coverage by 0.01%.
The diff coverage is 42.3%.

@@            Coverage Diff             @@
##           master    #4376      +/-   ##
==========================================
+ Coverage   66.49%   66.51%   +0.01%     
==========================================
  Files         178      178              
  Lines       25470    25491      +21     
==========================================
+ Hits        16937    16956      +19     
+ Misses       7395     7394       -1     
- Partials     1138     1141       +3

Impacted Files	Coverage Δ
cmd/gateway-main.go	`15.62% <42.3%> (+4.51%)`	⬆️
cmd/fs-v1-background-append.go	`78.52% <0%> (-2.69%)`	⬇️
cmd/update-notifier.go	`81.66% <0%> (+19.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18c4e5d...9d431b4. Read the comment docs.

krishnasrinivas · 2017-05-22T21:32:18Z

@balamurugana @krisis please check now

balamurugana

Test cases need a fix.

balamurugana · 2017-05-23T00:45:16Z

cmd/gateway-main_test.go

+		{":9000", "http://localhost:9001", true},
+		{"123.123.123.123:9000", "http://localhost:9000", false},
+		{":9000", "http://localhost:9000", false},
+		{":9000", nonLoopBackIP + ":9000", false},


I would recommend to validate error messages than valid bool.

we had a discussion here about this bala with @harshavardhana , the problem with doing string comparison is the code becomes brittle, i.e if we change the error message string the test case breaks. So I have kept it as "valid bool" for now. But we can have a discussion in one of the meetings so that we can consistently follow one method

balamurugana · 2017-05-23T00:50:15Z

cmd/gateway-main_test.go

+		endpointAddr string
+		valid        bool
+	}{
+		{":9000", "http://localhost:9001", true},


you could add a case where endpointAddr is a remote host. you could use example.org domains.

harshavardhana · 2017-05-24T19:14:40Z

cmd/gateway-main_test.go

 	testCases := []struct {
 		serverAddr   string
 		endpointAddr string
-		valid        bool
+		errMsg       stringB


stringB is invalid, compilation issue here.

harshavardhana · 2017-05-24T19:16:19Z

cmd/gateway-main_test.go

@@ -62,23 +62,24 @@ func TestValidateGatewayArguments(t *testing.T) {
 	}
 	nonLoopBackIP := nonLoopBackIPs.ToSlice()[0]

+	errMsg := "endpoint points to the local gateway"


You can re-purpose the message like errEndpointLoop ? which is returned by the actual function and you are just using errEndpointLoop.Error() to compare. Naked strings like this can invite issues like typo etc avoids changing at multiple places.

krishnasrinivas · 2017-05-24T20:50:19Z

I would recommend to validate error messages than valid bool.

@balamurugana had a discussion here about this with @harshavardhana , the problem with doing string comparison is the code becomes brittle, i.e if we change the error message string the test case breaks. So I have kept it as "valid bool" for now. But we can have a discussion in one of the meetings so that we can consistently follow it everywhere in our code.

krisis · 2017-05-27T04:52:38Z

cmd/gateway-main.go

-		}
-	}
+	err := validateGatewayArguments(serverAddr, endpointAddr)
+	fatalIf(err, "Unable to validate")


"Unable to validate" is not correct. We did validate and found the arguments invalid. We could say something like "Invalid arguments: custom endpoint and gateway addresses are identical."

krisis · 2017-05-27T04:54:21Z

cmd/gateway-main_test.go

+	for i, test := range testCases {
+		err := validateGatewayArguments(test.serverAddr, test.endpointAddr)
+		if test.valid && err != nil {
+			t.Errorf("Test %d expected not to return erro but got %s", i+1, err)


typo: s/erro/error

fixes minio#4355

harshavardhana · 2017-06-08T17:42:35Z

cmd/gateway-main.go

@@ -179,6 +180,36 @@ func parseGatewayEndpoint(arg string) (endPoint string, secure bool, err error)
 	}
 }

+// Validate gateway arguments.
+func validateGatewayArguments(serverAddr, endpointAddr string) error {
+	if err := CheckLocalServerAddr(serverAddr); err != nil {


@krisis can you take a look.. ?

@harshavardhana yes, this addresses the problem my PR fixes. You can close my PR in favour.

harshavardhana requested review from donatello and vadmeste May 20, 2017 01:25

vadmeste approved these changes May 20, 2017

View reviewed changes

deekoder requested review from krisis and removed request for donatello May 20, 2017 17:59

krisis requested changes May 22, 2017

View reviewed changes

balamurugana requested changes May 22, 2017

View reviewed changes

krishnasrinivas changed the title ~~Validate gateway argumetns~~ Validate gateway arguments May 22, 2017

balamurugana requested changes May 23, 2017

View reviewed changes

harshavardhana requested changes May 24, 2017

View reviewed changes

krishnasrinivas force-pushed the validate-gateway-args branch from 7d3cd07 to 64a9faf Compare May 24, 2017 19:57

harshavardhana previously approved these changes May 24, 2017

View reviewed changes

harshavardhana added the do-not-close label May 25, 2017

deekoder modified the milestone: GCS May 25, 2017

deekoder added the priority: high label May 26, 2017

krisis requested changes May 27, 2017

View reviewed changes

Validate gateway argumetns

ed319f7

fixes minio#4355

krishnasrinivas added 3 commits June 1, 2017 15:24

Refactor and add unit tests

e644bb4

fix review comments

f565b39

Change error message

9d431b4

krishnasrinivas dismissed harshavardhana’s stale review via 9d431b4 June 1, 2017 22:24

krishnasrinivas force-pushed the validate-gateway-args branch from 64a9faf to 9d431b4 Compare June 1, 2017 22:24

deekoder removed priority: high priority: low labels Jun 1, 2017

harshavardhana reviewed Jun 8, 2017

View reviewed changes

krisis mentioned this pull request Jun 8, 2017

gateway: serverAddr must be local and have a port #4504

Closed

8 tasks

harshavardhana removed the do-not-close label Jun 8, 2017

harshavardhana approved these changes Jun 8, 2017

View reviewed changes

harshavardhana merged commit 2c56788 into minio:master Jun 8, 2017

harshavardhana mentioned this pull request Jun 9, 2017

gateway throws different error messages for invalid --address argument #4503

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate gateway arguments #4376

Validate gateway arguments #4376

krishnasrinivas commented May 19, 2017 •

edited

Loading

mention-bot commented May 19, 2017

balamurugana commented May 19, 2017

balamurugana commented May 19, 2017

krishnasrinivas commented May 19, 2017

balamurugana commented May 19, 2017

krishnasrinivas commented May 19, 2017

balamurugana commented May 19, 2017

harshavardhana commented May 20, 2017

vadmeste left a comment

krisis left a comment

krisis May 22, 2017

krisis May 22, 2017

balamurugana left a comment

krishnasrinivas commented May 22, 2017

codecov-io commented May 22, 2017 •

edited

Loading

krishnasrinivas commented May 22, 2017

balamurugana left a comment

balamurugana May 23, 2017

krishnasrinivas May 24, 2017

balamurugana May 23, 2017

harshavardhana May 24, 2017

harshavardhana May 24, 2017

krishnasrinivas commented May 24, 2017

krisis May 27, 2017

krisis May 27, 2017

harshavardhana Jun 8, 2017

krisis Jun 8, 2017

Validate gateway arguments #4376

Validate gateway arguments #4376

Conversation

krishnasrinivas commented May 19, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented May 19, 2017

balamurugana commented May 19, 2017

balamurugana commented May 19, 2017

krishnasrinivas commented May 19, 2017

balamurugana commented May 19, 2017

krishnasrinivas commented May 19, 2017

balamurugana commented May 19, 2017

harshavardhana commented May 20, 2017

vadmeste left a comment

Choose a reason for hiding this comment

krisis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balamurugana left a comment

Choose a reason for hiding this comment

krishnasrinivas commented May 22, 2017

codecov-io commented May 22, 2017 • edited Loading

Codecov Report

krishnasrinivas commented May 22, 2017

balamurugana left a 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

krishnasrinivas commented May 24, 2017

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

krishnasrinivas commented May 19, 2017 •

edited

Loading

codecov-io commented May 22, 2017 •

edited

Loading