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

feat: Add custom tls signer for ECP Provider. #1402

Merged
merged 12 commits into from
Nov 30, 2023

Conversation

clundin25
Copy link
Contributor

No description provided.

@clundin25 clundin25 requested review from a team as code owners October 30, 2023 21:41
@clundin25 clundin25 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 31, 2023
@clundin25 clundin25 added owlbot:run Add this label to trigger the Owlbot post processor. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Nov 30, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 30, 2023
Copy link
Contributor

@arithmetic1728 arithmetic1728 left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments.

self._enterprise_cert_file_path.encode("ascii"),
):
raise exceptions.MutualTLSChannelError(
"failed to configure SSL context"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe update the error message to say which approach was used, something like "failed to configure SSL context using provider lib" / "failed to configure SSL context using offload lib". This will make it easier to identify issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added!

@@ -197,23 +213,31 @@ def __init__(self, enterprise_cert_file_path):
}
"""
self._enterprise_cert_file_path = enterprise_cert_file_path
self._cert = None
self._sign_callback = None
self._provider_lib = None
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep the following lines?

self._cert = None
self._sign_callback = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, that was a mistake. I'll add those back.

@clundin25 clundin25 added kokoro:run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Nov 30, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 30, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 30, 2023
@clundin25 clundin25 changed the title feat: Migrate custom tls signer to ECP Provider. feat: Add custom tls signer for ECP Provider. Nov 30, 2023
@clundin25 clundin25 added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 30, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 30, 2023
@clundin25 clundin25 merged commit 39eb287 into googleapis:main Nov 30, 2023
14 checks passed
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.

None yet

3 participants