Firstly, I'm going to take this discussion to dev@, since I think it would be valuable information for the community and it doesn't contain any private information.
Secondly, we currently have a simple rule that should be sufficient for 1.0. If an authorization::Action ends in "_WITH_FOO", then an authorizer module writer knows to expect a Foo in the `Object.value` field. Otherwise, we need explicit documentation in a comment on the action explaining to the module writer what Object fields they should expect to find set. I tried to express this in https://reviews.apache.org/r/47876/ After 1.0, we can consider deprecating the _WITH_FOO's in favor of Object.FooInfo's. As for upgrading/deprecation, check out the approach we took for RUN_TASK_WITH_USER->RUN_TASK in https://reviews.apache.org/r/47891/ . Here we created RUN_TASK as an alias with the same enum value as RUN_TASK_WITH_USER, and set the legacy `value` field as expected by older authorizers, as well as the newer `framework_info` and `task_info` fields. This way, an old authorizer can recompile and continue to function with the old code, and can update to the new action at their leisure. However, as you point out, the authorizer API changed drastically in 0.29, so every authorizer module will have to recompile, making our careful deprecation of the authorization::Action quite unnecessary. cc: BenjaminB Re: 3 or 4 states (authorized, not authorized, unsupported, temporary-no-answer): An authorizer should be able to define how it wants to handle unknown/unsupported actions. In the case of the local authorizer, it could just return `permissive`. Proper error handling of unknown enum values is always considered best practice. >From the perspective of the caller (Mesos), there are only a few meaningful responses: authorized, forbidden, error. Error could be an error with the request/acl/parameters such that a retry would always result in another error; or the error could be something fixable with a retry, e.g. temporary unavailability of the ACL-provider service. In other parts of Mesos we've introduced Reason to differentiate the error types. On Mon, May 30, 2016 at 11:47 PM, Alex R <al...@apache.org> wrote: > +Till, +Alexander, +Adam since they were working on the authorizer > interface. > > MPark, thanks a lot for driving this while I'm out. I would like to > capture here more thoughts we've been discussing with you offline recently > and broaden the audience so other folks understand changes better. > > I would like to get rid / deprecate set/remove quota authz actions before > 0.29 (1.0.0) because it feels better to enter 1.0.0 state with a cleaner > API. Importantly, I would like to deprecate these action in favour update > quota in a *separate release* to where update quota endpoint is > introduced. This allows for a more straightforward upgrade path for > operators. > > To clarify my comment about updating the authorizer interface: I'm > reluctant to drive interface updates based on local needs; but of course it > is fine to update it once we are sure that the local change represents (or > will represent) a repeating pattern. > > I've looked at your patches and I think the approach you take is good: it > localizes hacks in local authorizer and quota handler, while keeping > general interface clean. > > With your patches, you move away from mentioning object in the action, > i.e. UPDATE_QUOTA instead of UPDATE_QUOTA_WITH_ROLE. I wonder, is it > something we should do for all other actions *prior to* 1.0.0? It looks > like authz interface may need a bit of refactoring—again!—to keep things > consistent. > > However, your approach does not solve the problem of upgrading and > deprecating authz actions in general. I think it is fine for now, but as > food for thought I would like us to think about what we should change in > the authorizer interface to allow easy deprecations. Maybe having four > states will help: authorized, not authorized, unsupported, > temporary-no-answer. > > > On 30 May 2016 at 19:48, Zhitao Li <zhitaoli...@gmail.com> wrote: > >> Hi Michael, >> >> I like the direction of the new patchset. I was not aware of the >> direction of new fields in `message Object` when working on initial patch, >> but this seems less technical debt and more expressive. >> >> One general comment: maybe document clearly how new fields like `task` or >> `quotaInfo` and `value` should be used together? This protobuf is not >> "internal" but new fields could confuse people who need to understand >> authorizer internal w/o documentation. >> >> >> >> On Mon, May 30, 2016 at 6:29 AM, Michael Park <mp...@apache.org> wrote: >> >>> If someone is maintaining a custom authorizer module, this is >>>> effectively an API change to them, and they need to think about three >>>> states (authorized/forbidden/unsupported) instead of true/false. >>>> >>> >>> While speaking to Vinod, he reminded me that we're actually breaking API >>> in 0.29.0 since we've introduced *authorized* which handles a *Request* >>> object, >>> as opposed to an *authorize* function per ACL. >>> https://github.com/apache/mesos/blob/0.28.1-rc2/include/mesos/authorizer/authorizer.hpp >>> >>> I'd also point out the previous migration* from ShutdownFramework to >>>> TeardownFramework *also forced custom authorizer module owner to >>>> handle the case IIUIC, so this is not even the first precedent. >>>> >>> >>> The backwards compatibility requirement there is for the >>> *ShutdownFramework* ACL, which is specific to *LocalAuthorizer*. >>> Note that we did not introduce a *SHUTDOWN_FRAMEWORK* action enum. >>> >>> Similarly, we can actually just remove the *SET_QUOTA* and >>> * REMOVE_QUOTA* actions, >>> but we need to continue to support the *SetQuota* and *RemoveQuota* ACLs >>> for the *LocalAuthorizer*. >>> >>> I remember that Alex said that he'd be "pretty reluctant to take a patch >>>> if the solution requires changing the Authorizer interface", so I >>>> think you should seriously consult with him before backing this out. >>>> >>> >>> Again, we're already breaking the Authorizer interface. >>> >>> --- >>> >>> I spoke to Alex, and from what I've gathered, he really wants to >>> deprecate the *SET_QUOTA* and *REMOVE_QUOTA* actions. >>> Similar to *SHUTDOWN_FRAMEWORK*, I don't think we need to introduce >>> these actions at all. >>> >>> Here is my proposal: >>> >>> - Remove the *SET_QUOTA_WITH_ROLE* and *REMOVE_QUOTA_WITH_PRINCIPAL* >>> actions. >>> - Rename *UPDATE_QUOTA_WITH_ROLE* to *UPDATE_QUOTA*. >>> - Introduce an *optional QuotaInfo quota_info;* to the >>> *authorization::Object*. >>> - *authorization::Object::quota_info* is the field that is set when >>> the action is *UPDATE_QUOTA*. >>> >>> For custom authorizers, they simply handle the *UPDATE_QUOTA* action, >>> and use the information available in *QuotaInfo* to >>> perform whatever authorization they please. >>> >>> For *LocalAuthorizer*, we still need a hack. We need one for the cases >>> where *SetQuota* and *RemoveQuota* ACLs >>> are used. Since *UPDATE_QUOTA* + *QuotaInfo* could be for *set* or >>> *remove*, we wouldn't know which ACL to use. >>> So we're actually missing a bit here. Rather than mucking with >>> setting/unsetting of *QuotaInfo*, I think a "cleaner" solution >>> is to use the *optional string value;* field to encode this >>> information. Since this field is not claimed to be set at the public API, >>> the deprecation strategy is to simply stop setting the field once we're >>> done with the deprecation cycle. >>> >>> The following are patches to implement this strategy. >>> >>> https://reviews.apache.org/r/48037/ >>> >>> https://reviews.apache.org/r/48038/ >>> >>> https://reviews.apache.org/r/48039/ >>> >>> https://reviews.apache.org/r/48040/ >>> >>> Thanks, >>> >>> MPark >>> >> >> >> >> -- >> Cheers, >> >> Zhitao Li >> > >
