-
Notifications
You must be signed in to change notification settings - Fork 499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-11041. Add admin request filter for S3 requests and UGI support for GrpcOmTransport #7268
base: master
Are you sure you want to change the base?
Conversation
@ivanzlenko please take a look |
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
...op-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java
Outdated
Show resolved
Hide resolved
...op-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
General question: do we have any new tests for this? |
Hi @ivanzlenko, new tests have not yet been added. We will need to modify existing tests to add user as an admin and generate request. Waiting for go on code change before modifying the tests. |
@devabhishekpal new filter could and should be covered with unit tests at least. |
Yes, these new cases will be added to check for the filter working |
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @devabhishekpal for updating the patch.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java
Outdated
Show resolved
Hide resolved
Thanks @devabhishekpal for tiding up code! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @devabhishekpal for working on this.
- Please enable existing robot tests related to HDDS-11041, e.g.:
[Tags] robot:skip # TODO: Enable after HDDS-11041 is done. |
- Please add a new test case similar to
... By Username For Other User
, but withtestuser2
(non-admin) logged in, and expecting request to be rejected.
Hi @ivanzlenko, @adoroszlai, @myskov, @vtutrinov could you take another look at the changes?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @devabhishekpal for updating the patch. Mostly looks good.
I just realized that GrpcOmTransport
is not used in any acceptance test environments. Can you please enable it in ozonesecure
env. by the following change?
diff --git hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml
index 39d26c362f..3fb7525f20 100644
--- hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml
+++ hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml
@@ -96,7 +96,7 @@ services:
- 9878:9878
env_file:
- ./docker-config
- command: ["/opt/hadoop/bin/ozone","s3g"]
+ command: ["/opt/hadoop/bin/ozone","s3g","-Dozone.om.transport.class=org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory"]
environment:
OZONE_OPTS:
recon:
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java
Outdated
Show resolved
Hide resolved
...op-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java
Outdated
Show resolved
Hide resolved
…rt for acceptance env
...op-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java
Outdated
Show resolved
Hide resolved
...one/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java
Outdated
Show resolved
Hide resolved
Thanks for the review @adoroszlai , addressed the test failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @devabhishekpal for iterating on the patch.
@devabhishekpal from my point of view should be enough to have unit tests with mocks to verify that the contract for the filter will remain the same with any changes and extensively cover this functionality with integration tests. Unit tests should be simple and I don't think it will be a good to write something very comprehensive to mimic what could be done with integration tests. |
Thanks for the input @ivanzlenko. |
What changes were proposed in this pull request?
HDDS-11041. Add admin request filter for S3 requests and UGI support for GrpcOmTransport
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11041
How was this patch tested?
Patch was tested using unit tests