Merge pull request juju#13490 from tlm/aws-instance-profile

juju#13490 This commit adds support instance profiles auto creation. Including the associated role and policy needed to scope permissions correctly. Accompanying this PR is also a discourse doc on the permissions Juju requires when talking with AWS. ## QA steps ```sh juju bootstrap --bootstrap-constraints="instance-role=auto" aws/ap-southeast-2 test-tlm-controller ``` You then need to confirm with the aws cli that the associated instance profile elements were created. Make sure that you see a role with the same here attached to the instance profile. ```sh aws iam get-instance-profile --instance-profile-name juju-controller-test-tlm-controller ``` Add some machines to Juju and deploy a charm with storage to confirm permissions are operating correctly. Check HA works ``` $ juju enable-ha juju enable-ha maintaining machines: 0 adding machines: 1, 2 $ juju status Model Controller Cloud/Region Version SLA Timestamp controller test-tlm-controller aws/ap-southeast-2 2.9.20.1 unsupported 09:34:28+10:00 Machine State DNS Inst id Series AZ Message 0 started 13.211.59.65 i-f00f00f00f00f00f00 focal ap-southeast-2b running 1 pending pending focal attaching aws instance profile arn:aws:iam::123456789:instance-profile/juju-controller-test-tlm-controller 2 pending pending focal attaching aws instance profile arn:aws:iam::123456789:instance-profile/juju-controller-test-tlm-controller ``` Check destroy-controller works ``` $ juju destroy-controller test-tlm-controller --destroy-all-models ``` ## Documentation changes Permission list: https://discourse.charmhub.io/t/juju-aws-permissions/5307 Doc Link: https://discourse.charmhub.io/t/using-aws-instance-profiles-with-juju-2-9/5185/3 ## Bug reference N/A
sdg0919 · Nov 26, 2021 · 5ba3128 · 5ba3128
2 parents 6567679 + 1a40220
commit 5ba3128
Show file tree

Hide file tree

Showing 16 changed files with 713 additions and 227 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/broker.go b/environs/broker.go
@@ -93,7 +93,7 @@ type StartInstanceParams struct {
 
 	// CleanupCallback is a callback to be used to clean up any residual
 	// status-reporting output from StatusCallback.
-	CleanupCallback func(info string) error
+	CleanupCallback func() error
 
 	// StatusCallback is a callback to be used by the instance to report
 	// changes in status. Its signature is consistent with other

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/common/bootstrap.go b/provider/common/bootstrap.go
@@ -168,23 +168,37 @@ func BootstrapInstance(
 	// Print instance status reports status changes during provisioning.
 	// Note the carriage returns, meaning subsequent prints are to the same
 	// line of stderr, not a new line.
+	lastLength := 0
+	statusCleanedUp := false
 	instanceStatus := func(settableStatus status.Status, info string, data map[string]interface{}) error {
 		// The data arg is not expected to be used in this case, but
 		// print it, rather than ignore it, if we get something.
 		dataString := ""
 		if len(data) > 0 {
 			dataString = fmt.Sprintf(" %v", data)
 		}
-		fmt.Fprintf(ctx.GetStderr(), " - %s%s\r", info, dataString)
+		length := len(info) + len(dataString)
+		padding := ""
+		if lastLength > length {
+			padding = strings.Repeat(" ", lastLength-length)
+		}
+		lastLength = length
+		statusCleanedUp = false
+		fmt.Fprintf(ctx.GetStderr(), " - %s%s%s\r", info, dataString, padding)
 		return nil
 	}
 	// Likely used after the final instanceStatus call to white-out the
 	// current stderr line before the next use, removing any residual status
 	// reporting output.
-	statusCleanup := func(info string) error {
+	statusCleanup := func() error {
+		if statusCleanedUp {
+			return nil
+		}
+		statusCleanedUp = true
 		// The leading spaces account for the leading characters
 		// emitted by instanceStatus above.
-		fmt.Fprintf(ctx.GetStderr(), "   %s\r", info)
+		padding := strings.Repeat(" ", lastLength)
+		fmt.Fprintf(ctx.GetStderr(), "   %s\r", padding)
 		return nil
 	}
 
@@ -255,7 +269,7 @@ func BootstrapInstance(
 
 		select {
 		case <-ctx.Context().Done():
-			return nil, "", nil, errors.Annotatef(err, "starting controller (cancelled)")
+			return nil, "", nil, errors.Annotate(err, "starting controller (cancelled)")
 		default:
 		}
 
@@ -278,6 +292,10 @@ func BootstrapInstance(
 		return nil, "", nil, errors.Annotatef(err, "cannot start bootstrap instance in availability zone %q", zone)
 	}
 
+	err = statusCleanup()
+	if err != nil {
+		return nil, "", nil, errors.Annotate(err, "cleaning up status line")
+	}
 	msg := fmt.Sprintf(" - %s (%s)", result.Instance.Id(), formatHardware(result.Hardware))
 	// We need some padding below to overwrite any previous messages.
 	if len(msg) < 40 {

diff --git a/provider/ec2/client.go b/provider/ec2/client.go
@@ -53,6 +53,9 @@ type Client interface {
 	// here https://discourse.charmhub.io/t/juju-aws-permissions/5307
 	// We must keep this policy inline with our usage for operators that are
 	// using very strict permissions for Juju.
+	//
+	// You must also update the controllerRolePolicy document found in
+	// iam_docs.go.
 	AssociateIamInstanceProfile(context.Context, *ec2.AssociateIamInstanceProfileInput, ...func(*ec2.Options)) (*ec2.AssociateIamInstanceProfileOutput, error)
 	DescribeIamInstanceProfileAssociations(context.Context, *ec2.DescribeIamInstanceProfileAssociationsInput, ...func(*ec2.Options)) (*ec2.DescribeIamInstanceProfileAssociationsOutput, error)
 	DescribeInstances(context.Context, *ec2.DescribeInstancesInput, ...func(*ec2.Options)) (*ec2.DescribeInstancesOutput, error)

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,39 +203,41 @@ 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 {
-
-		// Added by tlm on 07/10/2021 (This will be removed very shortly).
-
-		// If the user has asked us to automatically create the instance
-		// profile for them we will return a not supported for now. This is till
-		// the remainder of the AWS work is complete and we have concrete role
-		// policies to use.
-		return nil, errors.NotSupportedf("instance profile creation with %s", awsInstanceProfileAutoCreateVal)
-
-		// Commenting out the below code till the above error is removed.
-
-		//controllerName, ok := args.ControllerConfig[controller.ControllerName].(string)
-		//if !ok {
-		//	return nil, errors.NewNotValid(nil, "cannot find controller name in config")
-		//}
-		//controllerUUID := args.ControllerConfig[controller.ControllerUUIDKey].(string)
-		//instProfile, err := ensureControllerInstanceProfile(
-		//	ctx.Context(),
-		//	e.iamClient,
-		//	controllerName,
-		//	controllerUUID)
-		//if err != nil {
-		//	return nil, err
-		//}
-		//args.BootstrapConstraints.InstanceRole = instProfile.InstanceProfileName
-	}
-
 	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()
+		}
+		return "", err
+	}
+	return *instProfile.InstanceProfileName, nil
+}
+
+func (e *environ) SupportsInstanceRoles(_ context.ProviderCallContext) bool {
+	return true
+}
+
 // SupportsSpaces is specified on environs.Networking.
 func (e *environ) SupportsSpaces(ctx context.ProviderCallContext) (bool, error) {
 	return true, nil