Skip to content

tcp_proxy: remove support for v1 routes#4625

Closed
venilnoronha wants to merge 2 commits intoenvoyproxy:masterfrom
venilnoronha:remove-v1-tcp-routes
Closed

tcp_proxy: remove support for v1 routes#4625
venilnoronha wants to merge 2 commits intoenvoyproxy:masterfrom
venilnoronha:remove-v1-tcp-routes

Conversation

@venilnoronha
Copy link
Copy Markdown
Member

Description: This PR removes the previously deprecated v1 routes support in the TCP Proxy implementation. See #4622, #4487 and #4430 for more information.
Risk Level: Med
Testing: Existing tests suffice
Docs Changes: Added feature removal notice to version_history.rst
Release Notes: NA

Signed-off-by: Venil Noronha veniln@vmware.com

/cc @ggreenway @rshriram

@venilnoronha venilnoronha force-pushed the remove-v1-tcp-routes branch 2 times, most recently from 0a021ea to 72cd2e2 Compare October 5, 2018 22:47
@ggreenway
Copy link
Copy Markdown
Member

Unfortunately, (and I feel bad saying this after all the back and forth about when we can do this), I'm not sure we can delete this yet, because in FilterChainMatch (the new way of doing this), we still have:

  // [#not-implemented-hide:]
  repeated core.CidrRange source_prefix_ranges = 6;

So if we delete this, there's no way to configure routing based on source address (to the best of my understanding).

@rshriram Does this seem correct? Or am I missing something?

@mattklein123
Copy link
Copy Markdown
Member

So if we delete this, there's no way to configure routing based on source address (to the best of my understanding).

FYI this was recently asked about: #4535 (comment). Perhaps we can just get it implemented?

@venilnoronha
Copy link
Copy Markdown
Member Author

Sure, let's see what @rshriram and @PiotrSikora have to say.

@stale
Copy link
Copy Markdown

stale Bot commented Oct 15, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 15, 2018
@venilnoronha
Copy link
Copy Markdown
Member Author

not stale.

@stale stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 15, 2018
@PiotrSikora
Copy link
Copy Markdown
Contributor

IMHO, we need at least one release that supports both: the new-style filter chain matching based on source ports/IPs (#4457) and old-style v1 tcp proxy, which this PR removes.

@rshriram
Copy link
Copy Markdown
Member

sorry I missed this. Yes, we will miss source matching. But I don't think anyone is using it are they? docs are hiding source match. Istio isn't using it either for sure.
We have been saying that v1 is going to be deprecated and I view this as part of v1 removal.

@rshriram
Copy link
Copy Markdown
Member

I mean v1 api is going away.. and this is part of that process

@stale
Copy link
Copy Markdown

stale Bot commented Oct 25, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 25, 2018
@repokitteh-read-only

This comment has been minimized.

@venilnoronha
Copy link
Copy Markdown
Member Author

not stale.

@stale stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 25, 2018
@stale
Copy link
Copy Markdown

stale Bot commented Nov 1, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 1, 2018
@stale
Copy link
Copy Markdown

stale Bot commented Nov 8, 2018

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale Bot closed this Nov 8, 2018
@dio dio reopened this Dec 11, 2018
@stale stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 11, 2018
This commit removes the previously deprecated v1 routes support.

Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
@venilnoronha
Copy link
Copy Markdown
Member Author

This PR is synced with master now.

@venilnoronha
Copy link
Copy Markdown
Member Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #4625 (comment) was created by @venilnoronha.

see: more, trace.

@mattklein123
Copy link
Copy Markdown
Member

@ggreenway @PiotrSikora I haven't been following the history here. Can we merge this now?

@ggreenway
Copy link
Copy Markdown
Member

I quick check shows no change in the blockers for this: source_prefix_ranges is still not implemented.

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Dec 12, 2018

OK then I will close this for now since it's not going to get merged any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants