-
-
Notifications
You must be signed in to change notification settings - Fork 752
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
Introduce 'scope' field in key value model #2669
Conversation
doc['value'] = symmetric_decrypt(KeyValuePairAPI.crypto_key, model.value) | ||
encrypted = False | ||
|
||
scope = getattr(model, 'scope', SYSTEM_KV_PREFIX) |
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 personally view scope and key prefix as two different things and they don't need to be the same (scope is a user visible feature and key prefix for a particular scope is an implementation detail).
In any case, I would have those scopes:
user
- User scopes, keys are prefixed with some common prefix for this scope (user
..fooor
_user..foo`)global
- Anything which doesn't have a prefix (so any other existing key). We could explicitly prefix keys with a global scope, but the question is how to handle the keys without the prefix then.
Or do you want to introduce also a concept of no scope?
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 like "user" and "global" scopes as well. I even anticipate more scopes like "environment" etc. Our current story for key value store is everything is global and you can "refer" to a variable via {{system.foo}} even though "foo" is the actual key name. So for backward compatibility, I am setting the scope to "system" and don't want to introduce another name "global". If you guys think this is a bad idea, we should talk. /cc: @manasdk
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 did design this from scratch we would have the scope
property and assign it to system
/global
by default. The other choice would be to have {{system.foo}}
and {{system.user.foo}}
where system
is still a prefix and what follows is possibly a scope i.e. user
or something else in the future if we allow for user configurable scopes. I actually do prefer system
transitioning to a scope although it is really from the scope perspective better named global. I wish I had been prescient and made space for this when I added ability to look up kv-store values in rules, actions etc.
Taking this forward it would mean that when {{system.foo}}
and {{user.foo}}
we now grab 2 top level words i.e. system
and user
for the context addressable space. Not ideal but not really much worse that what we have already. I feel we should only grab {{$user.foo}}
or some such obscure token where 2 purposes are served -
$
suggest it will be replaced$user
is less-likely to appear in an addressable context
from_model_kwargs=from_model_kwargs, | ||
**kwargs | ||
) | ||
kvp_db = kvp_db[0] if kvp_db else None |
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.
why/how would get_one by name+scope return multiple values?
I feel this code needs to be in the service layer where there is a method get_one(scope, name)
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.
why/how would get_one by name+scope return multiple values?
It is not returning more than one value. It simply returns an array with one item which is both counter intuitive and blows up CLI.
|
||
name = me.StringField(required=True, unique=True) | ||
value = me.StringField() | ||
secret = me.BooleanField(default=False) | ||
expire_timestamp = me.DateTimeField() | ||
scope = me.StringField(default=SYSTEM_KV_PREFIX) |
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 happens to old data in DB with new code? Still works?
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 captured it in a TODO item. Backward compatibility. Yet to be done.
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 what happens when I read code and dont read the description :P
@lakshmi-kannan What does |
@manasdk Not yet
|
@@ -242,6 +242,25 @@ def _get_from_model_kwargs_for_request(self, request): | |||
""" | |||
return self.from_model_kwargs | |||
|
|||
def _get_one_by_scope_and_name(self, scope, name, from_model_kwargs=None): |
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.
@manasdk FYI
* master: (62 commits) Update affected tests. Update affected test. Remove some unecessary duplicated variable declarations. Update a base RBAC tests class to insert a user with no permissions and role assignments. Add RBAC tests for runner types API endpoint. Update "_get_by_name_or_id" method so it's consistent with "_get_by_ref_or_id" and throws if a resource is not found. Update changelog. Remove now old and unused assertion methods. Update more RBAC related assertion methods and update existing tests to use those new methods. Add more developer-friendly assertion methods and update affected code to utilize those new methods. Fix lint. Add new assertion methods for asserting RBAC permissions grants and update affected tests to use those new methods. Add new _user_doesnt_have_resource_db_permissions assertion method which is the inverse of the existing one and update tests to use it. Add a test case for disabled runner. Update more affected tests. Update affected tests - add missing required "name" attribute to the RunnerTypeDB objects. Use getattr, weak typing ftl. Add RBAC resolver tests for runner type resolver. Add a test case for disabling and re-enabling a runner. Only allow "enabled" attribute of the runner to be updated via the API. ...
* master: Add a test case for it. Update changelog. Allow register-setup-virtualenvs flag to be used in combination with register-all in the `st2-register-content script. Add missing abspath call. Update changelog.# On branch pack_test_fixture_loading_utils Add new "get_fixture_content" method to all the pack resource test classes. Conflicts: CHANGELOG.rst
""" | ||
instance = self.access.get_by_scope_and_name(scope=scope, name=name) | ||
if not instance: | ||
return instance |
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.
_get_one_*
methods usually return 404 (pecan.abort
) if a object is not found (and _get_by_*
throw StackStormDBObjectNotFoundError
if an object is not found, and _get_one
methods usually call _get_by
methods).
We should do the same here for consistency and also add corresponding 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.
Also seems like this is kinda key value pair controller specific so maybe it should live in base key value pair controller or similar?
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 made that change (StackStormDBObjectNotFoundError) already. I'll wait for you to merge that and make the corresponding change here.
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.
Also seems like this is kinda key value pair controller specific so maybe it should live in base key value pair controller or similar?
I had it in KeyValuePairController and manas said move it to some base layer. Geez. Ignoring.
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.
Addressed StackStormDBObjectNotFoundError - feef877
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.
Lesson -> don't listen to manas :P
j/k
I've added some in-line comments. Overall, I'm fine with other changes, but as mentioned on Slack, I would remove v2 stuff from this PR. I think we all need to get on the same page and have a consistent story for the v2 API so we don't need to deprecate v2 yet again in the near future when we plan to work on an improved API. Also, what's our plan for RBAC here? Will this be done separately, or? |
What is wrong with it? is the v2 API bad already? |
@manasdk The problem is that it's a one off-thing. We need a consistent story and plan for it (that means how paths are going to look like, responses, etc.). I'm fine with adding it as soon as we have a consistent story and plan for it, because then we can make sure this new v2 API follows all of those conventions. Tomorrow I plan to start a discussion thread on v2 API and we can continue discussion about that there. |
Is it ad-hoc? Isn't this exactly the reason i.e. we are changing a model therefore need to have a new endpoint. I am fine with the discussion driving the impl and then coming around to picking up changes here but why would you suggest reverting the changes? |
@manasdk Yes, and the same end result can be achieved using v1 API so I see no point in rushing v2 API until we have a clear picture on how we want it to look. |
* master: (28 commits) register ACTIONEXECUTIONSTATE_XCHG register LIVEACTION_STATUS_MGMT_XCHG exchange by default For consistency, make sure that _get_by method doesn't throw. Fix assertion method name. Add tests for config schema API endpoints. Fix lint. Update packs.uninstall action to also delete corresponding ConfigSchemaDB object. Now that it's a top-level endpoint it makes sense to have "get all" / list endpoint. Don't use pack cache in the tests. Add tests for pack and config schema registrar. Update changelog. Add config schema fixture for dummy_pack_1. Fix lint, add missing file. Add API endpoints RBAC tests for pack config schema. Fix RBAC for Pack Config Schema. Add "get_by_pack" method and fix resource registrar. Add comment and assertion to detect programmer errors. Fix "_get_by_pack_ref" method to throw if the resource doesn't exist. Update affected test - this user is now added as part of the base class. Fix dummy pack metadata. ... Conflicts: CHANGELOG.rst
@Kami: > Also, what's our plan for RBAC here? Will this be done separately, or? You should read the description sometimes before complaining everywhere :P |
@@ -33,9 +34,12 @@ class RootController(BaseRootController): | |||
|
|||
def __init__(self): | |||
v1_controller = v1_root.RootController() | |||
v2_controller = v2_root.RootController() |
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.
Probably forgot to remove this?
TODO:
Prefix based RBAC will be in another PR. Trying to scope things down.
CLI examples:
GET
LIST
v2 API examples
PUT
DELETE
GET
v2 APIs with secrets