)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c73e19297e52dd9ee08467e3659cce4c4bac7d99","unresolved":true,"context_lines":[{"line_number":11,"context_line":""},{"line_number":12,"context_line":"* all object requests"},{"line_number":13,"context_line":"* container GET, HEAD"},{"line_number":14,"context_line":"* account GET, HEAD"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"In these cases, X-Backend-* headers were transferred to backend"},{"line_number":17,"context_line":"requests implicitly as a consequence of *all* the headers in the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"4f4a1737_6a098e6e","line":14,"updated":"2023-04-28 17:00:45.000000000","message":"Wait, what?\n\n```\nswift/proxy/controllers/base.py\n1841:    def generate_request_headers(self, orig_req\u003dNone, additional\u003dNone,\n2206:            req, additional\u003dreq.headers)\n\nswift/proxy/controllers/obj.py\n457:        headers \u003d [self.generate_request_headers(req, additional\u003dreq.headers)\n2957:            req, additional\u003dreq.headers)\n```\n\nWhy don\u0027t we use `transfer\u003dTrue` there? Where all do we actually _want_ `transfer\u003dFalse`?\n\nMeanwhile, the one other place that uses `additional` looks like\n```\nadditional \u003d {\u0027X-Timestamp\u0027: Timestamp.now().internal}\nif policy_index is None:\n    additional[\u0027X-Backend-Storage-Policy-Default\u0027] \u003d \\\n        int(POLICIES.default)\nelse:\n    additional[\u0027X-Backend-Storage-Policy-Index\u0027] \u003d str(policy_index)\nheaders \u003d [self.generate_request_headers(req, transfer\u003dTrue,\n                                         additional\u003dadditional)\n           for _junk in range(n_outgoing)]\n```\nwhich feels like there\u0027s probably a bug there -- do we *really* ignore the *passed-in* storage policy info if it\u0027s already present in the env?","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"682fa4c23d108e2b6dc4dc95f2170379f96946bb","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"a0e5ec5a_c50fb67d","updated":"2023-04-14 04:36:11.000000000","message":"After I read the commit message, I started to question why would we allow proxy controller to pass x-backend- headers to backend for user requests, it only makes sense to pass those headers between internal services. Then after some study, I found out \u0027x-backend\u0027 is in the inbound_exclusions of gatekeeper, then the existing proxy controller logic will only allow internal services (like inventory servers) to pass those x-backend- headers to backend, right? if this is the case, I agree that there is no reason to not allow some operations to pass x-backend- headers, as long as we use them correctly and have good test coverages. Another question, is there a list of all current x-backend- headers? just want to make sure we have tests for them after we start to pass them to backend.","commit_id":"ffdbce88a165d4c0d555f5ea54963ec9fd9f8371"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"1d7859dc2901d493ec441091006a802062fb245c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"eaa87cf7_61bf4e11","in_reply_to":"6551366e_90016bd0","updated":"2023-04-14 19:14:13.000000000","message":"Ack","commit_id":"ffdbce88a165d4c0d555f5ea54963ec9fd9f8371"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"761e64918ff7c51aa9bc0ba8bf3c844c1ab5f3b2","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"6551366e_90016bd0","in_reply_to":"a0e5ec5a_c50fb67d","updated":"2023-04-14 09:07:53.000000000","message":"The commit message could be clearer: by client request I meant the request the proxy is handling, which has been \"cleaned\" of any x-backend-* headers by the gatekeeper middleware.\n\nI made a survey of x-backend-* headers. They are typically:\n\n* generated within the proxy server app controllers to send to backend servers, e.g. X-Backend-Storage-Policy-Index\n\n* generated by proxy server middleware to send to backend servers, e.g. X-Backend-Allow-Private-Methods\n\n* generated by daemons to send to backend servers e.g. X-Backend-Replication\n\n* generated by daemons to send via InternalClient\u0027s proxy pipeline to backend servers e.g. sharder sends x-backend-record-type\n\n* generated by backend servers for responses to the proxy, e.g. X-Backend-Timestamp\n\nMy list (it is possible I missed some):\n```\nUsed in request headers:\nX-Backend-Storage-Policy-Index\n    set by proxy container controller \n    set by SLO when making preauthed request to expire segments\n    set by reconciler\n    sharder when creating shard containers via direct client\n    container sync\n    container updater\n    reconstructor\n    obj replicator\n    obj updater\n\nX-Backend-Allow-Private-Methods:\n    SLO, container-deleter\nX-Backend-Storage-Policy-Default:\n    set by proxy container controller\nX-Backend-Auto-Create\n    sharder when creating new shard containers via direct client\nX-Backend-Ssync-Frag-Index\nX-Backend-Ssync-Node-Index\n    ssync\nx-backend-no-commit\n    ssync\nX-Backend-Replication-Headers\n    ssync\nX-Backend-Replication\n    ssync\nX-Backend-Clean-Expiring-Object-Queue\n    expirer\nx-backend-container-update-override-*\n    proxy EC obj controller puts\nX-Backend-Quoted-Container-Path\n    proxy obj controller\nX-Backend-Container-Path\n    no longer used\nX-Backend-Next-Part-Power\n    proxy obj controller\nX-Backend-Accept-Redirect\n    obj updater\nX-Backend-Accept-Quoted-Location\n    obj updater             \nX-Backend-Allow-Reserved-Names\n    proxy controller when getting container info\nx-backend-record-type\n    sharder via internal client\n    proxy container controller when listing\nx-backend-override-shard-name-filter\n proxy container controller when listing\nx-backend-include-deleted\n    sharder via internal client\nX-Backend-Override-Deleted\n    sharder via internal client\nX-Backend-Obj-Multiphase-Commit\nX-Backend-Obj-Metadata-Footer\nX-Backend-Obj-Multipart-Mime-Boundary\nX-Backend-Fragment-Preferences\nX-Backend-Ignore-Range-If-Metadata-Present\n\nUsed in response headers:\nX-Backend-Obj-Content-Length\nX-Backend-Fake-Account-Listing\n    account server\nX-Backend-Recheck-Account-Existence\n    account server\nX-Backend-Timestamp\nx-backend-put-timestamp\nx-backend-delete-timestamp\nx-backend-status-changed-at\nX-Backend-Data-Timestamp\nX-Backend-Durable-Timestamp\nX-Backend-Fragments\nX-Backend-Content-Type\nx-backend-sharding-state\nX-Backend-Cached-Results\n```","commit_id":"ffdbce88a165d4c0d555f5ea54963ec9fd9f8371"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"fdcc0ab052e982ade7ad60442e57122a8b1af559","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e2c97852_b33b5774","updated":"2023-04-14 09:18:33.000000000","message":"Updated the commit message, no other changes.","commit_id":"41b0f7a848c802d46564950c59fa2c3fcfa46ffc"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c73e19297e52dd9ee08467e3659cce4c4bac7d99","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"d999ae7d_484b1430","updated":"2023-04-28 17:00:45.000000000","message":"I think I\u0027m OK with merging this, but I wonder if we aren\u0027t going far enough -- I\u0027ll see what I can do in a follow-up.","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c5a08b06125aed1cc3059e1b97b3e117504113c0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"fe83f6ed_5fd13043","updated":"2023-04-28 17:20:22.000000000","message":"Never mind, I was being dumb -- this all LGTM.","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2cd6050d648d9b1f9afcc907bb5ac3cdacb7dffc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"c0c8959b_19e2a5b2","updated":"2023-04-28 16:19:33.000000000","message":"recheck\n\nflakey probe test should now be fixed","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"}],"swift/proxy/controllers/base.py":[{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"1d7859dc2901d493ec441091006a802062fb245c","unresolved":true,"context_lines":[{"line_number":1850,"context_line":"        \"\"\""},{"line_number":1851,"context_line":"        headers \u003d HeaderKeyDict()"},{"line_number":1852,"context_line":"        if orig_req:"},{"line_number":1853,"context_line":"            headers.update((k.lower(), v)"},{"line_number":1854,"context_line":"                           for k, v in orig_req.headers.items()"},{"line_number":1855,"context_line":"                           if k.lower().startswith(\u0027x-backend-\u0027))"},{"line_number":1856,"context_line":"        # additional headers can override x-backend-* headers from orig_req"}],"source_content_type":"text/x-python","patch_set":2,"id":"b7601f31_9b2a595d","line":1853,"updated":"2023-04-14 19:14:13.000000000","message":"you probably have thought about this before, I wonder why you decided not to put the copy logic of \u0027x-backend-\u0027 headers into transfer_headers()?","commit_id":"41b0f7a848c802d46564950c59fa2c3fcfa46ffc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b9db6928c1d74fa37e34365d2a99518a06c3af4b","unresolved":true,"context_lines":[{"line_number":1850,"context_line":"        \"\"\""},{"line_number":1851,"context_line":"        headers \u003d HeaderKeyDict()"},{"line_number":1852,"context_line":"        if orig_req:"},{"line_number":1853,"context_line":"            headers.update((k.lower(), v)"},{"line_number":1854,"context_line":"                           for k, v in orig_req.headers.items()"},{"line_number":1855,"context_line":"                           if k.lower().startswith(\u0027x-backend-\u0027))"},{"line_number":1856,"context_line":"        # additional headers can override x-backend-* headers from orig_req"}],"source_content_type":"text/x-python","patch_set":2,"id":"2fee1de7_6452bbb1","line":1853,"in_reply_to":"b7601f31_9b2a595d","updated":"2023-04-17 10:35:42.000000000","message":"transfer_headers is not always called, but also there is an order of precedence implied by the order of the copies:\n\nx-backend-* in \u0027additional\u0027 can override \u0027x-backend-*\u0027 in the orig_req","commit_id":"41b0f7a848c802d46564950c59fa2c3fcfa46ffc"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"636d97e3a4407c6dcbb0579abba35b6c0dc0d6eb","unresolved":true,"context_lines":[{"line_number":1856,"context_line":"        # additional headers can override x-backend-* headers from orig_req"},{"line_number":1857,"context_line":"        if additional:"},{"line_number":1858,"context_line":"            headers.update(additional)"},{"line_number":1859,"context_line":"        if orig_req:"},{"line_number":1860,"context_line":"            if transfer:"},{"line_number":1861,"context_line":"                self.transfer_headers(orig_req.headers, headers)"},{"line_number":1862,"context_line":"            referer \u003d orig_req.as_referer()"}],"source_content_type":"text/x-python","patch_set":2,"id":"c48c705c_50cf34d6","line":1859,"updated":"2023-04-19 06:07:25.000000000","message":"It is a little annoying to see 2 `if orig_req`. Can we just combine orig_req with transfer? I think it reads better. And add move the referrer up to the first `if orig_req`?\n\n  if orig_req:\n    ...\n    referer \u003d orig_req.as_referer()\n  else:\n    referrer \u003d \u0027\u0027\n  if additional:\n    ...\n  if transfer and orig_req:\n    self.transfer_headers(orig_req.headers, headers)\n\nWe\u0027re either pulling the referrer out of the orig_req or it\u0027s emppty either way, so additional headers wont be overwriting the referer.","commit_id":"41b0f7a848c802d46564950c59fa2c3fcfa46ffc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"7e6b9d24788b687bf4739dc29a4a1853c2d1d890","unresolved":true,"context_lines":[{"line_number":1856,"context_line":"        # additional headers can override x-backend-* headers from orig_req"},{"line_number":1857,"context_line":"        if additional:"},{"line_number":1858,"context_line":"            headers.update(additional)"},{"line_number":1859,"context_line":"        if orig_req:"},{"line_number":1860,"context_line":"            if transfer:"},{"line_number":1861,"context_line":"                self.transfer_headers(orig_req.headers, headers)"},{"line_number":1862,"context_line":"            referer \u003d orig_req.as_referer()"}],"source_content_type":"text/x-python","patch_set":2,"id":"8b0a55dc_a7ca0cf5","line":1859,"in_reply_to":"c48c705c_50cf34d6","updated":"2023-04-19 11:09:41.000000000","message":"Combining the two \u0027if orig_req\u0027 clauses would change the order of precedence:\n\n- first update with backend headers from orig_req: I\u0027m playing it safe by applying the new update first so that it won\u0027t override any existing update.\n\n- then update with additional headers\n\n- then, if transfer, update with any other orig_req headers\n\nThe referer assignment can probably be moved to get things a bit slicker though","commit_id":"41b0f7a848c802d46564950c59fa2c3fcfa46ffc"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c5a08b06125aed1cc3059e1b97b3e117504113c0","unresolved":true,"context_lines":[{"line_number":1858,"context_line":"            referer \u003d \u0027\u0027"},{"line_number":1859,"context_line":"        # additional headers can override x-backend-* headers from orig_req"},{"line_number":1860,"context_line":"        if additional:"},{"line_number":1861,"context_line":"            headers.update(additional)"},{"line_number":1862,"context_line":"        if orig_req and transfer:"},{"line_number":1863,"context_line":"            # transfer headers from orig_req can override additional headers"},{"line_number":1864,"context_line":"            self.transfer_headers(orig_req.headers, headers)"}],"source_content_type":"text/x-python","patch_set":3,"id":"a3bf4c42_58c97833","line":1861,"updated":"2023-04-28 17:20:22.000000000","message":"The prevalence of `additional\u003dreq.headers` still makes me nervous... doesn\u0027t sit right with me.","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c73e19297e52dd9ee08467e3659cce4c4bac7d99","unresolved":true,"context_lines":[{"line_number":1860,"context_line":"        if additional:"},{"line_number":1861,"context_line":"            headers.update(additional)"},{"line_number":1862,"context_line":"        if orig_req and transfer:"},{"line_number":1863,"context_line":"            # transfer headers from orig_req can override additional headers"},{"line_number":1864,"context_line":"            self.transfer_headers(orig_req.headers, headers)"},{"line_number":1865,"context_line":"        headers.setdefault(\u0027x-timestamp\u0027, Timestamp.now().internal)"},{"line_number":1866,"context_line":"        # orig_req and additional headers cannot override the following..."}],"source_content_type":"text/x-python","patch_set":3,"id":"8ae89d30_23dea82b","line":1863,"updated":"2023-04-28 17:00:45.000000000","message":"...and notably, this means that the backend headers from `orig_req` take precedence over overlapping backend headers in `additional` (when `transfer\u003dTrue`).","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c5a08b06125aed1cc3059e1b97b3e117504113c0","unresolved":true,"context_lines":[{"line_number":1860,"context_line":"        if additional:"},{"line_number":1861,"context_line":"            headers.update(additional)"},{"line_number":1862,"context_line":"        if orig_req and transfer:"},{"line_number":1863,"context_line":"            # transfer headers from orig_req can override additional headers"},{"line_number":1864,"context_line":"            self.transfer_headers(orig_req.headers, headers)"},{"line_number":1865,"context_line":"        headers.setdefault(\u0027x-timestamp\u0027, Timestamp.now().internal)"},{"line_number":1866,"context_line":"        # orig_req and additional headers cannot override the following..."}],"source_content_type":"text/x-python","patch_set":3,"id":"f1793cbb_80de90f3","line":1863,"in_reply_to":"8ae89d30_23dea82b","updated":"2023-04-28 17:20:22.000000000","message":"OH, `transfer_headers` doesn\u0027t transfer _all_ headers... naming is hard...","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"}],"test/unit/proxy/controllers/test_base.py":[{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"1d7859dc2901d493ec441091006a802062fb245c","unresolved":false,"context_lines":[{"line_number":1263,"context_line":"                       \u0027new-owner\u0027: \u0027Kun\u0027}"},{"line_number":1264,"context_line":"        dst_headers \u003d base.generate_request_headers(None,"},{"line_number":1265,"context_line":"                                                    additional\u003dsrc_headers,"},{"line_number":1266,"context_line":"                                                    transfer\u003dTrue)"},{"line_number":1267,"context_line":"        expected_headers \u003d {\u0027x-base-meta-size\u0027: \u0027151M\u0027,"},{"line_number":1268,"context_line":"                            \u0027connection\u0027: \u0027close\u0027}"},{"line_number":1269,"context_line":"        for k, v in expected_headers.items():"}],"source_content_type":"text/x-python","patch_set":2,"id":"c4c17f77_dadb7ecd","line":1266,"updated":"2023-04-14 19:14:13.000000000","message":"Here is the test for the new fix.","commit_id":"41b0f7a848c802d46564950c59fa2c3fcfa46ffc"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c73e19297e52dd9ee08467e3659cce4c4bac7d99","unresolved":true,"context_lines":[{"line_number":1263,"context_line":"                       \u0027new-owner\u0027: \u0027Kun\u0027}"},{"line_number":1264,"context_line":"        dst_headers \u003d base.generate_request_headers(None,"},{"line_number":1265,"context_line":"                                                    additional\u003dsrc_headers,"},{"line_number":1266,"context_line":"                                                    transfer\u003dTrue)"},{"line_number":1267,"context_line":"        expected_headers \u003d {\u0027x-base-meta-size\u0027: \u0027151M\u0027,"},{"line_number":1268,"context_line":"                            \u0027connection\u0027: \u0027close\u0027}"},{"line_number":1269,"context_line":"        for k, v in expected_headers.items():"}],"source_content_type":"text/x-python","patch_set":3,"id":"0bece0f6_d54f1fd3","line":1266,"updated":"2023-04-28 17:00:45.000000000","message":"OK, this isn\u0027t necessary to ensure the test passes, but demonstrates that setting `transfer\u003dTrue` doesn\u0027t cause an `AttributeError` or something when `orig_req` is `None`.","commit_id":"e19a165a7b56cd88c4fef6c72418f103140c94ae"}]}