UI: Fix Userdata registration from UI by Pearl1594 · Pull Request #8791 · apache/cloudstack

Pearl1594 · 2024-03-14T13:57:45Z

Description

This PR fixes: #8736

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

When "haha" is passed as userdata:

Userdata is base64 encoded:

Response:

When "haha" is base64 encoded and provided as userdata input:

Response:

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

codecov · 2024-03-14T14:06:44Z

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 31.01%. Comparing base (0208e09) to head (9b1c462).
Report is 1 commits behind head on 4.19.

Files	Patch %	Lines
ui/src/utils/plugins.js	0.00%	5 Missing ⚠️
ui/src/views/compute/RegisterUserData.vue	0.00%	2 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.19 #8791 +/- ##
============================================
+ Coverage 30.91% 31.01% +0.10% - Complexity 34249 34339 +90 
============================================
Files 5354 5354 Lines 376094 376482 +388 Branches 54696 54927 +231 ============================================
+ Hits 116258 116776 +518 + Misses 244542 244384 -158 - Partials 15294 15322 +28

Flag	Coverage Δ
simulator-marvin-tests	`24.79% <ø> (+0.07%)`	⬆️
uitests	`4.39% <0.00%> (-0.01%)`	⬇️
unit-tests	`16.65% <ø> (+0.07%)`	⬆️

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.

shwstppr

Code LGTM

weizhouapache · 2024-03-14T15:10:07Z

@Pearl1594
what do you think if we add a checkbox to indicate if userdata is base64-encoded ?

for example

Pearl1594 · 2024-03-14T15:14:24Z

@weizhouapache we could - do you mean that users would need to specify if its encoded or not? If so, wouldn't it be user friendly if we have the code decide if it's encoded or not and accordingly proceed ?

weizhouapache · 2024-03-14T15:21:59Z

@weizhouapache we could - do you mean that users would need to specify if its encoded or not? If so, wouldn't it be user friendly if we have the code decide if it's encoded or not and accordingly proceed ?

@Pearl1594
the userdata could be binary data.

let me explain with an example

cloud-init supports userdata compressed by gzip. I use the same userdata as #8739

$ echo "Apache CloudStack" |gzip |base64 -w0
H4sIAAAAAAAAA3MsSEzOSFVwzskvTQkuSUzO5gIAxXcOkxIAAAA=

when I register userdata with userdata H4sIAAAAAAAAA3MsSEzOSFVwzskvTQkuSUzO5gIAxXcOkxIAAAA=, I expect the the same value is saved in database. but the actual userdata in database is SDRzSUFBQUFBQUFBQTNNc1NFek9TRlZ3enNrdlRRa3VTVXpPNWdJQXhYY09reElBQUFBPQ==.

Pearl1594 · 2024-03-14T17:34:22Z

@weizhouapache sorry I didn't quite understand why the gzip in between.
Because without it i.e., echo "Apache CloudStack" | base64 -w0 if this is provided as input to the userdata param, we don't see the issue. That said, don't mind adding the checkbox. But I see an issue with decoding it when viewing it in the details view... since its is a gzip encoded string, it attempts to decode it and it would appear as:

weizhouapache · 2024-03-14T18:14:49Z

@weizhouapache sorry I didn't quite understand why the gzip in between.

refer to https://cloudinit.readthedocs.io/en/latest/explanation/format.html#gzip-compressed-content

Gzip compressed content
Content found to be gzip compressed will be uncompressed. The uncompressed data will then be used as if it were not compressed. This is typically useful because user data is limited to ~16384 [1] bytes.

Because without it i.e., echo "Apache CloudStack" | base64 -w0 if this is provided as input to the userdata param, we don't see the issue. That said, don't mind adding the checkbox. But I see an issue with decoding it when viewing it in the details view... since its is a gzip encoded string, it attempts to decode it and it would appear as:

I think it is OK as the userdata is binary.

moreover, if an user input a base64-encoded string on UI, we do not know if the user wants the string to be the userdata, or decode it as userdata, it would be good to ask the user to tell us.

Pearl1594 · 2024-03-14T18:18:39Z

Ok got it, I'll add a checkbox that will only be used by the ui to determine whether to encode the provided userdata or not..

weizhouapache · 2024-03-14T18:24:01Z

Ok got it, I'll add a checkbox that will only be used by the ui to determine whether to encode the provided userdata or not..

good ,looking forward to it

