Add disable path overlap validation flag #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fsero
requested review from
miguelbernadi,
joaoqalves,
tjamet and
carlosrejano
Fsero
force-pushed
the
allow_duplicates_4_4_2
branch
from
ea9a732 to
304fd94
Compare
carlosrejano
approved these changes
Jan 30, 2024
| if disablePathOverlapValidation { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Wouldn't we get the same behaviour if this check was done over the entire function call? I have not tried it, but reading the code seems we would get the same behaviour and a nicer API (no boolean parameter). Have you tested it?
|
|
||
| err = nginx.CheckIngress(duplicatedIngress) | ||
| if err == nil { | ||
| t.Errorf("expected errors but noone occurred") |
There was a problem hiding this comment.
Suggested change
| t.Errorf("expected errors but noone occurred") | |
| t.Errorf("expected errors but none occurred") |
| if err == nil { | ||
| t.Errorf("expected errors but noone occurred") | ||
| } | ||
| t.Run("if disablePathOverlap is enabled should not throw any error", func(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| t.Run("if disablePathOverlap is enabled should not throw any error", func(t *testing.T) { | |
| t.Run("when disablePathOverlap is enabled should not throw any error", func(t *testing.T) { |
Fsero
force-pushed
the
allow_duplicates_4_4_2
branch
from
304fd94 to
6a54322
Compare
|
@miguelbernadi addressed can you check? |
miguelbernadi
approved these changes
Feb 1, 2024
## What this PR does / why we need it: In kubernetes#5651 there was a request to throw an error when there are two ingresses defining the same host and path, which was implemented as part of the validation webhook. Despite of this there are clear rules on the ingress controller that describes what the controller would do in [such situation (the oldest rule wins)](https://github.com/kubernetes/ingress-nginx/blob/main/docs/how-it-works.md?plain=1#L27) Some users are relying on this validation behaviour to prevent misconfigurations, but there are use cases where allowing it, maybe temporarily, is helpful. Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9
Fsero
force-pushed
the
allow_duplicates_4_4_2
branch
from
6a54322 to
e8a10e1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
In kubernetes#5651 there was a request to throw an error when there are two ingresses defining the same host and path, which was implemented as part of the validation webhook.
Despite of this there are clear rules on the ingress controller that
describes what the controller would do in such situation (the oldest rule wins)
Some users are relying on this validation behaviour to prevent
misconfigurations, but there are use cases where allowing it, maybe temporarily, is helpful. Those use cases includes: