Skip to content

Add support for regional secret resource google_secret_manager_regional_secret #11678

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

Conversation

abheda-crest
Copy link
Contributor

@abheda-crest abheda-crest commented Sep 11, 2024

Add support for new regional secret resource google_secret_manager_regional_secret. I have also defined the iam_policy field in the RegionalSecret.yaml file to include the IAM resources and IAM datasources as well.

More info about regional secrets: https://cloud.google.com/secret-manager/docs/regional-secrets-overview

Notes:

  1. In RegionalSecret.yaml file, the versionAliases field is commented and the respective test cases covered in the resource_secret_manager_regional_secret_test.go.erb file are also commented because this field depends on creating regional_secret_version_resource. We intend to uncomment once this PR is merged and dependent tests can pass.
  2. As of now, the API reference of the regional secrets are being documented on GCP docs side. Hence, the link mentioned in the ResourceSecret.yaml file will be updated later.

Release Note Template for Downstream PRs (will be copied)

`google_secret_manager_regional_secret`

Copy link

google-cla bot commented Sep 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Sep 11, 2024
@abheda-crest abheda-crest force-pushed the secret-manager-regional-secret-resource-support branch from aceb2d5 to f3ed8a0 Compare September 11, 2024 07:50
@abheda-crest abheda-crest force-pushed the secret-manager-regional-secret-resource-support branch from 3d69eba to cebddcc Compare September 11, 2024 08:07
@abheda-crest abheda-crest marked this pull request as ready for review September 11, 2024 11:04
@github-actions github-actions bot requested a review from hao-nan-li September 11, 2024 11:05
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@hao-nan-li, 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.

# limitations under the License.
-%>
// As the API expects only one of ttl or expireTime
if d.HasChange("ttl") && !d.HasChange("expire_time") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be an opposite condition here:
To remove ttl if expiry_time is introduced in the update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the API expects only one of the ttl or expireTime field, I've added this condition for switching between ttl and expireTime field. I've confirmed this behaviour and I've also added respective test case for the same. This test case can be found here: https://github.com/GoogleCloudPlatform/magic-modules/pull/11678/files#diff-11e793af43830f42f81e70d049956097a6df58b14f018cb62e7f1727f2bcd211R407

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a test case to prove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I've already added the test case that handles the switching between these fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

# See the License for the specific language governing permissions and
# limitations under the License.
-%>
// As the API expects only one of ttl or expireTime
Copy link
Contributor

Choose a reason for hiding this comment

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

Corresponding Secrets files seems to be doing a bit more than this:
https://github.com/gptSanyam/magic-modules/blob/main/mmv1/templates/terraform/pre_update/secret_manager_secret.go.erb

