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.

Sign up for GitHub

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

Jump to bottom

feat: 💥 rework and allow update of namespace policy for Gateway #927

Merged
merged 6 commits into from
Oct 10, 2023
Merged

feat: 💥 rework and allow update of namespace policy for Gateway #927

merged 6 commits into from
Oct 10, 2023

Conversation

renebarbosafl
Copy link
Contributor

@renebarbosafl renebarbosafl commented Oct 6, 2023 •
edited
Loading

What does this PR do?

Enable support for configuring allowedRoutes within a gateway Listener

Motivation

When using this helm chart and integrating with kubeVela we found that It's not working properly (was breaking http-route traits) because the default policy is hardcoded as "Same" and the helm chart does not have the option to configure it as convenience so I've added this option.

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

mloiseleur
mloiseleur requested changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

Interesting, thanks for this !

My suggestion is for convenience and is not mandatory.
For merging this PR, test on this support are needed (and mandatory).

traefik/templates/gateway.yaml Outdated Show resolved Hide resolved
@renebarbosafl
Copy link
Contributor Author

Ok, @mloiseleur I pushed a new commit with your suggestion and enabled test support.
Thank you!

darkweaver87
darkweaver87 approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM

@mloiseleur
Copy link
Contributor

mloiseleur commented Oct 10, 2023 •
edited
Loading

@renebarbosafl Since it's a parameter of Gateway, shouldn't we move experimental.kubernetesGateway.namespacePolicy to experimental.kubernetesGateway.gateway.namespacePolicy ?

EDIT: or move all experimental.kubernetesGateway.gateway.* to experimental.kubernetesGateway. ?

@renebarbosafl
Copy link
Contributor Author

renebarbosafl commented Oct 10, 2023 •
edited
Loading

@mloiseleur Sure! I pushed a new commit moving everything from experimental.kubernetesGateway.gateway.* to experimental.kubernetesGateway. Tests were updated as well.

@mloiseleur mloiseleur changed the title feat: enable support for update Listener's allowedRoute inside Gateway feat: 💥 rework and allow update of namespace policy for Gateway Oct 10, 2023
mloiseleur
mloiseleur approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

Many thanks @renebarbosafl.
It's a breaking change and it's clearly better.
Are you ok with the title of the PR ?

@renebarbosafl
Copy link
Contributor Author

You're welcome, @mloiseleur
Yes, I am ok with the title of this PR. 🚀

@traefiker traefiker merged commit b04d479 into traefik:master Oct 10, 2023
1 check passed
@mloiseleur mloiseleur mentioned this pull request Oct 23, 2023
Merged
@renebarbosafl renebarbosafl mentioned this pull request Oct 28, 2023
Merged
2 tasks
This was referenced Nov 2, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mloiseleur mloiseleur mloiseleur approved these changes

@darkweaver87 darkweaver87 darkweaver87 approved these changes

Assignees
No one assigned
Labels
kind/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@renebarbosafl @mloiseleur @darkweaver87 @traefiker