Add role arn #5619

Jseph · 2022-01-14T19:10:30Z

Adds the option to use role_arn for the transfer service resource. Fixes hashicorp/terraform-provider-google#10880.

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.

I did generate this and make sure it could build. make test and make lint didn't do anything because there was no rule for the make target.

Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).

Looks like S3 sources aren't supported in the third party tests. Probably because it would require infrastructure to create resources in AWS S3?

Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).

There aren't any relevant tests as far as I know.

Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

storage: added field `transfer_spec.aws_s3_data_source.role_arn` to `google_storage_transfer_job`

modular-magician · 2022-01-14T19:12:28Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@slevenick, please review this PR or find an appropriate assignee.

slevenick

Can you add a test that uses the new field?

slevenick · 2022-01-14T19:19:44Z

mmv1/third_party/terraform/resources/resource_storage_transfer_job.go

@@ -349,7 +349,7 @@ func awsS3DataSchema() *schema.Resource {
 			},
 			"aws_access_key": {
 				Type:     schema.TypeList,
-				Required: true,
+				Optional: true,


Now that we are moving this from Required -> Optional can we add an AtLeastOneOf with aws_access_key & role_arn so that one of them must be specified?

We try to avoid allowing empty blocks in configs as they tend to cause problems

+1 , either of those is required, won't work with neither

Does it work with both set? If not, we can use ExactlyOneOf instead

Technically the backend allows you to set both but

There's no reason to do so

aws_access_key is never returned by the service

If role_arn is set it takes priority

so I think it's fine to wrap this in ExactlyOneOf even though the underlying proto couldn't wrap these fields in a OneOf to maintain backwards compatibility.

modular-magician · 2022-01-14T19:24:15Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 33 insertions(+), 22 deletions(-))
Terraform Beta: Diff ( 2 files changed, 15 insertions(+), 4 deletions(-))

slevenick · 2022-01-14T19:24:54Z

Tests can be found in this file: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/tests/resource_storage_transfer_job_test.go

Jseph · 2022-01-14T19:42:12Z

Sorry I'm having trouble with figuring out what to do for testing. There aren't currently any tests that use AwsS3Data and I'm not really sure how to add one. Do these tests actually end up creating real transfer jobs or are they just testing the parsing/serialization logic?

Jseph · 2022-01-14T19:44:27Z

Specifically this code causes me concern

resource "google_storage_bucket_iam_member" "data_sink" {
  bucket = google_storage_bucket.data_sink.name
  role   = "roles/storage.admin"
  member = "serviceAccount:${data.google_storage_transfer_project_service_account.default.email}"
}

It looks like it's creating a real GCS bucket. I don't think there's any way to do that for an AWS S3 bucket so I don't know how to write a test for this.

slevenick · 2022-01-14T19:54:01Z

Specifically this code causes me concern
resource "google_storage_bucket_iam_member" "data_sink" {
  bucket = google_storage_bucket.data_sink.name
  role   = "roles/storage.admin"
  member = "serviceAccount:${data.google_storage_transfer_project_service_account.default.email}"
}
It looks like it's creating a real GCS bucket. I don't think there's any way to do that for an AWS S3 bucket so I don't know how to write a test for this.

These tests do create real resources when run. Can we set some arbitrary string as the role_arn, or does the API enforce that the AWS resource actually exists?

modular-magician · 2022-01-14T19:59:19Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 22 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 3 files changed, 44 insertions(+), 27 deletions(-))

Jseph · 2022-01-14T20:04:39Z

These tests do create real resources when run. Can we set some arbitrary string as the role_arn, or does the API enforce that the AWS resource actually exists?

Yes the API actually checks that the source bucket exists and that the provided credentials are sufficient to access it. If a fake string is sent we'll get back a 4xx error.

slevenick · 2022-01-14T20:06:36Z

These tests do create real resources when run. Can we set some arbitrary string as the role_arn, or does the API enforce that the AWS resource actually exists?

Yes the API actually checks that the source bucket exists and that the provided credentials are sufficient to access it. If a fake string is sent we'll get back a 4xx error.

Ok that's tricky then, we won't have valid AWS credentials in the CI runner. Can you set up the correct resources yourself and test that this field works correctly in a real environment?

Jseph · 2022-01-14T22:27:16Z

These tests do create real resources when run. Can we set some arbitrary string as the role_arn, or does the API enforce that the AWS resource actually exists?

Yes the API actually checks that the source bucket exists and that the provided credentials are sufficient to access it. If a fake string is sent we'll get back a 4xx error.

Ok that's tricky then, we won't have valid AWS credentials in the CI runner. Can you set up the correct resources yourself and test that this field works correctly in a real environment?

Ok I've got someone with more terraform experience to help out on Tuesday. I'll update with the results thanks!

lanre-OG · 2022-01-20T17:43:41Z

Carried out the below test with the compiled local provider:

Create an STS resource with the role_arn parameter only : Successful
Create an STS resource with the aws_access_key parameter only: Successful
Create an STS resource with both aws_access_key and role_arn parameter specified: Error

