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

Added configmap option to turn on proxy protocol and set_real_ip_from #81

Merged
merged 1 commit into from
Nov 29, 2016

Conversation

thetechnick
Copy link
Contributor

As discussed in #73

@pleshakov
Copy link
Contributor

@thetechnick thx for another pull request!

Couple of suggestions:

  1. The reason I suggested to use the proxy-protocol and proxy-protocol-from keys is that setting those keys results in setting several (set_real_ip_from, real_ip_header) NGINX directives and changing the listen directive. So that using fewer keys makes configuration a little easier.
    If you'd like to have the set-real-ip-from key, I suggest to also add the real-ip-header key. This way to enable proxy protocol the user has to set proxy-protocol: "True" and if the users wants to see the IP from the proxy protocol in the logs as well as in the headers to backends, he will need to set the set-real-ip-from and real_ip_header keys.
    (Also, it makes sense to add the real-ip-recursive key as it is the last remaining directive of the real ip module http:https://nginx.org/en/docs/http/ngx_http_realip_module.html)

  2. If the the real_ip_header is set to proxy_protocol and the set_real_ip_from is set to the correct address, the $remote_addr variable will contain the IP obtained via proxy protocol. Thus, it's not necessary to use $proxy_protocol_addr variable in the proxy_set_header directives.

  3. In the TestGetMapKeyAsStringSlice test, I suggest to have:

expected := []string{"1.String","2.String","3.String"}
fmt.Println(expected)
if !reflect.DeepEqual(expected, slice) {
	t.Errorf("Unexpected return value:\nGot: %v\nExpected: %v", slice, expected)
}
  1. I created a pool request Fix configmaps and annotation parsing #82 that changes GetMapKeyAs* functions and fixes a few bugs in parsing when values were set to the config regardless of the presence of keys or annotations.

@thetechnick
Copy link
Contributor Author

thetechnick commented Nov 25, 2016

@pleshakov
Sounds good

Edit:
I will add the additional settings for real_ip_headers and real_ip_recursive. I think sticking to the same settings and names that nginx uses is better, than to simplify it for the user.

@pleshakov
Copy link
Contributor

@thetechnick thx. Looks good! I added few comments about the unit test.

@pleshakov pleshakov added this to the v0.7 milestone Nov 28, 2016
@thetechnick
Copy link
Contributor Author

@pleshakov
Where did you add your comments?
I have not found them, or am I too impatient :)

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

I added few comments about the unit test.

if !exists {
t.Errorf("The key 'key' must exist in the configMap")
}
expected := []string{"1.String,2.String,3.String"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The expected value should be expected := []string{"1.String", "2.String", "3.String"}

In this case, the condition in line 176 should be changed to if !reflect.DeepEqual(expected, slice)

expected := []string{"1.String,2.String,3.String"}
t.Log(expected)
if reflect.DeepEqual(expected, slice) {
t.Errorf("Unexpected return value:\nGot: %v\nExpected: %v", slice, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to print the slices as %#v a Go-syntax representation of the value

t.Errorf("Unexpected return value:\nGot: %#v\nExpected: %#v", slice, expected)

In your case, both []string{"1.String,2.String,3.String"} and []string{"1.String", "2.String", "3.String"} are printed as [1.String 2.String 3.String], which can lead to confusion:

        convert_test.go:177: Unexpected return value:
                Got: [1.String 2.String 3.String]
                Expected: [1.String 2.String 3.String]

@pleshakov
Copy link
Contributor

@thetechnick

Where did you add your comments?
I have not found them, or am I too impatient :)

My apologies. I forget to publish the comments

@thetechnick
Copy link
Contributor Author

@pleshakov
Ok the unittest was quite broken for this one :)
fixed in latest commit

@pleshakov
Copy link
Contributor

@thetechnick Great! Could you squash the two commits?

@thetechnick
Copy link
Contributor Author

@pleshakov done

@pleshakov pleshakov merged commit 9c72c30 into nginxinc:master Nov 29, 2016
@pleshakov
Copy link
Contributor

@thetechnick thx!

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.

None yet

2 participants