[WIP] MINOR: Use named class for ExpiringCredential to improve principalLogText() output

[ijuma]

ExpiringCredentialRefreshingLogin.principalLogText() outputs the expiring credential
simple class name, which is empty for anonymous classes.

I also took the change to remove effectively dead code given that some of
the checks would always have the same result.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[ijuma]

@rondagostino Do you recall why we had this code? I don't understand how we'd have no expiring credential and a non-null principal name. It seems like the latter would always be set from the expiring credential? Or am I missing something?

[rondagostino]

You are correct, given the way the code is written it is not possible to have both expiringCredential == null and principalName != null. I can't remember specifically why the code was written this way, but it was probably just being defensive in case the above constraint is ever violated. But the constraint is an intuitive one and the code will almost certainly not evolve in such a way as to violate it, so I think it may just cause the question to be raised and add to confusion.

[rondagostino]

Ok, I looked at this again, and I think there was a mistake combined with a poorly named variable that was causing confusion.

The private String principalName = null; variable that you want to remove should have originally been named private String lastKnownPrincipalName = null;

The reason this variable exists is because of the potential for this case to occur when trying to invoke reLogin():

            if (!hasExpiringCredential) {
                /*
                 * Re-login has failed because our login() invocation has not generated a
                 * credential but has also not generated an exception. We won't exit here;
                 * instead we will allow login retries in case we can somehow fix the issue (it
                 * seems likely to be a bug, but it doesn't hurt to keep trying to refresh).
                 */
                log.error("No Expiring Credential after a supposedly-successful re-login");
                principalName = null;

The bug is the line principalName = null; -- that line should not exist. If we had named the variable correctly we would have seen that we are blanking out the last-known principal name, which we should not do. This case should result in the expiring credential being null but the last known principal reflecting the principal that we last had, but with this line it doesn't happen -- we get a null credential and a null last-known name, which is pointless (and therefore I see why with the bad name you wanted to remove it). But keeping this last-known principal name around is why the logging-related method exists as follows (with the rename from principalName to lastKnownPrincipalName included):

    private String principalLogText() {
        return expiringCredential == null ? lastKnownPrincipalName
                : expiringCredential.getClass().getSimpleName() + ":" + lastKnownPrincipalName;
    }

Apologies for this morass -- two mistakes compounded into something quite confusing.