-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fixing bug where proxy user from flow parameter was not included #3306
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
Conversation
azkaban-web-server/build.gradle
Outdated
@@ -26,7 +26,7 @@ node { | |||
npmVersion = '5.6.0' | |||
|
|||
// Base URL for fetching node distributions (change if you have a mirror). | |||
distBaseUrl = 'https://nodejs.org/dist' | |||
distBaseUrl = 'https://direct.nodejs.org/dist/' |
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.
why is this change needed ?
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.
Get a Proxy error during building of azkaban-webserver.
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.
srs/gradle-node-plugin#292
Faced this issue.
// First add the proxy user from the top level flow parameter. Adding this here as individual job | ||
// may override this for any given reason. | ||
if (flowParam != null && flowParam.containsKey(USER_TO_PROXY)) { | ||
proxyUsersMap.add(flowParam.get(USER_TO_PROXY)); |
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.
What's the difference b/w this line and line 1184 ?
I.e proxyUsersMap.addAll(flow.getProxyUsers());
-> where are these proxy-users fetched from if not the flow-params ?
I assume they are from the .flow or .job files. However, in that case does not require a DAG walk ? (the comment above this code seems a bit confusing if this is the case)
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.
No. they are from the project permissions page. which is the where the comprehensive list lies, to do an authorization check in the beginning.
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.
for the .job .flow files we get them from here, ContainerImplUtils.getProxyUsersForFlow(), if we opt to do the DAG walk.
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 for addressing this!
…aban#3306) * Fixing bug where proxy user from flow parameter was not included * Removing variable instances of USER_TO_PROXY * Added separate unit test for flow param and moved check to allow unit testing --------- Co-authored-by: Utkarsh Kattishettar <[email protected]> RB=3898971
Adding a bug fix for including proxy users from flow param, in addition to users from the DAG.