Hi, This introduces a possibly uninitialized member `weight_info` which Coverity immediately detected. I filed MESOS-6604 for that. Could you please take that on @Alexander?
Cheers, Benjamin > On Nov 16, 2016, at 6:00 PM, [email protected] wrote: > > Enabled multiple field based authorization in the authorizer interface. > > Updates the authorizer interfaces and well as the local authorizer, > such that all actions which were limited to use a _role_ or a > _principal_ as an object, are able to use whole protobuf messages > as objects. This change enables more sofisticated authorization > mechanisms. > > Review: https://reviews.apache.org/r/52600/ > > > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/bc0e6d7b > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/bc0e6d7b > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/bc0e6d7b > > Branch: refs/heads/master > Commit: bc0e6d7b0b367e5ff67dd5f395e1e06938b02399 > Parents: 40c2e5f > Author: Alexander Rojas <[email protected]> > Authored: Tue Nov 15 19:04:25 2016 -0800 > Committer: Adam B <[email protected]> > Committed: Wed Nov 16 01:55:03 2016 -0800 > > ---------------------------------------------------------------------- > include/mesos/authorizer/authorizer.hpp | 6 +- > include/mesos/authorizer/authorizer.proto | 54 ++++++++++++ > src/authorizer/local/authorizer.cpp | 115 +++++++++++++++++++++---- > 3 files changed, 157 insertions(+), 18 deletions(-) > ---------------------------------------------------------------------- > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/bc0e6d7b/include/mesos/authorizer/authorizer.hpp > ---------------------------------------------------------------------- > diff --git a/include/mesos/authorizer/authorizer.hpp > b/include/mesos/authorizer/authorizer.hpp > index cb365c7..7217600 100644 > --- a/include/mesos/authorizer/authorizer.hpp > +++ b/include/mesos/authorizer/authorizer.hpp > @@ -61,7 +61,9 @@ public: > task_info(object.has_task_info() ? &object.task_info() : nullptr), > executor_info( > object.has_executor_info() ? &object.executor_info() : nullptr), > - quota_info(object.has_quota_info() ? &object.quota_info() : nullptr) > {} > + quota_info(object.has_quota_info() ? &object.quota_info() : nullptr), > + weight_info(object.has_weight_info() ? &object.weight_info() : > nullptr), > + resource(object.has_resource() ? &object.resource() : nullptr) {} > > const std::string* value; > const FrameworkInfo* framework_info; > @@ -69,6 +71,8 @@ public: > const TaskInfo* task_info; > const ExecutorInfo* executor_info; > const quota::QuotaInfo* quota_info; > + const WeightInfo* weight_info; > + const Resource* resource; > }; > > /** > > http://git-wip-us.apache.org/repos/asf/mesos/blob/bc0e6d7b/include/mesos/authorizer/authorizer.proto > ---------------------------------------------------------------------- > diff --git a/include/mesos/authorizer/authorizer.proto > b/include/mesos/authorizer/authorizer.proto > index b6a9f14..0696a62 100644 > --- a/include/mesos/authorizer/authorizer.proto > +++ b/include/mesos/authorizer/authorizer.proto > @@ -46,11 +46,17 @@ message Object { > optional TaskInfo task_info = 4; > optional ExecutorInfo executor_info = 5; > optional quota.QuotaInfo quota_info = 6; > + optional WeightInfo weight_info = 7; > + optional Resource resource = 8; > } > > > // List of authorizable actions supported in Mesos. > +// NOTE: Values in this enum should be kept in > +// numerical order to prevent accidental aliasing. > enum Action { > + option allow_alias = true; > + > // This must be the first enum value in this list, to > // ensure that if 'type' is not set, the default value > // is UNKNOWN. This enables enum values to be added > @@ -58,19 +64,67 @@ enum Action { > UNKNOWN = 0; > > // Actions named *_WITH_foo may set a foo in `Object.value`. > + > + // `REGISTER_FRAMEWORK` will have an object with `FrameworkInfo` set. > + // The `_WITH_ROLE` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + REGISTER_FRAMEWORK = 1; > REGISTER_FRAMEWORK_WITH_ROLE = 1; > > // `RUN_TASK` will have an object with `FrameworkInfo` and `TaskInfo` set. > RUN_TASK = 2; > > + // `TEARDOWN_FRAMEWORK` will have an object with `FrameworkInfo` set. > + // The `_WITH_PRINCIPAL` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + TEARDOWN_FRAMEWORK = 3; > TEARDOWN_FRAMEWORK_WITH_PRINCIPAL = 3; > + > + // `RESERVE_RESOURCES` will have an object with `Resource` set. > + // The `_WITH_ROLE` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + RESERVE_RESOURCES = 4; > RESERVE_RESOURCES_WITH_ROLE = 4; > + > + // `UNRESERVE_RESOURCES` will have an object with `Resource` set. > + // The `_WITH_PRINCIPAL` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + UNRESERVE_RESOURCES = 5; > UNRESERVE_RESOURCES_WITH_PRINCIPAL = 5; > + > + // `CREATE_VOLUME` will have an object with `Resource` set. > + // The `_WITH_ROLE` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + CREATE_VOLUME = 6; > CREATE_VOLUME_WITH_ROLE = 6; > + > + // `DESTROY_VOLUME` will have an object with `Resource` set. > + // The `_WITH_PRINCIPAL` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + DESTROY_VOLUME = 7; > DESTROY_VOLUME_WITH_PRINCIPAL = 7; > + > GET_ENDPOINT_WITH_PATH = 8; > VIEW_ROLE = 9; > + > + // `UPDATE_WEIGHT` will have an object with `WeightInfo` set. > + // The `_WITH_ROLE` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + UPDATE_WEIGHT = 10; > UPDATE_WEIGHT_WITH_ROLE = 10; > + > + // `GET_QUOTA` will have an object with `QuotaInfo` set. > + // The `_WITH_ROLE` alias is deprecated and will be removed after > + // Mesos 1.2's deprecation cycle ends. The `value` field will continue > + // to be set until that time. > + GET_QUOTA = 11; > GET_QUOTA_WITH_ROLE = 11; > > // `UPDATE_QUOTA` will have an object with a `QuotaInfo` set. > > http://git-wip-us.apache.org/repos/asf/mesos/blob/bc0e6d7b/src/authorizer/local/authorizer.cpp > ---------------------------------------------------------------------- > diff --git a/src/authorizer/local/authorizer.cpp > b/src/authorizer/local/authorizer.cpp > index f1dff65..77e05dd 100644 > --- a/src/authorizer/local/authorizer.cpp > +++ b/src/authorizer/local/authorizer.cpp > @@ -221,15 +221,7 @@ public: > } else { > switch (action_) { > // All actions using `object.value` for authorization. > - case authorization::REGISTER_FRAMEWORK_WITH_ROLE: > - case authorization::TEARDOWN_FRAMEWORK_WITH_PRINCIPAL: > - case authorization::RESERVE_RESOURCES_WITH_ROLE: > - case authorization::UNRESERVE_RESOURCES_WITH_PRINCIPAL: > - case authorization::CREATE_VOLUME_WITH_ROLE: > - case authorization::DESTROY_VOLUME_WITH_PRINCIPAL: > - case authorization::GET_QUOTA_WITH_ROLE: > case authorization::VIEW_ROLE: > - case authorization::UPDATE_WEIGHT_WITH_ROLE: > case authorization::GET_ENDPOINT_WITH_PATH: { > // Check object has the required types set. > CHECK_NOTNULL(object->value); > @@ -239,6 +231,92 @@ public: > > break; > } > + case authorization::REGISTER_FRAMEWORK: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->framework_info) { > + aclObject.add_values(object->framework_info->role()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > + case authorization::TEARDOWN_FRAMEWORK: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->framework_info) { > + aclObject.add_values(object->framework_info->principal()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > + case authorization::CREATE_VOLUME: > + case authorization::RESERVE_RESOURCES: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->resource) { > + aclObject.add_values(object->resource->role()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > + case authorization::DESTROY_VOLUME: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->resource) { > + aclObject.add_values( > + object->resource->disk().persistence().principal()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > + case authorization::UNRESERVE_RESOURCES: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->resource) { > + > aclObject.add_values(object->resource->reservation().principal()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > + case authorization::GET_QUOTA: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->quota_info) { > + aclObject.add_values(object->quota_info->role()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > + case authorization::UPDATE_WEIGHT: { > + aclObject.set_type(mesos::ACL::Entity::SOME); > + if (object->weight_info) { > + aclObject.add_values(object->weight_info->role()); > + } else if (object->value) { > + aclObject.add_values(*(object->value)); > + } else { > + aclObject.set_type(mesos::ACL::Entity::ANY); > + } > + > + break; > + } > case authorization::RUN_TASK: { > aclObject.set_type(mesos::ACL::Entity::SOME); > if (object->task_info && object->task_info->has_command() && > @@ -253,6 +331,7 @@ public: > } else { > aclObject.set_type(mesos::ACL::Entity::ANY); > } > + > break; > } > case authorization::ACCESS_MESOS_LOG: { > @@ -497,7 +576,7 @@ private: > vector<GenericACL> acls_; > > switch (action) { > - case authorization::REGISTER_FRAMEWORK_WITH_ROLE: > + case authorization::REGISTER_FRAMEWORK: > foreach ( > const ACL::RegisterFramework& acl, acls.register_frameworks()) { > GenericACL acl_; > @@ -509,7 +588,7 @@ private: > > return acls_; > break; > - case authorization::TEARDOWN_FRAMEWORK_WITH_PRINCIPAL: > + case authorization::TEARDOWN_FRAMEWORK: > foreach ( > const ACL::TeardownFramework& acl, acls.teardown_frameworks()) { > GenericACL acl_; > @@ -532,7 +611,7 @@ private: > > return acls_; > break; > - case authorization::RESERVE_RESOURCES_WITH_ROLE: > + case authorization::RESERVE_RESOURCES: > foreach (const ACL::ReserveResources& acl, acls.reserve_resources()) { > GenericACL acl_; > acl_.subjects = acl.principals(); > @@ -543,7 +622,7 @@ private: > > return acls_; > break; > - case authorization::UNRESERVE_RESOURCES_WITH_PRINCIPAL: > + case authorization::UNRESERVE_RESOURCES: > foreach ( > const ACL::UnreserveResources& acl, acls.unreserve_resources()) { > GenericACL acl_; > @@ -555,7 +634,7 @@ private: > > return acls_; > break; > - case authorization::CREATE_VOLUME_WITH_ROLE: > + case authorization::CREATE_VOLUME: > foreach (const ACL::CreateVolume& acl, acls.create_volumes()) { > GenericACL acl_; > acl_.subjects = acl.principals(); > @@ -566,7 +645,7 @@ private: > > return acls_; > break; > - case authorization::DESTROY_VOLUME_WITH_PRINCIPAL: > + case authorization::DESTROY_VOLUME: > foreach (const ACL::DestroyVolume& acl, acls.destroy_volumes()) { > GenericACL acl_; > acl_.subjects = acl.principals(); > @@ -577,7 +656,7 @@ private: > > return acls_; > break; > - case authorization::GET_QUOTA_WITH_ROLE: > + case authorization::GET_QUOTA: > foreach (const ACL::GetQuota& acl, acls.get_quotas()) { > GenericACL acl_; > acl_.subjects = acl.principals(); > @@ -629,7 +708,7 @@ private: > > return acls_; > break; > - case authorization::UPDATE_WEIGHT_WITH_ROLE: > + case authorization::UPDATE_WEIGHT: > foreach (const ACL::UpdateWeight& acl, acls.update_weights()) { > GenericACL acl_; > acl_.subjects = acl.principals(); > @@ -845,7 +924,9 @@ process::Future<bool> LocalAuthorizer::authorized( > request.object().has_task() || > request.object().has_task_info() || > request.object().has_executor_info() || > - request.object().has_quota_info()))); > + request.object().has_quota_info() || > + request.object().has_weight_info() || > + request.object().has_resource()))); > > typedef Future<bool> (LocalAuthorizerProcess::*F)( > const authorization::Request&); >
