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

Add validation for image-cloning and custom-images for kubevirt #1517

Merged
merged 1 commit into from
Dec 16, 2022
Merged

Add validation for image-cloning and custom-images for kubevirt #1517

merged 1 commit into from
Dec 16, 2022

Conversation

sankalp-r
Copy link
Contributor

What this PR does / why we need it:
This PR adds validation for PVC osImageSource for kubevirt machines.
Using standard cloned images and custom-images are allowed only if the respective option is enabled.

  • If the user tries to use a standard cloned image with image-cloning disabled, validation will fail.
  • If the user tries to use a custom image with the custom image disabled, validation will fail.
  • If the user tries to use a PVC source from a namespace other than the allowed ones, validation will fail.

Which issue(s) this PR fixes:

Fixes #kubermatic/kubermatic#11234

What type of PR is this?
/kind feature

Special notes for your reviewer:

Does this PR introduce a user-facing change? Then add your Release Note here:

NONE

Documentation:

NONE

Signed-off-by: Sankalp Rangare [email protected]

@kubermatic-bot kubermatic-bot added kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesn't merit a release note. docs/none Denotes a PR that doesn't need documentation (changes). dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/virtualization Denotes a PR or issue as being assigned to SIG Virtualization. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 13, 2022
hdurand0710
hdurand0710 suggested changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@hdurand0710 hdurand0710 left a comment

Choose a reason for hiding this comment

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

looks, small changes to help debugging/support.

pkg/cloudprovider/provider/kubevirt/provider.go Outdated
switch c.OSImageSource.PVC.Namespace {
case c.Namespace:
if !c.AllowCustomImages {
return errInvalidOsImage
Copy link
Contributor

Choose a reason for hiding this comment

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

May be we should return a more specific error that explains that custom image is not allowed.
It's a bit different from being invalid.

pkg/cloudprovider/provider/kubevirt/provider.go Outdated
return nil
}

func validateKubeVirtImages(sourcePVC string, existingDiskList cdiv1beta1.DataVolumeList, config *Config) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, can we return different specific errors:

  • standard image cloning not allowed
  • custom image cloning not allowed
  • invalid OS Image
    ?
    That will help in the logs.

@hdurand0710
Copy link
Contributor

/lgtm
/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2022
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 9b3fe808c43a4e049960b4ac08601d4b02daa729

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hdurand0710, sankalp-r

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

5 similar comments
@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@sankalp-r
Copy link
Contributor Author

/hold

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2022
@sankalp-r
Copy link
Contributor Author

/hold cancel

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022
@sankalp-r
Copy link
Contributor Author

/retest

@hdurand0710
Copy link
Contributor

/hold
until DNS is fixed on dev

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022
@hdurand0710
Copy link
Contributor

/unhold
/retest

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@kubermatic-bot kubermatic-bot merged commit e7e4ba8 into kubermatic:main Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdurand0710 hdurand0710 hdurand0710 requested changes

Assignees

@hdurand0710 hdurand0710

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/none Denotes a PR that doesn't need documentation (changes). kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/virtualization Denotes a PR or issue as being assigned to SIG Virtualization. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sankalp-r @hdurand0710 @kubermatic-bot @kubermatic-triage-bot