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

Add to backends the options for send-proxy variants for server. #693

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@drf
Copy link
Collaborator

commented Nov 7, 2017

Fixes #164

@tamalsaha

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

That was quick! Thanks @drf. We should add a some e2e tests following the pattern here: https://github.com/appscode/voyager/tree/master/test/e2e.

I think the test can confirm that the backend can report back the client IP it received from HAProxy. We probably need to change/add few things in the test framework to make that work. I am going to keep posted.

@drf

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2017

@tamalsaha cool! I'll hold until you release those modifications, then I can rebase and add tests to the PR.

@tamalsaha tamalsaha requested review from tamalsaha and diptadas Nov 7, 2017

@tamalsaha

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

I think we can use https://github.com/pires/go-proxyproto to do the parsing.

@drf

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 8, 2017

@tamalsaha the project you linked is what traefik uses for implementing PROXY, so I guess is more than ok. I can try and dive into your test framework if you feel like everything is already in place.

@tamalsaha

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

@drf, we are looking into updating our test server to support PROXY protocol. We should have something soon.

@@ -243,6 +243,17 @@ func (c *controller) generateConfig() error {
si.AcceptProxy = true
}

if c.Ingress.SendProxyV2SSLCN() {

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Nov 9, 2017

Member

@drf , instead of applying these annotations on the Ingress, I think these annotations should be applied to the backend Service and parsed from there. You should find other examples for this in the parser.

This comment has been minimized.

Copy link
@drf

drf Nov 9, 2017

Author Collaborator

you're right, too much copypasta. I'll fix this soon.

@tamalsaha

This comment has been minimized.

Copy link
Member

commented Nov 10, 2017

@drf, we are going to add tests for PROXY protocol and merge the change so that your contribution is clear. Is that ok with you?

@tamalsaha

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

Also, fyi, we added support for echo-ing PROXY info from 6767 port of the test server.

@diptadas diptadas force-pushed the drf:send-proxy branch from 11b4873 to beee67f Nov 13, 2017

@@ -472,6 +476,21 @@ func (r Ingress) AcceptProxy() bool {
return v
}

func ProxyProtocol(version string) string {
switch version {
case "v1":

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Nov 13, 2017

Member

type ProxyProtocolVersion string

@diptadas diptadas force-pushed the drf:send-proxy branch from beee67f to 2afc49f Nov 13, 2017

@tamalsaha tamalsaha merged commit a37347a into appscode:master Nov 13, 2017

@drf

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 14, 2017

Whoa, I had some delays but I see you guys did a much better job than anything I could have done. Thanks a lot for merging this! :)

@drf drf deleted the drf:send-proxy branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.