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

alts: Introduce AltsContext to allow outside packages accessing ALTS information #7862

Merged
merged 4 commits into from
Feb 9, 2021

Conversation

martin-schaub
Copy link
Contributor

No description provided.

@martin-schaub martin-schaub changed the title Introduce AltsContext to allow outside packages accessing ALTS information alts: Introduce AltsContext to allow outside packages accessing ALTS information Feb 2, 2021
@martin-schaub martin-schaub marked this pull request as ready for review February 2, 2021 12:30
alts/src/main/java/io/grpc/alts/AltsContext.java Outdated Show resolved Hide resolved
this.wrapped = wrapped;
}

@VisibleForTesting
Copy link
Member

Choose a reason for hiding this comment

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

Is this for testing this class or for letting other classes testing themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should make classes that use the AltsContext testable. However, without parameters this isn't very useful. Therefore I've changed the method to specify the service accounts (security level is fixed anyway).

Copy link
Member

Choose a reason for hiding this comment

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

While security level is fixed today, you can't actually test any code that looks at it if it is hard-coded during testing. It seems like maybe we should just suck it up and make a builder fascade as well, that delegates to HandshakerResult's builder. @jiangtaoli2016, thoughts?

I'm actually fairly okay with this as it stands, if we do want to leave it as-is. We tend to limit VisibleForTesting usage to instances where the project's own unit tests need access. For cases like this, we'd just call this "a public API". Let's remove the VisibleForTesting annotation here? I honestly don't see much harm someone could do, even using it in production.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with it, since AltsContext is just a wrapper of AltsInternalContext. This fixed security level is only exposed for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the VisibleForTesting annotation.

alts/src/main/java/io/grpc/alts/AltsContext.java Outdated Show resolved Hide resolved
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

One comment that could be small or could be more, depending on how @jiangtaoli2016 feels about it as well.

@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 Feb 3, 2021
@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 Feb 3, 2021
Copy link
Contributor

@jiangtaoli2016 jiangtaoli2016 left a comment

Choose a reason for hiding this comment

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

Thanks for creating the AltsContext Wrapper.

@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 Feb 5, 2021
@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 Feb 5, 2021
@martin-schaub
Copy link
Contributor Author

@ejona86 is there something missing or could you merge please?

@ejona86
Copy link
Member

ejona86 commented Feb 9, 2021

Oh, sorry. I had been waiting for the CI to finish and then failed to come back around to it.

@ejona86 ejona86 merged commit 514101d into grpc:master Feb 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2021
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