Skip to content

Conversation

@Aditya-Sood
Copy link
Contributor

Since all legal tests in unmarshal_lds_test.go explicitly use a router filter as the terminal filter
Have accordingly modified the "last not terminal filter" testcase to be the "last not router filter" test

Fixes #6259
RELEASE NOTES: none

Since all legal tests in unmarshal_lds_test.go explicitly use a router filter as the terminal filter
Have accordingly modified the "last not terminal filter" testcase to be the "last not router filter" test
@Aditya-Sood
Copy link
Contributor Author

hi @easwars, could you please review the changes?

I didn't create a new NonRouterHTTPFilter type because the existing test to check if the last filter is NOT of TerminalHTTPFilter type was failing the IsRouterFilter() already
So I just updated its error message to say "is not a router filter" instead

@Aditya-Sood
Copy link
Contributor Author

hi @easwars, is there any further action required from my end?

@easwars
Copy link
Contributor

easwars commented May 18, 2023

hi @easwars, is there any further action required from my end?

Hi @Aditya-Sood
Sorry, I've been busy with something very critical. I will get to the review asap. Please expect some latency though. Thanks for understanding.

}),
wantName: v3LDSTarget,
wantErr: "is not a terminal filter",
wantErr: "is not a router filter",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be exhaustive, can we have a test where the last filter is a terminal filter, but it is just not the router filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it @easwars, will update once it's done
thank you

@github-actions
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label May 29, 2023
@Aditya-Sood
Copy link
Contributor Author

commenting to refresh bot's PR status
was occupied last week, will try to close it by this weekend
thanks

@github-actions github-actions bot removed the stale label May 30, 2023
@zasweq
Copy link
Contributor

zasweq commented May 31, 2023

As per the discussion on #6248, the xDS Client handling is correct here. What we really want to get rid of is the no-op check in newInterceptor() mentioned in discussion. Thus, I am closing this PR for now. Thank you for the contribution though!

@zasweq zasweq closed this May 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xds: Delete check for router filter being last in config selector

4 participants