You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "myskov (via GitHub)" <gi...@apache.org> on 2023/12/29 12:20:24 UTC
[PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
myskov opened a new pull request, #5887:
URL: https://github.com/apache/ozone/pull/5887
## 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5887:
URL: https://github.com/apache/ozone/pull/5887#issuecomment-1873788079
Thanks @myskov for updating the patch.
@ChenSammi @ivanzlenko would you like to take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5887:
URL: https://github.com/apache/ozone/pull/5887#discussion_r1450835994
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java:
##########
@@ -96,6 +99,21 @@ void testSecretGenerate() throws IOException {
assertEquals(USER_NAME, response.getAwsAccessKey());
}
Review Comment:
Can be done in follow-up if needed.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java:
##########
@@ -96,6 +99,21 @@ void testSecretGenerate() throws IOException {
assertEquals(USER_NAME, response.getAwsAccessKey());
}
Review Comment:
Can be done in follow-up if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5887:
URL: https://github.com/apache/ozone/pull/5887#discussion_r1450831341
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot:
##########
@@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keywords Kinit test user testuser testuser.keytab
Review Comment:
@ivanzlenko `Revoke S3 secrets` (called in the next line) does revoke for `testuser`, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5887:
URL: https://github.com/apache/ozone/pull/5887#discussion_r1450849151
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot:
##########
@@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keywords Kinit test user testuser testuser.keytab
+... AND Revoke S3 secrets
+Test Teardown Run Keyword Revoke S3 secrets
*** Variables ***
${ENDPOINT_URL} http://s3g:9878
+${SECURITY_ENABLED} true
*** Test Cases ***
S3 Gateway Generate 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 PUT --negotiate -u : -v ${ENDPOINT_URL}/secret
- IF '${SECURITY_ENABLED}' == 'true'
- Should contain ${result} HTTP/1.1 200 OK ignore_case=True
- Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>
- ELSE
- Should contain ${result} S3 Secret endpoint is disabled.
- END
+ Should contain ${result} HTTP/1.1 200 OK ignore_case=True
+ Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>
+
+S3 Gateway Secret Already Exists
+ Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled
+ [Setup] Setup v4 headers
+ ${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret
+ Should contain ${result} HTTP/1.1 400 S3_SECRET_ALREADY_EXISTS ignore_case=True
S3 Gateway Generate Secret By Username
- Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab
- ${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2
- IF '${SECURITY_ENABLED}' == 'true'
- Should contain ${result} HTTP/1.1 200 OK ignore_case=True
- Should Match Regexp ${result} <awsAccessKey>.*</awsAccessKey><awsSecret>.*</awsSecret>
- ELSE
- 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
Review Comment:
Also here, can we keep `testuser2`?
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot:
##########
@@ -31,19 +32,12 @@ ${SECURITY_ENABLED} true
*** Test Cases ***
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
Review Comment:
I think @ivanzlenko is right, `[setup]` to generate secret is missing here.
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot:
##########
@@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keywords Kinit test user testuser testuser.keytab
Review Comment:
`Revoke S3 secrets` (called in the next line) does revoke for `testuser`, too.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java:
##########
@@ -96,6 +99,21 @@ void testSecretGenerate() throws IOException {
assertEquals(USER_NAME, response.getAwsAccessKey());
}
Review Comment:
Can be done in follow-up if needed.
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot:
##########
@@ -31,19 +32,12 @@ ${SECURITY_ENABLED} true
*** Test Cases ***
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
- IF '${SECURITY_ENABLED}' == 'true'
- Should contain ${result} HTTP/1.1 200 OK ignore_case=True
- ELSE
- Should contain ${result} S3 Secret endpoint is disabled.
- END
+ Should contain ${result} HTTP/1.1 200 OK ignore_case=True
S3 Gateway Revoke Secret By Username
- Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab
- ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2
- IF '${SECURITY_ENABLED}' == 'true'
- Should contain ${result} HTTP/1.1 200 OK ignore_case=True
- ELSE
- Should contain ${result} S3 Secret endpoint is disabled.
- END
\ No newline at end of file
+ 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
Review 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.)
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot:
##########
@@ -31,19 +32,12 @@ ${SECURITY_ENABLED} true
*** Test Cases ***
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
- IF '${SECURITY_ENABLED}' == 'true'
- Should contain ${result} HTTP/1.1 200 OK ignore_case=True
- ELSE
- Should contain ${result} S3 Secret endpoint is disabled.
- END
+ Should contain ${result} HTTP/1.1 200 OK ignore_case=True
S3 Gateway Revoke Secret By Username
- Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab
- ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2
- IF '${SECURITY_ENABLED}' == 'true'
- Should contain ${result} HTTP/1.1 200 OK ignore_case=True
- ELSE
- Should contain ${result} S3 Secret endpoint is disabled.
- END
\ No newline at end of file
+ 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5887:
URL: https://github.com/apache/ozone/pull/5887#discussion_r1440056191
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot:
##########
@@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keywords Kinit test user testuser testuser.keytab
Review Comment:
I think we should revoke secret for testuser as well, just in case.
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot:
##########
@@ -21,8 +21,9 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keyword Setup v4 headers
Review Comment:
Shouldn't we generate secret beforehand?
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java:
##########
@@ -96,6 +99,21 @@ void testSecretGenerate() throws IOException {
assertEquals(USER_NAME, response.getAwsAccessKey());
}
Review Comment:
Can we add a test to check 500 will be thrown for other exceptions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5887:
URL: https://github.com/apache/ozone/pull/5887#discussion_r1450831341
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot:
##########
@@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keywords Kinit test user testuser testuser.keytab
Review Comment:
@ivanzlenko `Revoke S3 secrets` (called in the next line) does revoke for `testuser`, too.
##########
hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot:
##########
@@ -21,30 +21,31 @@ Library String
Resource ../commonlib.robot
Resource ./commonawslib.robot
Test Timeout 5 minutes
-Suite Setup Setup s3 tests
Default Tags no-bucket-type
+Test Setup Run Keywords Kinit test user testuser testuser.keytab
Review Comment:
@ivanzlenko `Revoke S3 secrets` (called in the next line) does revoke for `testuser`, too, so this is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5887:
URL: https://github.com/apache/ozone/pull/5887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10014. Fixed internal error on generating S3 secret [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5887:
URL: https://github.com/apache/ozone/pull/5887#discussion_r1438415058
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java:
##########
@@ -55,15 +56,26 @@ public Response generate(@PathParam("username") String username)
return generateInternal(username);
}
- private Response generateInternal(@Nullable String username)
- throws IOException {
- S3SecretResponse s3SecretResponse = new S3SecretResponse();
- S3SecretValue s3SecretValue = generateS3Secret(username);
- s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
- s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
- AUDIT.logReadSuccess(buildAuditMessageForSuccess(
- S3GAction.GENERATE_SECRET, getAuditParameters()));
- return Response.ok(s3SecretResponse).build();
+ private Response generateInternal(@Nullable String username) throws IOException {
+ try {
+ S3SecretValue s3SecretValue = generateS3Secret(username);
+
+ S3SecretResponse s3SecretResponse = new S3SecretResponse();
+ s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
+ s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ S3GAction.GENERATE_SECRET, getAuditParameters()));
+ return Response.ok(s3SecretResponse).build();
+ } catch (OMException e) {
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ S3GAction.REVOKE_SECRET, getAuditParameters(), e));
Review Comment:
```suggestion
S3GAction.GENERATE_SECRET, getAuditParameters(), e));
```
##########
hadoop-ozone/dist/src/main/smoketest/commonlib.robot:
##########
@@ -51,3 +53,7 @@ Requires admin privilege
Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip privilege check in unsecure cluster
Kinit test user testuser2 testuser2.keytab
Access should be denied ${command}
+
+Revoke S3 secrets
+ Execute and Ignore Error ozone s3 revokesecret -y
+ Execute and Ignore Error ozone s3 revokesecret -y -u testuser
Review Comment:
Please move S3-specific keyword to `s3/commonawslib.robot`.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java:
##########
@@ -55,15 +56,26 @@ public Response generate(@PathParam("username") String username)
return generateInternal(username);
}
- private Response generateInternal(@Nullable String username)
- throws IOException {
- S3SecretResponse s3SecretResponse = new S3SecretResponse();
- S3SecretValue s3SecretValue = generateS3Secret(username);
- s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
- s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
- AUDIT.logReadSuccess(buildAuditMessageForSuccess(
- S3GAction.GENERATE_SECRET, getAuditParameters()));
- return Response.ok(s3SecretResponse).build();
+ private Response generateInternal(@Nullable String username) throws IOException {
+ try {
+ S3SecretValue s3SecretValue = generateS3Secret(username);
+
+ S3SecretResponse s3SecretResponse = new S3SecretResponse();
+ s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
+ s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ S3GAction.GENERATE_SECRET, getAuditParameters()));
+ return Response.ok(s3SecretResponse).build();
+ } catch (OMException e) {
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ S3GAction.REVOKE_SECRET, getAuditParameters(), e));
+ if (e.getResult() == OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS) {
+ return Response.status(BAD_REQUEST.getStatusCode(), e.getResult().toString()).build();
+ } else {
+ LOG.error("Can't execute revoke secret request: ", e);
Review Comment:
```suggestion
LOG.error("Can't execute get secret request: ", e);
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java:
##########
@@ -55,15 +56,26 @@ public Response generate(@PathParam("username") String username)
return generateInternal(username);
}
- private Response generateInternal(@Nullable String username)
- throws IOException {
- S3SecretResponse s3SecretResponse = new S3SecretResponse();
- S3SecretValue s3SecretValue = generateS3Secret(username);
- s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
- s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
- AUDIT.logReadSuccess(buildAuditMessageForSuccess(
- S3GAction.GENERATE_SECRET, getAuditParameters()));
- return Response.ok(s3SecretResponse).build();
+ private Response generateInternal(@Nullable String username) throws IOException {
+ try {
+ S3SecretValue s3SecretValue = generateS3Secret(username);
+
+ S3SecretResponse s3SecretResponse = new S3SecretResponse();
+ s3SecretResponse.setAwsSecret(s3SecretValue.getAwsSecret());
+ s3SecretResponse.setAwsAccessKey(s3SecretValue.getAwsAccessKey());
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
Review Comment:
I think this should have been a write request.
```suggestion
AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org