PR feedback and typo fix for AWS permissions.

PR Feedback for Instance Profile work and fixes typo in AWS permission.
sdg0919 · Nov 26, 2021 · 1a40220 · 1a40220
1 parent dd1e87c
commit 1a40220
Show file tree

Hide file tree

Showing 9 changed files with 115 additions and 65 deletions.
diff --git a/environs/bootstrap/bootstrap.go b/environs/bootstrap/bootstrap.go
@@ -564,6 +564,18 @@ func bootstrapIAAS(
 		return errors.Trace(err)
 	}
 
+	if bootstrapParams.BootstrapConstraints.HasInstanceRole() {
+		instanceRoleEnviron, ok := environ.(environs.InstanceRole)
+		if !ok || !instanceRoleEnviron.SupportsInstanceRoles(callCtx) {
+			return errors.NewNotSupported(nil, "instance role constraint for provider")
+		}
+
+		bootstrapParams, err = finaliseInstanceRole(callCtx, instanceRoleEnviron, bootstrapParams)
+		if err != nil {
+			return errors.Annotate(err, "finalising instance role for provider")
+		}
+	}
+
 	ctx.Verbosef("Starting new instance for initial controller")
 
 	result, err := environ.Bootstrap(ctx, callCtx, bootstrapParams)
@@ -648,6 +660,20 @@ func bootstrapIAAS(
 	return nil
 }
 
+func finaliseInstanceRole(
+	ctx context.ProviderCallContext,
+	ir environs.InstanceRole,
+	args environs.BootstrapParams,
+) (environs.BootstrapParams, error) {
+	if *args.BootstrapConstraints.InstanceRole !=
+		environs.InstanceProfileAutoCreate {
+		return args, nil
+	}
+	irName, err := ir.CreateAutoInstanceRole(ctx, args)
+	args.BootstrapConstraints.InstanceRole = &irName
+	return args, err
+}
+
 // Bootstrap bootstraps the given environment. The supplied constraints are
 // used to provision the instance, and are also set within the bootstrapped
 // environment.

diff --git a/environs/instancerole.go b/environs/instancerole.go
@@ -0,0 +1,26 @@
+// Copyright 2021 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package environs
+
+import (
+	"github.com/juju/juju/environs/context"
+)
+
+const (
+	// InstanceProfileAutoCreate defines the const value used for the constraint
+	// when instance profile creation should be done on behalf of the user.
+	InstanceProfileAutoCreate = "auto"
+)
+
+// InstanceRole defines the interface for environ providers to implement when
+// they offer InstanceRole support for their respective cloud.
+type InstanceRole interface {
+	// CreateAutoInstanceRole is responsible for setting up an instance role on
+	// behalf of the user.
+	CreateAutoInstanceRole(context.ProviderCallContext, BootstrapParams) (string, error)
+
+	// SupportsInstanceRoles indicates if Instance Roles are supported by this
+	// environ.
+	SupportsInstanceRoles(context.ProviderCallContext) bool
+}
diff --git a/provider/ec2/config.go b/provider/ec2/config.go
@@ -12,13 +12,6 @@ import (
 	"github.com/juju/juju/environs/config"
 )
 
