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

Refactor nginx ssl passthrough #614

Merged
merged 2 commits into from
Apr 20, 2017
Merged

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Apr 16, 2017

This takes ideas from https://github.com/kubermatic/k8sniff/

Why? Using nginx for passthrough implies we loose the source IP address

TODO:

  • test proxy protocol
  • verify source IP address in logs

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 16, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 45.507% when pulling e2fe3369a677b7a2e282d2e555eed4cca18bfccb on aledbf:refactor-passthrough into fc67b1d on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 45.492% when pulling e2fe3369a677b7a2e282d2e555eed4cca18bfccb on aledbf:refactor-passthrough into fc67b1d on kubernetes:master.

}
}
}
}
if len(isHTTPSfrom) > 0 {
if isHTTP {
for _, server := range isHTTPSfrom {
glog.Warningf("backend type mismatch on %v, assuming HTTP on ssl passthrough host %v", upstream.Name, server.Hostname)
Copy link
Contributor

Choose a reason for hiding this comment

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

This warning means the same backend is receiving http and https requests from the ingress controller. Any reason to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcmoraisjr yes, the current code (with the logic to set SSLPassthrough false) break SSLPassthrough.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcmoraisjr this change basically rollback #540

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcmoraisjr let me finish the PR and then we can test the haproxy controller.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcmoraisjr we really need to start with the e2e testing to avoid this regressions (this is not the first time)

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. I was only talking about the warning. I think #540 doesn't need to be rolled back, but only revert the logic?

I'll try to run e2e as well before every pr (was assuming Travis was doing it for us).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 45.516% when pulling 7285372 on aledbf:refactor-passthrough into 7f37635 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 45.523% when pulling aad8cd6 on aledbf:refactor-passthrough into 7f37635 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 45.466% when pulling 1abf036 on aledbf:refactor-passthrough into 7f37635 on kubernetes:master.

@aledbf aledbf changed the title WIP: Refactor passthrough Refactor passthrough Apr 19, 2017
@aledbf aledbf changed the title Refactor passthrough Refactor nginx passthrough Apr 19, 2017
@aledbf aledbf changed the title Refactor nginx passthrough Refactor nginx ssl passthrough Apr 19, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 46.413% when pulling 590bc0d on aledbf:refactor-passthrough into d2c7e90 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.09%) to 46.443% when pulling 590bc0d on aledbf:refactor-passthrough into d2c7e90 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.09%) to 46.443% when pulling 590bc0d on aledbf:refactor-passthrough into d2c7e90 on kubernetes:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants