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

feat(xds): filter-chain builder constructor require name #7131

Merged
merged 2 commits into from
Jul 5, 2023
Merged

feat(xds): filter-chain builder constructor require name #7131

merged 2 commits into from
Jul 5, 2023

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Jun 27, 2023

Replaces FilterChainNameConfigurer with the addition of the name as constructor argument for FilterChainBuilder.

Checklist prior to review

@mmorel-35 mmorel-35 changed the title feat(xds): named resources (filter-chain) builders require name feat(xds): filter-chain builder constructor require name Jun 27, 2023
@mmorel-35 mmorel-35 marked this pull request as ready for review June 27, 2023 21:16
@mmorel-35 mmorel-35 requested a review from a team as a code owner June 27, 2023 21:16
@mmorel-35 mmorel-35 requested review from slonka and bartsmykla and removed request for a team June 27, 2023 21:16
@bartsmykla
Copy link
Contributor

I'm not sure, but I think this PR description is a little bit misleading. Constructors now expect a name as a second parameter, but we are not checking if the name is not empty. I would say that we should maybe require the non empty name here (even if envoy allows empty name in filter-chains)?

@jakubdyszkiewicz wdyt?

@mmorel-35
Copy link
Contributor Author

mmorel-35 commented Jun 28, 2023

It would be nice to require a name but right now only a few filter chains has a name so I just modified the builder interface to make it visible.
To make the name mandatory, the naming logic needs to be implemented first.
The same situation will happen when it comes to the RouteBuilder

@bartsmykla
Copy link
Contributor

@mmorel-35 after #7105 landed, could you update this PR?

@lobkovilya
Copy link
Contributor

@mmorel-35 could you please introduce a string const:

const AnonymousResource = ""

and pass it to every FilterChainBuilder that doesn't have a name yet. So in the future when we want to get rid of all anonymous resources (to close #2538) we could simply search for usage of this const.

Signed-off-by: Matthieu MOREL <[email protected]>
@mmorel-35
Copy link
Contributor Author

Hi @lobkovilya ,
It's done :) !

@bartsmykla bartsmykla merged commit ea60023 into kumahq:master Jul 5, 2023
@mmorel-35 mmorel-35 deleted the issue-2538/filter-chain branch July 5, 2023 04:47
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.

3 participants