Make allowInsecureRegistries follow Docker semantics

[chanseokoh]

Fixes #643.

Before:
call(URL) embedded the logic for handling allowInsecureRegistries.

Now:
The logic is broken out of call(URL) into callWithInsecureRegistryHandling(). call(URL) always contacts the endpoint and is irrelevant of allowInsecureRegistries. callWithInsecureRegistryHandling() is the one that handles allowInsecureRegistries.

[coollog]

Hmm, I feel like the checking should be done here for whether or not to handle insecure registries based on the flag so that it launches two codepaths - one for calling without allowing insecure registries, and one for calling with allowing insecure registries - rather than have that logic be encapsulated in callWithInsecureRegistryHandling. From the function call here, it seems like insecure registry handling is the default.

[chanseokoh]

it launches two codepaths

It's not exactly like there will be two codepaths based on allowInsecureRegistries. They cross-over, for example, you can first contact an HTTP server (failover-to-HTTP occurred) which redirects you to a secure HTTPS server (or vice versa). With callWithInsecureRegistryHandling(), you don't have to think about those things anymore. You just pass a URL to callWithInsecureRegistryHandling() recursively and it will handle things correctly.

From the function call here, it seems like insecure registry handling is the default.

You are saying the naming callWithInsecureRegistryHandling is giving you the impression that it will attempt insecure communication, right? I'll think about a better name, but if you have some suggestions, let me know.

[coollog]

Ah yes, I'm thinking about this more in the lines of having allowInsecureRegistries be checked in one location to launch separate codepaths, one for secure and one for insecure. The one for insecure will be essentially the one for secure, except with additional functionality for handling HTTPS connect/TLS verification errors (codepaths with cross-over, but a different entrypoint). So, rather than checking allowInsecureRegistries in multiple places in callWithInsecureRegistryHandling, a single check would determine if the insecure handling code entrypoint would be launched vs the secure handling code entrypoint. I think I need to think more about this too.

[chanseokoh]

So, rather than checking allowInsecureRegistries in multiple places in callWithInsecureRegistryHandling, a single check would determine if the insecure handling code entrypoint would be launched vs the secure handling code entrypoint.

I think I got what you mean. That makes sense too. I'll try your idea and push up changes, and then we can evaluate which one will look better.

[chanseokoh]

Here's the attempt with your idea, if I understood it correctly: 6df4b42

I'm fine with either ways.

[coollog]

nit: newline after

[coollog]

For example, here, I think this would allow HTTP fallback even if allowInsecureRegistries was false.

[chanseokoh]

That can never happen. If allowInsecureRegistries is false and the server is not a verifiable HTTPS, it will throw an exception.

        // Cannot verify the server (e.g., self-signed certificate or plain HTTP).
        if (!allowInsecureRegistries) {
          throw new InsecureRegistryException(url);
        }

[coollog]

Ah right, I need to read this more carefully.

[coollog]

I think this looks better, but going to request for more opinions @GoogleContainerTools/java-tools

[coollog]

Hmm, it seems that insecureConnectionFactory doesn't need to be an instance variable.

[chanseokoh]

In fact, connectionFactory doesn't have to be so either, but it was there from the beginning, and I believe it is for mocking Connection. insecureConnectionFactory is there for the same reason.

But I guess you will wonder why the code has this null check.

        Connection.getConnectionFactory(),
        null /* might never be used, so create lazily to delay throwing potential GeneralSecurityException */);

So this is to avoid the potential GeneralSecurityException when creating the insecureConnectionFactory. For example, if the user never sets allowInsecureRegistries or the server was secure even if it was set, we can proceed without an error in the situation where GeneralSecurityException will otherwise be thrown if blindly created at the beginning.

[chanseokoh]

Here's the attempt with your idea, if I understood it correctly: 6df4b42
I'm fine with either ways.

I think this looks better, but going to request for more opinions @GoogleContainerTools/java-tools

Personally, I slightly prefer the previous approach for the following reasons:

1. After 6df4b42, you add one more method (possiblyInsecureCall) with similar names:
   • call()
   • callWithAllowInsecureRegistryHandling()
   • possiblyInsecureCall()
   • call(URL)
2. Although callWithAllowInsecureRegistryHandling() is now basically for determining the two different codepaths based on allowInsecureRegistries, the behavior of possiblyInsecureCall() is identical to that of callWithAllowInsecureRegistryHandling() if the endpoint is secure HTTPS, whether allowInsecureRegistries is true or not.

However, I usually and casually follow the feedback from reviewers, and the new code seems fine to me, so I'll go for the new code. It has its own merits too.

[coollog]

I think drawing out a state diagram might help with this - I'll sketch up something when I get back to somewhere with paper.

[coollog]

Feel free to dismiss this approval if making more changes.

Made non-negligible changes