-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for regional secret version resource google_secret_manager_regional_secret_version
#11699
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
Add support for regional secret version resource google_secret_manager_regional_secret_version
#11699
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @shuyama1, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
description: | | ||
Secret Manager regional secret resource. | ||
- !ruby/object:Api::Type::String | ||
name: location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a location parameter here doesn't make sense, we are already taking secret as a parameter.
Just like other params (secret_id, project) location should also be inferred from the secret param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base_url as seen in the Product.yaml contains {{location}}
parameter which is replaced using the ReplaceVars
function before making any API call. For this, we require the location field to be set in the terraform state. Hence, we have kept the location parameter as computed. We infer the location field using the pre_create and pre_read custom code and override the url to include the information about the location parameter in the base_url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a customer puts a value of location while creating secret version which is different from the one defined in secret?
We will override in pre_create and pre_read and the request goes to a different endpoint.
Such request with difference in location should ideally be rejected at the least. Can we introduce such a check if not done already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Customer won't be able to provide the location field as it is output only field from terraform side (i.e. Computed field). We will infer the location based on the regional secret resource reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah! missed the output = True bit.
Thanks for clarification.
decoder: templates/terraform/decoders/treat_destroyed_state_as_gone.erb | ||
pre_delete: templates/terraform/pre_delete/regional_secret_version_deletion_policy.go.erb | ||
pre_read: templates/terraform/pre_read/secret_manager_regional_secret_version.go.erb | ||
pre_create: templates/terraform/pre_create/secret_manager_regional_secret_version.go.erb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the pre-create needed?
It was not there in the SecretVersion: https://github.com/gptSanyam/magic-modules/blob/main/mmv1/products/secretmanager/SecretVersion.yaml#L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We infer the location field from the secret
field using the pre_create and pre_read custom code and override the url to include the information about the location parameter in the base_url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.
} | ||
d.SetId(name.(string)) | ||
|
||
_, err = expandSecretManagerRegionalRegionalSecretVersionEnabled(d.Get("enabled"), d, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this function defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is defined in the custom_expand custom code for the enabled field as we need to explicitly make the enable/disable call based on the value of the enabled
field.
versionRegex := regexp.MustCompile("projects/(.+)/locations/(.+)/secrets/(.+)/versions/(.+)$") | ||
|
||
parts := secretRegex.FindStringSubmatch(name) | ||
if len(parts) != 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the additional location check in most custom methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, we need the location field for the base_url of the regional APIs hence we do it for pre_create
, pre_read
and custom_import
functionalities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 34 Click here to see the affected service packages
Action takenFound 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a regional version of google_secret_manager_secret_version
. Overall looks good to me. I only have a small question regarding a test case. Thanks!
"github.com/hashicorp/terraform-provider-google/google/acctest" | ||
) | ||
|
||
func TestAccSecretManagerRegionalRegionalSecretVersion_import(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why do we need this test specifically, I don't see it's different from the first step of TestAccSecretManagerRegionalRegionalSecretVersion_update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included this test case to maintain consistency with the other resources and to test a basic import scenario. However, I understand your point about the redundancy, so I've decided to remove this test case.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 34 Click here to see the affected service packages
View the build log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
…er_regional_secret_version` (GoogleCloudPlatform#11699)
…er_regional_secret_version` (GoogleCloudPlatform#11699)
…er_regional_secret_version` (GoogleCloudPlatform#11699)
Add support for new regional secret version resource
google_secret_manager_regional_secret_version
.More info about regional secrets: https://cloud.google.com/secret-manager/docs/regional-secrets-overview
A point to be noted is that while importing the
google_secret_manager_regional_secret_version
resources having theis_secret_data_base64
field set to true, thesecret_data
is decoded from the base64-encoded string received from API. It is potentially due to condition in the flattener for payload (which is in place for the sake ofis_secret_data_base64
field to work). It seems the import is working fine when theis_secret_data_base64
is not provided or set to false. While doing a terraform import, there is no way to determine if the resource to be imported requires decoding of base64 response.References: hashicorp/terraform-provider-google#10129 & #8873
Release Note Template for Downstream PRs (will be copied)