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":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_27900\u003e replied on the change"}
Attention: {"person_ident":"Gerrit User 28356 \u003c28356@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_27900\u003e replied on the change"}

8305797037e04aa67186e4aec1e3ee8adfd25ab9

@@ -34,6 +34,23 @@
       "parentUuid": "23247a08_cc07ced4",
       "revId": "8305797037e04aa67186e4aec1e3ee8adfd25ab9",
       "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "ef34a7e6_752cec35",
+        "filename": "/PATCHSET_LEVEL",
+        "patchSetId": 15
+      },
+      "lineNbr": 0,
+      "author": {
+        "id": 27900
+      },
+      "writtenOn": "2024-04-12T11:27:22Z",
+      "side": 1,
+      "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"
     }
   ]
 }