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

Issuer plugin for Google CA Manager #4816

Merged
merged 17 commits into from Apr 22, 2024

Conversation

odopertchouk
Copy link
Contributor

@odopertchouk odopertchouk commented Apr 9, 2024

A plugin that handles certificate issuance via Google CA Manager.

The plugin requires GOOGLE_ACCOUNT_CREDENTIALS config variable, which should point at the file containing credentials that Lemur is using to connect to Google Cloud Platform.
These credentials normally would be for a service account that has permissions privateca.certificates.update, privateca.certificates.create and privateca.certificateAuthorities.get

This requires packages

  • google-cloud-private-ca package
  • protobuf
  • google-cloud-private-ca

Copy link
Contributor

@jmcrawford45 jmcrawford45 left a comment

Choose a reason for hiding this comment

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

First of all, thank you for the work you've put into creating this new issuer plugin for the Google Certificate Authority Service. I've left a few minor comments in the code for things that could be improved. Before merging, we'd like to have some tests in place that cover the main functionality of your plugin, such as certificate creation, revocation, and fetching. Additionally, if you could add a changelog entry to make consumers aware of the new functionality, that'd be appreciated.

We appreciate your contribution and look forward to seeing the improvements and tests. If you need any help or clarification, feel free to ask.

lemur/plugins/lemur_google_ca/plugin.py Outdated Show resolved Hide resolved
lemur/plugins/lemur_google_ca/plugin.py Outdated Show resolved Hide resolved
lemur/plugins/lemur_google_ca/plugin.py Outdated Show resolved Hide resolved
lemur/plugins/lemur_google_ca/plugin.py Outdated Show resolved Hide resolved
odopertchouk and others added 5 commits April 10, 2024 12:32
Remove duplicate import

Co-authored-by: Jared Crawford <jmcrawford45@gmail.com>
Fixed a typo in a role name

Co-authored-by: Jared Crawford <jmcrawford45@gmail.com>
@odopertchouk
Copy link
Contributor Author

First of all, thank you for the work you've put into creating this new issuer plugin for the Google Certificate Authority Service. I've left a few minor comments in the code for things that could be improved. Before merging, we'd like to have some tests in place that cover the main functionality of your plugin, such as certificate creation, revocation, and fetching. Additionally, if you could add a changelog entry to make consumers aware of the new functionality, that'd be appreciated.

We appreciate your contribution and look forward to seeing the improvements and tests. If you need any help or clarification, feel free to ask.

I have added some tests, but can you check that the tests are consistent enough with the rest of the project? I have tried to follow examples from other plugins, but not everything carries over.

Similarly, I have added a changelog entry under 1.7.0 - 2024-01-17 but I am not sure if it's the right location.

@odopertchouk
Copy link
Contributor Author

@jmcrawford45 The build checks have been stuck all day, is there something wrong, or they just need more time?

@odopertchouk
Copy link
Contributor Author

@jmcrawford45 now the build fails with an error
⚠️ Coverage reporter does not yet know how to process this file: .coverage
Not sure how to proceed, as I have no experience with it

@jmcrawford45
Copy link
Contributor

@jmcrawford45 now the build fails with an error ⚠️ Coverage reporter does not yet know how to process this file: .coverage Not sure how to proceed, as I have no experience with it

This is a known upstream issue coverallsapp/github-action#205 I've temporarily disabled coveralls checks and will get your PR merged in today. Thanks again for your contribution and patience!

@jmcrawford45 jmcrawford45 merged commit 3336107 into Netflix:main Apr 22, 2024
13 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

2 participants