weizhouapache · 2024-03-15T10:08:23Z

 app.config.globalProperties.$toBase64AndURIEncoded = function (text) {
- const base64regex = /^([0-9a-zA-Z+/]{4})*(([0-9a-zA-Z+/]{2}==)|([0-9a-zA-Z+/]{3}=))?$/
- if (base64regex.test(text)) {
+ if (isBase64(text)) {


@Pearl1594
thanks for the update.
with your latest change, do we still need this ?
if (isBase64(text)) { return text }

Do we not want the text that's not encoded to be encoded and saved in the DB .. I thought the idea of the flag was the prevent encoding an already encoded string

@Pearl1594
I think the logic can be simple as below
if checkbox is ticked, pass the origin input
if checkbox is not ticked, (Base64AndURI) encode the input and pass to API

oh, wait @Pearl1594
the method toBase64AndURIEncoded is being used in some other files..
ui/src/utils/plugins.js: app.config.globalProperties.$toBase64AndURIEncoded = function (text) { ui/src/views/compute/CreateAutoScaleVmGroup.vue: createVmGroupData.userdata = this.$toBase64AndURIEncoded(values.userdata) ui/src/views/compute/DeployVM.vue: deployVmData.userdata = this.$toBase64AndURIEncoded(values.userdata) ui/src/views/compute/DeployVnfAppliance.vue: createVnfAppData.userdata = this.$toBase64AndURIEncoded(values.userdata) ui/src/views/compute/EditVM.vue: params.userdata = this.$toBase64AndURIEncoded(values.userdata) ui/src/views/compute/RegisterUserData.vue: params.userdata = this.$toBase64AndURIEncoded(values.userdata) ui/src/views/compute/ResetUserData.vue: params.userdata = this.$toBase64AndURIEncoded(values.userdata)

@Pearl1594 I think the logic can be simple as below
* if checkbox is ticked, pass the origin input * if checkbox is not ticked, (Base64AndURI) encode the input and pass to API
That is what is happening in RegisterUserdata component, that we see if it's unchecked and call the encode method.

@Pearl1594 @weizhouapache is there a consensus on changes here?

I had forgotten about this PR - however, @weizhouapache 's input would be appreciated at this point.

yadvr

LGTM - didn't test/check cases

DaanHoogland · 2024-06-18T08:37:06Z

@weizhouapache is this ok by you? or else what still needs to be done ? cc @Pearl1594 ?

weizhouapache · 2024-06-18T10:10:53Z

@weizhouapache is this ok by you? or else what still needs to be done ? cc @Pearl1594 ?

@DaanHoogland @Pearl1594

verified ok

create a user data with "haha" ("Base64 encoded" is checked)
create a user data with "haha" ("Base64 encoded" is not checked)

it looks good in database and UI

weizhouapache · 2024-06-18T10:11:06Z

thanks @Pearl1594 , merging

* UI: Fix Userdata registration from UI * add isbase64 checkbox

UI: Fix Userdata registration from UI
0f67707

Pearl1594 requested review from harikrishna-patnala, shwstppr and weizhouapache March 14, 2024 13:57

boring-cyborgBot added the component:UI label Mar 14, 2024

shwstppr approved these changes Mar 14, 2024
View reviewed changes

Pearl1594 marked this pull request as draft March 14, 2024 18:17

add isbase64 checkbox
9b1c462

weizhouapache reviewed Mar 15, 2024
View reviewed changes

DaanHoogland linked an issue Jun 3, 2024 that may be closed by this pull request
Userdata is not registered as expected #8736
Closed

Pearl1594 marked this pull request as ready for review June 14, 2024 12:58

yadvr approved these changes Jun 18, 2024
View reviewed changes

weizhouapache merged commit 7534196 into 4.19Jun 18, 2024

weizhouapache mentioned this pull request Jun 18, 2024
Userdata is not registered as expected #8736
Closed

DaanHoogland deleted the fix-userdata-encoding branch June 18, 2024 13:45

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 20, 2024
UI: Fix Userdata registration from UI (apache#8791)
dd7385c
* UI: Fix Userdata registration from UI * add isbase64 checkbox

DaanHoogland added this to the 4.19.2.0 milestone Sep 6, 2024

Uh oh!

Conversation

Pearl1594 commented Mar 14, 2024• edited by DaanHoogland 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

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!

codecovBot commented Mar 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Mar 14, 2024

Uh oh!

Pearl1594 commented Mar 14, 2024

Uh oh!

weizhouapache commented Mar 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pearl1594 commented Mar 14, 2024

Uh oh!

weizhouapache commented Mar 14, 2024

Uh oh!

Pearl1594 commented Mar 14, 2024

Uh oh!

weizhouapache commented Mar 14, 2024

Uh oh!

weizhouapacheMar 15, 2024

Choose a reason for hiding this comment

Uh oh!

Pearl1594Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

weizhouapacheMar 15, 2024

Choose a reason for hiding this comment

Uh oh!

weizhouapacheMar 15, 2024

Choose a reason for hiding this comment

Uh oh!

Pearl1594Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

shwstpprMay 30, 2024

Choose a reason for hiding this comment

Uh oh!

Pearl1594May 30, 2024

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jun 18, 2024

Uh oh!

weizhouapache commented Jun 18, 2024

Uh oh!

weizhouapache commented Jun 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Pearl1594 commented Mar 14, 2024•
edited by DaanHoogland
Loading

codecovBot commented Mar 14, 2024•
edited
Loading

weizhouapache commented Mar 14, 2024•
edited
Loading