Skip to content

Conversation

@ejona86
Copy link
Member

@ejona86 ejona86 commented Sep 22, 2025

Considering as an alternative to #12340.

XdsSecurityClientServerTest is failing because the exception changed; I don't know why it is failing during a different part of handshaking now. The other test failures are white box tests that have to get updated, because of IgnoreUpdatesWatcher. If we like this, I don't know if you want to take it and fix the tests (and add tests), or wait for me.

@kannanjgithub
Copy link
Contributor

kannanjgithub commented Sep 23, 2025

The MTLS test failures in XdsSecurityClientServerTest are because you changed isMtls etc to check certHandle and rootCertHandle members instead of the certInstance and rootCertInstance members.
When executing

certHandle = certProviderInstanceConfig == null ? null
          : certificateProviderStore.createOrGetProvider(
              certInstance.getCertificateName(),
              certProviderInstanceConfig.pluginName(),
              certProviderInstanceConfig.config(),
              watcher,
              true)::close;

the file watcher cert provider already makes the callbacks for certs and roots and before this statement returns, both updateCerts and updateTrustedRoots are called, and they see that certHandle and rootCertHandle members are null, so no update happens and the Tls handshake times out after 10s.

I think we should revert the changes in isMtls etc to still continue to check certInstance and rootCertInstance member variables that were initialized earlier in the code flow.

I do like the approach, I will fix the tests and incorporate my changes on top of this.

@kannanjgithub
Copy link
Contributor

I have incorporated these changes into my PR #12340 with the fixes.

@ejona86
Copy link
Member Author

ejona86 commented Sep 23, 2025

the file watcher cert provider already makes the callbacks for certs and roots and before this statement returns

Ah, yes. I had seen that was a risk while working on this code, but I ended up stepping on it. I do think we should adjust isMtls() and friends are computed to make it less bug prone, but I agree with not doing it right now.

Closing, as this PR has been integrated into #12340.

@ejona86 ejona86 closed this Sep 23, 2025
@ejona86 ejona86 deleted the xds-system-cert branch September 23, 2025 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants