Add support for regional instance groups (#84)

Fixes #73
hashicorp · Jul 22, 2022 · 9e4fddb · 9e4fddb
1 parent eee8ee8
commit 9e4fddb
Show file tree

Hide file tree

Showing 6 changed files with 148 additions and 44 deletions.
diff --git 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/go-secure-stdlib/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
@@ -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
@@ -9,8 +9,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)
 }
 
@@ -40,8 +40,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
 	}
@@ -67,7 +68,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)
 		}
@@ -81,7 +82,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:
@@ -111,6 +112,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",
@@ -120,7 +129,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
@@ -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
@@ -35,33 +35,40 @@ 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
 	}
 
 	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
@@ -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)
+			}
 		})
 	}
 }