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

DOCS: Add clarification regarding ssl passthrough #2377

Merged
merged 1 commit into from
Apr 19, 2018
Merged

DOCS: Add clarification regarding ssl passthrough #2377

merged 1 commit into from
Apr 19, 2018

Conversation

r7vme
Copy link

@r7vme r7vme commented Apr 19, 2018

What this PR does / why we need it:

This PR adds clarification regarding using --enable-ssl-passthrough flag if external load balances are used.

I've plunged into the issue with Go-based SSL proxying done by ingress-controller. Finally i found out that i can safely disable built-in SSL proxying, if i already have proxy protocol enabled on load balancer. See my last comment for details.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
related: #2354

Special notes for your reviewer:
None

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 19, 2018
Copy link
Contributor

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

Sorry for being picky with typos, just trying to avoid future typo-only PRs :)

@@ -348,7 +348,7 @@ The annotation `nginx.ingress.kubernetes.io/ssl-passthrough` allows to configure
**Important:**

- Using the annotation `nginx.ingress.kubernetes.io/ssl-passthrough` invalidates all the other available annotations. This is because SSL Passthrough works in L4 (TCP).
- The use of this annotation requires the flag `--enable-ssl-passthrough` (By default it is disabled)
- The use of this annotation requires proxy protocol to be enabled in load balancer. For example enabling proxy protocol for AWS ELB described [here](https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html). If you're using ingress-controller without load balancer then the flag `--enable-ssl-passthrough` required (By default it is disabled).
Copy link
Contributor

@antoineco antoineco Apr 19, 2018

Choose a reason for hiding this comment

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

"in the load-balancer"
"P roxy P rotocol"
"for AWS ELB is described"
"flag --enable-ssl-passthroughis required"
"b y default"

Copy link
Author

Choose a reason for hiding this comment

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

done.

@r7vme
Copy link
Author

r7vme commented Apr 19, 2018

@antoineco Thanks 👍 Addressed.

@codecov-io
Copy link

Codecov Report

Merging #2377 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2377   +/-   ##
=======================================
  Coverage   39.12%   39.12%           
=======================================
  Files          73       73           
  Lines        5204     5204           
=======================================
  Hits         2036     2036           
  Misses       2878     2878           
  Partials      290      290

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 396a19b...54f1568. Read the comment docs.

@r7vme
Copy link
Author

r7vme commented Apr 19, 2018

Looks like failure is not related. Can i do anything?

@antoineco
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2018
@antoineco
Copy link
Contributor

/assign @aledbf

Copy link
Member

@aledbf aledbf left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco, r7vme

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2018
@k8s-ci-robot k8s-ci-robot merged commit 8e57a47 into kubernetes:master Apr 19, 2018
@antoineco
Copy link
Contributor

@r7vme it's being taken care of ;)
Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants