Uh oh!
There was an error while loading. Please reload this page.
TEZ-4739: Improve ACLManager - #524
Merged
Merged
Conversation
| return; | ||
| } | ||
| if (aclInfo == null) { | ||
| return; |
Contributor
There was a problem hiding this comment.
nit:
if (!aclsEnabled || aclInfo == null) {
return;
}
This comment was marked as outdated.
This comment was marked as outdated.
tez-yetus
commented
Jul 20, 2026
🎊 +1 overall
This message was automatically generated. |
Comment on lines
+80
to
+81
| this.users = new HashMap<>(amACLManager.users); | ||
| this.groups = new HashMap<>(amACLManager.groups); |
Member
There was a problem hiding this comment.
I think this is a Shallow Copy not a Deep Copy? Is it intentional? Though we are doing put here below. But if someone does in future .get(key).add(...) it might lead to some problems, can you check once
something like in future this.users.get(ACLType.AM_VIEW_ACL).add("new_user") inside this class, it will silently modify the global Application Master's ACLs. They might think they are only granting a user access to a specific DAG, but because of the shallow copy, they would accidentally be granting that user global AM access.
ContributorAuthor
The per-DAG ACLManager constructor previously did new HashMap<>(amACLManager.users) and new HashMap<>(amACLManager.groups), which is a shallow copy: the outer map is new, but each Set<String> value is still shared with the AM manager. Today only put(...) is used on those maps, so no leak occurs in practice — but any future this.users.get(ACLType.AM_VIEW_ACL).add(user) inside this class would silently mutate the AM's global ACLs and every sibling DAG's view of them. Deep-copy each inner Set on construction, and add a reflection-based regression test that mutates the DAG manager's set in place and asserts the AM manager is untouched — verified to fail against the shallow-copy version.
tez-yetus
commented
Jul 22, 2026
🎊 +1 overall
This message was automatically generated. |
ayushtkn
approved these changes
Jul 27, 2026
ayushtkn
left a comment
Member
There was a problem hiding this comment.
Thanx @abstractdog for the fix, Sorry for the delay got pulled into other stuff and this kept on slipping
Changes LGTM
Uh oh!
There was an error while loading. Please reload this page.
asf-gitbox-commits pushed a commit
that referenced
this pull request
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.