│ Error: Invalid combination of arguments
│ with google_storage_transfer_job.test-backup,
│ on main.tf line 92, in resource "google_storage_transfer_job" "test-backup":
│ 92: role_arn = var.role_arn
│ "transfer_spec.0.aws_s3_data_source.0.role_arn": only one of
│ transfer_spec.0.aws_s3_data_source.0.aws_access_key,transfer_spec.0.aws_s3_data_source.0.role_arn can be specified,
│ but transfer_spec.0.aws_s3_data_source.0.aws_access_key,transfer_spec.0.aws_s3_data_source.0.role_arn were specified.
Create an STS resource with neither aws_access_key and role_arn parameter specified: Error

│ Error: Invalid combination of arguments
│ with google_storage_transfer_job.test-backup,
│ on main.tf line 85, in resource "google_storage_transfer_job" "test-backup":
│ 85: aws_s3_data_source {
│ "transfer_spec.0.aws_s3_data_source.0.role_arn": one of
│ transfer_spec.0.aws_s3_data_source.0.aws_access_key,transfer_spec.0.aws_s3_data_source.0.role_arn must be specified

slevenick · 2022-01-20T19:38:01Z

/gcbrun

modular-magician · 2022-01-20T19:52:10Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 22 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 2 files changed, 22 insertions(+), 5 deletions(-))

modular-magician · 2022-01-20T21:42:49Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamMemberGenerated|TestAccCloudbuildWorkerPool_basic|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccServiceNetworkingPeeredDNSDomain_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=246233

slevenick

Looks good to me! Thanks for handling the testing

* Adds support for role_arn for storage_transfer_job * Fix syntax error * Fix build error. * Adds ExactlyOneOf check. Co-authored-by: Joseph Cox <[email protected]>

Joseph Cox added 3 commits January 12, 2022 13:37

Adds support for role_arn for storage_transfer_job

9166ea1

Fix syntax error

4e961e4

Fix build error.

21af2c5

modular-magician requested a review from slevenick January 14, 2022 19:12

slevenick requested changes Jan 14, 2022

View reviewed changes

Adds ExactlyOneOf check.

8056bb1

slevenick self-requested a review January 20, 2022 18:58

slevenick approved these changes Jan 21, 2022

View reviewed changes

slevenick merged commit 6f986f1 into GoogleCloudPlatform:master Jan 21, 2022

This was referenced Jan 21, 2022

Add role arn hashicorp/terraform-provider-google#10950

Merged

Add role arn hashicorp/terraform-provider-google-beta#3999

Merged

Jseph deleted the AddRoleArn branch January 21, 2022 19:07

binamov mentioned this pull request Jan 21, 2022

google_storage_transfer_job resource doesn't support ARN role hashicorp/terraform-provider-google#10844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add role arn #5619

Add role arn #5619

Jseph commented Jan 14, 2022 •

edited by slevenick

Loading

modular-magician commented Jan 14, 2022

slevenick left a comment

slevenick Jan 14, 2022

binamov Jan 14, 2022

slevenick Jan 14, 2022

Jseph Jan 14, 2022

modular-magician commented Jan 14, 2022

slevenick commented Jan 14, 2022

Jseph commented Jan 14, 2022

Jseph commented Jan 14, 2022

slevenick commented Jan 14, 2022

modular-magician commented Jan 14, 2022

Jseph commented Jan 14, 2022

slevenick commented Jan 14, 2022

Jseph commented Jan 14, 2022

lanre-OG commented Jan 20, 2022

slevenick commented Jan 20, 2022

modular-magician commented Jan 20, 2022

modular-magician commented Jan 20, 2022

slevenick left a comment

Add role arn #5619

Add role arn #5619

Conversation

Jseph commented Jan 14, 2022 • edited by slevenick Loading

modular-magician commented Jan 14, 2022

slevenick left a comment

Choose a reason for hiding this comment

slevenick Jan 14, 2022

Choose a reason for hiding this comment

binamov Jan 14, 2022

Choose a reason for hiding this comment

slevenick Jan 14, 2022

Choose a reason for hiding this comment

Jseph Jan 14, 2022

Choose a reason for hiding this comment

modular-magician commented Jan 14, 2022

Diff report:

slevenick commented Jan 14, 2022

Jseph commented Jan 14, 2022

Jseph commented Jan 14, 2022

slevenick commented Jan 14, 2022

modular-magician commented Jan 14, 2022

Diff report:

Jseph commented Jan 14, 2022

slevenick commented Jan 14, 2022

Jseph commented Jan 14, 2022

lanre-OG commented Jan 20, 2022

slevenick commented Jan 20, 2022

modular-magician commented Jan 20, 2022

Diff report:

modular-magician commented Jan 20, 2022

slevenick left a comment

Choose a reason for hiding this comment

Jseph commented Jan 14, 2022 •

edited by slevenick

Loading