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

Support API service agents in BootstrapPSARole() utils #12784

Closed
wants to merge 1 commit into from
Closed

Support API service agents in BootstrapPSARole() utils #12784

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion mmv1/third_party/terraform/acctest/bootstrap_iam_test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package acctest
import (
"fmt"
"log"
"strings"
"testing"

"github.com/hashicorp/terraform-provider-google/google/envvar"
Expand All @@ -14,6 +15,9 @@ import (
// policy grants the given service agents the given roles.
// prefix is usually "service-" and indicates the service agent should have the
// given prefix before the project number.
// If an address with an `@` (x@y) is passed, the address will be used
// verbatim, but still prefixed with `serviceAccount:prefix` and suffixed with
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

  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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// `gserviceaccount.com`
// This is important to bootstrap because using iam policy resources means that
// deleting them removes permissions for concurrent tests.
// Return whether the bindings changed.
Expand All @@ -38,7 +42,13 @@ func BootstrapAllPSARoles(t *testing.T, prefix string, agentNames, roles []strin

members := make([]string, len(agentNames))
for i, agentName := range agentNames {
members[i] = fmt.Sprintf("serviceAccount:%s%d@%s.iam.gserviceaccount.com", prefix, project.ProjectNumber, agentName)

// Allow partially bypassing the builtin logic of making up the email if an @ is included
if strings.Contains(agentName, "@") {
members[i] = fmt.Sprintf("serviceAccount:%s%s.gserviceaccount.com", prefix, agentName)
} else {
members[i] = fmt.Sprintf("serviceAccount:%s%d@%s.iam.gserviceaccount.com", prefix, project.ProjectNumber, agentName)
}
}

// Create the bindings we need to add to the policy.
Expand Down
Loading