Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s2a: Combine MtlsToS2ChannelCredentials and S2AChannelCredentials. #11544

Merged
merged 8 commits into from
Sep 30, 2024

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.
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.

4 participants