Support API service agents in BootstrapPSARole() utils

[wyardley]

Some service agents are in the form
[email protected]

Support this via a hacky solution where if @ is present, the interpolation of service accounts will be more limited. Thus either:
acctest.BootstrapPSARole(t, projectNumber, "@cloudservices", "roles/foo.bar") (ab)using prefix, or
acctest.BootstrapPSARole(t, "", "NNNNNNNNNNN@cloudservices", "roles/foo.bar")

can be used for [email protected], for example.
https://cloud.google.com/iam/docs/service-account-types#service-agents

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

[github-actions]

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

@ScottSuarez, 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.

[wyardley]

#12728 has more context on the reasoning behind this; totally get that it's hacky, and happy to make adjustments if someone has another idea.

cc: @ScottSuarez @melinath

My thinking is that the history would be cleaner if this can be merged on its own, though we can close this if #12728 comes through first.

Side note: there are some other bits like:

magic-modules/mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.tmpl

Lines 9930 to 9942 in b5bdbf8

	resource "google_compute_subnetwork_iam_member" "service_network_cloud_services" {
	project = google_compute_shared_vpc_host_project.host_project.project
	subnetwork = google_compute_subnetwork.shared_subnetwork.name
	role = "roles/compute.networkUser"
	member = "serviceAccount:${google_project.service_project.number}@cloudservices.gserviceaccount.com"
	}
	resource "google_compute_subnetwork_iam_member" "service_network_gke_user" {
	project = google_compute_shared_vpc_host_project.host_project.project
	subnetwork = google_compute_subnetwork.shared_subnetwork.name
	role = "roles/compute.networkUser"
	member = "serviceAccount:service-${google_project.service_project.number}@container-engine-robot.iam.gserviceaccount.com"
	}

that might make some sense to move to the bootstrap pattern once this is supported.

[ScottSuarez]

I am okay with this pattern @melinath, any resistance or thoughts?

[modular-magician]

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 ( 1 file changed, 11 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 1 file changed, 11 insertions(+), 1 deletion(-))

[wyardley]

I guess an alternative would be a set of BootstrapGSA() wrapper functions similar to the PSA ones? Could still leave that big function more or less as is, but make the wrapper function interface simpler and the way of calling it less ugly. And we’d then be able to bake in the project number at that layer.

[melinath]

Overall this seems like a reasonable thing to support in the bootstrap helper functions. I might be inclined to make the helper functions take an object instead of individual arguments at this point; then we could say things like "if no domain is specified, use the default" instead of having to introspect whether there's an @-symbol in the address provided.

