Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2986033003:
[scheduler]: ACLs phase 1 - per Job ACL specification and enforcement. (Closed)
|
Created:
3 years, 4 months ago by tandrii(chromium) Modified:
3 years, 4 months ago Reviewers:
Vadim Sh. CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org, nodir Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
Description[scheduler]: ACLs phase 1 - per Job ACL specification and enforcement.
This CL:
* allows specifying READER and OWNER acls per Job/Trigger definition:
* READER is who can see Job and **all** its invocations, including
those finished long ago under potentially more restrictive ACLs.
* OWNER is who can affect Job state manually through API or UI such
as `AbortJob` and `PauseJob`.
* boilerplate reduction by means of acl_set defined per project
and referenced in Job/Trigger definitions.
* ACL spec per Job/Trigger is **not yet** required. If not specified,
defaults to current behavior of READER=all OWNER=scheduler admins.
* ACLs introduced do not limit which Jobs can be triggered by which Triggers.
However, triggering functionality doesn't exist yet, so this doesn't matter.
On deployment:
* This CL has backwards compatibility and can be deployed without breaking
anything.
* Once deployed, this CL can be reverted and things will work as before.
This imples in particular that all configured READER ACLs will be ignored
and default to READER=all (see also warning in cron.proto).
* Plan:
1. Deploy this CL.
2. Update existing configs to specify ACLs with explicit READER=all and
OWNER=project-<NAME>-owners.
2. Deploy requirement to specify ACLs for all Jobs and remove currently
implicit default ACLs.
R=vadimsh@chromium.org
Bug=736770
Review-Url: https://codereview.chromium.org/2986033003
Committed: https://github.com/luci/luci-go/commit/0377d2719f9ea31ca5c1abd5b519ef25f9833571
Patch Set 1 #
Total comments: 4
Patch Set 2 : nodir@ review follow up #Patch Set 3 : Rename WRITER to OWNER. #Patch Set 4 : not enabled acls #Patch Set 5 : gofmt #Patch Set 6 : now safe to deploy #Patch Set 7 : pcg #
Total comments: 20
Patch Set 8 : review #Patch Set 9 : [WIP] ACLs into engine public API. #
Total comments: 5
Patch Set 10 : temp sync between laptops #Patch Set 11 : temp #Patch Set 12 : rework #
Total comments: 15
Patch Set 13 : Review. #Dependent Patchsets: Messages
Total messages: 127 (109 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2986033003/diff/1/scheduler/appengine/message... File scheduler/appengine/messages/cron.proto (right): https://codereview.chromium.org/2986033003/diff/1/scheduler/appengine/message... scheduler/appengine/messages/cron.proto:21: // "email:mail@example.com". Consider supporting prefix-less user emails, e.g. just "mail@example.com". That's what others do. https://cs.chromium.org/chromium/infra/luci/appengine/components/components/c... https://codereview.chromium.org/2986033003/diff/1/scheduler/appengine/message... scheduler/appengine/messages/cron.proto:23: string identity = 2; I think the term "identity" is reserved for one identity, not a group. The defined kinds of identities do not include "group" I recommend using another term
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2986033003/diff/1/scheduler/appengine/message... File scheduler/appengine/messages/cron.proto (right): https://codereview.chromium.org/2986033003/diff/1/scheduler/appengine/message... scheduler/appengine/messages/cron.proto:21: // "email:mail@example.com". On 2017/07/28 13:59:44, nodir wrote: > Consider supporting prefix-less user emails, e.g. just mailto:"mail@example.com". > That's what others do. > https://cs.chromium.org/chromium/infra/luci/appengine/components/components/c... Done. https://codereview.chromium.org/2986033003/diff/1/scheduler/appengine/message... scheduler/appengine/messages/cron.proto:23: string identity = 2; On 2017/07/28 13:59:44, nodir wrote: > I think the term "identity" is reserved for one identity, not a group. The > defined kinds of identities do not include "group" > > I recommend using another term You are right https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/server/... Changed.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks fine to me
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tandrii@chromium.org changed reviewers: - nodir@chromium.org
Vadim, PTAL I've not yet made use of ACLs in presentation and API. I can do it as this CL or as the next one, whichever way you prefer. Nodir to cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-go Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/37b2cf6f53195c10)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [WIP]: Scheduler: add ACLs definition to scheduler proto config. Inspired by buildbucket's ACLs, two permissions: READER & WRITER. These ACLs are specific to Job or Trigger definition, and translate to permissions on Job and its invocations. ACLs introduced so far do not control who can trigger what, but triggering functionality doesn't exist yet, so it doesn't matter. R=nodir@chromium.org, vadimsh@chromium.org Bug: 736770 ========== to ========== Scheduler ACLs phase 1: add definition to scheduler proto config. NO ACL enforcement is implemented yet. Once deployed and upon next refresh of task definitions from LUCI config service, datastore entity of Jobs will get updated with a new Acls field. However, nothing in the code expects Job.Acls to be set, so it's safe to deploy as well as revert. Inspired by buildbucket's ACLs, two permissions: READER & OWNER. These ACLs are specific to Job or Trigger definition, and translate to permissions on Job and its invocations. ACLs introduced so far do not control who can trigger what, but triggering functionality doesn't exist yet, so it doesn't matter. R=vadimsh@chromium.org Bug: 736770 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-go Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/37b379da50149a10)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Scheduler ACLs phase 1: add definition to scheduler proto config. NO ACL enforcement is implemented yet. Once deployed and upon next refresh of task definitions from LUCI config service, datastore entity of Jobs will get updated with a new Acls field. However, nothing in the code expects Job.Acls to be set, so it's safe to deploy as well as revert. Inspired by buildbucket's ACLs, two permissions: READER & OWNER. These ACLs are specific to Job or Trigger definition, and translate to permissions on Job and its invocations. ACLs introduced so far do not control who can trigger what, but triggering functionality doesn't exist yet, so it doesn't matter. R=vadimsh@chromium.org Bug: 736770 ========== to ========== Scheduler ACLs phase 1: add definition to scheduler proto config. NO ACL enforcement is implemented yet. Once deployed and upon next refresh of task definitions from LUCI config service, datastore entity of Jobs will get updated with a new Acls field. However, nothing in the code expects Job.Acls to be set, so it's safe to deploy as well as revert. Inspired by buildbucket's ACLs, two permissions: READER & OWNER. These ACLs are specific to Job or Trigger definition, and translate to permissions on Job and its invocations. ACLs introduced so far do not control who can trigger what, but triggering functionality doesn't exist yet, so it doesn't matter. R=vadimsh@chromium.org Bug: 736770 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
you can keep adding stuff to this CL https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... File scheduler/appengine/acl/acl.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:71: func ValidateAclSets(sets []*messages.AclSet) (AclSets, error) { you may or may not be interested in this thing: https://godoc.org/github.com/luci/luci-go/common/config/validation https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:130: } please validate granted_to: if it has prefix "group:", then make sure suffix is not empty. If it has no ':' then make sure identity.MakeIdentity("user:"+granted_to) doesn't return an error. Otherwise check that identity.MakeIdentity(granted_to) doesn't return an error. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:137: all := map[messages.Acl_Role]map[string]bool{ you may or may not be interested in https://godoc.org/github.com/luci/luci-go/common/data/stringset In particular its ToSlice method. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:169: if auth.CurrentIdentity(c).Email() == grant { this returns "" if CurretnIdentity is not of 'user' kind. A somewhat safer check would be if auth.CurrentIdentity(c) == identity.Identity("user:"+grant) { .... } https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:180: panic(fmt.Errorf("granted_to '%s' kind is not supported", grant)) just check auth.CurrentIdentity(c) == identity.Identity(grant) https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... File scheduler/appengine/catalog/catalog.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... scheduler/appengine/catalog/catalog.go:44: // jobIDRe is used to validate job ID field. remove this comment or add a similar one to aclSetIdRe https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... scheduler/appengine/catalog/catalog.go:46: aclSetIdRe = jobIDRe nit: aclSetIDRe https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... scheduler/appengine/catalog/catalog.go:250: return nil, errors.Annotate(err, "Invalid aclsets in a project %s", projectID).Err() I think we annotate with lower case https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:335: def.Acls.Equal(&e.Acls) && nit: swap e.Acls.Equal(&def.Acls)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:140001) has been deleted
Patchset #22 (id:430001) has been deleted
Patchset #8 (id:160001) has been deleted
Patchset #11 (id:220001) has been deleted
Patchset #20 (id:450001) has been deleted
Patchset #8 (id:180001) has been deleted
Patchset #12 (id:300001) has been deleted
Patchset #17 (id:410001) has been deleted
Patchset #16 (id:390001) has been deleted
Patchset #14 (id:350001) has been deleted
Patchset #9 (id:240001) has been deleted
Patchset #13 (id:370001) has been deleted
Patchset #12 (id:330001) has been deleted
Patchset #11 (id:70011) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #9 (id:280001) has been deleted
Patchset #8 (id:260001) has been deleted
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-go Mac Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/37b84904a475fe10)
ok, will do all as part of this CL, so ignore description for now. This patchset acts on all the comments % one. Key questions: 0. Does scheduler.cfg expose validation endpoint to Luci Config? I couldn't find it so far. Asking bcz I want to warn new scheduler.cfg revisions which don't specify ACLs of a job. 1. Is it good idea to add ACL understanding into engine itself s.t. GetAllJobs et al would apply filtering based on current user set in context? If Yes, how do I set and then detect that current identity is the scheduler AE itself? If Not, then I currently can't put into acl package due to circular dependency, though I can still put it into presentation/ and re-use in API and UI. Does this sound right? 2. For migration, I have to set default ACLs to OWNER=admins READER=all for all currently defined jobs. I intend to do so inside acl.ValidateTaskAcls. Next step would be adding a warning for all jobs without ACLs set. Next, going through all current configs and upgrading scheduler.cfg to new ACLs and while at it to not use taskdefwrapper. Next, requiring ACLs set for all jobs. Does this sound reasonable? https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... File scheduler/appengine/acl/acl.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:71: func ValidateAclSets(sets []*messages.AclSet) (AclSets, error) { On 2017/08/01 01:56:19, Vadim Sh. wrote: > you may or may not be interested in this thing: > https://godoc.org/github.com/luci/luci-go/common/config/validation Neat! I'm punting it to another CL to refactor catalog validation as well as acl. Btw, does scheduler service provide validation endpoint to be called by LUCI-config? https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:116: aclSetNameRe = regexp.MustCompile(`^[0-9A-Za-z_\-\.]{1,100}$`) changed here instead. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:130: } On 2017/08/01 01:56:18, Vadim Sh. wrote: > please validate granted_to: > > if it has prefix "group:", then make sure suffix is not empty. > If it has no ':' then make sure identity.MakeIdentity("user:"+granted_to) > doesn't return an error. > Otherwise check that identity.MakeIdentity(granted_to) doesn't return an error. Done. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:137: all := map[messages.Acl_Role]map[string]bool{ On 2017/08/01 01:56:19, Vadim Sh. wrote: > you may or may not be interested in > https://godoc.org/github.com/luci/luci-go/common/data/stringset > > In particular its ToSlice method. indeed I am :) Done. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:169: if auth.CurrentIdentity(c).Email() == grant { On 2017/08/01 01:56:19, Vadim Sh. wrote: > this returns "" if CurretnIdentity is not of 'user' kind. > > A somewhat safer check would be > > if auth.CurrentIdentity(c) == identity.Identity("user:"+grant) { > .... > } Done & refactored. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:180: panic(fmt.Errorf("granted_to '%s' kind is not supported", grant)) On 2017/08/01 01:56:19, Vadim Sh. wrote: > just check auth.CurrentIdentity(c) == identity.Identity(grant) Done. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... File scheduler/appengine/catalog/catalog.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... scheduler/appengine/catalog/catalog.go:44: // jobIDRe is used to validate job ID field. On 2017/08/01 01:56:19, Vadim Sh. wrote: > remove this comment or add a similar one to aclSetIdRe Done. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... scheduler/appengine/catalog/catalog.go:46: aclSetIdRe = jobIDRe On 2017/08/01 01:56:19, Vadim Sh. wrote: > nit: aclSetIDRe This isn't used actually, and name was wrong anyway. Removed. FTR, it's defined in acl.go. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ca... scheduler/appengine/catalog/catalog.go:250: return nil, errors.Annotate(err, "Invalid aclsets in a project %s", projectID).Err() On 2017/08/01 01:56:19, Vadim Sh. wrote: > I think we annotate with lower case OK, fixed. https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:335: def.Acls.Equal(&e.Acls) && On 2017/08/01 01:56:19, Vadim Sh. wrote: > nit: swap > > e.Acls.Equal(&def.Acls) Done.
On 2017/08/01 22:50:01, tandrii(chromium) wrote: > ok, will do all as part of this CL, so ignore description for now. This patchset > acts on all the comments % one. > > Key questions: > > 0. Does scheduler.cfg expose validation endpoint to Luci Config? I couldn't find > it so far. Asking bcz I want to warn new scheduler.cfg revisions which don't > specify ACLs of a job. > No, go luci-config client has no integration with luci-config's validation framework. > 1. Is it good idea to add ACL understanding into engine itself s.t. GetAllJobs > et al would apply filtering based on current user set in context? Yeah, I think it makes sense... > If Yes, how do I set and then detect that current identity is the scheduler AE > itself? I think we'll have two variants of methods: - one (private) doesn't do any ACL checks and just returns stuff - second (public) uses the first, but also checks ACLs Scheduler's guts will use first one. In practice I think it already works that way (scheduler's guts don't use GetAllJobs etc). > If Not, then I currently can't put into acl package due to circular > dependency, though I can still put it into presentation/ and re-use in API and > UI. Does this sound right? > > 2. For migration, I have to set default ACLs to OWNER=admins READER=all for all > currently defined jobs. I intend to do so inside acl.ValidateTaskAcls. Next step > would be adding a warning for all jobs without ACLs set. > Next, going through all current configs and upgrading scheduler.cfg to new ACLs > and while at it to not use taskdefwrapper. Next, requiring ACLs set for all > jobs. > Does this sound reasonable? > Yes, sounds great. Thanks for noticing and dealing with taskdefwrapper thing :)
https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... File scheduler/appengine/acl/acl.go (right): https://codereview.chromium.org/2986033003/diff/120001/scheduler/appengine/ac... scheduler/appengine/acl/acl.go:71: func ValidateAclSets(sets []*messages.AclSet) (AclSets, error) { On 2017/08/01 22:50:01, tandrii(chromium) wrote: > On 2017/08/01 01:56:19, Vadim Sh. wrote: > > you may or may not be interested in this thing: > > https://godoc.org/github.com/luci/luci-go/common/config/validation > > Neat! I'm punting it to another CL to refactor catalog validation as well as > acl. > Btw, does scheduler service provide validation endpoint to be called by > LUCI-config? No.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this is getting more involved than I hoped, but at least I know codebase much better. Still not finished, though READ acls are are already enforced in UI and API. Mainly posting to get feedback before I commit into more refactoring of internal engine Public/Private API. https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:74: GetAllJobsRA(c context.Context) ([]*Job, error) RA here and below stands for READER Access. I wish I can get rid of that prefix, but GetAllProjects will be an outlier then :( https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:87: ListInvocations(c context.Context, jobID string, pageSize int, cursor string) ([]*Invocation, string, error) this and two methods below will fetch JobId concurrently (or sequentially for ...byNonce) to ensure ACLs. Internal engine will call lowercase versions. SGTM? https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:133: TriggerInvocation(c context.Context, jobID string, triggeredBy identity.Identity) (int64, error) this and methods below: i want to remove triggeredBy and instead use whatever is in context and enforcing OWNER acl in the process. WDYT?
Not a fan of RA.
Let's split Engine into two: API that checks ACLs, and API for internal usage.
type Engine interface {
GetVisibleProjects()
GetVisibleJobs(...) ...
...
InternalAPI() EngineInternal
}
type EngineInternal interface {
GetAllProjects(...)
}
Where EngineInternal API doesn't do any ACL checks.
https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:87: ListInvocations(c context.Context, jobID string, pageSize int, cursor string) ([]*Invocation, string, error) On 2017/08/02 20:23:46, tandrii(chromium) wrote: > this and two methods below will fetch JobId concurrently (or sequentially for > ...byNonce) to ensure ACLs. Internal engine will call lowercase versions. SGTM? Yes. https://codereview.chromium.org/2986033003/diff/490001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:133: TriggerInvocation(c context.Context, jobID string, triggeredBy identity.Identity) (int64, error) On 2017/08/02 20:23:46, tandrii(chromium) wrote: > this and methods below: i want to remove triggeredBy and instead use whatever is > in context and enforcing OWNER acl in the process. WDYT? sgtm
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal I changed slightly your proposal: instead of InternalAPI in Engine (only called by frontend/handler), I made PublicAPI in internal api, which is what frontend/handler then passes to API server and UI. WDYT?
looks reasonable https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:68: // * if caller lacks have READER access to Jobs, methods behave as if Jobs "lacks have" ? https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:70: // * if caller lacks have OWNER access, calling mutating methods will result in same here https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:75: // GetVisibleProjects(c context.Context) ([]string, error) remove I assume it's not needed now? https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:171: PublicAPI() Engine document https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:687: return job, err nit: return job, nil https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:733: if job, err := e.GetVisibleJob(c, jobID); err != nil { strictly speaking, it would be better if we put ACLs into separate subentity of main Job entity (in fact, duplicate it there, keeping it also in Job), to avoid fetching frequently changing Job entity all the time (experiencing dscache cache misses). In practice it probably doesn't matter, QPS here is negligible. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:1189: if _, err := e.getOwnedJob(c, jobID); err != nil { why not move it into 'setPausedFlag'?
Description was changed from ========== Scheduler ACLs phase 1: add definition to scheduler proto config. NO ACL enforcement is implemented yet. Once deployed and upon next refresh of task definitions from LUCI config service, datastore entity of Jobs will get updated with a new Acls field. However, nothing in the code expects Job.Acls to be set, so it's safe to deploy as well as revert. Inspired by buildbucket's ACLs, two permissions: READER & OWNER. These ACLs are specific to Job or Trigger definition, and translate to permissions on Job and its invocations. ACLs introduced so far do not control who can trigger what, but triggering functionality doesn't exist yet, so it doesn't matter. R=vadimsh@chromium.org Bug: 736770 ========== to ========== [scheduler]: ACLs phase 1 - per Job ACL specification and enforcement. This CL: * allows specifying READER and OWNER acls per Job/Trigger definition: * READER is who can see Job and **all** its invocations, including those finished long ago under potentially more restrictive ACLs. * OWNER is who can affect Job state manually through API or UI such as `AbortJob` and `PauseJob`. * boilerplate reduction by means of acl_set defined per project and referenced in Job/Trigger definitions. * ACL spec per Job/Trigger is **not yet** required. If not specified, defaults to current behavior of READER=all OWNER=scheduler admins. * ACLs introduced do not limit which Jobs can be triggered by which Triggers. However, triggering functionality doesn't exist yet, so this doesn't matter. On deployment: * This CL has backwards compatibility and can be deployed without breaking anything. * Once deployed, this CL can be reverted and things will work as before. This imples in particular that all configured READER ACLs will be ignored and default to READER=all (see also warning in cron.proto). * Plan: 1. Deploy this CL. 2. Update existing configs to specify ACLs with explicit READER=all and OWNER=project-<NAME>-owners. 2. Deploy requirement to specify ACLs for all Jobs and remove currently implicit default ACLs. R=vadimsh@chromium.org Bug=736770 ==========
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[scheduler]: ACLs phase 1 - per Job ACL specification and enforcement.
This CL:
* allows specifying READER and OWNER acls per Job/Trigger definition:
* READER is who can see Job and **all** its invocations, including
those finished long ago under potentially more restrictive ACLs.
* OWNER is who can affect Job state manually through API or UI such
as `AbortJob` and `PauseJob`.
* boilerplate reduction by means of acl_set defined per project
and referenced in Job/Trigger definitions.
* ACL spec per Job/Trigger is **not yet** required. If not specified,
defaults to current behavior of READER=all OWNER=scheduler admins.
* ACLs introduced do not limit which Jobs can be triggered by which Triggers.
However, triggering functionality doesn't exist yet, so this doesn't matter.
On deployment:
* This CL has backwards compatibility and can be deployed without breaking
anything.
* Once deployed, this CL can be reverted and things will work as before.
This imples in particular that all configured READER ACLs will be ignored
and default to READER=all (see also warning in cron.proto).
* Plan:
1. Deploy this CL.
2. Update existing configs to specify ACLs with explicit READER=all and
OWNER=project-<NAME>-owners.
2. Deploy requirement to specify ACLs for all Jobs and remove currently
implicit default ACLs.
R=vadimsh@chromium.org
Bug=736770
==========
to
==========
[scheduler]: ACLs phase 1 - per Job ACL specification and enforcement.
This CL:
* allows specifying READER and OWNER acls per Job/Trigger definition:
* READER is who can see Job and **all** its invocations, including
those finished long ago under potentially more restrictive ACLs.
* OWNER is who can affect Job state manually through API or UI such
as `AbortJob` and `PauseJob`.
* boilerplate reduction by means of acl_set defined per project
and referenced in Job/Trigger definitions.
* ACL spec per Job/Trigger is **not yet** required. If not specified,
defaults to current behavior of READER=all OWNER=scheduler admins.
* ACLs introduced do not limit which Jobs can be triggered by which Triggers.
However, triggering functionality doesn't exist yet, so this doesn't matter.
On deployment:
* This CL has backwards compatibility and can be deployed without breaking
anything.
* Once deployed, this CL can be reverted and things will work as before.
This imples in particular that all configured READER ACLs will be ignored
and default to READER=all (see also warning in cron.proto).
* Plan:
1. Deploy this CL.
2. Update existing configs to specify ACLs with explicit READER=all and
OWNER=project-<NAME>-owners.
2. Deploy requirement to specify ACLs for all Jobs and remove currently
implicit default ACLs.
R=vadimsh@chromium.org
Bug=736770
==========
PTAL incl. new description. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:68: // * if caller lacks have READER access to Jobs, methods behave as if Jobs On 2017/08/04 23:07:26, Vadim Sh. wrote: > "lacks have" ? Done. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:70: // * if caller lacks have OWNER access, calling mutating methods will result in On 2017/08/04 23:07:27, Vadim Sh. wrote: > same here Done. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:75: // GetVisibleProjects(c context.Context) ([]string, error) On 2017/08/04 23:07:27, Vadim Sh. wrote: > remove > > I assume it's not needed now? Yep, Done. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:171: PublicAPI() Engine On 2017/08/04 23:07:27, Vadim Sh. wrote: > document Done. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:687: return job, err On 2017/08/04 23:07:26, Vadim Sh. wrote: > nit: return job, nil Done. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:733: if job, err := e.GetVisibleJob(c, jobID); err != nil { On 2017/08/04 23:07:27, Vadim Sh. wrote: > strictly speaking, it would be better if we put ACLs into separate subentity of > main Job entity (in fact, duplicate it there, keeping it also in Job), to avoid > fetching frequently changing Job entity all the time (experiencing dscache cache > misses). > > In practice it probably doesn't matter, QPS here is negligible. First, since API and UI QPS is indeed negligible, I'd rather do this later since IIUC it will still be easily doable once this CL lands. Second, do IIUC that you suggest to create smth like type JobACLs struct { _kind string `gae:"$kind,JobACLs"` _extra ds.PropertyMap `gae:"-,extra"` // JobKey is the key of parent Job entity. JobKey *ds.Key `gae:"$parent"` Acls acl.GrantsByRole `gae:",noindex"` } If so, i think i'll need to update ACLs of a job in a transaction to avoid inconsistent ACLs, right? I've added a TODO to consider doing this in the future. https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:1189: if _, err := e.getOwnedJob(c, jobID); err != nil { On 2017/08/04 23:07:26, Vadim Sh. wrote: > why not move it into 'setPausedFlag'? Good point. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... File scheduler/appengine/engine/engine.go (right): https://codereview.chromium.org/2986033003/diff/550001/scheduler/appengine/en... scheduler/appengine/engine/engine.go:733: if job, err := e.GetVisibleJob(c, jobID); err != nil { On 2017/08/07 12:48:17, tandrii(chromium) wrote: > On 2017/08/04 23:07:27, Vadim Sh. wrote: > > strictly speaking, it would be better if we put ACLs into separate subentity > of > > main Job entity (in fact, duplicate it there, keeping it also in Job), to > avoid > > fetching frequently changing Job entity all the time (experiencing dscache > cache > > misses). > > > > In practice it probably doesn't matter, QPS here is negligible. > > First, since API and UI QPS is indeed negligible, I'd rather do this later since > IIUC it will still be easily doable once this CL lands. > Sure, it was just a general thought. I don't think we need to do it anytime soon. > Second, do IIUC that you suggest to create smth like > > type JobACLs struct { > _kind string `gae:"$kind,JobACLs"` > _extra ds.PropertyMap `gae:"-,extra"` > > // JobKey is the key of parent Job entity. > JobKey *ds.Key `gae:"$parent"` > > Acls acl.GrantsByRole `gae:",noindex"` > } > > If so, i think i'll need to update ACLs of a job in a transaction to avoid > inconsistent ACLs, right? Right. > > I've added a TODO to consider doing this in the future.
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 570001, "attempt_start_ts": 1502178167041480,
"parent_rev": "d009bb905fb8f3f9d23f477c91039b055c0cf857", "commit_rev":
"0377d2719f9ea31ca5c1abd5b519ef25f9833571"}
Message was sent while issue was closed.
Description was changed from
==========
[scheduler]: ACLs phase 1 - per Job ACL specification and enforcement.
This CL:
* allows specifying READER and OWNER acls per Job/Trigger definition:
* READER is who can see Job and **all** its invocations, including
those finished long ago under potentially more restrictive ACLs.
* OWNER is who can affect Job state manually through API or UI such
as `AbortJob` and `PauseJob`.
* boilerplate reduction by means of acl_set defined per project
and referenced in Job/Trigger definitions.
* ACL spec per Job/Trigger is **not yet** required. If not specified,
defaults to current behavior of READER=all OWNER=scheduler admins.
* ACLs introduced do not limit which Jobs can be triggered by which Triggers.
However, triggering functionality doesn't exist yet, so this doesn't matter.
On deployment:
* This CL has backwards compatibility and can be deployed without breaking
anything.
* Once deployed, this CL can be reverted and things will work as before.
This imples in particular that all configured READER ACLs will be ignored
and default to READER=all (see also warning in cron.proto).
* Plan:
1. Deploy this CL.
2. Update existing configs to specify ACLs with explicit READER=all and
OWNER=project-<NAME>-owners.
2. Deploy requirement to specify ACLs for all Jobs and remove currently
implicit default ACLs.
R=vadimsh@chromium.org
Bug=736770
==========
to
==========
[scheduler]: ACLs phase 1 - per Job ACL specification and enforcement.
This CL:
* allows specifying READER and OWNER acls per Job/Trigger definition:
* READER is who can see Job and **all** its invocations, including
those finished long ago under potentially more restrictive ACLs.
* OWNER is who can affect Job state manually through API or UI such
as `AbortJob` and `PauseJob`.
* boilerplate reduction by means of acl_set defined per project
and referenced in Job/Trigger definitions.
* ACL spec per Job/Trigger is **not yet** required. If not specified,
defaults to current behavior of READER=all OWNER=scheduler admins.
* ACLs introduced do not limit which Jobs can be triggered by which Triggers.
However, triggering functionality doesn't exist yet, so this doesn't matter.
On deployment:
* This CL has backwards compatibility and can be deployed without breaking
anything.
* Once deployed, this CL can be reverted and things will work as before.
This imples in particular that all configured READER ACLs will be ignored
and default to READER=all (see also warning in cron.proto).
* Plan:
1. Deploy this CL.
2. Update existing configs to specify ACLs with explicit READER=all and
OWNER=project-<NAME>-owners.
2. Deploy requirement to specify ACLs for all Jobs and remove currently
implicit default ACLs.
R=vadimsh@chromium.org
Bug=736770
Review-Url: https://codereview.chromium.org/2986033003
Committed:
https://github.com/luci/luci-go/commit/0377d2719f9ea31ca5c1abd5b519ef25f9833571
==========
Message was sent while issue was closed.
Committed patchset #13 (id:570001) as https://github.com/luci/luci-go/commit/0377d2719f9ea31ca5c1abd5b519ef25f9833571 |