-const (
-	// awsInstanceProfileAutoCreateVal is the key used for the instance profile
-	// constraint to tell Juju to auto create an instance profile in AWS for the
-	// machine. This is only used for bootstrapped controllers.
-	awsInstanceProfileAutoCreateVal = "auto"
-)
-
 var configSchema = environschema.Fields{
 	"vpc-id": {
 		Description: "Use a specific AWS VPC ID (optional). When not specified, Juju requires a default VPC or EC2-Classic features to be available for the account/region.",

diff --git a/provider/ec2/environ.go b/provider/ec2/environ.go
@@ -194,15 +194,6 @@ func (e *environ) FinaliseBootstrapCredential(
 	}
 
 	instanceRoleName := *args.BootstrapConstraints.InstanceRole
-	if instanceRoleName == awsInstanceProfileAutoCreateVal {
-		controllerName, ok := args.ControllerConfig[controller.ControllerName].(string)
-		if !ok {
-			return cred, errors.NewNotValid(nil, "cannot find controller name in config")
-		}
-
-		instanceRoleName = controllerInstanceProfileName(controllerName)
-	}
-
 	newCred := cloud.NewCredential(cloud.InstanceRoleAuthType, map[string]string{
 		"instance-profile-name": instanceRoleName,
 	})
@@ -212,33 +203,39 @@ func (e *environ) FinaliseBootstrapCredential(
 // Bootstrap is part of the Environ interface.
 func (e *environ) Bootstrap(ctx environs.BootstrapContext, callCtx context.ProviderCallContext, args environs.BootstrapParams) (*environs.BootstrapResult, error) {
 	// We are going to take a look at the Bootstrap constraints and see if we have to make an instance profile
-	if args.BootstrapConstraints.HasInstanceRole() &&
-		*args.BootstrapConstraints.InstanceRole == awsInstanceProfileAutoCreateVal {
-		_, exists := args.ControllerConfig[controller.ControllerName]
-		if !exists {
-			return nil, errors.NewNotValid(nil, "cannot find controller name in config")
-		}
-		controllerName, ok := args.ControllerConfig[controller.ControllerName].(string)
-		if !ok {
-			return nil, errors.NewNotValid(nil, "controller name in config is not a valid string")
-		}
-		controllerUUID := args.ControllerConfig[controller.ControllerUUIDKey].(string)
-		instProfile, cleanups, err := ensureControllerInstanceProfile(
-			ctx.Context(),
-			e.iamClient,
-			controllerName,
-			controllerUUID)
-		if err != nil {
-			for _, c := range cleanups {
-				c()
-			}
-			return nil, err
+	r, err := common.Bootstrap(ctx, e, callCtx, args)
+	return r, maybeConvertCredentialError(err, callCtx)
+}
+
+func (e *environ) CreateAutoInstanceRole(
+	ctx context.ProviderCallContext,
+	args environs.BootstrapParams,
+) (string, error) {
+	_, exists := args.ControllerConfig[controller.ControllerName]
+	if !exists {
+		return "", errors.NewNotValid(nil, "cannot find controller name in config")
+	}
+	controllerName, ok := args.ControllerConfig[controller.ControllerName].(string)
+	if !ok {
+		return "", errors.NewNotValid(nil, "controller name in config is not a valid string")
+	}
+	controllerUUID := args.ControllerConfig[controller.ControllerUUIDKey].(string)
+	instProfile, cleanups, err := ensureControllerInstanceProfile(
+		ctx,
+		e.iamClient,
+		controllerName,
+		controllerUUID)
+	if err != nil {
+		for _, c := range cleanups {
+			c()
 		}
-		args.BootstrapConstraints.InstanceRole = instProfile.InstanceProfileName
+		return "", err
 	}
+	return *instProfile.InstanceProfileName, nil
+}
 
-	r, err := common.Bootstrap(ctx, e, callCtx, args)
-	return r, maybeConvertCredentialError(err, callCtx)
+func (e *environ) SupportsInstanceRoles(_ context.ProviderCallContext) bool {
+	return true
 }
 
 // SupportsSpaces is specified on environs.Networking.

diff --git a/provider/ec2/iam.go b/provider/ec2/iam.go
@@ -95,12 +95,6 @@ func iamClientFunc(
 	return iam.NewFromConfig(cfg), nil
 }
 
-// controllerInstanceProfileName is a convience function for idempotently
-// generating controller instance profile names.
-func controllerInstanceProfileName(controllerName string) string {
-	return fmt.Sprintf("juju-controller-%s", controllerName)
-}
-
 // ensureControllerInstanceProfile ensures that a controller Instance Profile
 // has been created for the supplied controller name in the specified AWS cloud.
 func ensureControllerInstanceProfile(
@@ -136,9 +130,14 @@ func ensureControllerInstanceProfile(
 	}
 
 	cleanups = append([]func(){func() {
-		_, _ = client.DeleteInstanceProfile(ctx, &iam.DeleteInstanceProfileInput{
+		_, err := client.DeleteInstanceProfile(ctx, &iam.DeleteInstanceProfileInput{
 			InstanceProfileName: res.InstanceProfile.InstanceProfileName,
 		})
+		if err != nil {
+			logger.Errorf("cleanup delete instance profile %q: %v",
+				*res.InstanceProfile.InstanceProfileName,
+				err)
+		}
 	}}, cleanups...)
 
 	_, err = client.AddRoleToInstanceProfile(ctx, &iam.AddRoleToInstanceProfileInput{
@@ -156,10 +155,16 @@ func ensureControllerInstanceProfile(
 	}
 
 	cleanups = append([]func(){func() {
-		_, _ = client.RemoveRoleFromInstanceProfile(ctx, &iam.RemoveRoleFromInstanceProfileInput{
+		_, err := client.RemoveRoleFromInstanceProfile(ctx, &iam.RemoveRoleFromInstanceProfileInput{
 			InstanceProfileName: res.InstanceProfile.InstanceProfileName,
 			RoleName:            role.RoleName,
 		})
+		if err != nil {
+			logger.Errorf("cleanup remove role %q from instance profile %q: %v",
+				*role.RoleName,
+				*res.InstanceProfile.InstanceProfileName,
+				err)
+		}
 	}}, cleanups...)
 
 	return res.InstanceProfile, cleanups, nil
@@ -197,9 +202,14 @@ func ensureControllerInstanceRole(
 	}
 
 	cleanups = append(cleanups, func() {
-		_, _ = client.DeleteRole(ctx, &iam.DeleteRoleInput{
+		_, err := client.DeleteRole(ctx, &iam.DeleteRoleInput{
 			RoleName: res.Role.RoleName,
 		})
+		if err != nil {
+			logger.Errorf("cleanup delete role %q: %v",
+				*res.Role.RoleName,
+				err)
+		}
 	})
 
 	_, err = client.PutRolePolicy(ctx, &iam.PutRolePolicyInput{
@@ -213,10 +223,16 @@ func ensureControllerInstanceRole(
 	}
 
 	cleanups = append([]func(){func() {
-		_, _ = client.DeleteRolePolicy(ctx, &iam.DeleteRolePolicyInput{
+		_, err := client.DeleteRolePolicy(ctx, &iam.DeleteRolePolicyInput{
 			PolicyName: aws.String(roleName),
 			RoleName:   res.Role.RoleName,
 		})
+		if err != nil {
+			logger.Errorf("cleanup delete role %q policy %q: %v",
+				*res.Role.RoleName,
+				roleName,
+				err)
+		}
 	}}, cleanups...)
 
 	return res.Role, cleanups, nil

diff --git a/provider/ec2/iam_docs.go b/provider/ec2/iam_docs.go
@@ -50,7 +50,7 @@ const (
         "ec2:DescribeSecurityGroups",
         "ec2:DescribeSpotPriceHistory",
         "ec2:DescribeSubnets",
-        "ec2:DescibeVolumes",
+        "ec2:DescribeVolumes",
         "ec2:DescribeVpcs",
         "ec2:DetachVolume",
         "ec2:RevokeSecurityGroupIngress",
@@ -70,6 +70,7 @@ const (
 		"iam:DeleteRolePolicy",
         "iam:GetInstanceProfile",
 		"iam:GetRole",
+		"iam:PassRole",
 		"iam:PutRolePolicy",
 		"iam:RemoveRoleFromInstanceProfile"
       ],

diff --git a/provider/ec2/iam_test.go b/provider/ec2/iam_test.go
@@ -4,27 +4,14 @@
 package ec2
 
 import (
-	//	"context"
-	//	"net/http"
-	//	time "time"
-	//
+	"context"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
-	//	awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http"
 	"github.com/aws/aws-sdk-go-v2/service/iam"
-
-	//	"github.com/aws/aws-sdk-go-v2/service/iam/types"
-	//	smithyhttp "github.com/aws/smithy-go/transport/http"
-	//	"github.com/juju/errors"
-	"context"
-
 	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	//
-	//	"github.com/juju/juju/environs/tags"
-
 	"github.com/juju/juju/provider/ec2/internal/testing"
 )
 

diff --git a/state/constraints.go b/state/constraints.go
@@ -25,6 +25,7 @@ type constraintsDoc struct {
 	Mem              *uint64
 	RootDisk         *uint64
 	RootDiskSource   *string
+	InstanceRole     *string
 	InstanceType     *string
 	Container        *instance.ContainerType
 	Tags             *[]string
@@ -43,6 +44,7 @@ func newConstraintsDoc(cons constraints.Value, id string) constraintsDoc {
 		Mem:              cons.Mem,
 		RootDisk:         cons.RootDisk,
 		RootDiskSource:   cons.RootDiskSource,
+		InstanceRole:     cons.InstanceRole,
 		InstanceType:     cons.InstanceType,
 		Container:        cons.Container,
 		Tags:             cons.Tags,
@@ -62,6 +64,7 @@ func (doc constraintsDoc) value() constraints.Value {
 		Mem:              doc.Mem,
 		RootDisk:         doc.RootDisk,
 		RootDiskSource:   doc.RootDiskSource,
+		InstanceRole:     doc.InstanceRole,
 		InstanceType:     doc.InstanceType,
 		Container:        doc.Container,
 		Tags:             doc.Tags,

diff --git a/state/migration_internal_test.go b/state/migration_internal_test.go
@@ -584,6 +584,7 @@ func (s *MigrationSuite) TestConstraintsDocFields(c *gc.C) {
 		"Mem",
 		"RootDisk",
 		"RootDiskSource",
+		"InstanceRole",
 		"InstanceType",
 		"Container",
 		"Tags",