Actually a bit confusing when see `temporary-no-answer`. On Tue, May 31, 2016 at 6:43 PM, Adam Bordelon <[email protected]> wrote:
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 > >> > > > > > -- Best Regards, Haosdent Huang
