Do not throw on initial LDAP connection failure #9

alechenninger · 2018-01-06T19:15:38Z

Throwing exceptions during LdapRolesProvider initialization can put
applications into a bad state where they do not recover on their own.
If LDAP connectivity is unavailable, we can at least let the
application keep running and report a more meaningful failure, as
opposed to failing to startup completely, unable to report to us
anything automatically (and thus requiring looking at logs and such).
This can also allow applications to recover on their own without
requiring manual intervention.

Throwing exceptions during LdapRolesProvider initialization can put applications into a bad state where they do not recover on their own. If LDAP connectivity is unavailable, we can at least let the application keep running and report a more meaningful failure, as opposed to failing to startup completely, unable to report to us anything automatically (and thus requiring looking at logs and such). This can also allow applications to recover on their own without requiring manual intervention.

dcrissman

So what happens now? Is an empty connection pool returned? Or will it try to make connections?

alechenninger · 2018-01-07T01:43:36Z

What happens now is what I described in the description. The LdapRolesProvider constructor throws an exception, which bubbles up. Depending on the application, this can prevent the application from deploying or starting successfully, never recovering without a restart.

alechenninger · 2018-01-07T01:46:01Z

I haven't had a chance to test this yet though, I'd like confirm that still. Specifically that it's failing at creating the pool as opposed to an earlier point.

alechenninger · 2018-01-07T01:48:15Z

Yeah nevermind, it's failing before it gets to the pool... I was hoping by the existence of that parameter it wouldn't try to connect until the pool was created. I'll have to get a little more creative.

This reverts commit fee195e.

When true, existing behavior remains. Existing behavior does not allow an instance to be constructed if LDAP connectivity cannot be established. When false, an instance will be created even if the connection fails. getUserRoles will fail until the connection is established. Connection will be retried on the next role lookup after the configured retry interval. Ideally connections would be retried in the background without a lookup being called, but this requires a thread pool, and a thread pool requires managing the life cycle of the roles provider, which might be difficult in the JBoss login module mode. For now this is a simpler alternative that should more or less work the same under traffic.

alechenninger · 2018-01-07T18:24:07Z

Ready for re-review

alechenninger · 2018-01-07T18:26:32Z

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapRolesProvider.java

-                    ldapConfiguration.getBindDn(),
-                    ldapConfiguration.getBindDNPwd()
-            );
+            ldapConnection = new LDAPConnection(options);


This and the above branch use a different LDAPConnection constructor so we create an LDAPConnection without actually connecting yet.

derek63 · 2018-01-08T12:44:56Z

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapRolesProvider.java

