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: encrypt option can only choose encrypted storage class when creating VM image #93

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

a110605
Copy link
Collaborator

@a110605 a110605 commented Jan 16, 2025

Summary

  • when choosing encrypt option, user can only choose encrypted storage class in create VM image page.
  • If default storage class set to encrypted sc, use longhorn as fallback.

PR Checklists

  • Do we need to backport this PR change to the Harvester Dashboard?
    • Yes, the relevant PR is at:
  • Are backend engineers aware of UI changes?
    • Yes, the backend owner is:

Related Issue

harvester/harvester#6792

Test screenshot/video

feature_img.mov

@a110605 a110605 self-assigned this Jan 16, 2025
@a110605 a110605 changed the title fix: when choose encrypt can only choose encrypted storage class when creating VM image fix: encrypt option can only choose encrypted storage class when creating VM image Jan 16, 2025
const defaultStorage = this.$store.getters[`${ inStore }/all`](STORAGE_CLASS).find((s) => s.isDefault);

// if default sc is encrypted, use longhorn as default
return defaultStorage.isEncrypted ? LONGHORN : defaultStorage?.metadata?.name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any chance that longhorn could be deleted, or is it not deletable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Longhorn sc can be deleted from UI. But backend won't actually delete it.
cc @Vicente-Cheng
image

Copy link
Contributor

Choose a reason for hiding this comment

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

That was actually deleted but soon was recreated by someone...


defaultStorageClassName() {
const inStore = this.$store.getters['currentProduct'].inStore;
const defaultStorage = this.$store.getters[`${ inStore }/all`](STORAGE_CLASS).find((s) => s.isDefault);
Copy link
Collaborator

Choose a reason for hiding this comment

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

fallback to longhorn is there is no defaultStorage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. If there is no default SC, we can fallback to longhorn.

if (this.value.spec?.securityParameters?.cryptoOperation === ENCRYPT) {
this.storageClassName = this.encryptedStorageClasses[0]?.name || LONGHORN;
} else { // URL / FILE / DECRYPT should use default storage class
this.storageClassName = this.defaultStorageClassName();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use this.value.metadata.annotations[HCI_ANNOTATIONS.STORAGE_CLASS] to retrieve the storageClassName on line 177.
Is this value always be the same of this.$store.getters[${ inStore }/all](STORAGE_CLASS).find((s) => s.isDefault)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated in 6e9350e

@a110605 a110605 requested a review from houhoucoop January 17, 2025 06:59
Copy link
Collaborator

@houhoucoop houhoucoop left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants