)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"dcf2e5c98c18b82104baa5ed273b7822c17c08ee","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This change fixes that in two (redundant) ways:"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"* policy is initialized in the web app at WSGI application"},{"line_number":15,"context_line":"  creation, once, before the web service starts"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"* The global that contains the _ENFORCER is not changed from"},{"line_number":18,"context_line":"  None to assigned until policy is fully loaded."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5fc1f717_4705a4c1","line":15,"updated":"2019-03-08 19:52:44.000000000","message":"Does this change behavior in any way, in terms of ability to change policy dynamically, or was that (whatever it was) always loaded only at startup anyway?","commit_id":"99091131131e65d43cde27503246c6f084ebdbc6"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"c916aa0faf114084a3ece5ab47fe35a180566c3c","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This change fixes that in two (redundant) ways:"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"* policy is initialized in the web app at WSGI application"},{"line_number":15,"context_line":"  creation, once, before the web service starts"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"* The global that contains the _ENFORCER is not changed from"},{"line_number":18,"context_line":"  None to assigned until policy is fully loaded."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5fc1f717_a7a7a8a7","line":15,"in_reply_to":"5fc1f717_4705a4c1","updated":"2019-03-08 19:58:44.000000000","message":"We were only explicitly loading at startup but oslo.policy itself does some magic to watch the policy file and check it for changes. See: https://docs.openstack.org/oslo.policy/latest/admin/policy-json-file.html","commit_id":"99091131131e65d43cde27503246c6f084ebdbc6"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"dcf2e5c98c18b82104baa5ed273b7822c17c08ee","unresolved":false,"context_lines":[{"line_number":17,"context_line":"* The global that contains the _ENFORCER is not changed from"},{"line_number":18,"context_line":"  None to assigned until policy is fully loaded."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Since we\u0027re in there anyway, _ENFORCER_PLACEMENT is renamed"},{"line_number":21,"context_line":"to _ENFORCER. See also a bug about deprecating the policy"},{"line_number":22,"context_line":"file configuration setting[1]."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Because of the change in when init() happens, the unit"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5fc1f717_e7f370e6","line":21,"range":{"start_line":20,"start_character":0,"end_line":21,"end_character":13},"updated":"2019-03-08 19:52:44.000000000","message":"This should have been done in a separate change for poor dumb me who was already having a hard enough time understanding the substance.","commit_id":"99091131131e65d43cde27503246c6f084ebdbc6"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"c916aa0faf114084a3ece5ab47fe35a180566c3c","unresolved":false,"context_lines":[{"line_number":17,"context_line":"* The global that contains the _ENFORCER is not changed from"},{"line_number":18,"context_line":"  None to assigned until policy is fully loaded."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Since we\u0027re in there anyway, _ENFORCER_PLACEMENT is renamed"},{"line_number":21,"context_line":"to _ENFORCER. See also a bug about deprecating the policy"},{"line_number":22,"context_line":"file configuration setting[1]."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Because of the change in when init() happens, the unit"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"5fc1f717_87c5cc3e","line":21,"range":{"start_line":20,"start_character":0,"end_line":21,"end_character":13},"in_reply_to":"5fc1f717_e7f370e6","updated":"2019-03-08 19:58:44.000000000","message":"I was going to do that, but my brain still remembering all the \"also change X\" comments from the refactoring stack. I know this is an entirely different thing, but that\u0027s where my brain was at.\n\nI\u0027m happy to split it if you prefer.","commit_id":"99091131131e65d43cde27503246c6f084ebdbc6"}],"placement/policy.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"b5793eb1c7d57eba478a2539765d7f1bafd4c915","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # to read the policy file from config option [oslo_policy]/policy_file"},{"line_number":42,"context_line":"        # which is used by nova. In other words, to have separate policy files"},{"line_number":43,"context_line":"        # for placement and nova, we have to use separate policy_file options."},{"line_number":44,"context_line":"        _ENFORCER \u003d policy.Enforcer("},{"line_number":45,"context_line":"            conf, policy_file\u003dconf.placement.policy_file)"},{"line_number":46,"context_line":"        _ENFORCER.register_defaults(policies.list_rules())"},{"line_number":47,"context_line":"        _ENFORCER.load_rules()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_6943a0b0","side":"PARENT","line":44,"updated":"2019-03-12 15:33:15.000000000","message":"So - the issue is that the global was getting initialized here and something was attempting to use it before defaults were registered and rules were loaded in lines 45 and 46?\n\nYou\u0027re mitigating that by making sure the enforcer in *this* scope is fully usable before assigning it to the global variable?","commit_id":"98f722a603e86a09adca977e2b94276f94ae8fa0"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"205718955bf8865328c0e868753275965d70f71a","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # to read the policy file from config option [oslo_policy]/policy_file"},{"line_number":42,"context_line":"        # which is used by nova. In other words, to have separate policy files"},{"line_number":43,"context_line":"        # for placement and nova, we have to use separate policy_file options."},{"line_number":44,"context_line":"        _ENFORCER \u003d policy.Enforcer("},{"line_number":45,"context_line":"            conf, policy_file\u003dconf.placement.policy_file)"},{"line_number":46,"context_line":"        _ENFORCER.register_defaults(policies.list_rules())"},{"line_number":47,"context_line":"        _ENFORCER.load_rules()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_dc113406","side":"PARENT","line":44,"in_reply_to":"5fc1f717_1c7d7c7e","updated":"2019-03-12 15:54:57.000000000","message":"Yeah, I can\u0027t remember if I saw it in the placement-perfload job (which creates 1000 resource providers from an asyncio driven tool called placeload) or somewhere else that I was doing similar things, but once I saw the error and looked at the code I realized \"oh yeah, this is not the way to go if we want to be safe\".\n\n(I\u0027ve got a blog post in progress on \"profiling wsgi apps\". Experiments for that might have been where I found it.)","commit_id":"98f722a603e86a09adca977e2b94276f94ae8fa0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"020150ef84080cc01bc27a092a6fa7005db5c03a","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # to read the policy file from config option [oslo_policy]/policy_file"},{"line_number":42,"context_line":"        # which is used by nova. In other words, to have separate policy files"},{"line_number":43,"context_line":"        # for placement and nova, we have to use separate policy_file options."},{"line_number":44,"context_line":"        _ENFORCER \u003d policy.Enforcer("},{"line_number":45,"context_line":"            conf, policy_file\u003dconf.placement.policy_file)"},{"line_number":46,"context_line":"        _ENFORCER.register_defaults(policies.list_rules())"},{"line_number":47,"context_line":"        _ENFORCER.load_rules()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_1c7d7c7e","side":"PARENT","line":44,"in_reply_to":"5fc1f717_29c598f1","updated":"2019-03-12 15:48:11.000000000","message":"Sure - that makes sense.\n\nThanks for the clarification, Chris. Did you hit this locally? I imagine it must have required quite a bit of churn to reproduce?","commit_id":"98f722a603e86a09adca977e2b94276f94ae8fa0"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a8d583bfce6ffcdbf47b8d5fc09fca6b486afcbe","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # to read the policy file from config option [oslo_policy]/policy_file"},{"line_number":42,"context_line":"        # which is used by nova. In other words, to have separate policy files"},{"line_number":43,"context_line":"        # for placement and nova, we have to use separate policy_file options."},{"line_number":44,"context_line":"        _ENFORCER \u003d policy.Enforcer("},{"line_number":45,"context_line":"            conf, policy_file\u003dconf.placement.policy_file)"},{"line_number":46,"context_line":"        _ENFORCER.register_defaults(policies.list_rules())"},{"line_number":47,"context_line":"        _ENFORCER.load_rules()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_29c598f1","side":"PARENT","line":44,"in_reply_to":"5fc1f717_6943a0b0","updated":"2019-03-12 15:40:01.000000000","message":"That\u0027s part of it, yes. That could happen because the call to init below could happen during two requests (in different threads in the same process) near the start of the process.\n\nThe other part is that that init() is now moved to before the application ever exposes itself over the web, so requests are only possible once the enforcer is fully formed.\n\nI could have left it as just the latter for the fix, but doing both seemed more future proof.","commit_id":"98f722a603e86a09adca977e2b94276f94ae8fa0"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"4e9fe92e2f25d99055443ed8a8e09eb7568fda02","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        _ENFORCER \u003d None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def init(conf):"},{"line_number":36,"context_line":"    \"\"\"Init an Enforcer class. Sets the _ENFORCER global.\"\"\""},{"line_number":37,"context_line":"    global _ENFORCER"},{"line_number":38,"context_line":"    if not _ENFORCER:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_bcf0e8fe","line":35,"range":{"start_line":35,"start_character":4,"end_line":35,"end_character":8},"updated":"2019-03-12 15:52:16.000000000","message":"Just a thought as I was reading through this: we could get this a more descriptive name, such as \u0027set_enforcer()\u0027 or something similar. \u0027init\u0027 sounds like we are instantiating a policy object.\n\nIAC this has nothing to do with this patch; just recording thoughts as they occur.","commit_id":"871b15ef93386c4d8191f94bc9358a426d51d927"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"f0ffc142765d32a652b3e39eb1752036226f5cea","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        _ENFORCER \u003d None"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"def init(conf):"},{"line_number":36,"context_line":"    \"\"\"Init an Enforcer class. Sets the _ENFORCER global.\"\"\""},{"line_number":37,"context_line":"    global _ENFORCER"},{"line_number":38,"context_line":"    if not _ENFORCER:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_3c195821","line":35,"range":{"start_line":35,"start_character":4,"end_line":35,"end_character":8},"in_reply_to":"5fc1f717_bcf0e8fe","updated":"2019-03-12 15:55:53.000000000","message":"yeah, makes sense. As I recall, this was a copy-with-adjustments from nova.","commit_id":"871b15ef93386c4d8191f94bc9358a426d51d927"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9e7e448be0bd1cf56dad960eaf511b8b57fbe299","unresolved":false,"context_lines":[{"line_number":45,"context_line":"            conf, policy_file\u003dconf.placement.policy_file)"},{"line_number":46,"context_line":"        _enforcer.register_defaults(policies.list_rules())"},{"line_number":47,"context_line":"        _enforcer.load_rules()"},{"line_number":48,"context_line":"        _ENFORCER \u003d _enforcer"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"def get_enforcer():"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_b1fb2a77","line":48,"updated":"2019-03-12 21:14:20.000000000","message":"Another way to do this would be something like:\n\nif not _ENFORCER:\n   lock()\n   if not _ENFORCER:\n      do the thing that sets _ENFORCER\n\nYeah? That way the synchronized part would only ever be on first access.\n\nI guess you kind of already have that, except without a lock and we could overwrite the global if two competing threads are racing to initialize the enforcer on first request.","commit_id":"871b15ef93386c4d8191f94bc9358a426d51d927"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9e7e448be0bd1cf56dad960eaf511b8b57fbe299","unresolved":false,"context_lines":[{"line_number":61,"context_line":"    return _ENFORCER"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":""},{"line_number":64,"context_line":"def authorize(context, action, target, do_raise\u003dTrue):"},{"line_number":65,"context_line":"    \"\"\"Verifies that the action is valid on the target in this context."},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"    :param context: instance of placement.context.RequestContext"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_31df3a0e","line":64,"range":{"start_line":64,"start_character":4,"end_line":64,"end_character":13},"updated":"2019-03-12 21:14:20.000000000","message":"Yeah so this is called from context.can:\n\nhttps://github.com/openstack/placement/blob/2588c7b8dae43c0197e787de85c7c76654b92538/placement/context.py#L52\n\nWhich is called while processing a request:\n\nhttps://github.com/openstack/placement/blob/2588c7b8dae43c0197e787de85c7c76654b92538/placement/handlers/resource_provider.py#L88\n\nAnd we should handle 403 errors from that here:\n\nhttps://github.com/openstack/placement/blob/2588c7b8dae43c0197e787de85c7c76654b92538/placement/handler.py#L222\n\nBut instead we get PolicyNotRegistered which is a 500 response.\n\nI think the init stuff was always meant as a singleton pattern to just create the enforcer and load the rules on first request rather than startup of the process, and I doubt anyone put a ton of thought into multi-threaded requests hitting this right away.","commit_id":"871b15ef93386c4d8191f94bc9358a426d51d927"}]}