-
Notifications
You must be signed in to change notification settings - Fork 805
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
feat(pipeline executions/orca) : Added ability to add roles to manual judgment stage. #3988
Conversation
|
||
@Override | ||
TaskResult execute(StageExecution stage) { | ||
StageData stageData = stage.mapTo(StageData) | ||
def stageAuthorized = stage.context.get('isAuthorized') |
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.
It would be nice if this were an encapsulating object within the context ...
def authorizationRequirements = stage.mapTo("/authorizationRequirements", ManualJudgmentAuthorizationRequirements)
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.
done.
Map outputs = processNotifications(stage, stageData, notificationState) | ||
|
||
return TaskResult.builder(executionStatus).context(outputs).build() | ||
} | ||
|
||
boolean checkManualJudgmentAuthorizedGroups(def stageRoles, def permissions, def username) { |
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.
Please type these parameters (even if it's groovy)
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.
done
def stageAuthorized = stage.context.get('isAuthorized') | ||
def stageRoles = stage.context.get('stageRoles') | ||
def permissions = stage.context.get('permissions') | ||
def username = stage.lastModified? stage.lastModified.user: stage.context.get('username') |
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.
I think you should be able to fetch the user from AuthenticatedRequest.getSpinnakerUser()
.
Defaulting to stage.context.get('username')
seems suspicious in this 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.
I should have deleted it. Kept it for executing the test cases. Modified the test 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.
I still don't believe accessing stage.lastModified
directly is correct, what does AuthenticatedRequest.getSpinnakerUser() return?
I believe it should return the correct user.
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.
stage.lastModified returns the user, who clicked on the 'continue' or 'stop' button of the manual judgment stage.
this should work. it returns the correct user.
If you still insist, i can change the 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.
Yes, lastModified
is an implementation detail. My preference would be to avoid referencing it directly.
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.
done...fetching user from AuthenticatedRequest.getSpinnakerUser().
@@ -204,6 +205,52 @@ class OperationsController { | |||
} | |||
} | |||
|
|||
private void addStageAuthorizedRoles(def request, Map pipeline) { |
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.
I don't understand what this is doing, or why it's doing it when we're starting a pipeline.
Is the idea not that a manual judgment stage can be configured with a set of roles needed to judge it?
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.
This method will check for manual judgment stages and will find out if the user is authorized to continue. Will set the isAuthorized flag to true , if authorized,
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.
It's still unclear why it needs to happen in the OperationsController
. I'm confused as this code seems to be manipulating every stage in the pipeline?
Overall this implementation is hard to follow.
My expectation was that the manual judgment stage would have a set of required roles in its context, and would check against those roles when determining whether or not to continue. That shouldn't require changes anywhere other than the manual judgement stage/task.
As an aside, there is a small loophole here in that any stage context can be PATCH'd. See https://github.com/spinnaker/orca/blob/master/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy#L475.
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.
It doesn't manipuate every stage. It only manipulates manual judgment stages.
Please refer the design document.
spinnaker/governance#80
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.
def stageList = pipeline.stages
def stageRoles = []
stageList.each { item ->
stageRoles = item.selectedStageRoles
item.isAuthorized = ManualJudgmentAuthzGroupsUtil.checkAuthorizedGroups(userRoles, stageRoles, permissions)
item.stageRoles = stageRoles
item.permissions = permissions
}
What am I missing here, that's iterating across pipeline.stages
.
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.
item.selectedStageRoles returns null for all the stages except manual judgment(if configured).
This decision to apply for all stages was taken in our design meet, so that in future, if we want to extend it to all stages, this will work.
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.
If we wanted to apply similar logic to all stages, we would do so with a different implementation (likely a preprocessor that could run before task execution).
selectedStageRoles
may be null, but you're also manipulating the context for every stage with stageRoles
and permissions
.
It also introduces another remote call in the flow to run a pipeline (to front50
) which is even with this feature as currently implemented would be unnecessary to anyone who isn't using roles in manual judgment stages. As a general rule, we try to remove unnecessary rpc calls on critical paths.
I'd like this changed, and really this bit removed.
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.
removed this piece of code completely now.
… judgment stage. This is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to Get the application roles, stage roles and the user roles. Check each of the user roles whether they are contained in the stage and application roles. Once the user role is contained in the manual judgment stage. We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions, if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages. if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages. If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true. Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute to the next stage or not. If yes/no, continue with the next stages/continues running the same stage. Enhanced ManualJudgmentStageSpec.groovy to Modified the testcases as per the requirement. Added ManualJudgmentAuthzGroupsUtil.groovy Added a utlity method to check authorized groups of the manual judgment stage. Added ManualJudgmentAuthzGroupsUtilSpec.groovy Added test cases check authorized groups method of ManualJudgmentAuthzGroupsUtil.groovy.
|
||
private final FiatPermissionEvaluator fiatPermissionEvaluator | ||
|
||
WaitForManualJudgmentTask(Optional<EchoService> echoService, Optional<FiatPermissionEvaluator> fpe) { |
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 @Autowired
annotation needed on this constructor?
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.
There is no autowired annotation on this constructor. Please check the file again.
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.
I realize there isn't, but this class was changed from field injection to constructor injection and for consistency we usually add an @Autowired
annotation to the constructor.
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.
Rob asks me to remove and you ask me to keep it. I cannot have conflicting views on the same thing. I do not know which way to go ahead.
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.
Most of the project is going to have @Autowired
on @Component
constructors. I believe it works in this case because the class has only one constructor, but the norm as far as I'm concerned is to annotate the constructors even if there is only one.
This is a minor point on this PR.
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.
Should i use @Autowired or not? Since, this is a minor point. I am dropping this feedback.
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.
I asked you to remove the @Autowired
properties in favor of a single constructor. My example did not include an @Autowired
annotation, but that wasn't really intended to be copy/pasted, either. While @Autowired
on a single constructor is optional, per @ajordens, precedent in the rest of the codebase does have us adding @Autowired
on single constructors anyway, probably for explicitness.
This is a good example for why keeping a single PR for a change is valuable: I had to click through 4 different PRs to find the original thread of feedback about this. Had it been a single PR, context for other reviewers would've been easily discoverable.
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.
done..added @Autowired
where: | ||
context || expectedStatus | ||
[judgmentStatus: "continue", isAuthorized: false, stageRoles: ['foo'], | ||
permissions: [WRITE: ["foo"],READ: ["foo","baz"], EXECUTE: ["foo"]]] || ExecutionStatus.SUCCEEDED |
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 is the difference between permissions
and stageRoles
? I thought the intention was you needed to have a particular role (or roles?).
The reason for WRITE/READ/EXECUTE isn't clear.
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.
stageRoles --> Roles configured at the manual judgment stage.
permissions --> roles configured during the application.creation.
Please refer the design document table for more clear picture.
spinnaker/governance#80
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.
Display all the groups the user has selected while creating application(permissions) in the authorized groups dropdown.
Sure. But that set of application permissions should just be looked up from the UI, it doesn't need to be stored in the stage context.
I'd like this PR to really be centered around the manual judgement stage and required user roles needed to act on it.
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.
I am unable to follow you here. Did not understand what you meant? Can you be more specific.
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.
Sorry, for some odd reason I typed "managed delivery" when I meant "manual judgment". I blame the late hour, sorry :(
I think what would be clearer is if I did a quick prototype/pr of what is in my mind, and we can compare. I'll do that this morning.
|
||
private final FiatPermissionEvaluator fiatPermissionEvaluator | ||
|
||
WaitForManualJudgmentTask(Optional<EchoService> echoService, Optional<FiatPermissionEvaluator> fpe) { |
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.
I realize there isn't, but this class was changed from field injection to constructor injection and for consistency we usually add an @Autowired
annotation to the constructor.
where: | ||
context || expectedStatus | ||
[judgmentStatus: "continue", isAuthorized: false, stageRoles: ['foo'], | ||
permissions: [WRITE: ["foo"],READ: ["foo","baz"], EXECUTE: ["foo"]]] || ExecutionStatus.SUCCEEDED |
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.
Display all the groups the user has selected while creating application(permissions) in the authorized groups dropdown.
Sure. But that set of application permissions should just be looked up from the UI, it doesn't need to be stored in the stage context.
I'd like this PR to really be centered around the manual judgement stage and required user roles needed to act on it.
@@ -204,6 +205,52 @@ class OperationsController { | |||
} | |||
} | |||
|
|||
private void addStageAuthorizedRoles(def request, Map pipeline) { |
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.
If we wanted to apply similar logic to all stages, we would do so with a different implementation (likely a preprocessor that could run before task execution).
selectedStageRoles
may be null, but you're also manipulating the context for every stage with stageRoles
and permissions
.
It also introduces another remote call in the flow to run a pipeline (to front50
) which is even with this feature as currently implemented would be unnecessary to anyone who isn't using roles in manual judgment stages. As a general rule, we try to remove unnecessary rpc calls on critical paths.
I'd like this changed, and really this bit removed.
Spent a few minutes this morning and put together #3991. How different are the intentions here than that? The I did mention earlier that this only works in the happy path but that |
EchoService echoService | ||
private final EchoService echoService | ||
|
||
@Autowired(required = false) |
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.
This is still not correct.
The @Autowired
should be on the constructor and not these attributes.
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.
I removed, but i find the @Autowired in many places. This should not be a concern.
@@ -104,6 +100,9 @@ class OperationsController { | |||
@Autowired(required = false) | |||
Front50Service front50Service | |||
|
|||
@Autowired(required = false) | |||
private FiatPermissionEvaluator fiatPermissionEvaluator |
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.
This does not seem 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.
done...
@@ -54,6 +56,11 @@ class WebConfiguration { | |||
return new MetricsInterceptor(registry, "controller.invocations", ["application"], ["BasicErrorController"]) | |||
} | |||
|
|||
@Bean | |||
ManualJudgmentAuthzGroupsUtil mnlJudgUtil(Optional<Front50Service> front50Service) { |
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.
Please do not truncate mnl
and Judg
when naming beans ... s/mnlJudgUtl/ManualJugementAuthorizationUtil
(or something.
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.
done
this.front50Service = front50Service.orElse(null); | ||
} | ||
|
||
public static boolean checkAuthorizedGroups( |
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.
You still have not added any inline comments here around what this function is intended to do.
I don't quite understand why we're checking against application permissions when the manual judgment could not have happened if the user didn't have EXECUTE.
@PreAuthorize("hasPermission(this.getPipeline(#id)?.application, 'APPLICATION', 'EXECUTE')")
@RequestMapping(value = "/pipelines/{id}/stages/{stageId}", method = RequestMethod.PATCH)
PipelineExecution updatePipelineStage(
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.
Added inline comments.
Application permissions has to be 'CREATE, WRITE, EXECUTE' to proceed further in the manual judgment stage.
This method checks if the logged in user has role in the manual judgment stage authorized groups.
We fetch the user roles and check if that role is authorized in the manual judgment stage role.
if the user role exists , then we check with the application permission roles.
If the application permission role has 'READ' then we return false(not authorized)
If the application permission role has 'CREATE, EXECUTE, WRITE' then we return true(authorized)
def appPermissions | ||
def stageRoles | ||
if (fiatEnabled) { | ||
appPermissions = getApplicationPermissions(stage) |
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.
I'd rather we not introduce a call to front50
if there aren't any required roles here. The majority of use-cases (and certainly everything pre-existing) will not need this.
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.
done..
… judgment stage. This is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to Get the application roles, stage roles and the user roles. Check each of the user roles whether they are contained in the stage and application roles. Once the user role is contained in the manual judgment stage. We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions, if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages. if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages. If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true. Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute toThis is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to tEnhanced OperationsController.groovy to GeeSGet the application roles, stage roless Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute toThis is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to tEnhanced OperationsController.groovy to GeeSGet the application roles, stage roless
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.
Going to merge this down and fast follow-up with a few stylistic tweaks.
I'll tag @sanopsmx on the follow-up PR.
I believe there are still some logical issues with this PR ... going to revert it and let's continue the effort over on #4023. |
This PR verifies the current user roles against `context.selectedStageRoles`. The operation is _only_ allowed if there is at least one overlapping role between `context.selectedStageRoles` and the current user roles. `context.selectedStageRoles` maps to what is currently being specified in the UI. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role. This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role (or the user is an admin). This is a rewrite of what originally went in as spinnaker#3988.
…Roles. The operation is only allowed if there is at least one overlapping role (or the user is an admin). This is a rewrite of what originally went in as spinnaker#3988.
… judgment stage. (spinnaker#3988) * feat(pipeline executions/orca) : Added ability to add roles to manual judgment stage. This is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to Get the application roles, stage roles and the user roles. Check each of the user roles whether they are contained in the stage and application roles. Once the user role is contained in the manual judgment stage. We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions, if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages. if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages. If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true. Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute to the next stage or not. If yes/no, continue with the next stages/continues running the same stage. Enhanced ManualJudgmentStageSpec.groovy to Modified the testcases as per the requirement. Added ManualJudgmentAuthzGroupsUtil.groovy Added a utlity method to check authorized groups of the manual judgment stage. Added ManualJudgmentAuthzGroupsUtilSpec.groovy Added test cases check authorized groups method of ManualJudgmentAuthzGroupsUtil.groovy. * feat(pipeline executions/orca) : Added ability to add roles to manual judgment stage. This is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to Get the application roles, stage roles and the user roles. Check each of the user roles whether they are contained in the stage and application roles. Once the user role is contained in the manual judgment stage. We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions, if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages. if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages. If yes/no, set a isAuthorized flag to true/false in each of the stage. By default, all the stages except Manual Judgment are true. Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute toThis is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to tEnhanced OperationsController.groovy to GeeSGet the application roles, stage roless Enhanced ManualJudgmentStage.groovy to Check for the flag in ManualJudgmentStage.groovy whether to execute toThis is part of: spinnaker/spinnaker#4792. Enhanced OperationsController.groovy to tEnhanced OperationsController.groovy to GeeSGet the application roles, stage roless Co-authored-by: Adam Jordens <[email protected]>
…o manual judgment stage. (spinnaker#3988)" (spinnaker#4024) This reverts commit b8c5a7d.
…Roles. (spinnaker#4023) The operation is only allowed if there is at least one overlapping role (or the user is an admin). This is a rewrite of what originally went in as spinnaker#3988.
When is the spinnaker 1.25 release date? Is this fix going to be in 1.25 release? |
Even this deck fix should be in this release. |
feat(pipeline executions/orca) : Added ability to add roles to manual judgment stage.
This is part of: spinnaker/spinnaker#4792.
Enhanced ManualJudgmentStage.groovy to
Get the application roles, stage roles and the user roles.
Check each of the user roles whether they are contained in the stage and application roles.
Once the user role is contained in the manual judgment stage.
We check for whether the stage role has 'READ', 'WRITE', 'EXECUTE', 'CREATE' role in the application permissions,
if the role has application permission as 'READ', then the user will not be allowed to proceed further to subsequent(downstream) stages.
if the role has application permission as 'WRITE, EXECUTE,CREATE', then the user will be allowed to proceed further to subsequent stages.
If yes/no, set a isAuthorized flag to true/false by checking the userRoles, stageRoles and application Roles.
If yes/no, continue with the next stages/continues running the same stage.
Enhanced ManualJudgmentStageSpec.groovy to
Modified the testcases as per the requirement.
Added ManualJudgmentAuthzGroupsUtil.groovy
Added a utlity method to check authorized groups of the manual judgment stage.
Added ManualJudgmentAuthzGroupsUtilSpec.groovy
Added test cases check authorized groups method of ManualJudgmentAuthzGroupsUtil.groovy.