)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"b875d23a7cbdd8672dc4636ec58c47eeb62e817f","unresolved":false,"context_lines":[{"line_number":16,"context_line":"\"pool_connection_timeout\" option however this is not used anywhere"},{"line_number":17,"context_line":"within the LDAP driver."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This commit involves leveraging that conf option to set a new"},{"line_number":20,"context_line":"OPT_NETWORK_TIMEOUT option on the LDAP object. This will"},{"line_number":21,"context_line":"raise ldap.SERVER_DOWN exceptions on timeout."},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"Signed-off-by: Kam Nasim \u003ckam.nasim@windriver.com\u003e"},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"9a629dbe_a5d38037","line":21,"range":{"start_line":19,"start_character":0,"end_line":21,"end_character":45},"updated":"2016-11-08 18:40:18.000000000","message":"I\u0027m not sure reusing the pool_connection_timeout is correct.  If I set use_pool\u003dfalse, then I would assume this timeout would have no effect, but in reality, the OPT_NETWORK_TIMEOUT has nothing to do with a pool.","commit_id":"f71db6795104fea3d3a5f2502fe0069ea77286ca"},{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"12144cabf1b645364588f8836fdd0b6240dfb348","unresolved":false,"context_lines":[{"line_number":13,"context_line":"loss."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"The LDAP configuration stanza (keystone.conf) provides a"},{"line_number":16,"context_line":"\"pool_connection_timeout\" option however this is not used anywhere"},{"line_number":17,"context_line":"within the LDAP driver."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This commit involves leveraging that conf option to set a new"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"7a77a97e_887860c6","line":16,"range":{"start_line":16,"start_character":1,"end_line":16,"end_character":24},"updated":"2016-11-16 04:49:56.000000000","message":"I\u0027m still not a fan of reusing this since it has nothing to do with ldap pooling.","commit_id":"4649f444fe826344c20824b70329aeec291d5286"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"0380f0b221ed782f94be219ca9e519c50cfeffce","unresolved":false,"context_lines":[{"line_number":13,"context_line":"loss."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"The LDAP configuration stanza (keystone.conf) provides a"},{"line_number":16,"context_line":"\"pool_connection_timeout\" option however this is not used anywhere"},{"line_number":17,"context_line":"within the LDAP driver."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This commit involves leveraging that conf option to set a new"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"7a77a97e_277a6bbd","line":16,"range":{"start_line":16,"start_character":1,"end_line":16,"end_character":24},"in_reply_to":"7a77a97e_887860c6","updated":"2016-11-20 14:34:05.000000000","message":"this is a bit frustrating, but i\u0027m OK with it. we have a common initialization for the ldap connection and ldappool connection, so if they share a config option, so be it. i don\u0027t think a lot of folks will be leveraging this option, especially since a timeout can be specified in /etc/ldap/ldap.conf","commit_id":"4649f444fe826344c20824b70329aeec291d5286"}],"keystone/conf/ldap.py":[{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"e9fc9d301f533c26140277eebc8b58fcf5b8ce44","unresolved":false,"context_lines":[{"line_number":524,"context_line":"    min\u003d-1,"},{"line_number":525,"context_line":"    help\u003dutils.fmt(\"\"\""},{"line_number":526,"context_line":"The connection timeout to use with the LDAP server. A value of `-1` means that"},{"line_number":527,"context_line":"connections will never timeout. This option will affect both pooled and"},{"line_number":528,"context_line":"unpooled LDAP connections, i.e. independant of `[ldap] use_pool` being enabled."},{"line_number":529,"context_line":"\"\"\"))"},{"line_number":530,"context_line":""},{"line_number":531,"context_line":"pool_connection_lifetime \u003d cfg.IntOpt("}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_ced33d5e","line":528,"range":{"start_line":527,"start_character":32,"end_line":528,"end_character":79},"updated":"2016-12-02 12:32:51.000000000","message":"I\u0027d prefer to add a new option (connection_timeout) and deprecate this one rather than changing the semantics.\n\nFor me it just looks weird to have a config option called \"pool_connection_timeout\" that \"affect both pooled and\nunpooled LDAP connections\",","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"4dfa6df233b2245922416e94eae1a7bc92f5e9fb","unresolved":false,"context_lines":[{"line_number":524,"context_line":"    min\u003d-1,"},{"line_number":525,"context_line":"    help\u003dutils.fmt(\"\"\""},{"line_number":526,"context_line":"The connection timeout to use with the LDAP server. A value of `-1` means that"},{"line_number":527,"context_line":"connections will never timeout. This option will affect both pooled and"},{"line_number":528,"context_line":"unpooled LDAP connections, i.e. independant of `[ldap] use_pool` being enabled."},{"line_number":529,"context_line":"\"\"\"))"},{"line_number":530,"context_line":""},{"line_number":531,"context_line":"pool_connection_lifetime \u003d cfg.IntOpt("}],"source_content_type":"text/x-python","patch_set":7,"id":"ba5201f7_f0fca773","line":528,"range":{"start_line":527,"start_character":32,"end_line":528,"end_character":79},"in_reply_to":"3a71b18c_2b457816","updated":"2017-01-09 05:03:43.000000000","message":"I added a new config option","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"c3c7b8def4473fba1115e8022b299fd7cc9585c7","unresolved":false,"context_lines":[{"line_number":524,"context_line":"    min\u003d-1,"},{"line_number":525,"context_line":"    help\u003dutils.fmt(\"\"\""},{"line_number":526,"context_line":"The connection timeout to use with the LDAP server. A value of `-1` means that"},{"line_number":527,"context_line":"connections will never timeout. This option will affect both pooled and"},{"line_number":528,"context_line":"unpooled LDAP connections, i.e. independant of `[ldap] use_pool` being enabled."},{"line_number":529,"context_line":"\"\"\"))"},{"line_number":530,"context_line":""},{"line_number":531,"context_line":"pool_connection_lifetime \u003d cfg.IntOpt("}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_2b457816","line":528,"range":{"start_line":527,"start_character":32,"end_line":528,"end_character":79},"in_reply_to":"3a71b18c_ced33d5e","updated":"2016-12-02 15:32:57.000000000","message":"yes, this issue has been discussed a number of times within this patch. Please see comments from Steve Martinelli, and some of the other reviewers","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"}],"keystone/identity/backends/ldap/common.py":[{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"4c9fb6b5ee677ecd40c825027ba4b306dc2285e8","unresolved":false,"context_lines":[{"line_number":581,"context_line":"    if debug_level is not None:"},{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout and timeout !\u003d -1:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ba5da102_11c73713","line":584,"updated":"2016-10-27 15:19:14.000000000","message":"I\u0027m not too familiar with the library, but what happens when timeout is zero.?","commit_id":"784fcd998be0881d5410ef21fcb8f06cd5e58ddf"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"6b8b1f5df02291ff13ba12062e7a69028538ed0a","unresolved":false,"context_lines":[{"line_number":581,"context_line":"    if debug_level is not None:"},{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout and timeout !\u003d -1:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ba5da102_d193fff8","line":584,"in_reply_to":"ba5da102_11c73713","updated":"2016-10-27 15:21:25.000000000","message":"Nevermind, forgot 0 evaluates to false so it won\u0027t do anything.","commit_id":"784fcd998be0881d5410ef21fcb8f06cd5e58ddf"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"4fea7d61c220939fa0b290d55e6934ed61c84d21","unresolved":false,"context_lines":[{"line_number":581,"context_line":"    if debug_level is not None:"},{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout and timeout !\u003d -1:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ba5da102_a94fedbb","line":584,"in_reply_to":"ba5da102_ade704e3","updated":"2016-10-28 14:38:04.000000000","message":"the config parser does validate for non-negative input, the check for timeout \u003d -1 is a special case to handle tox where the sample keystone.conf passes in -1.\n\nBut yes I suppose adding a timeout \u003e 0 check would be the right way to go","commit_id":"784fcd998be0881d5410ef21fcb8f06cd5e58ddf"},{"author":{"_account_id":17973,"name":"Ludovic Beliveau","email":"lbeliveau@gmail.com","username":"lbeliveau"},"change_message_id":"94d4440633d34c5413132e8810aeb7ab6d0b4be8","unresolved":false,"context_lines":[{"line_number":581,"context_line":"    if debug_level is not None:"},{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout and timeout !\u003d -1:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"ba5da102_ade704e3","line":584,"in_reply_to":"ba5da102_d193fff8","updated":"2016-10-28 08:27:31.000000000","message":"Shouldn\u0027t it be timeout \u003e 0 in case timeout is wrongfully to a value \u003c 0.","commit_id":"784fcd998be0881d5410ef21fcb8f06cd5e58ddf"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"68090f892b02c81daf201383be0c76a44d48745c","unresolved":false,"context_lines":[{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            LOG.warning(_LW("},{"line_number":1302,"context_line":"                \u0027Timed out (%(timeout)s seconds) waiting to connect\u0027"},{"line_number":1303,"context_line":"                \u0027 to the LDAP Server(%(url)s).\u0027) %"},{"line_number":1304,"context_line":"                {\u0027timeout\u0027: self.pool_conn_timeout, \u0027url\u0027: self.LDAP_URL})"},{"line_number":1305,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1306,"context_line":"                timeout\u003dself.pool_conn_timeout,"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_5e1e570c","line":1303,"range":{"start_line":1302,"start_character":16,"end_line":1303,"end_character":47},"updated":"2016-11-03 18:24:48.000000000","message":"you are just copying the message of the exception. Why not create the exception before, log its message and then raise it?","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"9d0ebe7fd34b948eb641a686c575cc931fa7c17e","unresolved":false,"context_lines":[{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            LOG.warning(_LW("},{"line_number":1302,"context_line":"                \u0027Timed out (%(timeout)s seconds) waiting to connect\u0027"},{"line_number":1303,"context_line":"                \u0027 to the LDAP Server(%(url)s).\u0027) %"},{"line_number":1304,"context_line":"                {\u0027timeout\u0027: self.pool_conn_timeout, \u0027url\u0027: self.LDAP_URL})"},{"line_number":1305,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1306,"context_line":"                timeout\u003dself.pool_conn_timeout,"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_30e7eead","line":1303,"range":{"start_line":1302,"start_character":16,"end_line":1303,"end_character":47},"in_reply_to":"ba5da102_5e1e570c","updated":"2016-11-04 14:50:59.000000000","message":"yeah it can also be done that way","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":17973,"name":"Ludovic Beliveau","email":"lbeliveau@gmail.com","username":"lbeliveau"},"change_message_id":"13b98a9188a665edc56b8fb36d42df8b8038aa9e","unresolved":false,"context_lines":[{"line_number":581,"context_line":"    if debug_level is not None:"},{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout is not None and timeout \u003e 0:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5da102_e5512fda","line":584,"updated":"2016-11-04 18:27:14.000000000","message":"Pythonic way to do this would be:\n\nif timeout and timeout \u003e 0:\n\nNo need to explicitly check against None.","commit_id":"f71db6795104fea3d3a5f2502fe0069ea77286ca"},{"author":{"_account_id":17973,"name":"Ludovic Beliveau","email":"lbeliveau@gmail.com","username":"lbeliveau"},"change_message_id":"301a1fe02550fa412e83dcf4e8313a51bec6c8b2","unresolved":false,"context_lines":[{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout is not None and timeout \u003e 0:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""},{"line_number":588,"context_line":"    using_ldaps \u003d url.lower().startswith(\"ldaps\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5da102_45f2db07","line":585,"updated":"2016-11-04 18:25:55.000000000","message":"nit: s/set Network/Set network/","commit_id":"f71db6795104fea3d3a5f2502fe0069ea77286ca"},{"author":{"_account_id":17973,"name":"Ludovic Beliveau","email":"lbeliveau@gmail.com","username":"lbeliveau"},"change_message_id":"301a1fe02550fa412e83dcf4e8313a51bec6c8b2","unresolved":false,"context_lines":[{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"        conn \u003d KeystoneLDAPHandler(conn\u003dconn)"},{"line_number":1267,"context_line":""},{"line_number":1268,"context_line":"        # the LDAP server may be down or connection may not"},{"line_number":1269,"context_line":"        # exist. If that is the case, the bind attempt will"},{"line_number":1270,"context_line":"        # fail with a server down exception."},{"line_number":1271,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5da102_e5e3ef31","line":1268,"updated":"2016-11-04 18:25:55.000000000","message":"nit: s/the/The/","commit_id":"f71db6795104fea3d3a5f2502fe0069ea77286ca"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"6968b0ebe7621337de6966a45dae0762a075109b","unresolved":false,"context_lines":[{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"    def _id_to_dn_string(self, object_id):"},{"line_number":1306,"context_line":"        return u\u0027%s\u003d%s,%s\u0027 % (self.id_attr,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a629dbe_13f2c46f","line":1303,"range":{"start_line":1296,"start_character":12,"end_line":1303,"end_character":34},"updated":"2016-11-08 14:50:54.000000000","message":"I tested this out and it didn\u0027t work quite right for me. connect() is not what raises SERVER_DOWN, simple_bind_s() raises it. If there is no user and password set (which is a valid configuration if anonymous binds are allowed), then this method skips attempting to bind and successfully returns a connection object. It fails later the first time it does try to bind, and the server returns a 500 instead of a 504. This could be fixed by just adding an anonymous bind if user and password aren\u0027t set.\n\nI saw another issue when use_pool is turned on, which is that ldappool eats the SERVER_DOWN error and raises its own, so again the server returns a 500 instead of a 504. I can just throw up a patch to make ldappool expose the error and then this will work, but I\u0027m curious if you saw that as well?","commit_id":"f71db6795104fea3d3a5f2502fe0069ea77286ca"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"6ac66139f4fa6b3e55070861671821bbc7f6cdb6","unresolved":false,"context_lines":[{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"    def _id_to_dn_string(self, object_id):"},{"line_number":1306,"context_line":"        return u\u0027%s\u003d%s,%s\u0027 % (self.id_attr,"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a77a97e_328972cb","line":1303,"range":{"start_line":1296,"start_character":12,"end_line":1303,"end_character":34},"in_reply_to":"9a629dbe_13f2c46f","updated":"2016-11-22 15:54:44.000000000","message":"Hi Colleen,\nAs you correctly surmised, the default Keystone behavior is to NOT attempt a bind on connect() if no username/password has been provided. It simply returns the connection uri without attempting an actual connection, therefore there is no way to validate the url, i.e. determine SERVER DOWN.\n\nYour suggestion is to attempt an anonymous bind on connect() if username/password is NOT set however that is a change in behavior to the Keystone LDAP driver, which is beyond the scope of this bug. I will defer this to Steve Martinelli. If the Keystone cores feel that this should be rolled in via this bug then I will make the change.\n\nThanks,\nKam","commit_id":"f71db6795104fea3d3a5f2502fe0069ea77286ca"},{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"12144cabf1b645364588f8836fdd0b6240dfb348","unresolved":false,"context_lines":[{"line_number":582,"context_line":"        ldap.set_option(ldap.OPT_DEBUG_LEVEL, debug_level)"},{"line_number":583,"context_line":""},{"line_number":584,"context_line":"    if timeout is not None and timeout \u003e 0:"},{"line_number":585,"context_line":"        # set Network connection timeout"},{"line_number":586,"context_line":"        ldap.set_option(ldap.OPT_NETWORK_TIMEOUT, timeout)"},{"line_number":587,"context_line":""},{"line_number":588,"context_line":"    using_ldaps \u003d url.lower().startswith(\"ldaps\")"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_a8919c54","line":585,"range":{"start_line":585,"start_character":14,"end_line":585,"end_character":21},"updated":"2016-11-16 04:49:56.000000000","message":"network","commit_id":"4649f444fe826344c20824b70329aeec291d5286"},{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"12144cabf1b645364588f8836fdd0b6240dfb348","unresolved":false,"context_lines":[{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"        conn \u003d KeystoneLDAPHandler(conn\u003dconn)"},{"line_number":1267,"context_line":""},{"line_number":1268,"context_line":"        # the LDAP server may be down or connection may not"},{"line_number":1269,"context_line":"        # exist. If that is the case, the bind attempt will"},{"line_number":1270,"context_line":"        # fail with a server down exception."},{"line_number":1271,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_0889b026","line":1268,"range":{"start_line":1268,"start_character":10,"end_line":1268,"end_character":13},"updated":"2016-11-16 04:49:56.000000000","message":"The","commit_id":"4649f444fe826344c20824b70329aeec291d5286"},{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"12144cabf1b645364588f8836fdd0b6240dfb348","unresolved":false,"context_lines":[{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"        conn \u003d KeystoneLDAPHandler(conn\u003dconn)"},{"line_number":1267,"context_line":""},{"line_number":1268,"context_line":"        # the LDAP server may be down or connection may not"},{"line_number":1269,"context_line":"        # exist. If that is the case, the bind attempt will"},{"line_number":1270,"context_line":"        # fail with a server down exception."},{"line_number":1271,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a77a97e_288cac34","line":1268,"range":{"start_line":1268,"start_character":41,"end_line":1268,"end_character":51},"updated":"2016-11-16 04:49:56.000000000","message":"a connection","commit_id":"4649f444fe826344c20824b70329aeec291d5286"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"e9fc9d301f533c26140277eebc8b58fcf5b8ce44","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_6e822996","line":1297,"range":{"start_line":1297,"start_character":16,"end_line":1297,"end_character":50},"updated":"2016-12-02 12:32:51.000000000","message":"Why not simply wrap this with a try/except if it is the part effectivelly throwing the exception?","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"469ae6e3508645e6ace8828798c24226d6571be8","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_9bbaac09","line":1297,"in_reply_to":"3a71b18c_22b9ce79","updated":"2016-12-07 17:16:41.000000000","message":"Have you dropped by IRC to talk with anyone about their concerns? How have you been asking for reviews? Have you been helping shorten the review queue by reviewing? All of these are ways to help move code reviews along.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"70e3e7b39753b9357221bccf5e7616df861a2a1f","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_22b9ce79","line":1297,"range":{"start_line":1297,"start_character":16,"end_line":1297,"end_character":50},"in_reply_to":"3a71b18c_37ccae69","updated":"2016-12-05 18:18:04.000000000","message":"I agree in theory that exceptions should be fine grained, however the only function that can return a ldap.SERVER_DOWN exception is simple_bind_s. Narrowing would not benefit anything in that cause and would only hold up this patch as I will have to respin and ask again for core and reviewer Upvotes. As is this has happened several times within this patch.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"c3c7b8def4473fba1115e8022b299fd7cc9585c7","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_abe34832","line":1297,"range":{"start_line":1297,"start_character":16,"end_line":1297,"end_character":50},"in_reply_to":"3a71b18c_6e822996","updated":"2016-12-02 15:32:57.000000000","message":"yes, simple_bind_s() is the one that throws the exception but it really does not make a difference if we encapsulate the overall subsection as conn.connect() simply initializes the connection object","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"bf289cd4ab31e4c2037d7ec77af8cad39eb41b99","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_e7cc7663","line":1297,"in_reply_to":"3a71b18c_9bbaac09","updated":"2016-12-09 17:51:58.000000000","message":"Thanks Lance, and yes I have been conversing with some of the folks on this review over IRC","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"ec2b0f21989bf0542d13f948b4e0e4b3b2dc69c8","unresolved":false,"context_lines":[{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_37ccae69","line":1297,"range":{"start_line":1297,"start_character":16,"end_line":1297,"end_character":50},"in_reply_to":"3a71b18c_abe34832","updated":"2016-12-05 17:50:13.000000000","message":"I\u0027ve had a discussion with Morgan in the IRC channel and we both agreed that try/except clauses should be as narrowed as possible, making it clear what piece of code can raise that exception and avoiding to the exception be caught when it was thrown by something else.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"469ae6e3508645e6ace8828798c24226d6571be8","unresolved":false,"context_lines":[{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_2f807246","line":1301,"updated":"2016-12-07 17:16:41.000000000","message":"Running coverage on this patch shows that this is never tested.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"bf289cd4ab31e4c2037d7ec77af8cad39eb41b99","unresolved":false,"context_lines":[{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_e7d636c2","line":1301,"in_reply_to":"3a71b18c_2f807246","updated":"2016-12-09 17:51:58.000000000","message":"right, none of the higher layer code paths explicitly check for this exception (for now) and it would be simply logged. The inent of this fix is to raise the exception instead of stalling indefinitely (or until TCP timeout) as that causes higher layer code paths (or puppet-keystone if configuring ldap identity backend through that) to fail.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"e2eab697701a79436ad5c2a01d5e12343069e12d","unresolved":false,"context_lines":[{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"    def _id_to_dn_string(self, object_id):"},{"line_number":1306,"context_line":"        return u\u0027%s\u003d%s,%s\u0027 % (self.id_attr,"}],"source_content_type":"text/x-python","patch_set":7,"id":"7a77a97e_9e6265d7","line":1303,"range":{"start_line":1296,"start_character":12,"end_line":1303,"end_character":34},"updated":"2016-11-22 10:30:55.000000000","message":"This is still not going to work for anonymous binds, see my comment on ps5","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"92f6feec9a7346ad65f7521409273a0b25f95c96","unresolved":false,"context_lines":[{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"    def _id_to_dn_string(self, object_id):"},{"line_number":1306,"context_line":"        return u\u0027%s\u003d%s,%s\u0027 % (self.id_attr,"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_ec361de6","line":1303,"range":{"start_line":1296,"start_character":12,"end_line":1303,"end_character":34},"in_reply_to":"3a71b18c_4e22cd02","updated":"2016-12-03 16:35:18.000000000","message":"I don\u0027t really agree. The try/except is wrapping around the creation of a connection object, which sometimes makes a network connection and sometimes doesn\u0027t. Therefore the exception handling only sometimes handles the thing it was meant to handle, and the rest of the time drops it on the floor for it to crash somewhere else. I think it\u0027s within the scope of this patch to fix it correctly for all cases.\n\nOn the other hand I don\u0027t want to be the one holding up this patch so I\u0027m fine with fixing it in a subsequent patch.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"bf289cd4ab31e4c2037d7ec77af8cad39eb41b99","unresolved":false,"context_lines":[{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"    def _id_to_dn_string(self, object_id):"},{"line_number":1306,"context_line":"        return u\u0027%s\u003d%s,%s\u0027 % (self.id_attr,"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_7909ea2b","line":1303,"range":{"start_line":1296,"start_character":12,"end_line":1303,"end_character":34},"in_reply_to":"3a71b18c_ec361de6","updated":"2016-12-09 17:51:58.000000000","message":"Colleen,\n\nYou already have a review out to add anonymous bind if username and password is missing:\nhttps://review.openstack.org/#/c/407561\n\nIf that change goes in then we will always generate SERVER_DOWN exception in get_connection() whether username or password are missing/provided.\n\nI will address Samuel\u0027s concern and just wrap the bind() call in a try/except block.\n\nIs there anything else that you would like for me to add?","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"e9fc9d301f533c26140277eebc8b58fcf5b8ce44","unresolved":false,"context_lines":[{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"            # not all LDAP servers require authentication, so we don\u0027t bind"},{"line_number":1295,"context_line":"            # if we don\u0027t have any user/pass"},{"line_number":1296,"context_line":"            if user and password:"},{"line_number":1297,"context_line":"                conn.simple_bind_s(user, password)"},{"line_number":1298,"context_line":""},{"line_number":1299,"context_line":"            return conn"},{"line_number":1300,"context_line":"        except ldap.SERVER_DOWN:"},{"line_number":1301,"context_line":"            raise exception.LDAPServerConnectionError("},{"line_number":1302,"context_line":"                timeout\u003dself.pool_conn_timeout,"},{"line_number":1303,"context_line":"                url\u003dself.LDAP_URL)"},{"line_number":1304,"context_line":""},{"line_number":1305,"context_line":"    def _id_to_dn_string(self, object_id):"},{"line_number":1306,"context_line":"        return u\u0027%s\u003d%s,%s\u0027 % (self.id_attr,"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_4e22cd02","line":1303,"range":{"start_line":1296,"start_character":12,"end_line":1303,"end_character":34},"in_reply_to":"7a77a97e_9e6265d7","updated":"2016-12-02 12:32:51.000000000","message":"I\u0027d be fine with opening another bug(s) and fixing what you\u0027ve seen. It seems to be beyond the scope of this patch, do you agree?","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"2a9e05e27035b1a1de70559e3f6ddae6508ae699","unresolved":false,"context_lines":[{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        if conn_timeout is not None and conn_timeout \u003e 0:"},{"line_number":519,"context_line":"            # set network connection timeout"},{"line_number":520,"context_line":"             self.conn.set_option(ldap.OPT_NETWORK_TIMEOUT, conn_timeout)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        if use_tls:"},{"line_number":523,"context_line":"            self.conn.start_tls_s()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_500f212e","line":520,"updated":"2017-01-09 13:36:23.000000000","message":"Why did this get moved out of _common_ldap_initialization and why did it change from ldap.set_option to self.conn.set_option? I think it\u0027s causing the LDAPConnectionTimeoutTest tests to fail (in addition to the FakeLdap class needing an update)","commit_id":"572c35a06d438c50ea170f37cddd1df515a850e2"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"36852ffff50bdf54cad07e76b0df1e6c1b31b30b","unresolved":false,"context_lines":[{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        if conn_timeout is not None and conn_timeout \u003e 0:"},{"line_number":519,"context_line":"            # set network connection timeout"},{"line_number":520,"context_line":"             self.conn.set_option(ldap.OPT_NETWORK_TIMEOUT, conn_timeout)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        if use_tls:"},{"line_number":523,"context_line":"            self.conn.start_tls_s()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_7df86ec5","line":520,"in_reply_to":"ba5201f7_2c35def6","updated":"2017-01-09 19:17:26.000000000","message":"i thought it was a LDAPHandler specific value, and didn\u0027t need to be in the common portion (which I thought was used for LDAPHandler and PooledLDAPHandler?)","commit_id":"572c35a06d438c50ea170f37cddd1df515a850e2"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"5e3820f23d28218d1c17ddea1c6fed21b7a6a7cc","unresolved":false,"context_lines":[{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        if conn_timeout is not None and conn_timeout \u003e 0:"},{"line_number":519,"context_line":"            # set network connection timeout"},{"line_number":520,"context_line":"             self.conn.set_option(ldap.OPT_NETWORK_TIMEOUT, conn_timeout)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        if use_tls:"},{"line_number":523,"context_line":"            self.conn.start_tls_s()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_2c35def6","line":520,"in_reply_to":"ba5201f7_500f212e","updated":"2017-01-09 18:20:02.000000000","message":"(first of all many thanks to Steve M. for keeping the fire going for this review... I was on holidays)\n\nditto. I had it inside common_ldap_intialization since that is where the rest of the LDAP set option magic was being done. Is there a specific concern with having it in there?","commit_id":"572c35a06d438c50ea170f37cddd1df515a850e2"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"4d17deeac0806c0ddf802ff2ccc101b1e8d95603","unresolved":false,"context_lines":[{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        if conn_timeout is not None and conn_timeout \u003e 0:"},{"line_number":519,"context_line":"            # set network connection timeout"},{"line_number":520,"context_line":"             self.conn.set_option(ldap.OPT_NETWORK_TIMEOUT, conn_timeout)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        if use_tls:"},{"line_number":523,"context_line":"            self.conn.start_tls_s()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_bd573625","line":520,"in_reply_to":"ba5201f7_7df86ec5","updated":"2017-01-09 19:33:00.000000000","message":"Kam, feel free to pick this back up when you\u0027re back from holidays. I just wanted to add the new config option, rebase and general clean up, sorry if I created a mess :)","commit_id":"572c35a06d438c50ea170f37cddd1df515a850e2"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"c9aaeec8221eff4e5be753e5298b8555d442e517","unresolved":false,"context_lines":[{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        if conn_timeout is not None and conn_timeout \u003e 0:"},{"line_number":519,"context_line":"            # set network connection timeout"},{"line_number":520,"context_line":"             self.conn.set_option(ldap.OPT_NETWORK_TIMEOUT, conn_timeout)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        if use_tls:"},{"line_number":523,"context_line":"            self.conn.start_tls_s()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba5201f7_04827100","line":520,"in_reply_to":"ba5201f7_bd573625","updated":"2017-01-10 21:11:42.000000000","message":"thanks Steve for the new connection_timeout option, and your assumption was correct, pool_conn_timeout to apply to PooledLDAPHandler and the new connection_timeout to apply to PythonLDAPHandler.\n\nI am back from holidays. I have to tweak your changes slightly to make them work, as well as add a mock for our UT and then I am hoping we are more or less done.","commit_id":"572c35a06d438c50ea170f37cddd1df515a850e2"}],"keystone/tests/unit/identity/backends/test_ldap_common.py":[{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"68090f892b02c81daf201383be0c76a44d48745c","unresolved":false,"context_lines":[{"line_number":301,"context_line":"class LDAPConnectionTimeoutTest(unit.TestCase):"},{"line_number":302,"context_line":"    \"\"\"Test for Network Connection timeout on LDAP URL connection.\"\"\""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"    def test_connectivity_timeout_no_conn_pool(self):"},{"line_number":305,"context_line":"        url \u003d \u0027ldap://localhost\u0027"},{"line_number":306,"context_line":"        conn_timeout \u003d 1  # 1 second"},{"line_number":307,"context_line":"        self.config_fixture.config(group\u003d\u0027ldap\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_7e4ddbc2","line":304,"range":{"start_line":304,"start_character":8,"end_line":304,"end_character":46},"updated":"2016-11-03 18:24:48.000000000","message":"this test (and the one below) tries to connect to ldap://localhost, right? Is there a reason why you are not mocking the response of the method instead of trying to create a real connection?","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"db10216fef68a626ef8a3fdda568a748ab2dfeb3","unresolved":false,"context_lines":[{"line_number":301,"context_line":"class LDAPConnectionTimeoutTest(unit.TestCase):"},{"line_number":302,"context_line":"    \"\"\"Test for Network Connection timeout on LDAP URL connection.\"\"\""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"    def test_connectivity_timeout_no_conn_pool(self):"},{"line_number":305,"context_line":"        url \u003d \u0027ldap://localhost\u0027"},{"line_number":306,"context_line":"        conn_timeout \u003d 1  # 1 second"},{"line_number":307,"context_line":"        self.config_fixture.config(group\u003d\u0027ldap\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a629dbe_fe93834d","line":304,"range":{"start_line":304,"start_character":8,"end_line":304,"end_character":46},"in_reply_to":"9a629dbe_dd8ec926","updated":"2016-11-07 16:29:55.000000000","message":"Agree with Rodrigo. On principle, in unit tests every attempt at outside communication is to be mocked.\n\nThis unit test should test only 2 things:\n1. setting the configuration option results in the setting of the ldap option\n2. *If* we handle the server_down exception, ensure we handle it correctly.\n\nBut aside from seeing it thrown, it doesn\u0027t look like we do anything with the exception, so I would even say there is no need to mock anything and it is fine to leave the assertRaises part out.\n\nTesting the correct operation of keystone + python-ldap + ldap server, really is the purpose of integration tests.","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"9d0ebe7fd34b948eb641a686c575cc931fa7c17e","unresolved":false,"context_lines":[{"line_number":301,"context_line":"class LDAPConnectionTimeoutTest(unit.TestCase):"},{"line_number":302,"context_line":"    \"\"\"Test for Network Connection timeout on LDAP URL connection.\"\"\""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"    def test_connectivity_timeout_no_conn_pool(self):"},{"line_number":305,"context_line":"        url \u003d \u0027ldap://localhost\u0027"},{"line_number":306,"context_line":"        conn_timeout \u003d 1  # 1 second"},{"line_number":307,"context_line":"        self.config_fixture.config(group\u003d\u0027ldap\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_b0577e25","line":304,"range":{"start_line":304,"start_character":8,"end_line":304,"end_character":46},"in_reply_to":"ba5da102_7e4ddbc2","updated":"2016-11-04 14:50:59.000000000","message":"that would mean I add a mock for simple_bind_s and add logic to generate exception on timeout. Why would I do that extra work when the ldap lib handles it for me? If there is a concern that ldap://localhost will connect to an actual LDAP running on dev environment, then we can change it to something like ldap://fakeurl\n\nThe purpose of this test is also to ensure that the OPT_NETWORK_TIMEOUT option is properly exercised by the python ldap library","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"25e4886b81f1a9553a4f8df6e966a719c8676908","unresolved":false,"context_lines":[{"line_number":301,"context_line":"class LDAPConnectionTimeoutTest(unit.TestCase):"},{"line_number":302,"context_line":"    \"\"\"Test for Network Connection timeout on LDAP URL connection.\"\"\""},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"    def test_connectivity_timeout_no_conn_pool(self):"},{"line_number":305,"context_line":"        url \u003d \u0027ldap://localhost\u0027"},{"line_number":306,"context_line":"        conn_timeout \u003d 1  # 1 second"},{"line_number":307,"context_line":"        self.config_fixture.config(group\u003d\u0027ldap\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a629dbe_dd8ec926","line":304,"range":{"start_line":304,"start_character":8,"end_line":304,"end_character":46},"in_reply_to":"ba5da102_b0577e25","updated":"2016-11-07 15:36:15.000000000","message":"adding a mock is not extra work, is the correct way to test this. The handling of OPT_NETWORK_TIMEOUT is task of the underlying library, not ours. Additionally, if you really want to check the integration, unit tests *are not* the place to do it.","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"68090f892b02c81daf201383be0c76a44d48745c","unresolved":false,"context_lines":[{"line_number":320,"context_line":"        # verify that simple bind times out"},{"line_number":321,"context_line":"        self.assertRaises(ldap.SERVER_DOWN,"},{"line_number":322,"context_line":"                          ldap_connection.conn.simple_bind_s,"},{"line_number":323,"context_line":"                          \u0027\u0027, \u0027\u0027)"},{"line_number":324,"context_line":""},{"line_number":325,"context_line":"    def test_connectivity_timeout_with_conn_pool(self):"},{"line_number":326,"context_line":"        url \u003d \u0027ldap://localhost\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_5e3d7729","line":323,"range":{"start_line":323,"start_character":26,"end_line":323,"end_character":32},"updated":"2016-11-03 18:24:48.000000000","message":"you don\u0027t need to pass this: https://github.com/openstack/keystone/blob/master/keystone/identity/backends/ldap/common.py#L445","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"9d0ebe7fd34b948eb641a686c575cc931fa7c17e","unresolved":false,"context_lines":[{"line_number":320,"context_line":"        # verify that simple bind times out"},{"line_number":321,"context_line":"        self.assertRaises(ldap.SERVER_DOWN,"},{"line_number":322,"context_line":"                          ldap_connection.conn.simple_bind_s,"},{"line_number":323,"context_line":"                          \u0027\u0027, \u0027\u0027)"},{"line_number":324,"context_line":""},{"line_number":325,"context_line":"    def test_connectivity_timeout_with_conn_pool(self):"},{"line_number":326,"context_line":"        url \u003d \u0027ldap://localhost\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_50162256","line":323,"range":{"start_line":323,"start_character":26,"end_line":323,"end_character":32},"in_reply_to":"ba5da102_5e3d7729","updated":"2016-11-04 14:50:59.000000000","message":"when I don\u0027t pass any value, user and password are set to None and the Python LDAP library will return a generic LDAP Error which is not what we want.","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"68090f892b02c81daf201383be0c76a44d48745c","unresolved":false,"context_lines":[{"line_number":342,"context_line":"        # verify that simple bind times out"},{"line_number":343,"context_line":"        self.assertRaises(ldappool.BackendError,"},{"line_number":344,"context_line":"                          ldap_connection.conn.simple_bind_s,"},{"line_number":345,"context_line":"                          \u0027\u0027, \u0027\u0027)"},{"line_number":346,"context_line":""},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"class SslTlsTest(unit.TestCase):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_fe0acb77","line":345,"range":{"start_line":345,"start_character":26,"end_line":345,"end_character":32},"updated":"2016-11-03 18:24:48.000000000","message":"you don\u0027t need to pass this: https://github.com/openstack/keystone/blob/master/keystone/identity/backends/ldap/common.py#L445","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"9d0ebe7fd34b948eb641a686c575cc931fa7c17e","unresolved":false,"context_lines":[{"line_number":342,"context_line":"        # verify that simple bind times out"},{"line_number":343,"context_line":"        self.assertRaises(ldappool.BackendError,"},{"line_number":344,"context_line":"                          ldap_connection.conn.simple_bind_s,"},{"line_number":345,"context_line":"                          \u0027\u0027, \u0027\u0027)"},{"line_number":346,"context_line":""},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"class SslTlsTest(unit.TestCase):"}],"source_content_type":"text/x-python","patch_set":4,"id":"ba5da102_d002320b","line":345,"range":{"start_line":345,"start_character":26,"end_line":345,"end_character":32},"in_reply_to":"ba5da102_fe0acb77","updated":"2016-11-04 14:50:59.000000000","message":"see above","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"df3352fd0876f2d3aee9dd9f2e6435b14ba11b49","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        #"},{"line_number":319,"context_line":"        # We will not verify if an LDAP bind returns the timeout"},{"line_number":320,"context_line":"        # exception as that would fall under the realm of"},{"line_number":321,"context_line":"        # integration testing. If the LDAP option is set properly,"},{"line_number":322,"context_line":"        # and we get back a valid connection URI then that should"},{"line_number":323,"context_line":"        # suffice for this unit test."},{"line_number":324,"context_line":"        self.assertEqual(conn_timeout,"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_09d72b24","line":321,"updated":"2016-11-29 22:46:21.000000000","message":"Well - we could mock the .connect() method to wait a certain amount of time longer than the timeout. Which wouldn\u0027t require integration of any other systems.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"bf289cd4ab31e4c2037d7ec77af8cad39eb41b99","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        #"},{"line_number":319,"context_line":"        # We will not verify if an LDAP bind returns the timeout"},{"line_number":320,"context_line":"        # exception as that would fall under the realm of"},{"line_number":321,"context_line":"        # integration testing. If the LDAP option is set properly,"},{"line_number":322,"context_line":"        # and we get back a valid connection URI then that should"},{"line_number":323,"context_line":"        # suffice for this unit test."},{"line_number":324,"context_line":"        self.assertEqual(conn_timeout,"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_0f54281f","line":321,"in_reply_to":"3a71b18c_9151abdb","updated":"2016-12-09 17:51:58.000000000","message":"yes you are right, the test does not cover the exception which is why your tox passed. Since we don\u0027t call bind(), no call to the underlying library is made to raise that exception (I did have that bind() in previous patch sets but that was more akin to integration testing). \n\nI will mock simple_bind_s() to timeout after OPT_NETWORK_TIMEOUT and generate that exception.  I might have a few questions on that as I chug along and will message you on IRC.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"f55e059b69d32c8b5ad6b95ce18d6df664923a26","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        #"},{"line_number":319,"context_line":"        # We will not verify if an LDAP bind returns the timeout"},{"line_number":320,"context_line":"        # exception as that would fall under the realm of"},{"line_number":321,"context_line":"        # integration testing. If the LDAP option is set properly,"},{"line_number":322,"context_line":"        # and we get back a valid connection URI then that should"},{"line_number":323,"context_line":"        # suffice for this unit test."},{"line_number":324,"context_line":"        self.assertEqual(conn_timeout,"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_89225b00","line":321,"in_reply_to":"5a74a57a_09d72b24","updated":"2016-11-29 22:53:07.000000000","message":"we assume that the underlying PythonLDAP/LDAPPool libraries correctly use the OPT_NETWORK_TIMEOUT option, infact earlier patch sets of this commit were attempting a bind (but that is more akin to an integration test) and validating just that.\n\nI digress, what good would mocking connect do? Would we just read the value of OPT_NETWORK_TIMEOUT and set it as a manual timeout? What good would that serve us, we already know that OPT_NETWORK_TIMEOUT is set properly (the assertEqual below)","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"469ae6e3508645e6ace8828798c24226d6571be8","unresolved":false,"context_lines":[{"line_number":318,"context_line":"        #"},{"line_number":319,"context_line":"        # We will not verify if an LDAP bind returns the timeout"},{"line_number":320,"context_line":"        # exception as that would fall under the realm of"},{"line_number":321,"context_line":"        # integration testing. If the LDAP option is set properly,"},{"line_number":322,"context_line":"        # and we get back a valid connection URI then that should"},{"line_number":323,"context_line":"        # suffice for this unit test."},{"line_number":324,"context_line":"        self.assertEqual(conn_timeout,"}],"source_content_type":"text/x-python","patch_set":7,"id":"3a71b18c_9151abdb","line":321,"in_reply_to":"5a74a57a_89225b00","updated":"2016-12-07 17:16:41.000000000","message":"I\u0027m sure the underlying libraries use the options correctly, and how they use them isn\u0027t our responsibility to test. My whole reason for suggesting a mock was that you\u0027d be able to perform a negative test by mocking whatever you\u0027re expecting an exception from and ensuring the try/except in your code is doing what it should. Running coverage shows your tests don\u0027t cover the exception you\u0027re introducing in this commit. I can revert the change [0] and the tests still pass [1]. I wouldn\u0027t want to lose your fix because it was lost to regression.\n\n\n[0] http://paste.openstack.org/show/591674/\n[1] http://paste.openstack.org/show/591675/","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"df3352fd0876f2d3aee9dd9f2e6435b14ba11b49","unresolved":false,"context_lines":[{"line_number":343,"context_line":"        #"},{"line_number":344,"context_line":"        # We will not verify if an LDAP bind returns the timeout"},{"line_number":345,"context_line":"        # exception as that would fall under the realm of"},{"line_number":346,"context_line":"        # integration testing. If the LDAP option is set properly,"},{"line_number":347,"context_line":"        # and we get back a valid connection URI then that should"},{"line_number":348,"context_line":"        # suffice for this unit test."},{"line_number":349,"context_line":"        self.assertEqual(conn_timeout,"}],"source_content_type":"text/x-python","patch_set":7,"id":"5a74a57a_09be4bd4","line":346,"updated":"2016-11-29 22:46:21.000000000","message":"Same comment here.","commit_id":"6e55733cf14cc97ae5dcae9490a15a6b9377b6f8"},{"author":{"_account_id":17860,"name":"Samuel de Medeiros Queiroz","email":"samueldmq@gmail.com","username":"samueldmq"},"change_message_id":"6063a3211dce7eeb11f68b7caaa035d3bfa6bb4b","unresolved":false,"context_lines":[{"line_number":320,"context_line":"        # exception as that would fall under the realm of"},{"line_number":321,"context_line":"        # integration testing. If the LDAP option is set properly,"},{"line_number":322,"context_line":"        # and we get back a valid connection URI then that should"},{"line_number":323,"context_line":"        # suffice for this unit test."},{"line_number":324,"context_line":"        self.assertEqual(conn_timeout,"},{"line_number":325,"context_line":"                         ldap.get_option(ldap.OPT_NETWORK_TIMEOUT))"},{"line_number":326,"context_line":"        self.assertEqual(url, ldap_connection.conn.conn._uri)"}],"source_content_type":"text/x-python","patch_set":10,"id":"9a57fde8_e3b3fa08","line":323,"updated":"2017-01-12 09:39:00.000000000","message":"We could be mocking things and adding a negative tests, as pointed out by Lance on patchset 7.","commit_id":"2d239cfbc37573f245e6560b42117828b73d19b9"}],"releasenotes/notes/bug-1636950-8fa1a47fce440977.yaml":[{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"313d6274aa216496d15679cecdaa56426a54e3e9","unresolved":false,"context_lines":[{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - \u003e"},{"line_number":4,"context_line":"    [`bug 1636950 \u003chttps://bugs.launchpad.net/keystone/+bug/1636950\u003e`_]"},{"line_number":5,"context_line":"    Use the previously unused \"pool_connection_timeout\""},{"line_number":6,"context_line":"    configuration option, avaiable within the ldap stanza,"},{"line_number":7,"context_line":"    to set a new OPT_NETWORK_TIMEOUT option on the LDAP backend."},{"line_number":8,"context_line":"    This allows the LDAP server to return a SERVER_DOWN exception,"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"ba5da102_877537fb","line":5,"range":{"start_line":5,"start_character":31,"end_line":5,"end_character":54},"updated":"2016-11-02 18:21:12.000000000","message":"use backticks `` instead of quotes \"","commit_id":"11605ef0330bd7531d210b6f00f52b5daf2f6fc7"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"313d6274aa216496d15679cecdaa56426a54e3e9","unresolved":false,"context_lines":[{"line_number":3,"context_line":"  - \u003e"},{"line_number":4,"context_line":"    [`bug 1636950 \u003chttps://bugs.launchpad.net/keystone/+bug/1636950\u003e`_]"},{"line_number":5,"context_line":"    Use the previously unused \"pool_connection_timeout\""},{"line_number":6,"context_line":"    configuration option, avaiable within the ldap stanza,"},{"line_number":7,"context_line":"    to set a new OPT_NETWORK_TIMEOUT option on the LDAP backend."},{"line_number":8,"context_line":"    This allows the LDAP server to return a SERVER_DOWN exception,"},{"line_number":9,"context_line":"    if a) the LDAP URL is incorrect, or b) there is a connection"}],"source_content_type":"text/x-yaml","patch_set":3,"id":"ba5da102_e766f34f","line":6,"range":{"start_line":6,"start_character":46,"end_line":6,"end_character":50},"updated":"2016-11-02 18:21:12.000000000","message":"``[ldap]``","commit_id":"11605ef0330bd7531d210b6f00f52b5daf2f6fc7"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"313d6274aa216496d15679cecdaa56426a54e3e9","unresolved":false,"context_lines":[{"line_number":9,"context_line":"    if a) the LDAP URL is incorrect, or b) there is a connection"},{"line_number":10,"context_line":"    failure/link loss."},{"line_number":11,"context_line":"other:"},{"line_number":12,"context_line":"  - The default value of \"pool_connection_timeout\" is -1"},{"line_number":13,"context_line":"    in keystone.conf, implying that option is disabled."},{"line_number":14,"context_line":"    Set a non-zero timeout value (in seconds) to enable"},{"line_number":15,"context_line":"    this option."}],"source_content_type":"text/x-yaml","patch_set":3,"id":"ba5da102_6753c36b","line":12,"range":{"start_line":12,"start_character":26,"end_line":12,"end_character":49},"updated":"2016-11-02 18:21:12.000000000","message":"use backticks `` instead of quotes \"","commit_id":"11605ef0330bd7531d210b6f00f52b5daf2f6fc7"},{"author":{"_account_id":6482,"name":"Steve Martinelli","email":"s.martinelli@gmail.com","username":"stevemar"},"change_message_id":"313d6274aa216496d15679cecdaa56426a54e3e9","unresolved":false,"context_lines":[{"line_number":10,"context_line":"    failure/link loss."},{"line_number":11,"context_line":"other:"},{"line_number":12,"context_line":"  - The default value of \"pool_connection_timeout\" is -1"},{"line_number":13,"context_line":"    in keystone.conf, implying that option is disabled."},{"line_number":14,"context_line":"    Set a non-zero timeout value (in seconds) to enable"},{"line_number":15,"context_line":"    this option."}],"source_content_type":"text/x-yaml","patch_set":3,"id":"ba5da102_c7c80f25","line":13,"range":{"start_line":13,"start_character":7,"end_line":13,"end_character":20},"updated":"2016-11-02 18:21:12.000000000","message":"wrap this in backticks\n\n  ``keystone.conf``","commit_id":"11605ef0330bd7531d210b6f00f52b5daf2f6fc7"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"68090f892b02c81daf201383be0c76a44d48745c","unresolved":false,"context_lines":[{"line_number":11,"context_line":"other:"},{"line_number":12,"context_line":"  - The default value of ``pool_connection_timeout`` is -1"},{"line_number":13,"context_line":"    in ``keystone.conf``, implying that option is disabled."},{"line_number":14,"context_line":"    Set a non-zero timeout value (in seconds) to enable"},{"line_number":15,"context_line":"    this option."}],"source_content_type":"text/x-yaml","patch_set":4,"id":"ba5da102_1eadbf9a","line":14,"range":{"start_line":14,"start_character":10,"end_line":14,"end_character":18},"updated":"2016-11-03 18:24:48.000000000","message":"non-zero also implies negative values, think you need to be clearer about the restrictions here","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":21109,"name":"Kam Nasim","email":"kam.nasim@windriver.com","username":"knasim-wrs"},"change_message_id":"c9e3b68bdcd13b572f48a30df683a5d13692d9ff","unresolved":false,"context_lines":[{"line_number":11,"context_line":"other:"},{"line_number":12,"context_line":"  - The default value of ``pool_connection_timeout`` is -1"},{"line_number":13,"context_line":"    in ``keystone.conf``, implying that option is disabled."},{"line_number":14,"context_line":"    Set a non-zero timeout value (in seconds) to enable"},{"line_number":15,"context_line":"    this option."}],"source_content_type":"text/x-yaml","patch_set":4,"id":"ba5da102_b3f068cb","line":14,"range":{"start_line":14,"start_character":10,"end_line":14,"end_character":18},"in_reply_to":"ba5da102_1eadbf9a","updated":"2016-11-04 14:58:10.000000000","message":"we are explicitly mentioning that timeout is in seconds. A negative time value doesn\u0027t make a lot of sense in that context.","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":11022,"name":"Rodrigo Duarte Sousa","email":"rodrigodsousa@gmail.com","username":"rodrigods"},"change_message_id":"25e4886b81f1a9553a4f8df6e966a719c8676908","unresolved":false,"context_lines":[{"line_number":11,"context_line":"other:"},{"line_number":12,"context_line":"  - The default value of ``pool_connection_timeout`` is -1"},{"line_number":13,"context_line":"    in ``keystone.conf``, implying that option is disabled."},{"line_number":14,"context_line":"    Set a non-zero timeout value (in seconds) to enable"},{"line_number":15,"context_line":"    this option."}],"source_content_type":"text/x-yaml","patch_set":4,"id":"9a629dbe_3db505cd","line":14,"range":{"start_line":14,"start_character":10,"end_line":14,"end_character":18},"in_reply_to":"ba5da102_b3f068cb","updated":"2016-11-07 15:36:15.000000000","message":"it doesn\u0027t, but I still believe a rewording is necessary.","commit_id":"b7dead1fb5d215f90fabc1924b1578eecb4140b5"},{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"12144cabf1b645364588f8836fdd0b6240dfb348","unresolved":false,"context_lines":[{"line_number":3,"context_line":"  - \u003e"},{"line_number":4,"context_line":"    [`bug 1636950 \u003chttps://bugs.launchpad.net/keystone/+bug/1636950\u003e`_]"},{"line_number":5,"context_line":"    Use the previously unused ``pool_connection_timeout``"},{"line_number":6,"context_line":"    configuration option, avaiable within the ``[ldap]`` stanza,"},{"line_number":7,"context_line":"    to set a new OPT_NETWORK_TIMEOUT option on the LDAP backend."},{"line_number":8,"context_line":"    This allows the LDAP server to return a SERVER_DOWN exception,"},{"line_number":9,"context_line":"    if a) the LDAP URL is incorrect, or b) there is a connection"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"7a77a97e_a89fdc4e","line":6,"range":{"start_line":6,"start_character":26,"end_line":6,"end_character":34},"updated":"2016-11-16 04:49:56.000000000","message":"available","commit_id":"4649f444fe826344c20824b70329aeec291d5286"},{"author":{"_account_id":8119,"name":"Eric Brown","email":"eric_wade_brown@yahoo.com","username":"ericwb"},"change_message_id":"12144cabf1b645364588f8836fdd0b6240dfb348","unresolved":false,"context_lines":[{"line_number":9,"context_line":"    if a) the LDAP URL is incorrect, or b) there is a connection"},{"line_number":10,"context_line":"    failure/link loss."},{"line_number":11,"context_line":"other:"},{"line_number":12,"context_line":"  - The default value of ``pool_connection_timeout`` is -1"},{"line_number":13,"context_line":"    in ``keystone.conf``, implying that option is disabled."},{"line_number":14,"context_line":"    Set a positive timeout value (in seconds) to enable"},{"line_number":15,"context_line":"    this option. This option affects both pooled and unpooled"},{"line_number":16,"context_line":"    LDAP connections."}],"source_content_type":"text/x-yaml","patch_set":6,"id":"7a77a97e_68ba2470","line":15,"range":{"start_line":12,"start_character":1,"end_line":15,"end_character":2},"updated":"2016-11-16 04:49:56.000000000","message":"Technically, its not necessarily disabled since if not set, the python-ldap client uses the value of NETWORK_TIMEOUT from /etc/ldap/ldap.conf","commit_id":"4649f444fe826344c20824b70329aeec291d5286"}]}