User data content size validation, and related code improvements by sureshanaparti · Pull Request #8418 · apache/cloudstack

sureshanaparti · 2023-12-29T08:19:19Z

Description

This PR validates the user data with actual length only, and has some code improvements.

Fixes #8415

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

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Manually tested managed user data registration, and instance creation from UI.

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

sureshanaparti · 2023-12-29T08:21:07Z

@blueorangutan package

blueorangutan · 2023-12-29T08:22:04Z

@sureshanaparti 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 · 2023-12-29T08:27:41Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (6d916ca) 30.85% compared to head (c95f2ce) 30.80%.
Report is 24 commits behind head on main.

Files	Patch %	Lines
...pache/cloudstack/userdata/UserDataManagerImpl.java	40.00%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8418      +/-   ##
============================================
- Coverage     30.85%   30.80%   -0.05%     
+ Complexity    34048    33979      -69     
============================================
  Files          5341     5341              
  Lines        374861   374888      +27     
  Branches      54518    54520       +2     
============================================
- Hits         115659   115491     -168     
- Misses       243973   244127     +154     
- Partials      15229    15270      +41

Flag	Coverage Δ
simulator-marvin-tests	`24.72% <33.33%> (-0.04%)`	⬇️
uitests	`4.39% <ø> (ø)`
unit-tests	`16.46% <41.66%> (+<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.

DaanHoogland

clgtm, just one functional question

blueorangutan · 2023-12-29T09:26:20Z

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

harikrishna-patnala

@sureshanaparti, along with these changes is it better to call the registerUserData API using POST from UI ?

yadvr · 2024-01-03T12:35:26Z

@sureshanaparti can you review the outstanding comments?
@blueorangutan package

blueorangutan · 2024-01-03T12:36:03Z

@rohityadavcloud 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 · 2024-01-03T14:05:06Z

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

sureshanaparti · 2024-01-04T10:52:20Z

@sureshanaparti, along with these changes is it better to call the registerUserData API using POST from UI ?

changes done

sureshanaparti · 2024-01-04T10:52:48Z

@blueorangutan package

blueorangutan · 2024-01-04T10:54:03Z

@sureshanaparti 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 · 2024-01-04T12:00:33Z

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

sureshanaparti · 2024-01-04T12:28:48Z

@blueorangutan test

blueorangutan · 2024-01-04T12:36:05Z

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

sureshanaparti · 2024-01-08T07:17:05Z

@blueorangutan package

blueorangutan · 2024-01-08T07:18:03Z

@sureshanaparti 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 · 2024-01-08T08:19:28Z

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

sureshanaparti · 2024-01-08T08:25:08Z

@blueorangutan test

blueorangutan · 2024-01-08T08:32:05Z

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

JoaoJandre · 2024-01-08T15:35:26Z

@sureshanaparti can you rebase with 4.18.2? As the related issue is also aimed at 4.18.2

JoaoJandre

Overall looks good, just a few remarks

blueorangutan · 2024-01-08T22:21:34Z

[SF] Trillian test result (tid-8750)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48273 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8418-t8750-kvm-centos7.zip
Smoke tests completed. 118 look OK, 3 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.03	test_ssvm.py
test_04_cpvm_internals	`Failure`	0.04	test_ssvm.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	614.36	test_kubernetes_clusters.py
test_08_migrate_vm	`Error`	0.06	test_vm_life_cycle.py

sureshanaparti · 2024-06-13T07:15:14Z

@JoaoJandre can you check the changes on your remarks?

@JoaoJandre pls check your remarks. thanks.

sureshanaparti · 2024-06-13T07:19:41Z

@blueorangutan package

blueorangutan · 2024-06-13T07:22:03Z

@sureshanaparti 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 · 2024-06-13T08:31:12Z

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

JoaoJandre

CLGTM, did not test it.

kiranchavala

@sureshanaparti as discussed, the userdata registration from the browser is not working

Regsiter a userdata of size 686kb

test2.zip

Error

sureshanaparti · 2024-06-13T12:36:29Z

@blueorangutan package

blueorangutan · 2024-06-13T12:38:04Z

@sureshanaparti 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 · 2024-06-13T13:48:57Z

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

kiranchavala

@sureshanaparti

As discussed, the registerUserdata api call Is failing with 400 error , if the userdata size is of 916kb

…ements

sureshanaparti · 2024-06-14T10:21:39Z

@blueorangutan package

blueorangutan · 2024-06-14T10:22:03Z

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

sureshanaparti · 2024-06-14T10:26:15Z

@sureshanaparti

As discussed, the registerUserdata api call Is failing with 400 error , if the userdata size is of 916kb

@kiranchavala Allowed size is for the userdata after base64 encoding. Updated descriptions for the cmd param and config vm.userdata.max.length.

MaxSizeInBytesAfterEncoding = (SizeInBytesBeforeEncoding / 3 * 4) + 4

So, it seems about 786432 bytes / 768 KB actual user data supported

blueorangutan · 2024-06-14T11:17:52Z

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

kiranchavala · 2024-06-14T11:24:43Z

@blueorangutan package

blueorangutan · 2024-06-14T11:26:03Z

@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 · 2024-06-14T12:30:38Z

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

sureshanaparti · 2024-06-15T07:17:11Z

@blueorangutan test

blueorangutan · 2024-06-15T07:18:03Z

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

blueorangutan · 2024-06-15T19:56:36Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2024-06-17T08:45:04Z

@kiranchavala all good now?

kiranchavala

LGTM, Followed these steps

Global setting (vm.userdata.max.length) change it to 1048576
Compute > userdata > register userdata

registering a user data

A Post request will be sent

and the logs will contain the userdata size

Configured base64 encoded user data size: 914652 bytes, actual user data size: 685987 bytes

LOGS


[root@ref-trl-6845-k-Mr8-kiran-chavala-mgmt1 ~]# cat   /var/log/cloudstack/management/management-server.log  |grep -i "logid:c0d4d084"
2024-06-14 09:26:08,934 DEBUG [c.c.a.ApiServlet] (qtp931675031-19:ctx-56f46fc4) (logid:c0d4d084) ===START===  10.0.3.251 -- POST  command=registerUserData&response=json
2024-06-14 09:26:08,935 DEBUG [c.c.a.ApiServlet] (qtp931675031-19:ctx-56f46fc4) (logid:c0d4d084) Two factor authentication is already verified for the user 2, so skipping
2024-06-14 09:26:08,943 DEBUG [c.c.a.ApiServer] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) CIDRs from which account 'Account [{"accountName":"admin","id":2,"uuid":"7669013f-2a0a-11ef-9a70-1e00da00037e"}]' is allowed to perform API calls: 0.0.0.0/0,::/0
2024-06-14 09:26:08,946 INFO  [o.a.c.a.DynamicRoleBasedAPIAccessChecker] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) Account [Account [{"accountName":"admin","id":2,"uuid":"7669013f-2a0a-11ef-9a70-1e00da00037e"}]] is Root Admin or Domain Admin, all APIs are allowed.
2024-06-14 09:26:08,947 WARN  [o.a.c.a.ProjectRoleBasedApiAccessChecker] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) Project is null, ProjectRoleBasedApiAccessChecker only applies to projects, returning API [registerUserData] for user [User {"username":"admin","uuid":"766a609c-2a0a-11ef-9a70-1e00da00037e"}.] as allowed.
2024-06-14 09:26:08,948 DEBUG [o.a.c.a.StaticRoleBasedAPIAccessChecker] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) RoleService is enabled. We will use it instead of StaticRoleBasedAPIAccessChecker.
2024-06-14 09:26:08,949 DEBUG [o.a.c.r.ApiRateLimitServiceImpl] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) API rate limiting is disabled. We will not use ApiRateLimitService.
2024-06-14 09:26:09,102 INFO  [o.a.c.u.UserDataManagerImpl] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) Configured base64 encoded user data size: 914652 bytes, actual user data size: 685987 bytes
2024-06-14 09:26:09,421 DEBUG [c.c.a.ApiServlet] (qtp931675031-19:ctx-56f46fc4 ctx-bdb563ce) (logid:c0d4d084) ===END===  10.0.3.251 -- POST  command=registerUserData&response=json

Sent a userdata of higher size > 1048576 , and a 400 exception will be thrown

…che#8418) Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com>

boring-cyborg Bot added the component:networking label Dec 29, 2023

DaanHoogland reviewed Dec 29, 2023

View reviewed changes

Comment thread engine/userdata/src/main/java/org/apache/cloudstack/userdata/UserDataManagerImpl.java

DaanHoogland reviewed Dec 29, 2023

View reviewed changes

harikrishna-patnala reviewed Dec 29, 2023

View reviewed changes

yadvr added this to the 4.19.1.0 milestone Jan 3, 2024

yadvr marked this pull request as ready for review January 3, 2024 12:35

boring-cyborg Bot added the component:UI label Jan 4, 2024

sureshanaparti changed the title ~~User data content size validation with actual length only, and some code improvements~~ User data content size validation with actual length only, register managed user data using POST call from UI, and some code improvements Jan 8, 2024

JoaoJandre reviewed Jan 8, 2024

View reviewed changes

sureshanaparti changed the title ~~User data content size validation with actual length only, register managed user data using POST call from UI, and some code improvements~~ User data content size validation with actual length only, and some code improvements Jan 10, 2024

JoaoJandre approved these changes Jun 13, 2024

View reviewed changes

kiranchavala reviewed Jun 13, 2024

View reviewed changes

Use POST call to register user data

92006d0

kiranchavala reviewed Jun 14, 2024

View reviewed changes

Check user data length with base64 encoded data, and some code improv…

49349e9

…ements

boring-cyborg Bot added the component:api label Jun 14, 2024

sureshanaparti changed the title ~~User data content size validation with actual length only, and some code improvements~~ User data content size validation, and related code improvements Jun 14, 2024

kiranchavala approved these changes Jun 18, 2024

View reviewed changes

kiranchavala removed the status:needs-testing label Jun 18, 2024

DaanHoogland merged commit 8b02624 into apache:4.19 Jun 18, 2024

DaanHoogland deleted the userdata-validation-and-improvements branch June 18, 2024 18:07

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 24, 2024

User data content size validation, and related code improvements (apa…

106989c

…che#8418) Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com>

Conversation

sureshanaparti commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

sureshanaparti commented Dec 29, 2023

Uh oh!

blueorangutan commented Dec 29, 2023

Uh oh!

codecov Bot commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 29, 2023

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jan 3, 2024

Uh oh!

blueorangutan commented Jan 3, 2024

Uh oh!

blueorangutan commented Jan 3, 2024

Uh oh!

sureshanaparti commented Jan 4, 2024

Uh oh!

sureshanaparti commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 4, 2024

Uh oh!

sureshanaparti commented Jan 4, 2024

Uh oh!

blueorangutan commented Jan 4, 2024

Uh oh!

sureshanaparti commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

sureshanaparti commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

JoaoJandre commented Jan 8, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

sureshanaparti commented Jun 13, 2024

Uh oh!

sureshanaparti commented Jun 13, 2024

Uh oh!

blueorangutan commented Jun 13, 2024

Uh oh!

blueorangutan commented Jun 13, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchavala left a comment

Choose a reason for hiding this comment

sureshanaparti commented Dec 29, 2023 •

edited

Loading

codecov Bot commented Dec 29, 2023 •

edited

Loading

sureshanaparti commented Jun 14, 2024 •

edited

Loading

kiranchavala left a comment •

edited

Loading