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.

Sign up for GitHub

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

Jump to bottom

feat: CCP secret manager lookup(https://github.com/ansible-collections/google.cloud/pull/357) #628

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat: CCP secret manager lookup(https://github.com/ansible-collections/google.cloud/pull/357) #628

wants to merge 6 commits into from

Conversation

gkorolev
Copy link

@gkorolev gkorolev commented May 8, 2024

SUMMARY

GCP secret manager lookup by @levonet
Trying to push enhancement done by @levonet and address minor fixes, requested by @toumorokoshi.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

GCP secret manager lookup plugin

@gkorolev
Copy link
Author

gkorolev commented May 9, 2024

@toumorokoshi can you pls review and approve if OK

@toumorokoshi
Copy link
Collaborator

Sorry, I no longer am employed by Google and therefore cannot merge this PR.

I guess if @SirGitsalot gives me approval I'm happy to merge some PRs - although I think releases will still have to be maintained by Google.

@toumorokoshi toumorokoshi requested a review from SirGitsalot May 10, 2024 05:03
@gkorolev
Copy link
Author

Hello, Guys
@toumorokoshi thank you for your reply. @SirGitsalot is there any way you can review the PR and merge if OK?

@SirGitsalot
Copy link
Collaborator

Looks good - there's a few linter nits that shouldn't affect functionality that need to be fixed though:

ERROR: Found 2 no-smart-quotes issue(s) which need to be resolved:
ERROR: plugins/lookup/gcp_secret_access.py:51:9: use ASCII quotes `'` and `"` instead of Unicode quotes
ERROR: plugins/lookup/gcp_secret_resource_id.py:50:9: use ASCII quotes `'` and `"` instead of Unicode quotes

ERROR: Found 4 pep8 issue(s) which need to be resolved:
ERROR: plugins/lookup/gcp_secret_access.py:19:161: E501: line too long (167 > 160 characters)
ERROR: plugins/lookup/gcp_secret_access.py:51:161: E501: line too long (224 > 160 characters)
ERROR: plugins/lookup/gcp_secret_resource_id.py:50:161: E501: line too long (224 > 160 characters)
ERROR: plugins/plugin_utils/gcp_utils.py:43:24: E225: missing whitespace around operator

ERROR: Found 4 pylint issue(s) which need to be resolved:
ERROR: plugins/plugin_utils/gcp_utils.py:49:35: ansible-format-automatic-specification: Format string contains automatic field numbering specification
ERROR: plugins/plugin_utils/gcp_utils.py:55:39: ansible-format-automatic-specification: Format string contains automatic field numbering specification
ERROR: plugins/plugin_utils/gcp_utils.py:70:20: ansible-format-automatic-specification: Format string contains automatic field numbering specification
ERROR: plugins/plugin_utils/gcp_utils.py:105:20: ansible-format-automatic-specification: Format string contains automatic field numbering specification

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/lookup/gcp_secret_access.py
- name: GCP_SERVICE_ACCOUNT_FILE
notes:
- When I(secret) is the first option in the term string, C(secret=) is not required (see examples).
- If you’re running your application elsewhere, you should download a service account JSON keyfile and point to it using the secret option or an environment variable C(GOOGLE_APPLICATION_CREDENTIALS="/path/to/keyfile.json").
Copy link

@jack-rep jack-rep Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- If you’re running your application elsewhere, you should download a service account JSON keyfile and point to it using the secret option or an environment variable C(GOOGLE_APPLICATION_CREDENTIALS="/path/to/keyfile.json").
- If you're running your application elsewhere, you should download a service account JSON keyfile and
- point to it using the secret option or an environment variable C(GOOGLE_APPLICATION_CREDENTIALS="/path/to/keyfile.json").

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/lookup/gcp_secret_resource_id.py
- name: GCP_SERVICE_ACCOUNT_FILE
notes:
- When I(secret) is the first option in the term string, C(secret=) is not required (see examples).
- If you’re running your application elsewhere, you should download a service account JSON keyfile and point to it using the secret option or an environment variable C(GOOGLE_APPLICATION_CREDENTIALS="/path/to/keyfile.json").
Copy link

@jack-rep jack-rep Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- If you’re running your application elsewhere, you should download a service account JSON keyfile and point to it using the secret option or an environment variable C(GOOGLE_APPLICATION_CREDENTIALS="/path/to/keyfile.json").
- If you're running your application elsewhere, you should download a service account JSON keyfile and
- point to it using the secret option or an environment variable C(GOOGLE_APPLICATION_CREDENTIALS="/path/to/keyfile.json").

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/lookup/gcp_secret_access.py
- google-cloud-secret-manager >= 1.0.0
description:
- Retrieve secret contents from GCP Secret Manager.
- Accessing to secret content requires the Secret Manager Secret Accessor role (C(roles/secretmanager.secretAccessor)) on the secret, project, folder, or organization.
Copy link

Choose a reason for hiding this comment

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

Suggested change
- Accessing to secret content requires the Secret Manager Secret Accessor role (C(roles/secretmanager.secretAccessor)) on the secret, project, folder, or organization.
- Accessing to secret content requires:
- the Secret Manager Secret Accessor role (C(roles/secretmanager.secretAccessor)) on the secret, project, folder, or organization.

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/plugin_utils/gcp_utils.py

def client(self, secretmanager):
if self.access_token is not None:
credentials=google.oauth2.credentials.Credentials(self.access_token, scopes=self.scope)
Copy link

Choose a reason for hiding this comment

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

Suggested change
credentials=google.oauth2.credentials.Credentials(self.access_token, scopes=self.scope)
credentials = google.oauth2.credentials.Credentials(self.access_token, scopes=self.scope)

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/plugin_utils/gcp_utils.py
if self.service_account_file is not None:
path = os.path.realpath(os.path.expanduser(self.service_account_file))
if not os.path.exists(path):
raise AnsibleError("File {} was not found.".format(path))
Copy link

Choose a reason for hiding this comment

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

Suggested change
raise AnsibleError("File {} was not found.".format(path))
raise AnsibleError("File {0} was not found.".format(path))

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/plugin_utils/gcp_utils.py
try:
info = json.load(file_obj)
except ValueError as e:
raise AnsibleError("File {} is not a valid json file.".format(path))
Copy link

Choose a reason for hiding this comment

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

Suggested change
raise AnsibleError("File {} is not a valid json file.".format(path))
raise AnsibleError("File {0} is not a valid json file.".format(path))

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/plugin_utils/gcp_utils.py
credentials = identity_pool.Credentials.from_info(info, scopes=self.scope)
else:
raise AnsibleError(
"Type is {}, expected one of authorized_user, service_account, external_account.".format(credential_type)
Copy link

Choose a reason for hiding this comment

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

Suggested change
"Type is {}, expected one of authorized_user, service_account, external_account.".format(credential_type)
"Type is {0}, expected one of authorized_user, service_account, external_account.".format(credential_type)

jack-rep
jack-rep reviewed Jun 4, 2024
View reviewed changes
plugins/plugin_utils/gcp_utils.py
if self.secret_id is None:
raise AnsibleError("{0} lookup plugin required option: secret or resource id".format(self.plugin_name))

self.name = "projects/{}/secrets/{}/versions/{}".format(self.project_id, self.secret_id, self.version_id)
Copy link

Choose a reason for hiding this comment

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

Suggested change
self.name = "projects/{}/secrets/{}/versions/{}".format(self.project_id, self.secret_id, self.version_id)
self.name = "projects/{0}/secrets/{1}/versions/{2}".format(self.project_id, self.secret_id, self.version_id)

@jack-rep
Copy link

jack-rep commented Jun 4, 2024

Looks good - there's a few linter nits that shouldn't affect functionality that need to be fixed though:

@SirGitsalot I made suggestions to fix all the lint errors, can you please have another look and merge?

@jack-rep
Copy link

jack-rep commented Jun 5, 2024

@gkorolev or maybe you can merge my above suggestions if that's ok?

@jack-rep
Copy link

jack-rep commented Jun 7, 2024

is this a dup of #578?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jack-rep jack-rep jack-rep left review comments

@levonet levonet levonet approved these changes

@SirGitsalot SirGitsalot Awaiting requested review from SirGitsalot

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gkorolev @toumorokoshi @SirGitsalot @jack-rep @levonet