internal/dag: Fix comparison of include match conditions

[sunjayBhatia]

Previously includeMatchConditionsIdentical was just comparing adjacent includes and comparing conditions element by element rather than as a whole, ANDed together

Fixes #4907

[sunjayBhatia]

might do another pass on the implementation to see if we can do something more elegant

currently uses the existing conversions to dag types for match conditions that are then sorted and stringified so they can be compared

since this happens before the match conditions are deemed valid or not, this is a little odd maybe

[codecov]

Codecov Report

Merging #4931 (3e3ac65) into main (43c97d1) will increase coverage by 0.00%.
The diff coverage is 84.44%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4931   +/-   ##
=======================================
  Coverage   77.63%   77.63%           
=======================================
  Files         140      140           
  Lines       16894    16922   +28     
=======================================
+ Hits        13116    13138   +22     
- Misses       3521     3526    +5     
- Partials      257      258    +1     

Impacted Files	Coverage Δ
internal/dag/httpproxy_processor.go	92.35% <84.09%> (-0.44%)	⬇️
internal/dag/conditions.go	94.03% <100.00%> (ø)
internal/sorter/sorter.go	98.97% <0.00%> (+0.51%)	⬆️

[github-actions]

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

[sunjayBhatia]

from 37edb30

The included proxies that come before we find a duplicate are now not
marked as orphaned, which is a small behavior change.

maybe not desirable, would be good to hear what people thing about this

[skriss]

from 37edb30

The included proxies that come before we find a duplicate are now not
marked as orphaned, which is a small behavior change.

maybe not desirable, would be good to hear what people thing about this

If I'm reading it correctly, if we do end up finding a duplicate set of conditions on any two included proxies, then we end up not programming routes for any of the includes on the root HTTPProxy. Is that right? In that case, it does seem to me like all of those included proxies that didn't end up getting programmed should probably be called orphaned.

[skriss]

Possibly an easy fix would be on

contour/internal/dag/httpproxy_processor.go

Line 675 in dc0d593

delete(p.orphaned, types.NamespacedName{Name: includedProxy.Name, Namespace: includedProxy.Namespace})

instead of deleting from p.orphaned, just keep track of a list of child proxies that we want to eventually remove from p.orphaned, then if we get to

contour/internal/dag/httpproxy_processor.go

Line 677 in dc0d593

we know we didn't return early due to duplicate conditions, so go ahead and delete from p.orphaned there

[sunjayBhatia]

If I'm reading it correctly, if we do end up finding a duplicate set of conditions on any two included proxies, then we end up not programming routes for any of the includes on the root HTTPProxy. Is that right? In that case, it does seem to me like all of those included proxies that didn't end up getting programmed should probably be called orphaned.

ah good point, I was kinda thinking of the orphaned relationship similar to the Gateway/Route attachedRoutes idea so it might be ok, however in this case orphaned means a bit more than just the reference between the resources is good but that config has been generated, which you're right in this case will not be

[skriss]

If I'm reading it correctly, if we do end up finding a duplicate set of conditions on any two included proxies, then we end up not programming routes for any of the includes on the root HTTPProxy. Is that right? In that case, it does seem to me like all of those included proxies that didn't end up getting programmed should probably be called orphaned.

ah good point, I was kinda thinking of the orphaned relationship similar to the Gateway/Route attachedRoutes idea so it might be ok, however in this case orphaned means a bit more than just the reference between the resources is good but that config has been generated, which you're right in this case will not be

Yeah I think in this case it's probably safest to stick to the existing semantics, where child proxies that have an invalid include of them are called orphaned, rather than some form of attached + invalid.

[sunjayBhatia]

we can preserve some more of the includes/routes etc. that are valid by not returning early which would mean some traffic is still able to be served: 3ff0eaa

If I'm reading it correctly, if we do end up finding a duplicate set of conditions on any two included proxies, then we end up not programming routes for any of the includes on the root HTTPProxy.

this commit makes it so the valid stuff is still programmed (missed that I had it return rather than continue) so its more like other cases of invalid include match conditions

[skriss]

thanks for the updates, LGTM!