[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abnsy
Once this PR has been reviewed and has the lgtm label, please assign yue9944882 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

The committers listed above are authorized under a signed CLA.

• ✅ login: abnsy / name: Albern S. (8aef180)

Welcome @abnsy!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Can you clarify (especially w/ pointers to documentation) why this is preferable to using the OS default key store?

Can you clarify (especially w/ pointers to documentation) why this is preferable to using the OS default key store?

Sure....happy to clarify.....The main reason is consistency. When sslcontext is null Java falls back to whatever CA bundle the OS or JVM provides. On containers, Windows, Linux distros,or JVM vendors the trusted roots can be completely different....By initializing SSLContext explicitly, the client always behaves the same way and we know exactly what trust store is used... JSSE also recommends using your own SSLContext if you want predictable TLS behavior https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization Not trying to override the OS trust store just avoiding the silent fallback that varies across environments... thanks

So it seems to me that by specifying a null keystore when you initialize the trust store you get whatever CA bundles are OS installed anyway, so it's not clear to me that this code is actually doing anything.

It also seems to me based on reading https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#CustomizingStores that you can specify all this stuff via java properties and the defaults will just work.

Perhaps it would be useful to add a unit test which illustrates the problem with the current code and then show how this update fixes it?

Apologies for being picky here, but whenever we mess with encrypt/auth code I want to make sure it's for a good reason.

So it seems to me that by specifying a null keystore when you initialize the trust store you get whatever CA bundles are OS installed anyway, so it's not clear to me that this code is actually doing anything.

It also seems to me based on reading https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#CustomizingStores that you can specify all this stuff via java properties and the defaults will just work.

Perhaps it would be useful to add a unit test which illustrates the problem with the current code and then show how this update fixes it?

Apologies for being picky here, but whenever we mess with encrypt/auth code I want to make sure it's for a good reason.

Thanks for the detailed feedback that helps.

About the null keystore part you’re right that when you do tmf.init((KeyStore) null), you end up with whatever CA set the JVM/OS provides The intent wasn’t to replace that, but to make the behavior explicit
instead of relying on the implicit fallback that only happens when sslContext
is left null. Right now when sslContext stays null the behavior depends on which
HttpsURLConnection implementation you hit and what the environment does.
For example:

• some distroless container images ship with a very limited CA set,
• some enterprise Windows environments inject internal root CAs,
• Alpine uses a different CA bundle than Debian-based images,
• and some embedded JVM runtimes include their own CA store.

So the fallback is whatever the platform feels like, while explicitly
initializing SSLContext always gives the same behavior across environments. I'm absolutely fine switching this to TLSv1.2 to stay consistent with the rest of the code and avoid older protocol versions.

On the unit test idea.....yeah, that makes sense. I can put together a small test
showing the difference between:

1. sslContext == null (current behavior)
2. sslContext initialized with tmf.init(null)
3. sslContext initialized with custom CA