(There are also some broader things that would be nice to change like: Removing PSA terminology, since folks aren't familiar with it, and maybe reducing the number of helper functions to reduce what users need to know about - but those are likely beyond the scope of this PR.)

[wyardley]

Yeah, 100% - this is probably a little clunky, and not ideal interface-wise, and also agree as a user who's not super steeped in all the internal terminology, something more generic would probably be easier to work with.

An object could make some sense (and might make default behavior simpler?), and also just using an array all the time (for both the member(s) and role(s), ideally) vs. having both the array and string variants would be a bit tidier?

I'd be happy to file a separate GH issue if it would help in tracking making some of these improvements.

I'm guessing trying to change the interface too dramatically could get kind of messy, given the number of tests that would probably need adjustment? But I'm totally happy to make small adjustments or changes here if they can be done relatively simply.

[ScottSuarez]

In accordance with Stephen's suggestion. Wdyt about the following pattern? I can understand how it may get difficult to understand weird implicit logic. This change might make things more explicit overall at the consumer level without needing to know about special behavior for certain characters.

// ServiceAccountConfig holds the configuration for service account creation
type ServiceAccountConfig struct {
    // Name of the service account
    Name string
    // Optional domain override. If empty, defaults to project's iam.gserviceaccount.com
    Domain string
    // Optional project number override. If empty, uses the current project number
    ProjectNumber string
}

// BootstrapRoleConfig holds the configuration for bootstrapping IAM roles
type DesiredIAMRoles struct {
    // Prefix to be added to all service account emails
    Prefix string
    // List of service account configurations
    ServiceAccounts []ServiceAccountConfig
    // IAM roles to be assigned
    Roles []string
}

// BootstrapIAMRoles bootstraps IAM roles for service accounts
// Returns whether the bindings changed
func BootstrapIAMRoles(t *testing.T, config DesiredIAMRoles) bool {

[wyardley]

High level, this seems to be a much better design, albeit one I may not want to get too deep into implementing as an afternoon project 😅

Couple questions:

1. So basically, new function, with the idea that we'd create this method, and then phase out the wrapper functions later? And it would still wrap BootstrapAllPSARoles() for now, just providing a cleaner interface, but with the idea of doing more structural fixes later?
2. It seems like there's really two basic common patterns, so like ${prefix}-${projectNumber}@foo.iam.gserviceaccount.com (is this PSA?), where "prefix" is usually service- and then [project_number]@foo.gserviceaccount.com (Google Service Agent?). So I guess in this example, you'd have prefix as nil or empty string to do the latter, and set it to service- for the former?
   In fact, even in my current use case, could possibly hard-code the project number bit into the function, which would simplify my use case further.
3. If we're making naming it "IAM" vs. something specific to service accounts, maybe needs a param for whether to include the serviceAccount: prefix too? Not sure if that's too much YAGNI?

[ScottSuarez]

1. Exactly, or we can maintain some of the higher level wrappers if we feel they are applicable still. While allowing us to more flexibly use what's underneath.
2. hmm good point on {prefix}-${projectNumber}@foo.iam.gserviceaccount.com. We might want to omit project number altogether from the ServiceAccountConfig object. With the existing calling into our function. I believe you are correct on PSA (Project Service Account) being this specific pattern.
3. That sound reasonable to me! I would prefer it to be optional but customizability is always good.

[wyardley]

Ok, let me see if I can come up with something that uses that pattern, and if it's possible to do this while keeping BootstrapAllPSARoles() more or less as it is. It might delay #12728, which I've now rolled a fix for another breaking test into, so up to you if you want to ship that first or wait for this.

Ideally, there would be some tests as well, for the logic of constructing email addresses, at least, and maybe for some of the role related stuff... but that may also have to wait for some bigger project, especially since adding them with the way the functions are structured now might involve either adding some new functions, or figuring out how to stub some of the network calls.

[ScottSuarez]

I left a suggestion. If it's too complex of a change we can merge this as is and open a ticket!

I think the number of changes could be rather small since we usually use wrapper functions to call this, but feel free to take a look.

[modular-magician]

Tests analytics

Total tests: 4449
Passed tests: 4018
Skipped tests: 426
Affected tests: 5

Click here to see the affected service packages

All service packages are affected

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccApigeeEnvironmentAddonsConfig_apigeeEnvAddonsAnalyticsTestExample
• TestAccApigeeEnvironment_apigeeEnvironmentUpdateTest
• TestAccDataSourceGoogleGkeHubFeature_basic
• TestAccEphemeralServiceAccountKey_basic
• TestAccStorageTransferReplicationJob_basic

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccDataSourceGoogleGkeHubFeature_basic [Debug log]
TestAccStorageTransferReplicationJob_basic [Debug log]

🔴 Tests failed when rerunning REPLAYING mode:
TestAccDataSourceGoogleGkeHubFeature_basic [Error message] [Debug log]
TestAccStorageTransferReplicationJob_basic [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.

---

🔴 Tests failed during RECORDING mode:
TestAccApigeeEnvironmentAddonsConfig_apigeeEnvAddonsAnalyticsTestExample [Error message] [Debug log]
TestAccApigeeEnvironment_apigeeEnvironmentUpdateTest [Error message] [Debug log]
TestAccEphemeralServiceAccountKey_basic [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[melinath]

Actually, after thinking this over some more, I think we should make an even more basic change... one of the struggles with the current implementation is that from the tests, it's pretty hard to understand what is actually being granted to what. I think we should stop doing templating at all (except for project_number replacement) and just allow a passthrough of the members being bootstrapped.

I have some time & can take a stab at this today.

[wyardley]

@melinath Ok, thanks! I thought about this a little more last night, but haven't started doing anything... if you come up with something that's usable, that works great for me, and then I can adjust that other PR with the tests! I actually kind of like some of the magic here, because there really are only 2-3 patterns in the existing (and probably future?) use cases, but I get what you're saying as well.

Hopefully these changes will produce an outcome that will be easier to understand / read for everyone, as well as be more in line with the overall style of the project.

[melinath]

yeah, I think there's maybe room for additional helper functions to make it easier to build IAM member names, but that should be separate IMO from the bootstrapping logic.

[melinath]

opened #12796

[wyardley]

Closing since @melinath has opened #12796