apache / cloudstack

Apache CloudStack is an opensource Infrastructure as a Service (IaaS) cloud computing platform
https://cloudstack.apache.org/
Apache License 2.0
2.1k stars 1.11k forks source link

Fix userdata append header restrictions #9575

Closed nvazquez closed 2 months ago

nvazquez commented 2 months ago

Description

This PR fixes the userdata header restrictions on the APPEND operation:

Fixes: #7918

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

nvazquez commented 2 months ago

@blueorangutan package

blueorangutan commented 2 months ago

@nvazquez 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.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 15.08%. Comparing base (1a403f1) to head (97dd3b8). Report is 9 commits behind head on 4.19.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## 4.19 #9575 +/- ## ========================================== Coverage 15.08% 15.08% - Complexity 11181 11185 +4 ========================================== Files 5406 5406 Lines 472915 473104 +189 Branches 58400 59117 +717 ========================================== + Hits 71336 71367 +31 - Misses 393637 393795 +158 Partials 7942 7942 ``` | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/9575/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/9575/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.30% <ø> (ø)` | | | [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/9575/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.80% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

blueorangutan commented 2 months ago

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

nvazquez commented 2 months ago

@blueorangutan test

blueorangutan commented 2 months ago

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

kiranchavala commented 2 months ago

@nvazquez do you mean the second userdata2 should not contain #cloud-config as header?

Userdata1

#cloud-config
chpasswd:
  list: |
    root:password
  expire: False

Userdata2

write_files:
- path: /root/CLOUD_INIT_WAS_HERE

I have tried deploying a vm with 2 userdata but it was not successfull

weizhouapache commented 2 months ago

@nvazquez do you mean the second userdata2 should not contain #cloud-config as header?

Userdata1

#cloud-config
chpasswd:
  list: |
    root:password
  expire: False

Userdata2

write_files:
- path: /root/CLOUD_INIT_WAS_HERE

I have tried deploying a vm with 2 userdata but it was not successfull

@kiranchavala I think the idea is

nvazquez commented 2 months ago

@blueorangutan package

nvazquez commented 2 months ago

@kiranchavala thanks for testing, that was an issue on the simple append in case the second cloudconfig userdata didn't have a header, I have fixed it, Can you verify on the latest packages? Building them now

blueorangutan commented 2 months ago

@nvazquez 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 commented 2 months ago

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

blueorangutan commented 2 months ago

[SF] Trillian test result (tid-11151) Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8 Total time taken: 47353 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9575-t11151-kvm-ol8.zip Smoke tests completed. 131 look OK, 2 have errors, 0 did not run Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_02_list_cpvm_vm Failure 0.05 test_ssvm.py
test_04_cpvm_internals Failure 0.05 test_ssvm.py
test_01_secure_vm_migration Error 134.38 test_vm_life_cycle.py
test_01_secure_vm_migration Error 134.38 test_vm_life_cycle.py
kiranchavala commented 2 months ago

@nvazquez @weizhouapache

Thanks for confirming

During testing found that

Test case 1 > works fine

  1. Register userdata 1
    
    #cloud-config
    chpasswd:
    list: |
    root:password
    expire: False

2. Register userdata 2

write_files:

attach userdata 1 to template with append

Deploy vm with advance settings userdata 2

Screenshot 2024-08-26 at 4 05 54 PM


Test case 2 : The user data doesn't append if we deploy the vm with manual userdata

Screenshot 2024-08-26 at 4 07 45 PM


Test case 3 : Works fine if the manual userdata contains the header #cloud-config

Screenshot 2024-08-26 at 4 10 11 PM

nvazquez commented 2 months ago

@kiranchavala I could reproduce the issue for case 2) - it is related to the parameter sent in the UI when it is not encoded, it encodes with an extra character that makes cloud-init fail as it doesn't recognize the character:

Screenshot 2024-08-26 at 10 40 53

As a workaround, I encoded the userdata first and passed it on the deploy VM manual entry

kiranchavala commented 2 months ago

@kiranchavala I could reproduce the issue for case 2) - it is related to the parameter sent in the UI when it is not encoded, it encodes with an extra character that makes cloud-init fail as it doesn't recognize the character:

Screenshot 2024-08-26 at 10 40 53

As a workaround, I encoded the userdata first and passed it on the deploy VM manual entry

Thanks @nvazquez works fine with api call

is it possible to remove the limitation from the UI ?

nvazquez commented 2 months ago

@kiranchavala I've added a fix for the UI encoding, by invoking encodeURI instead of encodeURIComponent for the base64 encoded userdata, this prevents encoding base64 special characters

Screenshot 2024-08-27 at 19 13 54

(in the example, before the fix the last == chars were encoded as well by the encodeURIComponent function)

cc @shwstppr @weizhouapache

kiranchavala commented 2 months ago

@blueorangutan package

blueorangutan commented 2 months ago

@kiranchavala 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 commented 2 months ago

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

shwstppr commented 2 months ago

@kiranchavala I've added a fix for the UI encoding, by invoking encodeURI instead of encodeURIComponent for the base64 encoded userdata, this prevents encoding base64 special characters

Screenshot 2024-08-27 at 19 13 54

(in the example, before the fix the last == chars were encoded as well by the encodeURIComponent function)

cc @shwstppr @weizhouapache

@nvazquez I tested a main branch env. I passed the following userdata in the deploy VM form,

abcdefghijklmnop

In the DB I could see the following,

mysql> select * from user_vm where id=5\G;
*************************** 1. row ***************************
               id: 5
           iso_id: NULL
     display_name: NULL
        user_data: YWJjZGVmZ2hpamtsbW5vcA==
     user_data_id: NULL
user_data_details: NULL
update_parameters: 1
     user_vm_type: NULL
1 row in set (0.00 sec)

ERROR: 
No query specified

So I'm not sure if it is an UI issue

nvazquez commented 2 months ago

@shwstppr can you retry with the userdata Kiran was using?

shwstppr commented 2 months ago

@shwstppr can you retry with the userdata Kiran was using?

@nvazquez I tried the same userdata as well,

This is what UI sent as part of the API request, image And value stored in the DB,

mysql> select * from user_vm where id=7\G;
*************************** 1. row ***************************
               id: 7
           iso_id: NULL
     display_name: NULL
        user_data: d3JpdGVfZmlsZXM6Ci0gcGF0aDogL3Jvb3QvQ0xPVURfSU5JVF9XQVNfSEVSRQ==
     user_data_id: NULL
user_data_details: NULL
update_parameters: 1
     user_vm_type: NULL
1 row in set (0.01 sec)

Anyway, since the PR is tested and merged now, we can check later if there comes any issue in the future (less likely)