From 04687b61e2bb1be7bc542fbf2e5c024e578f5ee8 Mon Sep 17 00:00:00 2001 From: Danek Duvall Date: Wed, 11 Mar 2020 00:22:36 +0000 Subject: [PATCH] Add support for regional instance groups Fixes #73 --- plugin/authorizer_client_gcp.go | 56 +++++++++++++++++++++-------- plugin/authorizer_client_stubbed.go | 7 ++-- plugin/authorizer_gce.go | 23 ++++++++---- plugin/authorizer_gce_test.go | 48 +++++++++++++++++++++++++ plugin/helpers.go | 33 ++++++++++------- plugin/helpers_test.go | 25 +++++++++---- 6 files changed, 148 insertions(+), 44 deletions(-) diff --git a/plugin/authorizer_client_gcp.go b/plugin/authorizer_client_gcp.go index 482eb8fe..8c7ea1fc 100644 --- a/plugin/authorizer_client_gcp.go +++ b/plugin/authorizer_client_gcp.go @@ -3,7 +3,6 @@ package gcpauth import ( "context" "fmt" - "strings" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/sdk/helper/strutil" @@ -22,42 +21,52 @@ type gcpClient struct { iamSvc *iam.Service } -func (c *gcpClient) InstanceGroups(ctx context.Context, project string, boundInstanceGroups []string) (map[string][]string, error) { - // map of zone names to a slice of instance group names in that zone. +func (c *gcpClient) InstanceGroups(ctx context.Context, project string, boundInstanceGroups []string) (map[string][]string, map[string][]string, error) { + // maps of zone/region names to a slice of instance group names in that + // location. igz := make(map[string][]string) + igr := make(map[string][]string) + // AggregatedList, unlike all the other InstanceGroupsService methods, + // returns both zonal and regional instance groups. if err := c.computeSvc.InstanceGroups. AggregatedList(project). Fields("items/*/instanceGroups/name"). Pages(ctx, func(l *compute.InstanceGroupAggregatedList) error { for k, v := range l.Items { - // Some groups returned are regional - // TODO(emilymye, #73): Support regions? - if strings.Contains(k, "/regions/") { - c.logger.Debug("ignoring instance groups under region in instance group aggregated list", "key", k) - continue - } - - zone, err := zoneFromSelfLink(k) + zone, region, err := zoneOrRegionFromSelfLink(k) if err != nil { return err } for _, g := range v.InstanceGroups { if strutil.StrListContains(boundInstanceGroups, g.Name) { - igz[zone] = append(igz[zone], g.Name) + if zone != "" { + igz[zone] = append(igz[zone], g.Name) + } + if region != "" { + igr[region] = append(igr[region], g.Name) + } } } } return nil }); err != nil { - return nil, err + return nil, nil, err } - return igz, nil + return igz, igr, nil +} + +func (c *gcpClient) InstanceGroupContainsInstance(ctx context.Context, project, zone, region, group, instanceSelfLink string) (bool, error) { + if zone != "" { + return c.zoneInstanceGroupContainsInstance(ctx, project, zone, group, instanceSelfLink) + } else { + return c.regionInstanceGroupContainsInstance(ctx, project, region, group, instanceSelfLink) + } } -func (c *gcpClient) InstanceGroupContainsInstance(ctx context.Context, project, zone, group, instanceSelfLink string) (bool, error) { +func (c *gcpClient) zoneInstanceGroupContainsInstance(ctx context.Context, project, zone, group, instanceSelfLink string) (bool, error) { var req compute.InstanceGroupsListInstancesRequest resp, err := c.computeSvc.InstanceGroups. ListInstances(project, zone, group, &req). @@ -74,6 +83,23 @@ func (c *gcpClient) InstanceGroupContainsInstance(ctx context.Context, project, return false, nil } +func (c *gcpClient) regionInstanceGroupContainsInstance(ctx context.Context, project, region, group, instanceSelfLink string) (bool, error) { + var req compute.RegionInstanceGroupsListInstancesRequest + resp, err := c.computeSvc.RegionInstanceGroups. + ListInstances(project, region, group, &req). + Filter(fmt.Sprintf("instance eq %s", instanceSelfLink)). + Context(ctx). + Do() + if err != nil { + return false, err + } + + if resp != nil && len(resp.Items) > 0 { + return true, nil + } + return false, nil +} + func (c *gcpClient) ServiceAccount(ctx context.Context, name string) (string, string, error) { account, err := c.iamSvc.Projects.ServiceAccounts. Get(name). diff --git a/plugin/authorizer_client_stubbed.go b/plugin/authorizer_client_stubbed.go index 8b0a69f4..33c11fae 100644 --- a/plugin/authorizer_client_stubbed.go +++ b/plugin/authorizer_client_stubbed.go @@ -8,15 +8,16 @@ var _ client = (*stubbedClient)(nil) // GCP are "stubbed" instead of hitting the actual API. type stubbedClient struct { instanceGroupsByZone map[string][]string + instanceGroupsByRegion map[string][]string instanceGroupContainsInstance bool saId, saEmail string } -func (c *stubbedClient) InstanceGroups(_ context.Context, _ string, _ []string) (map[string][]string, error) { - return c.instanceGroupsByZone, nil +func (c *stubbedClient) InstanceGroups(_ context.Context, _ string, _ []string) (map[string][]string, map[string][]string, error) { + return c.instanceGroupsByZone, c.instanceGroupsByRegion, nil } -func (c *stubbedClient) InstanceGroupContainsInstance(_ context.Context, _, _, _, _ string) (bool, error) { +func (c *stubbedClient) InstanceGroupContainsInstance(_ context.Context, _, _, _, _, _ string) (bool, error) { return c.instanceGroupContainsInstance, nil } diff --git a/plugin/authorizer_gce.go b/plugin/authorizer_gce.go index 6e32f6b9..fa4b9537 100644 --- a/plugin/authorizer_gce.go +++ b/plugin/authorizer_gce.go @@ -10,8 +10,8 @@ import ( ) type client interface { - InstanceGroups(context.Context, string, []string) (map[string][]string, error) - InstanceGroupContainsInstance(context.Context, string, string, string, string) (bool, error) + InstanceGroups(context.Context, string, []string) (map[string][]string, map[string][]string, error) + InstanceGroupContainsInstance(context.Context, string, string, string, string, string) (bool, error) ServiceAccount(context.Context, string) (string, string, error) } @@ -41,8 +41,9 @@ func AuthorizeGCE(ctx context.Context, i *AuthorizeGCEInput) error { } } - // Parse the zone name from the self-link URI if given. - zone, err := zoneFromSelfLink(i.instanceZone) + // Parse the zone name from the self-link URI if given; compute + // instances are always zonal. + zone, _, err := zoneOrRegionFromSelfLink(i.instanceZone) if err != nil { return err } @@ -68,7 +69,7 @@ func AuthorizeGCE(ctx context.Context, i *AuthorizeGCEInput) error { // For each bound instance group, verify the group exists and that the // instance is a member of that group. if len(i.boundInstanceGroups) > 0 { - igz, err := i.client.InstanceGroups(ctx, i.project, i.boundInstanceGroups) + igz, igr, err := i.client.InstanceGroups(ctx, i.project, i.boundInstanceGroups) if err != nil { return fmt.Errorf("failed to list instance groups for project %q: %s", i.project, err) } @@ -82,7 +83,7 @@ func AuthorizeGCE(ctx context.Context, i *AuthorizeGCEInput) error { break } - var group, zone string + var group, zone, region string switch { case len(i.boundZones) > 0: @@ -112,6 +113,14 @@ func AuthorizeGCE(ctx context.Context, i *AuthorizeGCEInput) error { } } } + for r, groups := range igr { + for _, grp := range groups { + if grp == g { + group = g + region = r + } + } + } } if group == "" { return fmt.Errorf("instance group %q does not exist in regions %q for project %q", @@ -121,7 +130,7 @@ func AuthorizeGCE(ctx context.Context, i *AuthorizeGCEInput) error { return fmt.Errorf("instance group %q is not bound to any zones or regions", g) } - ok, err := i.client.InstanceGroupContainsInstance(ctx, i.project, zone, group, i.instanceSelfLink) + ok, err := i.client.InstanceGroupContainsInstance(ctx, i.project, zone, region, group, i.instanceSelfLink) if err != nil { return fmt.Errorf("failed to list instances in instance group %q for project %q: %s", group, i.project, err) diff --git a/plugin/authorizer_gce_test.go b/plugin/authorizer_gce_test.go index 2e0cc36a..446d4379 100644 --- a/plugin/authorizer_gce_test.go +++ b/plugin/authorizer_gce_test.go @@ -240,6 +240,38 @@ func TestAuthorizeGCE(t *testing.T) { true, `instance is not part of instance groups ["my-instance-group"]`, }, + { + "bound_regional_instance_groups_no_exist_bound_regions", + &AuthorizeGCEInput{ + client: &stubbedClient{ + instanceGroupsByRegion: map[string][]string{ + "us-east1": []string{"other-instance-group"}, + }, + instanceGroupContainsInstance: true, + }, + instanceZone: "us-east1-a", + boundInstanceGroups: []string{"my-instance-group"}, + boundRegions: []string{"us-east1"}, + }, + true, + `instance group "my-instance-group" does not exist in regions ["us-east1"]`, + }, + { + "bound_regional_instance_groups_no_contains_instance", + &AuthorizeGCEInput{ + client: &stubbedClient{ + instanceGroupsByRegion: map[string][]string{ + "us-east1": []string{"my-instance-group"}, + }, + instanceGroupContainsInstance: false, + }, + instanceZone: "us-east1-a", + boundInstanceGroups: []string{"my-instance-group"}, + boundRegions: []string{"us-east1"}, + }, + true, + `instance is not part of instance groups ["my-instance-group"]`, + }, // service account { @@ -338,6 +370,22 @@ func TestAuthorizeGCE(t *testing.T) { false, "", }, + { + "success_regional_instance_group_region_binding", + &AuthorizeGCEInput{ + client: &stubbedClient{ + instanceGroupsByRegion: map[string][]string{ + "us-east1": []string{"my-instance-group"}, + }, + instanceGroupContainsInstance: true, + }, + instanceZone: "us-east1-a", + boundInstanceGroups: []string{"my-instance-group"}, + boundRegions: []string{"us-east1"}, + }, + false, + "", + }, } for _, tc := range cases { diff --git a/plugin/helpers.go b/plugin/helpers.go index a8b845d4..a067a147 100644 --- a/plugin/helpers.go +++ b/plugin/helpers.go @@ -35,8 +35,8 @@ func validateFields(req *logical.Request, data *framework.FieldData) error { // // If the zone is a self-link, it is converted into a human name first. If the // zone cannot be converted to a region, an error is returned. -func zoneToRegion(zone string) (string, error) { - zone, err := zoneFromSelfLink(zone) +func zoneToRegion(input string) (string, error) { + zone, _, err := zoneOrRegionFromSelfLink(input) if err != nil { return "", err } @@ -44,24 +44,31 @@ func zoneToRegion(zone string) (string, error) { if i := strings.LastIndex(zone, "-"); i > -1 { return zone[0:i], nil } - return "", fmt.Errorf("failed to extract region from zone name %q", zone) + return "", fmt.Errorf("failed to extract region from zone name %q", input) } -// zoneFromSelfLink converts a zone self-link into the human zone name. -func zoneFromSelfLink(zone string) (string, error) { - prefix := "zones/" +// zoneOrRegionFromSelfLink converts a zone or region self-link into the human +// zone or region names. +func zoneOrRegionFromSelfLink(selfLink string) (string, string, error) { + zPrefix := "zones/" + rPrefix := "regions/" + var zone, region string - if zone == "" { - return "", fmt.Errorf("failed to extract zone from self-link %q", zone) + if selfLink == "" { + return "", "", fmt.Errorf("failed to extract zone or region from self-link %q", selfLink) } - if strings.Contains(zone, "/") { - if i := strings.LastIndex(zone, prefix); i > -1 { - zone = zone[i+len(prefix) : len(zone)] + if strings.Contains(selfLink, "/") { + if i := strings.LastIndex(selfLink, zPrefix); i > -1 { + zone = selfLink[i+len(zPrefix) : len(selfLink)] + } else if i := strings.LastIndex(selfLink, rPrefix); i > -1 { + region = selfLink[i+len(rPrefix) : len(selfLink)] } else { - return "", fmt.Errorf("failed to extract zone from self-link %q", zone) + return "", "", fmt.Errorf("failed to extract zone or region from self-link %q", selfLink) } + } else { + return selfLink, "", nil } - return zone, nil + return zone, region, nil } diff --git a/plugin/helpers_test.go b/plugin/helpers_test.go index e2a9e1f6..b795551e 100644 --- a/plugin/helpers_test.go +++ b/plugin/helpers_test.go @@ -62,25 +62,35 @@ func TestZoneToRegion(t *testing.T) { } } -func TestZoneFromSelfLink(t *testing.T) { +func TestZoneOrRegionFromSelfLink(t *testing.T) { t.Parallel() cases := []struct { - link string - zone string - err bool + link string + zone string + region string + err bool }{ { "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-east1-d", "us-east1-d", + "", false, }, { "https://www.googleapis.com/compute/v1/projects/my-project/regions/us-east1", "", + "us-east1", + false, + }, + { + "https://www.googleapis.com/compute/v1/projects/my-project/badbadbad/us-east1", + "", + "", true, }, { + "", "", "", true, @@ -94,13 +104,16 @@ func TestZoneFromSelfLink(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - res, err := zoneFromSelfLink(tc.link) + z, r, err := zoneOrRegionFromSelfLink(tc.link) if (err != nil) != tc.err { t.Fatal(err) } - if res != tc.zone { + if z != tc.zone { t.Errorf("expected %q to convert to %q", tc.link, tc.zone) } + if r != tc.region { + t.Errorf("expected %q to convert to %q", tc.link, tc.region) + } }) } }