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.

Sign up for GitHub

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

Jump to bottom

Allow TCPProxy to work with Includes in HTTPProxy #1655

Closed
stevesloka opened this issue Oct 8, 2019 · 5 comments · Fixed by #1761
Closed

Allow TCPProxy to work with Includes in HTTPProxy #1655

stevesloka opened this issue Oct 8, 2019 · 5 comments · Fixed by #1761
Assignees
@davecheney
Labels
priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone
1.0.0-rc.2

Comments

@stevesloka
Copy link
Member

Describe the solution you'd like
#1654 added support for TCPProxy at the root httpproxy level, this issue is to describe allowing this to happen from an include outside the root.
@stevesloka stevesloka mentioned this issue Oct 8, 2019
Merged
@davecheney davecheney modified the milestones: 1.0.0-rc.2, 1.0.0-rc.1 Oct 9, 2019
@davecheney
Copy link
Contributor

@stevesloka I think this has to slip to rc.2

@stevesloka stevesloka modified the milestones: 1.0.0-rc.1, 1.0.0-rc.2 Oct 11, 2019
@davecheney davecheney added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Oct 14, 2019
@stevesloka stevesloka self-assigned this Oct 15, 2019
@davecheney davecheney assigned davecheney and unassigned stevesloka Oct 15, 2019
@davecheney
Copy link
Contributor

Assigning to myself as I have some suggestions for a design.

@davecheney davecheney removed their assignment Oct 21, 2019
@davecheney davecheney added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Oct 21, 2019
@davecheney
Copy link
Contributor

My proposal is to implement TCPProxy inclusion following the same mechanism as IngressRoute TCPProxy delegation. Specifically;

  • There can be at most one TCPProxy defined per root HTTPProxy.
  • spec.tcpproxy can include an array of services or a include key, but not both.
  • spec.tcpproxy.include is a simplified version of spec.includes, it does have a conditions key.
  • Processing of the TCPProxy include chain continues until the first TCPProxy with non empty services key is found. Those services become the backing services for the TCPProxy for that root HTTPProxy.

@youngnick
Copy link
Member

So, it has a conditions key, but setting it doesn't really do anything? Why have it at all?

@davecheney
Copy link
Contributor

davecheney commented Oct 22, 2019 •
edited
Loading

spec.tcpproxy.include is a simplified version of spec.includes, it does have a conditions key.

I've changed the tcp of spec.tcpproxy.include to be a new type which does not have any conditions. I think we can get away with this because the key, include, hasn't changed since rc.1 and the validations we added in #1731 won't apply til rc.2 so in theory no user's yaml will be broken by this change.

@davecheney davecheney self-assigned this Oct 22, 2019
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
dde8929 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
@davecheney davecheney mentioned this issue Oct 22, 2019
Merged
3 tasks
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
b4bab1e 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
6f8baec 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
f57990d 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
2e5283f 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
189570a 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 22, 2019
@davecheney
all: permit TCPProxy inclusion
e76d4a4 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 23, 2019
@davecheney
all: permit TCPProxy inclusion
36ded8e 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Oct 23, 2019
@davecheney
all: permit TCPProxy inclusion
4f8d2a2 
Fixes projectcontour#1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit that referenced this issue Oct 23, 2019
@davecheney
all: permit TCPProxy inclusion
f86c5cb 
Fixes #1655

rc.1 shipped with a spec.tcpproxy.include key but it didn't do anything.
This PR allows a TCPProxy stanza to include the TCPProxy in another
HTTPProxy document. This mirrors the logic that was added to
IngressRoute in 0.9/0.10 (approx.)

- [ ] Update generated api and crd validations
- [ ] Update httpproxy 1.0 docs
- [ ] Add feature test

Signed-off-by: Dave Cheney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davecheney davecheney

Labels
priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Milestone
1.0.0-rc.2
Development

Successfully merging a pull request may close this issue.

all: permit TCPProxy inclusion davecheney/contour
3 participants
@davecheney @stevesloka @youngnick