-        BindRequest bindRequest = new SimpleBindRequest(ldapConfiguration.getBindDn(), ldapConfiguration.getBindDNPwd());
-        BindResult bindResult = ldapConnection.bind(bindRequest);
+        if (!readyForConnectionAttempt()) {
+            // It's too early to retry connecting again.


I know we want to have automated connection retries after a certain time period (if no requests come in that trigger connectIfNeeded()), but if we're getting requests, why would we want to wait, as it is possible that the connection works the next time you try it before the specified time interval passes.

If all requests wait, and it takes a while, we could fill thread pools server side and/or client side as lots of requests could pile up. It could also cause cascading failure if timeouts are hit by clients. It's the same idea as hystrix. We can control how often we want to retry with the retry interval config parameter. There's not much advantage I think to retrying multiple times within a second for example, but I suppose someone could even configure that time to 0 and then we would constantly retry.

Yeah I guess we wouldn't define a long interval anyway, so what you're saying makes sense.

derek63 · 2018-01-08T12:47:06Z

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapRolesProvider.java

-        if (bindResult.getResultCode() != ResultCode.SUCCESS) {
-            LOGGER.error("Error binding to LDAP" + bindResult.getResultCode());
-            throw new LDAPException(bindResult.getResultCode(), "Error binding to LDAP");
+        if (!attemptingConnect.compareAndSet(/*expect*/ false, /*if false then set to*/ true)) {


Please consider revising how this is documented, (either with non-inline comments or extracting this out to a method call or something), as I find these inline comments vary hard to read.

Okay, so the comment is awesome, and describes very well the iceberg of reasoning under this one line, but it still has the /* inline comments */

Maybe I am alone in being bothered by these

Haha, I usually inline comment any parameters that are ambiguous (like boolean flags), but if you think it hurts readability in this case I'm fine with removing them.

derek63 · 2018-01-08T12:49:45Z

jboss-cert-ldap-login-module/src/main/java/org/esbtools/auth/jboss/CertLdapLoginModule.java

@@ -63,6 +63,7 @@
    public static final String ROLES_CACHE_EXPIRY_MS = "rolesCacheExpiryMS";
    public static final String ENVIRONMENT = "environment";
    public static final String ALL_ACCESS_OU = "allAccessOu";
+    public static final String RETRY_INTERVAL_SECONDS = "retryIntervalSeconds";

    private static final String[] ALL_VALID_OPTIONS = {
            AUTH_ROLE_NAME, SERVER, PORT, SEARCH_BASE, BIND_DN, BIND_PWD, USE_SSL,


You need to add RETRY_INTERVAL_SECONDS to ALL_VALID_OPTIONS, otherwise we'll get warnings in the logs

derek63 · 2018-01-08T14:38:01Z

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapRolesProvider.java

-        if (bindResult.getResultCode() != ResultCode.SUCCESS) {
-            LOGGER.error("Error binding to LDAP" + bindResult.getResultCode());
-            throw new LDAPException(bindResult.getResultCode(), "Error binding to LDAP");
+        if (!attemptingConnect.compareAndSet(/*expect*/ false, /*if false then set to*/ true)) {


Okay, so the comment is awesome, and describes very well the iceberg of reasoning under this one line, but it still has the /* inline comments */

Maybe I am alone in being bothered by these

dcrissman · 2018-01-08T14:26:40Z

cert-ldap-login-module-common/src/main/java/org/esbtools/auth/ldap/LdapRolesProvider.java

    private LDAPConnectionPool connectionPool;
+    private volatile LDAPException connectionException;


It is odd to have an exception stored volatile.

Some other thread might have assigned an exception while another thread goes to read it, we want to avoid reading an earlier failure or worse seeing null. It's not a huge deal if it's not immediately seen but because the variable won't be checked that often I figured better to be correct.

alechenninger requested review from derek63 and dcrissman January 6, 2018 19:15

Also comment initialConnections parameter

c20307e

dcrissman reviewed Jan 6, 2018

View reviewed changes

alechenninger changed the title ~~Do not throw on initial LDAP connection failure~~ [Do not merge] Do not throw on initial LDAP connection failure Jan 7, 2018

alechenninger added 3 commits January 6, 2018 21:08

Experiment with failFast flag

fee195e

Revert "Experiment with failFast flag"

ab7226b

This reverts commit fee195e.

alechenninger changed the title ~~[Do not merge] Do not throw on initial LDAP connection failure~~ Do not throw on initial LDAP connection failure Jan 7, 2018

alechenninger added 2 commits January 6, 2018 23:54

Stop ldap server after test

20de6e1

Add missing ;

445922d

alechenninger commented Jan 7, 2018

View reviewed changes

derek63 requested changes Jan 8, 2018

View reviewed changes

alechenninger added 2 commits January 8, 2018 09:30

Add RETRY_INTERVAL_SECONDS to valid options list

7a19359

Better document the atomic CAS around connection attempts

5dcc0d8

derek63 requested changes Jan 8, 2018

View reviewed changes

dcrissman reviewed Jan 8, 2018

View reviewed changes

Shorten the inline parameter comments to just the parameter names

8afa24a

derek63 approved these changes Jan 10, 2018

View reviewed changes

dcrissman merged commit 4235aa6 into esbtools:master Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not throw on initial LDAP connection failure #9

Do not throw on initial LDAP connection failure #9

alechenninger commented Jan 6, 2018

dcrissman left a comment

alechenninger commented Jan 7, 2018

alechenninger commented Jan 7, 2018 •

edited

alechenninger commented Jan 7, 2018

alechenninger commented Jan 7, 2018

alechenninger Jan 7, 2018

derek63 Jan 8, 2018

alechenninger Jan 8, 2018 •

edited

derek63 Jan 8, 2018

derek63 Jan 8, 2018 •

edited

derek63 Jan 8, 2018

alechenninger Jan 8, 2018

derek63 Jan 8, 2018

derek63 Jan 8, 2018

dcrissman Jan 8, 2018

alechenninger Jan 8, 2018

		private LDAPConnectionPool connectionPool;
		private volatile LDAPException connectionException;

Do not throw on initial LDAP connection failure #9

Do not throw on initial LDAP connection failure #9

Conversation

alechenninger commented Jan 6, 2018

dcrissman left a comment

Choose a reason for hiding this comment

alechenninger commented Jan 7, 2018

alechenninger commented Jan 7, 2018 • edited

alechenninger commented Jan 7, 2018

alechenninger commented Jan 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alechenninger Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek63 Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alechenninger commented Jan 7, 2018 •

edited

alechenninger Jan 8, 2018 •

edited

derek63 Jan 8, 2018 •

edited