minio: fix store user creation by shwstppr · Pull Request #8425 · apache/cloudstack

shwstppr · 2024-01-02T08:46:00Z

Description

To prevent error during multi-user access, use account UUID to create/access user on the procider side. Also, update existing secret key for a user that already exist.

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?

Create a new account testuser
Create a minio bucket for the account
Delete the account
Again create account with the same name
Try creating a minio bucket and observe the failure,

2024-01-03 12:34:27,976 DEBUG [o.a.c.s.o.BucketApiServiceImpl] (API-Job-Executor-83:ctx-8824ed0d job-95 ctx-1ec9d7a6) (logid:5741e607) Failed to create bucket with name: bucket2
com.cloud.utils.exception.CloudRuntimeException: Bucket access credentials unavailable for account: testuser
	at org.apache.cloudstack.storage.datastore.driver.MinIOObjectStoreDriverImpl.createBucket(MinIOObjectStoreDriverImpl.java:103)
	at org.apache.cloudstack.storage.object.store.ObjectStoreImpl.createBucket(ObjectStoreImpl.java:108)
	at org.apache.cloudstack.storage.object.BucketApiServiceImpl.createBucket(BucketApiServiceImpl.java:147)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
	at org.apache.cloudstack.network.contrail.management.EventUtils$EventInterceptor.invoke(EventUtils.java:107)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:175)
	at com.cloud.event.ActionEventInterceptor.invoke(ActionEventInterceptor.java:52)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:175)
	at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:97)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:215)
	at com.sun.proxy.$Proxy394.createBucket(Unknown Source)
	at org.apache.cloudstack.api.command.user.bucket.CreateBucketCmd.execute(CreateBucketCmd.java:190)
	at com.cloud.api.ApiDispatcher.dispatch(ApiDispatcher.java:172)
	at com.cloud.api.ApiAsyncJobDispatcher.runJob(ApiAsyncJobDispatcher.java:112)
	at org.apache.cloudstack.framework.jobs.impl.AsyncJobManagerImpl$5.runInContext(AsyncJobManagerImpl.java:632)
	at org.apache.cloudstack.managed.context.ManagedContextRunnable$1.run(ManagedContextRunnable.java:48)
	at org.apache.cloudstack.managed.context.impl.DefaultManagedContext$1.call(DefaultManagedContext.java:55)
	at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.callWithContext(DefaultManagedContext.java:102)
	at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.runWithContext(DefaultManagedContext.java:52)
	at org.apache.cloudstack.managed.context.ManagedContextRunnable.run(ManagedContextRunnable.java:45)
	at org.apache.cloudstack.framework.jobs.impl.AsyncJobManagerImpl$5.run(AsyncJobManagerImpl.java:580)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)

Upgrade to PR packages and bucket creation works without any error. On MinIO dashboard user created with uuid can be seen,

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

To prevent error during multi-user access, use account UUID to create/access user on the procider side. Also, update existing secret key for a user that already exist. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

codecov · 2024-01-02T08:53:57Z

Codecov Report

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

Comparison is base (9d4f071) 30.80% compared to head (2a6de43) 30.02%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
...e/datastore/driver/MinIOObjectStoreDriverImpl.java	43.75%	13 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8425      +/-   ##
============================================
- Coverage     30.80%   30.02%   -0.79%     
+ Complexity    33981    33032     -949     
============================================
  Files          5341     5341              
  Lines        374864   374891      +27     
  Branches      54518    54528      +10     
============================================
- Hits         115485   112561    -2924     
- Misses       244114   247332    +3218     
+ Partials      15265    14998     -267

