)]}'
{"octavia/network/drivers/neutron/allowed_address_pairs.py":[{"author":{"_account_id":6737,"name":"Edward Hope-Morley","email":"edward.hope-morley@canonical.com","username":"hopem"},"change_message_id":"b2d64c451066f0a8f30f6321132185fd1d6561d9","unresolved":true,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"cfbb0e14_6c163502","line":187,"updated":"2021-08-16 16:36:09.000000000","message":"Would this not have the same affect as making the update_ports.append on line 177 conditional? If it does then i\u0027d prefer that as it would be one/two liner.","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"1de2e12e5dd8bae83819b6d7ece7034d30bf68f4","unresolved":true,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"e59bedc4_a60a73ba","line":187,"in_reply_to":"1cc6b961_e7198ce2","updated":"2021-08-17 12:00:54.000000000","message":"and if only one rule for peer port HAPROXY_BASE_PEER_PORT is added, the code of line 177/178 must move outside of the listener loop.","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":10366,"name":"Hemanth N","email":"hemanth.nakkina@canonical.com","username":"Hemanth"},"change_message_id":"270d449c01a12d5fc3036964fe8487e5eba520e2","unresolved":false,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"cf649a16_80f722af","line":187,"in_reply_to":"acd6352c_7e6afec2","updated":"2021-08-17 11:07:25.000000000","message":"Edward comments are resolved.\n\nBodo,\nFrom Octavia API version \u003e 0.5, the peer ports are combined [1] and only one peer port \n with value constants.HAPROXY_BASE_PEER_PORT configured in haproxy conf [2]. So even though database have peer ports with incremented values for each listener (starting from 1025), haproxy has only single port configured.\nLeaving the change to constants.HAPROXY_BASE_PEER_PORT instead of listener.peer_port\n\n[1] https://opendev.org/openstack/octavia/src/commit/b89c929c12fb262f59ba320a37f2a5bf4109df98/octavia/amphorae/drivers/haproxy/rest_api_driver.py#L141-L149\n[2] https://opendev.org/openstack/octavia/src/commit/b89c929c12fb262f59ba320a37f2a5bf4109df98/octavia/common/jinja/haproxy/combined_listeners/templates/macros.j2#L23","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"6d05ddb5b561ce87aa06137d4fc1e160a279a942","unresolved":true,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"ed5d44eb_f3f41c40","line":187,"in_reply_to":"aee66042_d4c453c3","updated":"2021-08-18 08:16:06.000000000","message":"Only adding a rule for HAPROXY_BASE_PEER_PORT seems to break tests that expect rules for all peer ports of all listeners. So maybe it\u0027s better to move the block back into the loop, like this (may need to be adapted to fit the line length limit):\n\n    tcp_lower \u003d constants.PROTOCOL_TCP.lower()\n    if (listener.peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:\n        updated_ports.append((listener.peer_port, tcp_lower, None))\n\nor update the unit tests","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"4a42b240d7dc0f5c29ec63e407c7ebf244da88ca","unresolved":true,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1cc6b961_e7198ce2","line":187,"in_reply_to":"cf649a16_80f722af","updated":"2021-08-17 11:48:12.000000000","message":"Hemanth, you\u0027re right in terms of haproxy conf, but here in the security group handling the code still works with listener.peer_port, which could be 1026 for the 2nd listener. Maybe it shouldn\u0027t use listener.peer_port. So the code in line 177/178 needs to be changed too.\nOtherwise adding a 2nd listener for TCP port 1026 and allowed cidr 0.0.0.0/0 will fail.\n\nHow about changing L177/L178 to\n\n    tcp_lower \u003d constants.PROTOCOL_TCP.lower()\n    peer_port \u003d constants.HAPROXY_BASE_PEER_PORT\n    if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:\n        updated_ports.append((peer_port, tcp_lower, None))","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":31976,"name":"Bodo Petermann","email":"b.petermann@syseleven.de","username":"bpetermann"},"change_message_id":"b4f51130576bbb260b775fc61e9d883413a6a8ff","unresolved":true,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"acd6352c_7e6afec2","line":187,"in_reply_to":"cfbb0e14_6c163502","updated":"2021-08-17 10:53:43.000000000","message":"I agree. Also, HAPROXY_BASE_PEER_PORT is not necessarily the correct port to look for. It should be listener.peer_port.\n\nSo maybe the change could look like\n\n    tcp \u003d constants.PROTOCOL_TCP.lower()\n    if (listener.peer_port, tcp, \"0.0.0.0/0\") not in updated_ports:\n        updated_ports.append((listener.peer_port, tcp, None))","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":10366,"name":"Hemanth N","email":"hemanth.nakkina@canonical.com","username":"Hemanth"},"change_message_id":"95679f4d412bf7be79a27e1511690c2b4a5a1d38","unresolved":false,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"aee66042_d4c453c3","line":187,"in_reply_to":"e59bedc4_a60a73ba","updated":"2021-08-18 03:46:26.000000000","message":"Hi Bodo\n\nThanks for pointing the multi listener case. Modified the code as per your suggestions.","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":10366,"name":"Hemanth N","email":"hemanth.nakkina@canonical.com","username":"Hemanth"},"change_message_id":"39a5b85f80e0c11f678f43333e982c7d9b888aaf","unresolved":false,"context_lines":[{"line_number":184,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":185,"context_line":"            tcp_lower,"},{"line_number":186,"context_line":"            \"0.0.0.0/0\""},{"line_number":187,"context_line":"        )"},{"line_number":188,"context_line":"        peer_port_cidr_none \u003d ("},{"line_number":189,"context_line":"            constants.HAPROXY_BASE_PEER_PORT,"},{"line_number":190,"context_line":"            tcp_lower,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1d9a1d0f_449fe456","line":187,"in_reply_to":"ed5d44eb_f3f41c40","updated":"2021-08-18 08:54:43.000000000","message":"Fixed in Patchset 4, my mistake not to check unit test cases earlier. Now they are fixed.\nUnit test cases have peer port security group even if it is UDP listener, probably just to pass the earlier tests. But peer port is not required in case of UDP listeners since it uses lvs. So adjusted the unit test cases accordingly.","commit_id":"f404aec964c80edeb0e5485f1b025cb695ec1fd3"},{"author":{"_account_id":29244,"name":"Gregory Thiemonge","email":"gthiemon@redhat.com","username":"gthiemonge"},"change_message_id":"634a6f3b5c91694855ee6600e1c78360a28c0db2","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        tcp_lower \u003d constants.PROTOCOL_TCP.lower()"},{"line_number":180,"context_line":"        peer_port \u003d constants.HAPROXY_BASE_PEER_PORT"},{"line_number":181,"context_line":"        if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:"},{"line_number":182,"context_line":"            updated_ports.append((peer_port, tcp_lower, None))"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"        # Just going to use port_range_max for now because we can assume that"},{"line_number":185,"context_line":"        # port_range_max and min will be the same since this driver is"}],"source_content_type":"text/x-python","patch_set":4,"id":"2c4a10e4_9954a98c","line":182,"range":{"start_line":182,"start_character":34,"end_line":182,"end_character":43},"updated":"2021-08-18 12:00:52.000000000","message":"the behavior changes here, it was previously listener.peer_port which is a unique port for a listener in a load balancer.\nit looks like it\u0027s not used anymore in the jinja template, but I\u0027m not sure if this is a bug or not.\nperhaps other reviewers will have more insights about that.","commit_id":"01f722669dae870d5af038363a0a6bc63165943e"},{"author":{"_account_id":10366,"name":"Hemanth N","email":"hemanth.nakkina@canonical.com","username":"Hemanth"},"change_message_id":"7c92179a9695f49c905b3b85dc779ce01c6e965a","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        tcp_lower \u003d constants.PROTOCOL_TCP.lower()"},{"line_number":180,"context_line":"        peer_port \u003d constants.HAPROXY_BASE_PEER_PORT"},{"line_number":181,"context_line":"        if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:"},{"line_number":182,"context_line":"            updated_ports.append((peer_port, tcp_lower, None))"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"        # Just going to use port_range_max for now because we can assume that"},{"line_number":185,"context_line":"        # port_range_max and min will be the same since this driver is"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a2bc729_863494dc","line":182,"range":{"start_line":182,"start_character":34,"end_line":182,"end_character":43},"in_reply_to":"0c988948_490144f5","updated":"2021-08-20 10:46:57.000000000","message":"Gregory,\nUpdated PS5 with the per-listener peer port logic.","commit_id":"01f722669dae870d5af038363a0a6bc63165943e"},{"author":{"_account_id":29244,"name":"Gregory Thiemonge","email":"gthiemon@redhat.com","username":"gthiemonge"},"change_message_id":"dd4245fc9745d16cab48d4a446f0508e30a2319a","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        tcp_lower \u003d constants.PROTOCOL_TCP.lower()"},{"line_number":180,"context_line":"        peer_port \u003d constants.HAPROXY_BASE_PEER_PORT"},{"line_number":181,"context_line":"        if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:"},{"line_number":182,"context_line":"            updated_ports.append((peer_port, tcp_lower, None))"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"        # Just going to use port_range_max for now because we can assume that"},{"line_number":185,"context_line":"        # port_range_max and min will be the same since this driver is"}],"source_content_type":"text/x-python","patch_set":4,"id":"6203f3b9_58dffe2a","line":182,"range":{"start_line":182,"start_character":34,"end_line":182,"end_character":43},"in_reply_to":"2c4a10e4_9954a98c","updated":"2021-08-19 06:43:03.000000000","message":"we discussed it during the last weekly meeting, per-listener peer_port is still supported (until split_listener configuration files are removed, probably during the yoga cycle). So we should use \u0027listener.peer_port\u0027 here","commit_id":"01f722669dae870d5af038363a0a6bc63165943e"},{"author":{"_account_id":10366,"name":"Hemanth N","email":"hemanth.nakkina@canonical.com","username":"Hemanth"},"change_message_id":"b8090e09f870c7545ccddf19f3ea12e6b831eaed","unresolved":true,"context_lines":[{"line_number":179,"context_line":"        tcp_lower \u003d constants.PROTOCOL_TCP.lower()"},{"line_number":180,"context_line":"        peer_port \u003d constants.HAPROXY_BASE_PEER_PORT"},{"line_number":181,"context_line":"        if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:"},{"line_number":182,"context_line":"            updated_ports.append((peer_port, tcp_lower, None))"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"        # Just going to use port_range_max for now because we can assume that"},{"line_number":185,"context_line":"        # port_range_max and min will be the same since this driver is"}],"source_content_type":"text/x-python","patch_set":4,"id":"0c988948_490144f5","line":182,"range":{"start_line":182,"start_character":34,"end_line":182,"end_character":43},"in_reply_to":"6203f3b9_58dffe2a","updated":"2021-08-19 07:05:25.000000000","message":"Thanks Gregory, I will change the logic accordingly.","commit_id":"01f722669dae870d5af038363a0a6bc63165943e"},{"author":{"_account_id":29244,"name":"Gregory Thiemonge","email":"gthiemon@redhat.com","username":"gthiemonge"},"change_message_id":"85a5e5df622b24b33f42c6527b84939a270f6321","unresolved":true,"context_lines":[{"line_number":181,"context_line":"        # does not have the peer_port entry with allowed_cidr 0.0.0.0/0"},{"line_number":182,"context_line":"        tcp_lower \u003d constants.PROTOCOL_TCP.lower()"},{"line_number":183,"context_line":"        for peer_port in listener_peer_ports:"},{"line_number":184,"context_line":"            if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:"},{"line_number":185,"context_line":"                updated_ports.append((peer_port, tcp_lower, None))"},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"        # Just going to use port_range_max for now because we can assume that"},{"line_number":188,"context_line":"        # port_range_max and min will be the same since this driver is"},{"line_number":189,"context_line":"        # responsible for creating these rules"}],"source_content_type":"text/x-python","patch_set":5,"id":"143081dc_1d38a051","line":186,"range":{"start_line":184,"start_character":12,"end_line":186,"end_character":0},"updated":"2021-08-23 10:41:33.000000000","message":"nit: you could have added that block in the previous for loop, so it would not require to create a new array.","commit_id":"9005bb3d433d2383389900132f95b967aaf51337"},{"author":{"_account_id":10366,"name":"Hemanth N","email":"hemanth.nakkina@canonical.com","username":"Hemanth"},"change_message_id":"3d1a918bf034f57e3bc1536674915edee25eff14","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        # does not have the peer_port entry with allowed_cidr 0.0.0.0/0"},{"line_number":182,"context_line":"        tcp_lower \u003d constants.PROTOCOL_TCP.lower()"},{"line_number":183,"context_line":"        for peer_port in listener_peer_ports:"},{"line_number":184,"context_line":"            if (peer_port, tcp_lower, \"0.0.0.0/0\") not in updated_ports:"},{"line_number":185,"context_line":"                updated_ports.append((peer_port, tcp_lower, None))"},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"        # Just going to use port_range_max for now because we can assume that"},{"line_number":188,"context_line":"        # port_range_max and min will be the same since this driver is"},{"line_number":189,"context_line":"        # responsible for creating these rules"}],"source_content_type":"text/x-python","patch_set":5,"id":"6b734a07_c3d557ce","line":186,"range":{"start_line":184,"start_character":12,"end_line":186,"end_character":0},"in_reply_to":"143081dc_1d38a051","updated":"2021-08-24 06:42:06.000000000","message":"In case of multi-listener, the original reported issue may arise when peer port is same as protocol_port of different listener. So we cannot do this logic inside previous for loop.","commit_id":"9005bb3d433d2383389900132f95b967aaf51337"}]}