HDDS-10014. Fixed internal error on generating S3 secret #5887
Conversation
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @myskov for the patch.
...one/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java
Outdated
Show resolved
Hide resolved
...one/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java
Outdated
Show resolved
Hide resolved
...one/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Doroszlai, Attila <[email protected]>
|
Thanks @myskov for updating the patch. @ChenSammi @ivanzlenko would you like to take a look? |
| Test Timeout 5 minutes | ||
| Suite Setup Setup s3 tests | ||
| Default Tags no-bucket-type | ||
| Test Setup Run Keywords Kinit test user testuser testuser.keytab |
There was a problem hiding this comment.
I think we should revoke secret for testuser as well, just in case.
There was a problem hiding this comment.
@ivanzlenko Revoke S3 secrets (called in the next line) does revoke for testuser, too, so this is fine.
| Test Timeout 5 minutes | ||
| Suite Setup Setup s3 tests | ||
| Default Tags no-bucket-type | ||
| Test Setup Run Keyword Setup v4 headers |
There was a problem hiding this comment.
Shouldn't we generate secret beforehand?
| assertEquals(USER_SECRET, response.getAwsSecret()); | ||
| assertEquals(USER_NAME, response.getAwsAccessKey()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add a test to check 500 will be thrown for other exceptions?
There was a problem hiding this comment.
Can be done in follow-up if needed.
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks again @myskov for the patch. Based on @ivanzlenko's comments I took another look.
| S3 Gateway Revoke Secret | ||
| Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit HTTP user | ||
| Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled | ||
| ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret |
There was a problem hiding this comment.
I think @ivanzlenko is right, [setup] to generate secret is missing here.
| Should contain ${result} S3 Secret endpoint is disabled. | ||
| END | ||
| Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled | ||
| [Setup] Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser |
There was a problem hiding this comment.
Also, I think it would be better if setup used CLI to generate the secret, not HTTP. (Similarly to how secretgenerate.robot uses CLI to revoke in setup/teardown.)
| END | ||
| Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled | ||
| [Setup] Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser | ||
| ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser |
There was a problem hiding this comment.
Can we keep "by username" test case generate/revoke for testuser2? Since testuser is executing the test case, this verifies it works for other user.
| Should contain ${result} S3 Secret endpoint is disabled. | ||
| END | ||
| Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled | ||
| ${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser |
There was a problem hiding this comment.
Also here, can we keep testuser2?
….com/myskov/ozone into HDDS-10014_generate_secret_if_exists
There was a problem hiding this comment.
Thanks @myskov for updating the patch. Thanks @ivanzlenko for the review.
…ache#5887) (cherry picked from commit 27c1f9c)
…ache#5887) (cherry picked from commit 27c1f9c)
3 participants
What changes were proposed in this pull request?
I added exception handling on S3 secret generation.
Also, I had to refactor robot tests on secret generation/revoke to isolate them and make them more readable. Previously, "revoke" tests were dependent on "generate" tests.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10014
How was this patch tested?
added a unit test and an acceptance test