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

fix auto_allocate validation message to account for v2 ip auto allocation #52422

Merged
merged 3 commits into from
Aug 1, 2024

Conversation

ilrudie
Copy link
Contributor

@ilrudie ilrudie commented Jul 30, 2024

Please provide a description of this PR:
suppress auto_allocate message in validation if auto-allocate ip controller is enabled and service entry did not opt out

fixes #52420

…roller is enabled and service entry did not opt out

Signed-off-by: ilrudie <ian.rudie@solo.io>
@ilrudie ilrudie requested a review from a team as a code owner July 30, 2024 21:00
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 30, 2024
@@ -2777,6 +2777,15 @@ var ValidateServiceEntry = RegisterValidateFunc("ValidateServiceEntry",
}
}

// check for v2 auto IP allocation or opt-out by user
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming we have identical logic elsewhere. Can we rely on a util function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good point. such a function does already exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooof, that produces a cyclic import so I'll need to circle back on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the location of the helper functions to a new location in order to resolve the cyclic imports. @stevenctl, Can you take a look at if where I landed it is appropriate?

@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 30, 2024
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 30, 2024
if features.EnableIPAutoallocate {
v, ok := cfg.Meta.Labels[constants.EnableV2AutoAllocationLabel]
if !ok || !strings.EqualFold(v, "false") {
autoAllocation = true
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 make sure I'm clear on the logic, if EnableV2AutoAllocationLabel label is false, then autoAllocation is true? Why would it not be if EnableV2AutoAllocationLabel label is true, then autoAllocation is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a ! before the EqualFold so the logic is labelValue != false.

Generally Steven's comment to re-use the logic used elsewhere probably makes the most sense whenever I have time to resolve the cyclic import issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh duh, I overlooked that. Makes sense then. Thank you for clarifying!

Signed-off-by: ilrudie <ian.rudie@solo.io>
@ilrudie ilrudie requested a review from a team as a code owner July 31, 2024 15:33
@ilrudie ilrudie added cherrypick/master Set this label on a PR to auto-merge from a release branch to master cherrypick/release-1.23 Set this label on a PR to auto-merge it to the release-1.23 branch do-not-merge/hold Block automatic merging of a PR. and removed cherrypick/master Set this label on a PR to auto-merge from a release branch to master labels Jul 31, 2024
Signed-off-by: ilrudie <ian.rudie@solo.io>
@ilrudie ilrudie removed the do-not-merge/hold Block automatic merging of a PR. label Aug 1, 2024
@ilrudie
Copy link
Contributor Author

ilrudie commented Aug 1, 2024

/retest

@istio-testing istio-testing merged commit 5876038 into istio:master Aug 1, 2024
27 checks passed
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new pull request created: #52460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking cherrypick/release-1.23 Set this label on a PR to auto-merge it to the release-1.23 branch size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

correct validation for V2 ip auto-allocation
6 participants