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

Allow uploading of ISO for creating kubernetes supported versions #9561

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vishesh92
Copy link
Member

@vishesh92 vishesh92 commented Aug 21, 2024

Description

This PR fixes #8417 by allowing the user to register an ISO by uploading from local.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 9.09091% with 120 lines in your changes missing coverage. Please review.

Project coverage is 15.55%. Comparing base (b215abc) to head (6d378b5).
Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
...bernetes/version/KubernetesVersionManagerImpl.java 22.22% 41 Missing and 1 partial ⚠️
...tUploadParamsForKubernetesSupportedVersionCmd.java 0.00% 36 Missing ⚠️
...che/cloudstack/api/AbstractGetUploadParamsCmd.java 0.00% 21 Missing ⚠️
...api/command/user/iso/GetUploadParamsForIsoCmd.java 0.00% 18 Missing ⚠️
...oudstack/api/response/GetUploadParamsResponse.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##               main    #9561     +/-   ##
===========================================
  Coverage     15.55%   15.55%             
- Complexity    12010    12015      +5     
===========================================
  Files          5500     5501      +1     
  Lines        481850   482046    +196     
  Branches      61843    59786   -2057     
===========================================
+ Hits          74935    74978     +43     
- Misses       398628   398779    +151     
- Partials       8287     8289      +2     
Flag Coverage Δ
uitests 4.16% <ø> (-0.01%) ⬇️
unittests 16.33% <9.09%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10728

@FelipeM525
Copy link
Collaborator

@vishesh92 would you mind adding some images to the description?

@shwstppr
Copy link
Contributor

@vishesh92 will this create access issues? Currently, k8s ISOs are registered as public and are accessible to all. But now will it allow linking any existing ISO which can be seen by root admin.
Also, earlier such ISOs were owned by system account to prevent direct deletion. Is there a check to prevent deletion if it is linked to a k8s version?

@vishesh92
Copy link
Member Author

@vishesh92 will this create access issues? Currently, k8s ISOs are registered as public and are accessible to all. But now will it allow linking any existing ISO which can be seen by root admin. Also, earlier such ISOs were owned by system account to prevent direct deletion. Is there a check to prevent deletion if it is linked to a k8s version?

I can add checks on the backend to ensure that the ISO has the required configuration settings. But it might be confusing for the end user.

Let me revisit the approach and directly allow uploading of ISO for k8s.

@vishesh92 vishesh92 changed the title Provide option to use existing ISOs for kubernetes supported version Allow uploading of ISO for creating kubernetes supported versions Aug 22, 2024
@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10747

@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@weizhouapache
Copy link
Member

@vishesh92
To be frank, I think this is too complicated.
We could add an API to transform a regular ISO (registered or uploaded from local) to a CKS ISO, it will be simple and cover mose cases (e.g. Update an existing ISO to support CKS)

@vishesh92
Copy link
Member Author

@vishesh92 To be frank, I think this is too complicated. We could add an API to transform a regular ISO (registered or uploaded from local) to a CKS ISO, it will be simple and cover mose cases (e.g. Update an existing ISO to support CKS)

@weizhouapache My initial PR was for linking an existing ISO with a CKS Supported version (main...shapeblue:cloudstack:k8s-version-add-iso-id-backup). @shwstppr had some concerns regarding the iso's permissions. So, I made the implementation similar to how it is there right now for templates & ISOs.

@weizhouapache
Copy link
Member

weizhouapache commented Aug 23, 2024

@vishesh92 To be frank, I think this is too complicated. We could add an API to transform a regular ISO (registered or uploaded from local) to a CKS ISO, it will be simple and cover mose cases (e.g. Update an existing ISO to support CKS)

@weizhouapache My initial PR was for linking an existing ISO with a CKS Supported version (main...shapeblue:cloudstack:k8s-version-add-iso-id-backup). @shwstppr had some concerns regarding the iso's permissions. So, I made the implementation similar to how it is there right now for templates & ISOs.

we can add some restrictions

  • only root admin can do (same as add k8s version API)
  • the ISO will be changed to the same properties as k8s ISO (e.g. owner is system account, all users can access)

I had a quick look at your initial PR, it looks ok to me

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10772

@vishesh92
Copy link
Member Author

@vishesh92 To be frank, I think this is too complicated. We could add an API to transform a regular ISO (registered or uploaded from local) to a CKS ISO, it will be simple and cover mose cases (e.g. Update an existing ISO to support CKS)

@weizhouapache My initial PR was for linking an existing ISO with a CKS Supported version (main...shapeblue:cloudstack:k8s-version-add-iso-id-backup). @shwstppr had some concerns regarding the iso's permissions. So, I made the implementation similar to how it is there right now for templates & ISOs.

we can add some restrictions

* only root admin can do (same as add k8s version API)

* the ISO will be changed to the same properties as k8s ISO (e.g. owner is system account, all users can access)

I had a quick look at your initial PR, it looks ok to me

@weizhouapache I am not sure about changing the properties of the existing ISO. I can maybe create a copy of the ISO with required ownership and properties. Do you think that would be a better approach?

Copy link

github-actions bot commented Sep 6, 2024

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@JoaoJandre JoaoJandre modified the milestones: 4.20.0.0, 4.21.0.0 Sep 10, 2024
@rohityadavcloud rohityadavcloud modified the milestones: 4.21.0.0, 4.20.1.0 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an option to upload a kubernetes ISO from Local
7 participants