Update patch set 15
Patch Set 15:
(1 comment)
Patch-set: 15
Attention: {"person_ident":"Gerrit User 27900 \u003c27900@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_28356\u003e replied on the change"}
Attention: {"person_ident":"Gerrit User 28356 \u003c28356@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_28356\u003e replied on the change"}
This commit is contained in:
Gerrit User 28356
Gerrit Code Review
parent
2449aa8040
commit
918345bdaa
|
|
@ -51,6 +51,24 @@
|
|||
"message": "I am really sorry to wrongly blame you for introducing the domain attribute.\nYou touched so much code around it and documented it in a way that immediately\nunderlines the issue for me that I made a conclusion you introduced it without\ndooing a deeper check on git blame (btw, I did not mean that you previous\nchange in whole was unnecessary, it only it uncovered certain issues).\n\nLet me start from describing the issue with the domain as a showcase of\npotential issues what is later deepen by adding of the projects_json. You\ndocumented mapping example (in the domain spec and repeat here) as:\n```\n\"local\": [\n {\n \"domain\": {\n \"name\": \"{2}\"\n },\n \"user\": {\n \"domain\": {\n \"name\": \"{2}\"\n },\n ...\n },\n \"projects\": [\n {\n \"domain\": {\n \"name\": \"{2}\"\n },\n ...\n }\n ]\n }\n```\n\nThere are 3 places that define the domain name. It enables following bug:\n\n```\n\"local\": [\n {\n \"domain\": {\n \"name\": \"{1}\"\n },\n \"user\": {\n \"domain\": {\n \"name\": \"{2}\"\n },\n ...\n },\n \"projects\": [\n {\n \"domain\": {\n \"name\": \"{3}\"\n },\n ...\n }\n ]\n }\n```\n\nCan user or even developer tell what will happen in this case without looking\ninto the code?\nThis is very nondeterministic and error-prone (both for keystone developers and\nusers). Even if we skip looking at \"projects\" for now there are 2 places where\nvalue can become different either by accident on the operator side or by the\nkeystone maintainer in the treatment and deciding on what takes precedence. It\ncan be even implemented correctly but later corrupted during some refactoring.\nAnd there is no chance to catch this error with input validation. I am not\nfocusing on the \"domain\" as an issue of this spec, I just describe the issue.\n\nNow lets have a look at \"projects_json\". Is there something what prevents\nfollowing?\n\n\n\"local\": [\n {\n \"user\": {\n \"domain\": {\n \"name\": \"{1}\"\n }\n },\n \"domain\": {\n \"name\": \"{2}\"\n },\n \"projects\": [\n {\n \"name\": \"{3}\",\n }\n ],\n \"projects_json\":\"{4}\"\n }\n],\n```\n\nThis is even more nondeterministic.\n\n- operator does not know how it is going to be handled\n- no possibility for input validation (without ugly hacks)\n- error prone since it requires \"if\" branches in the code with order of\n branches playing significant role. Having \"if\" in 2 places in the code where\n they check \"projects or projects_json\" in different order leads to undefined\n behavior and bugs. An now imagine there is some support for the mapping api in CLI or other tool where similar validation need to be implemented (and surely it is going to be done there differently because ...)\n- and again - this style differs from ALL other OpenStack apis (at least of the\n core ones)\n\nInstead making \"projects\" be an object OR a string allows you to be absolutely deterministic and do reasonable input validation (look at Nova/Cinder/Neutron API for exactly same pattern). Also in the code directly the chance of introducing bug is much lower since you more or less cover finite enum kinds. Once doing that I would directly extend the \"projects\" object schema to include \"domain\" similarly to all other places across OpenStack where user/project/group/etc are having direct attribute \"domain\". Later (separately) we could address preventing domain value conflicting or deprecate it as such (on the upper level).\n\nI am not intending to crucify anybody, I am just underlining an API design that is different from everything else in OpenStack and is introducing potential bugs. After spending last 6 years deep in the wild jungle of OpenStack APIs (from the tooling side) I notice such things and want us not to introduce new deviations and error-prone patterns.\n\nI will still keep my \"-1\" due to the reasons outlined above and let Keystone cores decide.",
|
||||
"revId": "8305797037e04aa67186e4aec1e3ee8adfd25ab9",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": false,
|
||||
"key": {
|
||||
"uuid": "b788664e_f5c8b476",
|
||||
"filename": "/PATCHSET_LEVEL",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 0,
|
||||
"author": {
|
||||
"id": 28356
|
||||
},
|
||||
"writtenOn": "2024-04-12T14:43:36Z",
|
||||
"side": 1,
|
||||
"message": "Thanks for taking this further and debating here. I find these opportunities precious to evolve the project. I understand why you thought we had added that. However, I also do not understand the current structure and its reasoning. Therefore, we have always worked to add on top of it, while maintaining the previous options and behaviors.\n\n\u003e Can user or even developer tell what will happen in this case without looking into the code?\n\nWell, depends on how versed they are. I mean, looking at what you described, I would expect (without looking at the code) that a domain is created with parameter \"{1}\", but not used; then, a user is created on domain \"{2}\", and a project on domain \"{3}\", and the user on domain \"{2}\" will receive permission on a project on domain \"{3}\".\n\nI would not consider it a bug, but rather a flexibility that the current implementation allows. It is not bad, it is just flexible; maybe too flexible for most people. What can be done in these situation, which OpenStack sometimes lack is logs that describe the processing, and that can be used by operators to understand the behaviors of the system. While implementing the initial patch of the schema_version, I worked to add that.\n\nTherefore, what we seem to be lacking is documentation, with some more examples, and not necessarily some other changes. Anyways, if we wish to change that, we can create a new schema_version that removes those options/parameters.\n\n\u003e Now lets have a look at \"projects_json\". Is there something what prevents following?\n\nFor this situation, again, it depends on how the person interprets things. There are two possibilities. Override \"projects\" (the hardcoded one) with \"projects_json\" or complement them. I am the author of the patch, therefore, I prefer the addition, instead of the override in this situation, which is what I implemented [1].\n\n```\noperator does not know how it is going to be handled\n```\n\nFor this one, we can improve/add documentation and work from there. There will never be a clear option/feature set without them.\n\n```\n- no possibility for input validation (without ugly hacks)\n\n```\n\nI did not understand what you mean with that.\n\n```\n- error prone since it requires \"if\" branches in the code with order of\n branches playing significant role. Having \"if\" in 2 places in the code where\n they check \"projects or projects_json\" in different order leads to undefined\n behavior and bugs. An now imagine there is some support for the mapping api in CLI or other tool where similar validation need to be implemented (and surely it is going to be done there differently because ...)\n\n```\n\nWe are not implementing like that adding many more conditionals and so on; ee are using Object orientation. Moreover, we can make clear in the spec that the project_json appends to the hard-coded project definition. That is simple and easy to achieve. And then everybody would be on the same page regarding this behavior.\n\n```\n- and again - this style differs from ALL other OpenStack apis (at least of the core ones)\n\n```\nI also do not understand what you mean by this one as well.\n\n```\n\n\nInstead making \"projects\" be an object OR a string allows you to be absolutely deterministic and do reasonable input validation (look at Nova/Cinder/Neutron API for exactly same pattern). Also in the code directly the chance of introducing bug is much lower since you more or less cover finite enum kinds. Once doing that I would directly extend the \"projects\" object schema to include \"domain\" similarly to all other places across OpenStack where user/project/group/etc are having direct attribute \"domain\". Later (separately) we could address preventing domain value conflicting or deprecate it as such (on the upper level).\nI am not intending to crucify anybody, I am just underlining an API design that is different from everything else in OpenStack and is introducing potential bugs. After spending last 6 years deep in the wild jungle of OpenStack APIs (from the tooling side) I notice such things and want us not to introduce new deviations and error-prone patterns.\n```\n\nWe are validating the project_json with the project schema [2]. I will add this to the spec, but this was already expected. This might be just a problem of communication.\n\n\n[1] https://review.opendev.org/c/openstack/keystone/+/742235/6/keystone/federation/utils.py#1059\n\n[2] https://review.opendev.org/c/openstack/keystone/+/742235/6/keystone/federation/utils.py#1092",
|
||||
"parentUuid": "ef34a7e6_752cec35",
|
||||
"revId": "8305797037e04aa67186e4aec1e3ee8adfd25ab9",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Reference in New Issue