Skip to content

Conversation

@rmehta19
Copy link
Contributor

No description provided.

@rmehta19 rmehta19 mentioned this pull request Sep 20, 2024
@rmehta19 rmehta19 changed the title Combine MtlsToS2ChannelCredentials and S2AChannelCredentials. s2a: Combine MtlsToS2ChannelCredentials and S2AChannelCredentials. Sep 20, 2024
@rmehta19 rmehta19 marked this pull request as ready for review September 25, 2024 22:41
File privateKeyFile = new File(privateKeyPath);
File certChainFile = new File(certChainPath);
File trustBundleFile = new File(trustBundlePath);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should verify that the files exist so that you can provide a meaningful error message if they don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2407bb6


public ChannelCredentials build() {
public ChannelCredentials build() throws IOException {
checkState(!isNullOrEmpty(s2aAddress), "S2A address must not be null or empty.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these are already checked in newBuilder(). They should be verified when being set, not during build(), so even if we add set methods later, we wouldn't want them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 36222a0

String certChainPath = "src/test/resources/client_cert.pem";
String trustBundlePath = "src/test/resources/root_cert.pem";
File privateKeyFile = new File(privateKeyPath);
if (!privateKeyFile.exists()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no point to doing this in the test. TlsChannelCredentials will throw if the files don't exist. Ditto in the other test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a81e242

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 27, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 27, 2024
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 27, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 27, 2024
@larry-safran larry-safran merged commit a140e1b into grpc:master Sep 30, 2024
15 checks passed
kannanjgithub pushed a commit to kannanjgithub/grpc-java that referenced this pull request Oct 23, 2024
…rpc#11544)

* Combine MtlsToS2ChannelCredentials and S2AChannelCredentials.

* Check if file exists.

* S2AChannelCredentials API requires credentials used for client-s2a channel.

* remove MtlsToS2A library in BUILD.

* Don't check state twice.

* Don't check for file existence in tests.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants