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.

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

Remove FIXME for de-duping matches #885

Closed
wants to merge 2 commits into from

Conversation

sjberman
Copy link
Collaborator

Per the ticket, I'm not able to reproduce the issue. It may have been fixed already, so closing out the ticket and removing the FIXME. If it pops up again in the future, we can reopen.

Closes #662

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Per the ticket, I'm not able to reproduce the issue. It may have been fixed already, so closing out the ticket and removing the FIXME. If it pops up again in the future, we can reopen.
@sjberman sjberman requested a review from a team as a code owner July 19, 2023 14:56
@github-actions github-actions bot added the tech-debt Short-term pain, long-term benefit label Jul 19, 2023
@pleshakov
Copy link
Contributor

here is an example of to reproduce duplicated matches:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: test
spec:
  parentRefs:
    - name: gateway
  hostnames:
    - "cafe.example.com"
  rules:
    - matches:
        - path:
            type: PathPrefix
            value: /coffee
          headers:
            - name: version
              value: v2
      backendRefs:
        - name: coffee-v2-svc
          port: 80
    - matches: # same matches
        - path:
            type: PathPrefix
            value: /coffee
          headers:
            - name: version
              value: v2
      backendRefs:
        - name: coffee-v1-svc # different destination
          port: 80

leads to:

    location /coffee_prefix_route0 {
        internal;

        proxy_set_header Host $gw_api_compliant_host;
        proxy_pass http://default_coffee-v2-svc_80$request_uri;
    }

    location /coffee_prefix_route1 {
        internal;

        proxy_set_header Host $gw_api_compliant_host;
        proxy_pass http://default_coffee-v1-svc_80$request_uri;
    }

    location /coffee/ {
        set $http_matches "[{\"redirectPath\":\"/coffee_prefix_route0\",\"headers\":[\"version:v2\"]},{\"redirectPath\":\"/coffee_prefix_route1\",\"headers\":[\"version:v2\"]}]";
        js_content httpmatches.redirect;

    }

    location = /coffee {
        set $http_matches "[{\"redirectPath\":\"/coffee_prefix_route0\",\"headers\":[\"version:v2\"]},{\"redirectPath\":\"/coffee_prefix_route1\",\"headers\":[\"version:v2\"]}]";
        js_content httpmatches.redirect;

    }

where $http_matches includes duplicated matches (they test the same header for the same value).

if you put the conflicting rules above in different HTTPRoutes resources, you will duplicate matches too.

Also, it looks like we have a bug here. I would expect that for resolving conflicting rules, because the rule coffee-v2-svc goes before the rule coffee-1-svc in the HTTPRoute, it will win. @kate-osborn thoughts?

@sjberman
Copy link
Collaborator Author

@pleshakov Good find, I think the example you provided is different from what caused the original bug to be found. In that case, we had duplicate matches forwarding to the same place, so they were redundant. In this case, they are different backends, so technically not redundant, but more of a conflict defined by the user. The ordering may be a bug as you mentioned, and the duplicate matches should be taken care of in this case so only the winning backend is forwarded to.

Since this was intended to be an unscheduled, one-day task, I can close this PR and copy your comment into the bug so we can better schedule it in the future since there appears to be real work to be done here.

@sjberman sjberman mentioned this pull request Jul 20, 2023
@sjberman sjberman closed this Jul 20, 2023
@sjberman sjberman deleted the tech-debt/de-dupe-matches branch July 20, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Short-term pain, long-term benefit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

De-dupe http matches
3 participants