Let's make a list of things it is doing and then reason about what needs to be removed for regional secrets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
The corresponding file does two tasks:

  1. To enable the updation of replication field explicitly and updating the updateMask and ultimately the url to incorporate the changes in updateMask.
  2. To handle the behaviour of ttl and expireTime as mentioned in the comment (Add support for regional secret resource google_secret_manager_regional_secret #11678 (comment))

In regional secrets, there is no field for replication that requires explicit handling for updation. Hence, I need to only handle the ttl and expireTime scenario

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thanks.

@@ -616,6 +616,11 @@ var ServicesListBeta = mapOf(
"displayName" to "Secretmanager",
"path" to "./google-beta/services/secretmanager"
),
"secretmanagerregional" to mapOf(
"name" to "secretmanagerregional",
"displayName" to "Secretmanagerregional",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?
We are setting displayName = Secrets Manager in products.yaml, is this an override.
I think this is the name used in the terrafrom registry to list the products, we want both regional and non-regional resources to be listed under the same listing.
https://github.com/GoogleCloudPlatform/magic-modules/pull/11678/files#diff-c1cff9a2fbc2183ca2638f5cc05943ff90a42751de6dad07cece718f78bf9fe3R16

Copy link
Contributor

Choose a reason for hiding this comment

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

Verified, existing resources like IAM are also adding this.

@@ -611,6 +611,11 @@ var ServicesListGa = mapOf(
"displayName" to "Secretmanager",
"path" to "./google/services/secretmanager"
),
"secretmanagerregional" to mapOf(
"name" to "secretmanagerregional",
"displayName" to "Secretmanagerregional",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as beta.

Copy link
Contributor Author

@abheda-crest abheda-crest Sep 11, 2024

Choose a reason for hiding this comment

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

The Action for teamcity was failing regarding this. Before I've added this snippet, the workflow was failing: https://github.com/GoogleCloudPlatform/magic-modules/actions/runs/10806885008/job/29976554704. I am not sure if we need to add this snippet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Verified, existing resources like IAM are also adding this.

Copy link
Contributor

@gptSanyam gptSanyam left a comment

Choose a reason for hiding this comment

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

LGTM

@modular-magician modular-magician added service/secretmanager and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Sep 11, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 19 files changed, 4573 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 17 files changed, 4563 insertions(+), 3 deletions(-))
terraform-google-conversion: Diff ( 4 files changed, 603 insertions(+))
Open in Cloud Shell: Diff ( 24 files changed, 715 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4018
Passed tests: 3582
Skipped tests: 411
Affected tests: 25

Click here to see the affected service packages

All service packages are affected

#### Non-exercised tests

Tests were added that are skipped in VCR:

  • TestAccSecretManagerRegionalRegionalSecretIamBindingGenerated_withAndWithoutCondition
  • TestAccSecretManagerRegionalRegionalSecretIamMemberGenerated_withAndWithoutCondition

Action taken

Found 25 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccComputeInstance_confidentialHyperDiskBootDisk
  • TestAccSecretManagerRegionalRegionalSecretIamBindingGenerated
  • TestAccSecretManagerRegionalRegionalSecretIamBindingGenerated_withCondition
  • TestAccSecretManagerRegionalRegionalSecretIamMemberGenerated
  • TestAccSecretManagerRegionalRegionalSecretIamMemberGenerated_withCondition
  • TestAccSecretManagerRegionalRegionalSecretIamPolicyGenerated
  • TestAccSecretManagerRegionalRegionalSecretIamPolicyGenerated_withCondition
  • TestAccSecretManagerRegionalRegionalSecretIam_iamBindingUpdate
  • TestAccSecretManagerRegionalRegionalSecretIam_iamPolicyUpdate
  • TestAccSecretManagerRegionalRegionalSecret_annotationsUpdate
  • TestAccSecretManagerRegionalRegionalSecret_cmekUpdate
  • TestAccSecretManagerRegionalRegionalSecret_expireTimeUpdate
  • TestAccSecretManagerRegionalRegionalSecret_import
  • TestAccSecretManagerRegionalRegionalSecret_labelsUpdate
  • TestAccSecretManagerRegionalRegionalSecret_regionalSecretConfigBasicExample
  • TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithCmekExample
  • TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithExpireTimeExample
  • TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithRotationExample
  • TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithTtlExample
  • TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithVersionDestroyTtlExample
  • TestAccSecretManagerRegionalRegionalSecret_rotationInfoUpdate
  • TestAccSecretManagerRegionalRegionalSecret_topicsUpdate
  • TestAccSecretManagerRegionalRegionalSecret_ttlUpdate
  • TestAccSecretManagerRegionalRegionalSecret_updateBetweenTtlAndExpireTime
  • TestAccSecretManagerRegionalRegionalSecret_versionDestroyTtlUpdate

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeInstance_confidentialHyperDiskBootDisk[Debug log]
TestAccSecretManagerRegionalRegionalSecretIamBindingGenerated[Debug log]
TestAccSecretManagerRegionalRegionalSecretIamBindingGenerated_withCondition[Debug log]
TestAccSecretManagerRegionalRegionalSecretIamMemberGenerated[Debug log]
TestAccSecretManagerRegionalRegionalSecretIamMemberGenerated_withCondition[Debug log]
TestAccSecretManagerRegionalRegionalSecretIamPolicyGenerated[Debug log]
TestAccSecretManagerRegionalRegionalSecretIamPolicyGenerated_withCondition[Debug log]
TestAccSecretManagerRegionalRegionalSecretIam_iamBindingUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecretIam_iamPolicyUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_annotationsUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_cmekUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_expireTimeUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_import[Debug log]
TestAccSecretManagerRegionalRegionalSecret_labelsUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_regionalSecretConfigBasicExample[Debug log]
TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithCmekExample[Debug log]
TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithExpireTimeExample[Debug log]
TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithRotationExample[Debug log]
TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithTtlExample[Debug log]
TestAccSecretManagerRegionalRegionalSecret_regionalSecretWithVersionDestroyTtlExample[Debug log]
TestAccSecretManagerRegionalRegionalSecret_rotationInfoUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_topicsUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_ttlUpdate[Debug log]
TestAccSecretManagerRegionalRegionalSecret_updateBetweenTtlAndExpireTime[Debug log]
TestAccSecretManagerRegionalRegionalSecret_versionDestroyTtlUpdate[Debug log]
$\textcolor{red}{\textsf{Tests failed when rerunning REPLAYING mode:}}$
TestAccComputeInstance_confidentialHyperDiskBootDisk[Error message] [Debug log]

Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made.

Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

@abheda-crest
Copy link
Contributor Author

I think that TestAccComputeInstance_confidentialHyperDiskBootDisk has been fixed very recently as a part of this PR: #11615

@hao-nan-li
Copy link
Contributor

I think that TestAccComputeInstance_confidentialHyperDiskBootDisk has been fixed very recently as a part of this PR: #11615

I don't think that test is affected by your change.

Copy link
Contributor

@hao-nan-li hao-nan-li left a comment

Choose a reason for hiding this comment

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

Thanks for adding the new resource in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants