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: adds X509 workload cert logic #1527

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

aeitzman
Copy link
Contributor

Adds support for X509 workload logic that reads from the certificate_config.json file. This PR does not actually call that logic yet - hooks for actually using the new logic will come when the credential type is added.

Copy link
Contributor

@andyrzhao andyrzhao left a comment

Choose a reason for hiding this comment

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

LGTM w.r.t business logic.

google/auth/transport/_mtls_helper.py Outdated Show resolved Hide resolved
google/auth/transport/_mtls_helper.py Outdated Show resolved Hide resolved
google/auth/transport/_mtls_helper.py Outdated Show resolved Hide resolved
@@ -63,20 +65,20 @@ def _check_dca_metadata_path(metadata_path):
return metadata_path


def _read_dca_metadata_file(metadata_path):
"""Loads context aware metadata from the given path.
def _read_json_file(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function's name is too vague. Can you rename it to provide more context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to load Json file, but that's not much better. Not sure what would be more descriptive, this method does simply just read a file and calls Json.load while wrapping in the appropriate error if it fails - open to suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is something like _load_workload_json_file or something. The code is generic but describing when / what it is used for is useful.

Otherwise it may get shoved in a util file down the line

Copy link
Contributor

Choose a reason for hiding this comment

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

With the changes in this PR, this function is used to load json configs for both secure connect as well as workload, hence the generic naming. I think it's fine as is - maybe add a function comment to clarify usage. It does seem more suitable in a generic util file, but I don't have a strong opinion 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.

added another sentence in the comment explaining the method is used for both x509 and secure connect, this definitely could be moved to a util file in the future though, just didn't think that was necessary for this PR.

@aeitzman aeitzman added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 10, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 10, 2024
@aeitzman aeitzman added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 10, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 10, 2024
@aeitzman aeitzman added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 10, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 10, 2024
@aeitzman aeitzman merged commit 05220e0 into googleapis:main Jun 10, 2024
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.

6 participants