feat: add webhook for validating module config virtualization #571
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
yaroslavborbat
force-pushed
the
feat/mc-webhook
branch
12 times, most recently
from
fdc2c14 to
db29e92
Compare
Isteb4k
previously approved these changes
Dec 17, 2024
diafour
reviewed
Dec 17, 2024
diafour
reviewed
Dec 17, 2024
images/virtualization-artifact/pkg/controller/moduleconfig/util.go
Outdated
Show resolved
Hide resolved
diafour
reviewed
Dec 17, 2024
images/virtualization-artifact/pkg/controller/moduleconfig/util.go
Outdated
Show resolved
Hide resolved
diafour
reviewed
Dec 17, 2024
images/virtualization-artifact/pkg/controller/moduleconfig/reduce_cidrs_validator.go
Outdated
Show resolved
Hide resolved
diafour
reviewed
Dec 17, 2024
images/virtualization-artifact/pkg/controller/moduleconfig/reduce_cidrs_validator.go
Outdated
Show resolved
Hide resolved
diafour
reviewed
Dec 17, 2024
images/virtualization-artifact/pkg/controller/moduleconfig/reduce_cidrs_validator.go
Outdated
Show resolved
Hide resolved
yaroslavborbat
force-pushed
the
feat/mc-webhook
branch
from
16560b2 to
537e7a0
Compare
diafour
reviewed
Dec 20, 2024
diafour
reviewed
Dec 20, 2024
images/virtualization-artifact/pkg/controller/moduleconfig/reduce_cidrs_validator.go
Outdated
Show resolved
Hide resolved
yaroslavborbat
force-pushed
the
feat/mc-webhook
branch
from
becb13c to
7d715fc
Compare
diafour
previously approved these changes
Dec 23, 2024
yaroslavborbat
force-pushed
the
feat/mc-webhook
branch
from
b89f001 to
05daee3
Compare
Isteb4k
requested changes
Jan 9, 2025
images/virtualization-artifact/pkg/controller/moduleconfig/moduleconfig_webhook.go
Outdated
Show resolved
Hide resolved
images/virtualization-artifact/pkg/controller/moduleconfig/util.go
Outdated
Show resolved
Hide resolved
images/virtualization-artifact/pkg/controller/moduleconfig/cidrs_validator.go
Outdated
Show resolved
Hide resolved
images/virtualization-artifact/pkg/controller/moduleconfig/util.go
Outdated
Show resolved
Hide resolved
images/virtualization-artifact/pkg/controller/moduleconfig/remove_cidrs_validator.go
Outdated
Show resolved
Hide resolved
Isteb4k
requested changes
Jan 9, 2025
| for i, v := range input { | ||
| strVal, ok := v.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("failed to convert value %v to a string", v) |
There was a problem hiding this comment.
Is this internal error? Or user set wrong value to mc
There was a problem hiding this comment.
This error occurs because the user provided an incorrect value to mc. However, this scenario should be impossible since we have validation in place at the OpenAPI level.
There was a problem hiding this comment.
May be add "please report a bug" here?
Or add the error log here
Or even both are better
yaroslavborbat
force-pushed
the
feat/mc-webhook
branch
2 times, most recently
from
194ca0b to
69e7fde
Compare
Signed-off-by: yaroslavborbat <[email protected]>
yaroslavborbat
force-pushed
the
feat/mc-webhook
branch
from
3ff48f9 to
6821f61
Compare
Isteb4k
requested changes
Jan 13, 2025
| for i, v := range input { | ||
| strVal, ok := v.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("failed to convert value %v to a string", v) |
There was a problem hiding this comment.
May be add "please report a bug" here?
Or add the error log here
Or even both are better
Isteb4k
requested changes
Jan 13, 2025
|
|
||
| leases := &virtv2.VirtualMachineIPAddressLeaseList{} | ||
| if err := v.client.List(ctx, leases); err != nil { | ||
| return nil, fmt.Errorf("failed to list VirtualMachineIPAddressLeases: %w", err) |
There was a problem hiding this comment.
It's important to ask user report about our bug here or ask to try again later
And log the error
Please check other errors as well: the user must either understand that he screwed up somewhere, or understand that the error occurred on our side and he should to either try again later, or report the bug.
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.
Description
Add a webhook for ModuleConfig Virtualization
Why do we need it, and what problem does it solve?
It is forbidden to delete used CIDRs
What is the expected result?
Checklist