Flag	Coverage Δ
simulator-marvin-tests	`23.64% <0.00%> (-1.09%)`	⬇️
uitests	`4.39% <ø> (+<0.01%)`	⬆️
unit-tests	`16.46% <43.75%> (+<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.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-01-02T10:01:07Z

@blueorangutan package

blueorangutan · 2024-01-02T10:06:05Z

@shwstppr 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-02T11:32:41Z

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

vishesh92

clgtm

shwstppr · 2024-01-02T11:53:29Z

@blueorangutan test

blueorangutan · 2024-01-02T11:54:03Z

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

harikrishna-patnala

code LGTM

blueorangutan · 2024-01-03T01:07:53Z

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

Test	Result	Time (s)	Test File
test_create_pvlan_network	`Error`	0.06	test_pvlan.py

DaanHoogland

clgtm. I think we missed a big issue before, putting the Bucket VO and Dao in package com.cloud instead of org.apache.cloudstack. Nothing to do about that in the scope of this PR, though.

DaanHoogland · 2024-01-03T08:06:00Z

            }
        } catch (Exception e) {
-            s_logger.debug("User does not exist. Creating user: "+accessKey);
+            s_logger.debug("User does not exist. Creating user: " + accessKey);


Suggested change

s_logger.debug("User does not exist. Creating user: " + accessKey);

s_logger.debug(String.format("User does not exist. Creating user: %s", accessKey), e);

or error level?

@DaanHoogland I don't think we need error level here. Based on the code I understand ideally user shouldn't exist on the object store we create one. Though I'm not sure if we should catchall

Made some changes to error handling

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

sureshanaparti

clgtm

vladimirpetrov · 2024-01-04T23:00:24Z

While testing this PR, I found the following issue, here are the steps to reproduce:

As admin, create a bucket
Create a new account test1
Log in with the account and create a minio bucket
Log in as admin, delete the account
Again create account with the same name
Log in with the account and create another minio bucket
Upload a file (not really necessary)
Using the same account try to create a bucket that already exists (the one created in the first step). The operation will fail, obviously.
Now go to the bucket again - the uploaded file is missing. Try to upload a new file. The operation fails with the following message:

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-01-05T09:37:02Z

@vladimirpetrov I was able to reproduce the issue and have added a fix in the latest commit

@blueorangutan package

blueorangutan · 2024-01-05T09:38:03Z

@shwstppr 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-05T11:29:59Z

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

shwstppr · 2024-01-07T06:42:52Z

@blueorangutan test

blueorangutan · 2024-01-07T06:44:03Z

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

blueorangutan · 2024-01-07T19:26:47Z

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

Test	Result	Time (s)	Test File

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-01-08T12:22:12Z

@blueorangutan package

blueorangutan · 2024-01-08T12:24:03Z

@shwstppr 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-08T13:40:38Z

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

vladimirpetrov · 2024-01-08T15:48:30Z

@blueorangutan package

blueorangutan · 2024-01-08T15:50:04Z

@vladimirpetrov 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-08T16:54:38Z

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

shwstppr · 2024-01-08T19:42:14Z

@blueorangutan test

blueorangutan · 2024-01-08T19:44:03Z

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

blueorangutan · 2024-01-09T10:08:15Z

[SF] Trillian test result (tid-8763)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 50354 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8425-t8763-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_08_upgrade_kubernetes_ha_cluster	`Failure`	978.85	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	3607.96	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	`Failure`	46.96	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	89.12	test_kubernetes_clusters.py
test_08_migrate_vm	`Error`	0.06	test_vm_life_cycle.py
test_05_vmschedule_test_e2e	`Failure`	361.66	test_vm_schedule.py

vladimirpetrov

LGTM based on manual testing.

To prevent errors during multi-user access, use account UUID to create/access user on the provider side. Also, update the existing secret key for a user that already exists.

minio: fix store user creation

aa22ba6

To prevent error during multi-user access, use account UUID to create/access user on the procider side. Also, update existing secret key for a user that already exist. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborg Bot added the component:storage label Jan 2, 2024

test fix

adb8785

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

apache deleted a comment from blueorangutan Jan 2, 2024

vishesh92 approved these changes Jan 2, 2024

View reviewed changes

harikrishna-patnala approved these changes Jan 2, 2024

View reviewed changes

DaanHoogland approved these changes Jan 3, 2024

View reviewed changes

shwstppr added this to the 4.19.0.0 milestone Jan 3, 2024

shwstppr marked this pull request as ready for review January 4, 2024 05:12

log fix

fba6c56

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

sureshanaparti approved these changes Jan 4, 2024

View reviewed changes

yadvr approved these changes Jan 4, 2024

View reviewed changes

shwstppr assigned vladimirpetrov Jan 4, 2024

shwstppr added the status:needs-testing label Jan 4, 2024

prevent secret key update when not returned in minio response

a28b78a

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

handle RuntimeException

2a6de43

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

vladimirpetrov approved these changes Jan 9, 2024

View reviewed changes

shwstppr requested review from DaanHoogland, harikrishna-patnala and vishesh92 January 9, 2024 11:12

DaanHoogland added status:ready-for-merge and removed status:needs-testing labels Jan 9, 2024

shwstppr removed the status:ready-for-merge label Jan 9, 2024

shwstppr merged commit d6ac91f into apache:main Jan 9, 2024

DaanHoogland deleted the fix-objectstore-user branch January 9, 2024 12:16

	s_logger.debug("User does not exist. Creating user: " + accessKey);
	s_logger.debug(String.format("User does not exist. Creating user: %s", accessKey), e);

Conversation

shwstppr commented Jan 2, 2024 • 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!

codecov Bot commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shwstppr commented Jan 2, 2024

Uh oh!

blueorangutan commented Jan 2, 2024

Uh oh!

blueorangutan commented Jan 2, 2024

Uh oh!

vishesh92 left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Jan 2, 2024

Uh oh!

blueorangutan commented Jan 2, 2024

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jan 3, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 3, 2024

Choose a reason for hiding this comment

Uh oh!

shwstppr Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

shwstppr Jan 9, 2024

Choose a reason for hiding this comment

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirpetrov commented Jan 4, 2024

Uh oh!

shwstppr commented Jan 5, 2024

Uh oh!

blueorangutan commented Jan 5, 2024

Uh oh!

blueorangutan commented Jan 5, 2024

Uh oh!

shwstppr commented Jan 7, 2024

Uh oh!

blueorangutan commented Jan 7, 2024

Uh oh!

blueorangutan commented Jan 7, 2024

Uh oh!

shwstppr commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

vladimirpetrov commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

shwstppr commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 8, 2024

Uh oh!

blueorangutan commented Jan 9, 2024

shwstppr commented Jan 2, 2024 •

edited

Loading

codecov Bot commented Jan 2, 2024 •

edited

Loading