design: Reworking HTTPProxy duplicate route detection and reporting #5045

sunjayBhatia · 2023-02-04T01:15:11Z

Closes #5019.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2023-02-04T01:16:07Z

Still not exactly happy with how to report errors/status etc., would appreciate opinions on that section

codecov · 2023-02-04T01:20:44Z

Codecov Report

Merging #5045 (8c73cf3) into main (19a4c9f) will decrease coverage by 0.01%.
Report is 323 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5045      +/-   ##
==========================================
- Coverage   77.48%   77.47%   -0.01%     
==========================================
  Files         138      138              
  Lines       16904    16904              
==========================================
- Hits        13098    13097       -1     
- Misses       3551     3552       +1     
  Partials      255      255

see 1 file with indirect coverage changes

github-actions · 2023-02-28T00:21:53Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-03-16T00:22:29Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

skriss · 2023-03-16T23:49:22Z

Sorry @sunjayBhatia, with everything else going on I have not had a chance to digest this yet, but it's in the queue.

github-actions · 2023-03-31T00:20:07Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-04-22T00:19:50Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-05-22T00:20:18Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-06-06T00:22:04Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-07-06T00:25:54Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

clayton-gonsalves

In general, this LGTM. Some questions and comments.

However, I do worry about silently breaking folks/adding possible perf implications when we do roll this out. Having a community discussion/poll regarding this should alleviate this.

clayton-gonsalves · 2023-08-14T09:17:43Z

design/httpproxy-include-route-duplicates-design.md

+  conditions:
+  - errors:
+    - message: duplicate match conditions defined on route matching path prefix: "/foo"
+      reason: DuplicateMatchConditions
+      status: "True"
+      type: RouteError
+    lastTransitionTime: ...
+    message: At least one error present, see Errors for details
+    observedGeneration: ...
+    reason: ErrorPresent
+    status: "False"
+    type: Valid
+  currentStatus: invalid
+  description: At least one error present, see Errors for details
+  ...


nit: I cant quite remember the contour standard, but since we program the route anyway, would a warning be more apt?
just from a UX perspective an error come across as non-recoverable and implies that none of the routes were programmed.

clayton-gonsalves · 2023-08-14T09:21:38Z

design/httpproxy-include-route-duplicates-design.md

+    status: "True"
+    type: Valid
+    warnings:
+    - message: duplicate match conditions defined in included HTTPProxy "default/example-child2"


I would like to see both the "offending" child proxies here. If I think about this from the pov of a cluster admin, I would like the information of which proxies are clashing. providing just one, will leave me to hunt for the other one myself.

clayton-gonsalves · 2023-08-14T09:23:32Z

design/httpproxy-include-route-duplicates-design.md

+      status: "True"
+      type: IncludeError
+  currentStatus: invalid
+  description: Valid HTTPProxy


in this case will the child proxies have any error/warning conditions set?

clayton-gonsalves · 2023-08-14T09:26:23Z

design/httpproxy-include-route-duplicates-design.md

+
+This might lead to complications in accounting where routes came from which could get unwieldy to implement, but some combination of this and the currently proposed solution could be used.
+
+### Add a name field to routes


this does seem appealing. can see a few usecases where having the name can be helpful.

clayton-gonsalves · 2023-08-14T09:28:08Z

design/httpproxy-include-route-duplicates-design.md

+### Exposing match conditions from parent HTTPProxies down to child status
+We propose adding details of the full set of duplicate match conditions to be included in Status Conditions detailing why a route is invalid.
+This may be information not explicitly known/shared with the owner of the child HTTPProxy.
+Since a backend configured on such a route will receive a request with all of that information, I do not think it is a security concern to include that detail in most cases, but something to think about in case any organizations have an issue with this.


I dont think it should be a problem, but adding a contour config flag to toggle this would be nice.

github-actions · 2023-09-01T00:19:47Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-10-07T00:18:15Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions · 2023-11-06T00:19:39Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

design: Reworking HTTPProxy duplicate route detection and reporting

8c73cf3

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia added the release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. label Feb 4, 2023

sunjayBhatia marked this pull request as ready for review February 4, 2023 01:15

sunjayBhatia requested a review from a team as a code owner February 4, 2023 01:15

sunjayBhatia requested review from tsaarni, stevesloka and skriss and removed request for a team February 4, 2023 01:15

skriss added the kind/design Categorizes issue or PR as related to design. label Feb 13, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 28, 2023

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 1, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 16, 2023

sunjayBhatia added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 16, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 31, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2023

github-actions bot closed this May 22, 2023

sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 22, 2023

sunjayBhatia reopened this May 22, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 6, 2023

github-actions bot closed this Jul 6, 2023

sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 9, 2023

sunjayBhatia reopened this Aug 9, 2023

sunjayBhatia requested a review from clayton-gonsalves August 9, 2023 00:40

clayton-gonsalves reviewed Aug 14, 2023

View reviewed changes

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 1, 2023

sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 22, 2023

sunjayBhatia mentioned this pull request Sep 22, 2023

Should we sort routes produced by HTTPPRoxy #5773

Closed

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 7, 2023

github-actions bot closed this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

design: Reworking HTTPProxy duplicate route detection and reporting #5045

design: Reworking HTTPProxy duplicate route detection and reporting #5045

sunjayBhatia commented Feb 4, 2023 •

edited by skriss

Loading

sunjayBhatia commented Feb 4, 2023

codecov bot commented Feb 4, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023

github-actions bot commented Mar 16, 2023

skriss commented Mar 16, 2023

github-actions bot commented Mar 31, 2023

github-actions bot commented Apr 22, 2023

github-actions bot commented May 22, 2023

github-actions bot commented Jun 6, 2023

github-actions bot commented Jul 6, 2023

clayton-gonsalves left a comment

clayton-gonsalves Aug 14, 2023

clayton-gonsalves Aug 14, 2023

clayton-gonsalves Aug 14, 2023

clayton-gonsalves Aug 14, 2023

clayton-gonsalves Aug 14, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Oct 7, 2023

github-actions bot commented Nov 6, 2023


		This might lead to complications in accounting where routes came from which could get unwieldy to implement, but some combination of this and the currently proposed solution could be used.

		### Add a name field to routes

design: Reworking HTTPProxy duplicate route detection and reporting #5045

design: Reworking HTTPProxy duplicate route detection and reporting #5045

Conversation

sunjayBhatia commented Feb 4, 2023 • edited by skriss Loading

sunjayBhatia commented Feb 4, 2023

codecov bot commented Feb 4, 2023 • edited Loading

Codecov Report

github-actions bot commented Feb 28, 2023

github-actions bot commented Mar 16, 2023

skriss commented Mar 16, 2023

github-actions bot commented Mar 31, 2023

github-actions bot commented Apr 22, 2023

github-actions bot commented May 22, 2023

github-actions bot commented Jun 6, 2023

github-actions bot commented Jul 6, 2023

clayton-gonsalves left a comment

Choose a reason for hiding this comment

clayton-gonsalves Aug 14, 2023

Choose a reason for hiding this comment

clayton-gonsalves Aug 14, 2023

Choose a reason for hiding this comment

clayton-gonsalves Aug 14, 2023

Choose a reason for hiding this comment

clayton-gonsalves Aug 14, 2023

Choose a reason for hiding this comment

clayton-gonsalves Aug 14, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 1, 2023

github-actions bot commented Oct 7, 2023

github-actions bot commented Nov 6, 2023

sunjayBhatia commented Feb 4, 2023 •

edited by skriss

Loading

codecov bot commented Feb 4, 2023 •

edited

Loading