-
Notifications
You must be signed in to change notification settings - Fork 496
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
[GEP-30] Rework API Server Proxy to Drop Proxy Protocol #10793
Conversation
Welcome @knht! |
Hi @knht. Thanks for your PR. I'm waiting for a gardener member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/assign |
/assign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing this enhancement proposal.
Please find below the first round of feedback.
863b8cf
to
06ce19d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for bearing with my feedback and applying it. I had a second read through the GEP and the major remaining issues from my point of view are the following:
- Reuse of the VPN infrastructure including the existing port
- Rollout
The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
You can:
/lifecycle stale |
/remove-lifecycle stale I will take over this PR and the GEP implementation (in the new year though) |
The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
You can:
/lifecycle stale |
/remove-lifecycle stale |
feff807
to
85acddb
Compare
I incorporated all feedback into the GEP. The diagrams still need to be updated though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for taking the time to rework the proposal. It looks very good now.
I only have a few minor remarks.
c37f68e
to
59c46c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ScheererJ thanks for your feedback and patience :)
I addressed the latest round of feedback in a new commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Looks like a great unification and I like that we get rid of the separate ext-authz-server.
LGTM label has been added. Git tree hash: 7662bc2c3358898d623a51098ac04e7aaf35160a
|
3a056a8
to
d9a3966
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the changes.
/lgtm
/approve
LGTM label has been added. Git tree hash: 4c0d8e832710e0511c2439b6eab636cabc03586e
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DockToFuture, ScheererJ 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 |
How to categorize this PR?
/area networking
/kind enhancement
What this PR does / why we need it:
This GEP proposes to rework the API server proxy to use HTTP CONNECT instead of the proxy protocol, so that we can support more scenarios like using the ACL extension with opaque (non-transparent) LoadBalancers.
Which issue(s) this PR fixes:
Part of #11214
Special notes for your reviewer:
Release note: