You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ashishkumar50 (via GitHub)" <gi...@apache.org> on 2023/07/13 02:27:13 UTC
[GitHub] [ozone] ashishkumar50 opened a new pull request, #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
ashishkumar50 opened a new pull request, #5056:
URL: https://github.com/apache/ozone/pull/5056
## What changes were proposed in this pull request?
External access authorizer can be shared in same service. In case of ranger we can see we initialize multiple times in same service which eventually does refresh policies internally inside ranger plugin. Any request at this point doesn't work as policy check returns null. Which can be avoided by initialize only once during OM startup.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8997
## How was this patch tested?
Run existing tests.
--
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
[GitHub] [ozone] ashishkumar50 commented on pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #5056:
URL: https://github.com/apache/ozone/pull/5056#issuecomment-1633447015
@smengcl, Please help to review.
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #5056:
URL: https://github.com/apache/ozone/pull/5056#discussion_r1261953680
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java:
##########
@@ -104,19 +105,29 @@ public OmMetadataReader(KeyManager keyManager,
this.metrics = omMetadataReaderMetrics;
this.perfMetrics = ozoneManager.getPerfMetrics();
if (isAclEnabled) {
- accessAuthorizer = getACLAuthorizerInstance(configuration);
- if (accessAuthorizer instanceof OzoneNativeAuthorizer) {
- OzoneNativeAuthorizer authorizer =
- (OzoneNativeAuthorizer) accessAuthorizer;
- isNativeAuthorizerEnabled = true;
- authorizer.setVolumeManager(volumeManager);
- authorizer.setBucketManager(bucketManager);
- authorizer.setKeyManager(keyManager);
- authorizer.setPrefixManager(prefixManager);
- authorizer.setAdminCheck(ozoneManager::isAdmin);
- authorizer.setReadOnlyAdminCheck(ozoneManager::isReadOnlyAdmin);
- authorizer.setAllowListAllVolumes(allowListAllVolumes);
+ Class<? extends IAccessAuthorizer> clazz = configuration.getClass(
+ OZONE_ACL_AUTHORIZER_CLASS, OzoneAccessAuthorizer.class,
+ IAccessAuthorizer.class);
+ if (bNeedAccessAuthInitialization ||
+ clazz.getSimpleName().equals("OzoneNativeAuthorizer")) {
+ // In case of NativeAuthorizer always re-initialize
+ accessAuthorizer = getACLAuthorizerInstance(configuration, clazz);
+ if (accessAuthorizer instanceof OzoneNativeAuthorizer) {
+ OzoneNativeAuthorizer authorizer =
+ (OzoneNativeAuthorizer) accessAuthorizer;
+ isNativeAuthorizerEnabled = true;
+ authorizer.setVolumeManager(volumeManager);
+ authorizer.setBucketManager(bucketManager);
+ authorizer.setKeyManager(keyManager);
+ authorizer.setPrefixManager(prefixManager);
+ authorizer.setAdminCheck(ozoneManager::isAdmin);
+ authorizer.setReadOnlyAdminCheck(ozoneManager::isReadOnlyAdmin);
+ authorizer.setAllowListAllVolumes(allowListAllVolumes);
+ } else {
+ isNativeAuthorizerEnabled = false;
+ }
} else {
+ accessAuthorizer = ozoneManager.getAccessAuthorizer();
Review Comment:
Looks like `accessAuthorizer` isn't closed or destroyed upon `OmMetadataReader` instance closure. Should be fine to share it like this IMO.
--
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
[GitHub] [ozone] adoroszlai commented on pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5056:
URL: https://github.com/apache/ozone/pull/5056#issuecomment-1635650459
> problem is for `nativeAuthorizer` which always need new instance.
Ah, OK, sorry, I missed that part.
--
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
[GitHub] [ozone] adoroszlai merged pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5056:
URL: https://github.com/apache/ozone/pull/5056
--
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
[GitHub] [ozone] ashishkumar50 commented on a diff in pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #5056:
URL: https://github.com/apache/ozone/pull/5056#discussion_r1261961345
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java:
##########
@@ -104,19 +105,29 @@ public OmMetadataReader(KeyManager keyManager,
this.metrics = omMetadataReaderMetrics;
this.perfMetrics = ozoneManager.getPerfMetrics();
if (isAclEnabled) {
- accessAuthorizer = getACLAuthorizerInstance(configuration);
- if (accessAuthorizer instanceof OzoneNativeAuthorizer) {
- OzoneNativeAuthorizer authorizer =
- (OzoneNativeAuthorizer) accessAuthorizer;
- isNativeAuthorizerEnabled = true;
- authorizer.setVolumeManager(volumeManager);
- authorizer.setBucketManager(bucketManager);
- authorizer.setKeyManager(keyManager);
- authorizer.setPrefixManager(prefixManager);
- authorizer.setAdminCheck(ozoneManager::isAdmin);
- authorizer.setReadOnlyAdminCheck(ozoneManager::isReadOnlyAdmin);
- authorizer.setAllowListAllVolumes(allowListAllVolumes);
+ Class<? extends IAccessAuthorizer> clazz = configuration.getClass(
+ OZONE_ACL_AUTHORIZER_CLASS, OzoneAccessAuthorizer.class,
+ IAccessAuthorizer.class);
+ if (bNeedAccessAuthInitialization ||
+ clazz.getSimpleName().equals("OzoneNativeAuthorizer")) {
Review Comment:
Done
--
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
[GitHub] [ozone] ashishkumar50 commented on pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #5056:
URL: https://github.com/apache/ozone/pull/5056#issuecomment-1635612384
> Thanks @ashishkumar50 for working on this. While the patch is fine to minimize change, I think it would be better to let `OzoneManager` create and store `accessAuthorizer` just before creation of `OmMetadataReader`:
>
> https://github.com/apache/ozone/blob/300e7a76481480ae2651ff7214d7b9f146091e68/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L828-L829
>
> Then `OmMetadataReader` could simply store reference to `ozoneManager.getAccessAuthorizer()` without complex logic.
>
> https://github.com/apache/ozone/blob/300e7a76481480ae2651ff7214d7b9f146091e68/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java#L96-L97
@adoroszlai, Thanks for the review, We can create and store `accessAuthorizer` in `OzoneManager`, but problem is for `nativeAuthorizer` which always need new instance.
--
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
[GitHub] [ozone] adoroszlai commented on a diff in pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5056:
URL: https://github.com/apache/ozone/pull/5056#discussion_r1263468464
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java:
##########
@@ -90,7 +90,8 @@ public OmMetadataReader(KeyManager keyManager,
OzoneManager ozoneManager,
Logger log,
AuditLogger audit,
- OmMetadataReaderMetrics omMetadataReaderMetrics) {
+ OmMetadataReaderMetrics omMetadataReaderMetrics,
+ boolean bNeedAccessAuthInitialization) {
Review Comment:
Nit: please don't prefix variables with type.
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #5056:
URL: https://github.com/apache/ozone/pull/5056#discussion_r1261952169
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java:
##########
@@ -104,19 +105,29 @@ public OmMetadataReader(KeyManager keyManager,
this.metrics = omMetadataReaderMetrics;
this.perfMetrics = ozoneManager.getPerfMetrics();
if (isAclEnabled) {
- accessAuthorizer = getACLAuthorizerInstance(configuration);
- if (accessAuthorizer instanceof OzoneNativeAuthorizer) {
- OzoneNativeAuthorizer authorizer =
- (OzoneNativeAuthorizer) accessAuthorizer;
- isNativeAuthorizerEnabled = true;
- authorizer.setVolumeManager(volumeManager);
- authorizer.setBucketManager(bucketManager);
- authorizer.setKeyManager(keyManager);
- authorizer.setPrefixManager(prefixManager);
- authorizer.setAdminCheck(ozoneManager::isAdmin);
- authorizer.setReadOnlyAdminCheck(ozoneManager::isReadOnlyAdmin);
- authorizer.setAllowListAllVolumes(allowListAllVolumes);
+ Class<? extends IAccessAuthorizer> clazz = configuration.getClass(
+ OZONE_ACL_AUTHORIZER_CLASS, OzoneAccessAuthorizer.class,
+ IAccessAuthorizer.class);
+ if (bNeedAccessAuthInitialization ||
+ clazz.getSimpleName().equals("OzoneNativeAuthorizer")) {
Review Comment:
```suggestion
clazz.getSimpleName().equals(OzoneNativeAuthorizer.class.getSimpleName())) {
```
--
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
[GitHub] [ozone] adoroszlai commented on pull request #5056: HDDS-8997. Fix RangerOzoneAuthorizer initialization issue.
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5056:
URL: https://github.com/apache/ozone/pull/5056#issuecomment-1635655190
Thanks @ashishkumar50 for the patch, @smengcl for the review.
--
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