fix: surface EC2 request rejections on the EC2NodeClass validation condition

Author: somaz94

pkg/controllers/nodeclass/validation.go

@@ -56,23 +56,38 @@ import (
 )
 
 const (
-	requeueAfterTime                              = 10 * time.Minute
-	ConditionReasonCreateFleetAuthFailed          = "CreateFleetAuthCheckFailed"
-	ConditionReasonCreateLaunchTemplateAuthFailed = "CreateLaunchTemplateAuthCheckFailed"
-	ConditionReasonRunInstancesAuthFailed         = "RunInstancesAuthCheckFailed"
-	ConditionReasonInstanceProfileNotFound        = "InstanceProfileNotFound"
-	ConditionReasonDependenciesNotReady           = "DependenciesNotReady"
-	ConditionReasonTagValidationFailed            = "TagValidationFailed"
-	ConditionReasonKubeletExpressionInvalid       = "KubeletExpressionInvalid"
-	ConditionReasonKubeletExpressionEvalFailed    = "KubeletExpressionEvaluationFailed"
-	ConditionReasonKubeletExpressionsDisabled     = "KubeletExpressionsDisabled"
-	ConditionReasonDryRunDisabled                 = "DryRunDisabled"
+	requeueAfterTime                                    = 10 * time.Minute
+	ConditionReasonCreateFleetAuthFailed                = "CreateFleetAuthCheckFailed"
+	ConditionReasonCreateLaunchTemplateAuthFailed       = "CreateLaunchTemplateAuthCheckFailed"
+	ConditionReasonRunInstancesAuthFailed               = "RunInstancesAuthCheckFailed"
+	ConditionReasonCreateFleetValidationFailed          = "CreateFleetValidationFailed"
+	ConditionReasonCreateLaunchTemplateValidationFailed = "CreateLaunchTemplateValidationFailed"
+	ConditionReasonRunInstancesValidationFailed         = "RunInstancesValidationFailed"
+	ConditionReasonInstanceProfileNotFound              = "InstanceProfileNotFound"
+	ConditionReasonDependenciesNotReady                 = "DependenciesNotReady"
+	ConditionReasonTagValidationFailed                  = "TagValidationFailed"
+	ConditionReasonKubeletExpressionInvalid             = "KubeletExpressionInvalid"
+	ConditionReasonKubeletExpressionEvalFailed          = "KubeletExpressionEvaluationFailed"
+	ConditionReasonKubeletExpressionsDisabled           = "KubeletExpressionsDisabled"
+	ConditionReasonDryRunDisabled                       = "DryRunDisabled"
 )
 
 var ValidationConditionMessages = map[string]string{
-	ConditionReasonCreateFleetAuthFailed:          "Controller isn't authorized to call ec2:CreateFleet",
-	ConditionReasonCreateLaunchTemplateAuthFailed: "Controller isn't authorized to call ec2:CreateLaunchTemplate",
-	ConditionReasonRunInstancesAuthFailed:         "Controller isn't authorized to call ec2:RunInstances",
+	ConditionReasonCreateFleetAuthFailed:                "Controller isn't authorized to call ec2:CreateFleet",
+	ConditionReasonCreateLaunchTemplateAuthFailed:       "Controller isn't authorized to call ec2:CreateLaunchTemplate",
+	ConditionReasonRunInstancesAuthFailed:               "Controller isn't authorized to call ec2:RunInstances",
+	ConditionReasonCreateFleetValidationFailed:          "EC2 rejected the ec2:CreateFleet dry run",
+	ConditionReasonCreateLaunchTemplateValidationFailed: "EC2 rejected the ec2:CreateLaunchTemplate request",
+	ConditionReasonRunInstancesValidationFailed:         "EC2 rejected the ec2:RunInstances dry run",
+}
+
+// isTransientError returns true for errors that a retry can resolve without a change to the
+// EC2NodeClass, so validation should requeue rather than record a failure against the spec.
+func isTransientError(err error) bool {
+	return awserrors.IsRateLimitedError(err) ||
+		awserrors.IsServerError(err) ||
+		awserrors.IsNonTerminalError(err) ||
+		awserrors.IsInstanceProfileNotFound(err)
 }
 
 // validationCacheEntry stores a failed validation result with both the condition reason and the
@@ -289,11 +304,17 @@ func (v *Validation) validateCreateLaunchTemplateAuthorization(
 
 	launchTemplates, err := v.launchTemplateProvider.EnsureAll(ctx, nodeClass, nodeClaim, instanceTypes[:1], karpv1.CapacityTypeOnDemand, tags, string(tenancyType))
 	if err != nil {
-		if awserrors.IsRateLimitedError(err) || awserrors.IsServerError(err) {
+		if isTransientError(err) {
 			return nil, reconcile.Result{Requeue: true}, nil
 		}
 		if awserrors.IgnoreUnauthorizedOperationError(err) != nil {
-			// We should only ever receive UnauthorizedOperation so if we receive any other error it would be an unexpected state
+			// EC2 also rejects requests it considers malformed, which retrying can't resolve until the
+			// EC2NodeClass itself changes. Surface those on the status condition rather than returning an
+			// error that is only ever logged, and logged as if it were an authorization failure.
+			if message, ok := awserrors.ToAPIErrorMessage(err); ok {
+				v.updateCacheOnFailure(nodeClass, tags, ConditionReasonCreateLaunchTemplateValidationFailed, message)
+				return nil, reconcile.Result{RequeueAfter: requeueAfterTime}, nil
+			}
 			return nil, reconcile.Result{}, fmt.Errorf("validating ec2:CreateLaunchTemplate authorization, %w", err)
 		}
 		log.FromContext(ctx).Error(err, "unauthorized to call ec2:CreateLaunchTemplate")
@@ -321,12 +342,17 @@ func (v *Validation) validateCreateFleetAuthorization(
 	if _, err := v.ec2api.CreateFleet(ctx, createFleetInput, func(o *ec2.Options) {
 		o.Retryer = aws.NopRetryer{}
 	}); awserrors.IgnoreDryRunError(err) != nil {
-		if awserrors.IsRateLimitedError(err) || awserrors.IsServerError(err) {
+		if isTransientError(err) {
 			return reconcile.Result{Requeue: true}, nil
 		}
 		if awserrors.IgnoreUnauthorizedOperationError(err) != nil {
-			// Dry run should only ever return UnauthorizedOperation or DryRunOperation so if we receive any other error
-			// it would be an unexpected state
+			// A dry run also fails when EC2 considers the request itself invalid, which retrying can't
+			// resolve until the EC2NodeClass changes. Surface those on the status condition rather than
+			// returning an error that is only ever logged, and logged as if it were an authorization failure.
+			if message, ok := awserrors.ToAPIErrorMessage(err); ok {
+				v.updateCacheOnFailure(nodeClass, tags, ConditionReasonCreateFleetValidationFailed, message)
+				return reconcile.Result{RequeueAfter: requeueAfterTime}, nil
+			}
 			return reconcile.Result{}, fmt.Errorf("validating ec2:CreateFleet authorization, %w", err)
 		}
 		log.FromContext(ctx).Error(err, "unauthorized to call ec2:CreateFleet")
@@ -376,12 +402,18 @@ func (v *Validation) validateRunInstancesAuthorization(
 		// this means there is most likely an eventual consistency issue and we just need to requeue
 		return reconcile.Result{Requeue: true}, nil
 	}
-	if awserrors.IsRateLimitedError(firstSubnetErr) || awserrors.IsServerError(firstSubnetErr) {
+	if isTransientError(firstSubnetErr) {
 		return reconcile.Result{Requeue: true}, nil
 	}
 	if awserrors.IgnoreUnauthorizedOperationError(firstSubnetErr) != nil {
-		// Dry run should only ever return UnauthorizedOperation or DryRunOperation so if we receive any other error
-		// it would be an unexpected state
+		// A dry run also fails when EC2 considers the request itself invalid, e.g. a block device mapping
+		// whose volume is smaller than the AMI's snapshot. Retrying can't resolve that until the
+		// EC2NodeClass changes, so surface it on the status condition rather than returning an error that
+		// is only ever logged, and logged as if it were an authorization failure.
+		if message, ok := awserrors.ToAPIErrorMessage(firstSubnetErr); ok {
+			v.updateCacheOnFailure(nodeClass, tags, ConditionReasonRunInstancesValidationFailed, message)
+			return reconcile.Result{RequeueAfter: requeueAfterTime}, nil
+		}
 		return reconcile.Result{}, fmt.Errorf("validating ec2:RunInstances authorization, %w", firstSubnetErr)
 	}
 	log.FromContext(ctx).Error(firstSubnetErr, "unauthorized to call ec2:RunInstances")

pkg/controllers/nodeclass/validation_test.go

@@ -17,6 +17,7 @@ package nodeclass_test
 import (
 	"errors"
 	"fmt"
+	"net/http"
 	"time"
 
 	"github.com/awslabs/operatorpkg/object"
@@ -25,11 +26,13 @@ import (
 	"k8s.io/apimachinery/pkg/util/version"
 
 	"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/ec2"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
 	"github.com/aws/aws-sdk-go-v2/service/eks"
 	ekstypes "github.com/aws/aws-sdk-go-v2/service/eks/types"
 	"github.com/aws/smithy-go"
+	smithyhttp "github.com/aws/smithy-go/transport/http"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/tools/record"
@@ -441,7 +444,65 @@ var _ = Describe("NodeClass Validation Status Controller", func() {
 				}, fake.MaxCalls(4))
 			}, nodeclass.ConditionReasonRunInstancesAuthFailed,
 				"Controller isn't authorized to call ec2:RunInstances: User is not authorized to perform this operation due to a service control policy"),
+			Entry("should update status condition as NotReady when RunInstances rejects the request", func() {
+				awsEnv.EC2API.RunInstancesBehavior.Error.Set(&smithy.GenericAPIError{
+					Code:    "InvalidBlockDeviceMapping",
+					Message: "Volume of size 2GB is smaller than snapshot 'snap-0123456789abcdef0', expect size>= 20GB",
+				}, fake.MaxCalls(4))
+			}, nodeclass.ConditionReasonRunInstancesValidationFailed,
+				"EC2 rejected the ec2:RunInstances dry run: InvalidBlockDeviceMapping: Volume of size 2GB is smaller than snapshot 'snap-0123456789abcdef0', expect size>= 20GB"),
+			Entry("should update status condition as NotReady when CreateFleet rejects the request", func() {
+				awsEnv.EC2API.CreateFleetBehavior.Error.Set(&smithy.GenericAPIError{
+					Code:    "InvalidParameterCombination",
+					Message: "The parameter groupName cannot be used with the parameter subnet",
+				}, fake.MaxCalls(1))
+			}, nodeclass.ConditionReasonCreateFleetValidationFailed,
+				"EC2 rejected the ec2:CreateFleet dry run: InvalidParameterCombination: The parameter groupName cannot be used with the parameter subnet"),
+			Entry("should update status condition as NotReady when CreateLaunchTemplate rejects the request", func() {
+				awsEnv.EC2API.CreateLaunchTemplateBehavior.Error.Set(&smithy.GenericAPIError{
+					Code:    "InvalidParameterValue",
+					Message: "Invalid value 'not-a-device' for BlockDeviceMapping.DeviceName",
+				}, fake.MaxCalls(1))
+			}, nodeclass.ConditionReasonCreateLaunchTemplateValidationFailed,
+				"EC2 rejected the ec2:CreateLaunchTemplate request: InvalidParameterValue: Invalid value 'not-a-device' for BlockDeviceMapping.DeviceName"),
 		)
+		It("should requeue without failing validation when RunInstances returns a server error", func() {
+			// EC2's deserializers don't set a fault on the errors they return, so a server error is only
+			// recognizable by the status code of the response it was decoded from.
+			awsEnv.EC2API.RunInstancesBehavior.Error.Set(&awshttp.ResponseError{
+				ResponseError: &smithyhttp.ResponseError{
+					Response: &smithyhttp.Response{Response: &http.Response{StatusCode: 503}},
+					Err:      &smithy.GenericAPIError{Code: "Unavailable", Message: "The service is unavailable. Please try again shortly."},
+				},
+			}, fake.MaxCalls(4))
+			ExpectApplied(ctx, env.Client, nodeClass)
+			ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
+			nodeClass = ExpectExists(ctx, env.Client, nodeClass)
+			// Requeued rather than recorded: neither the success nor the failure path leaves the
+			// condition unknown with nothing cached.
+			Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsUnknown()).To(BeTrue())
+			Expect(awsEnv.ValidationCache.Items()).To(BeEmpty())
+		})
+		It("should requeue without failing validation when RunInstances is throttled", func() {
+			awsEnv.EC2API.RunInstancesBehavior.Error.Set(&smithy.GenericAPIError{
+				Code: "EC2ThrottledException",
+			}, fake.MaxCalls(4))
+			ExpectApplied(ctx, env.Client, nodeClass)
+			ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
+			nodeClass = ExpectExists(ctx, env.Client, nodeClass)
+			// Requeued rather than recorded: neither the success nor the failure path leaves the
+			// condition unknown with nothing cached.
+			Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsUnknown()).To(BeTrue())
+			Expect(awsEnv.ValidationCache.Items()).To(BeEmpty())
+		})
+		It("should return the error when RunInstances fails with a non-AWS error", func() {
+			awsEnv.EC2API.RunInstancesBehavior.Error.Set(fmt.Errorf("connection reset by peer"), fake.MaxCalls(4))
+			ExpectApplied(ctx, env.Client, nodeClass)
+			_ = ExpectObjectReconcileFailed(ctx, env.Client, controller, nodeClass)
+			nodeClass = ExpectExists(ctx, env.Client, nodeClass)
+			Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsFalse()).To(BeFalse())
+			Expect(awsEnv.ValidationCache.Items()).To(BeEmpty())
+		})
 		It("should succeed RunInstances validation when first subnet returns 500 but another subnet succeeds", func() {
 			// Fail the first RunInstances call (first subnet) with a server error,
 			// then let subsequent calls (remaining subnets) succeed via the default dry-run path

pkg/errors/errors.go

@@ -15,14 +15,24 @@ limitations under the License.
 package errors
 
 import (
+	"fmt"
 	"strings"
 
+	"github.com/aws/aws-sdk-go-v2/aws/retry"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
 	"github.com/aws/smithy-go"
 	"github.com/samber/lo"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
+// httpStatusCoder mirrors the interface the SDK's own retry policy matches on
+// (retry.RetryableHTTPStatusCode) — the transport error types the SDK wraps API errors in are the
+// only place the HTTP status code of the failed response survives.
+type httpStatusCoder interface {
+	error
+	HTTPStatusCode() int
+}
+
 const (
 	launchTemplateNameNotFoundCode                 = "InvalidLaunchTemplateName.NotFoundException"
 	RunInstancesInvalidParameterValueCode          = "InvalidParameterValue"
@@ -49,6 +59,18 @@ var (
 		"EntityAlreadyExists",
 	)
 
+	// nonTerminalErrorCodes are codes a request can fail with for reasons unrelated to the request
+	// itself, on top of the retryable and throttle codes the SDK already defines. AuthFailure and
+	// RequestExpired come from credential or clock skew, PendingVerification from account state, and
+	// the launch template codes from a template being garbage collected out from under a caller.
+	nonTerminalErrorCodes = sets.New(
+		"AuthFailure",
+		"RequestExpired",
+		"PendingVerification",
+		launchTemplateNameNotFoundCode,
+		"InvalidLaunchTemplateId.NotFound",
+	)
+
 	reservationCapacityExceededErrorCode = "ReservationCapacityExceeded"
 
 	// unfulfillableCapacityErrorCodes signify that capacity is temporarily unable to be launched
@@ -156,19 +178,62 @@ func IsServerError(err error) bool {
 	if err == nil {
 		return false
 	}
-	if apiErr, ok := lo.ErrorsAs[smithy.APIError](err); ok {
-		return apiErr.ErrorFault() == smithy.FaultServer
+	if apiErr, ok := lo.ErrorsAs[smithy.APIError](err); ok && apiErr.ErrorFault() == smithy.FaultServer {
+		return true
+	}
+	// EC2's generated deserializers return a smithy.GenericAPIError with no fault set, so the fault
+	// check above never matches an EC2 error. Fall back to the status code of the failed response,
+	// using the same set the SDK's own retry policy considers retryable.
+	if respErr, ok := lo.ErrorsAs[httpStatusCoder](err); ok {
+		_, retryable := retry.DefaultRetryableHTTPStatusCodes[respErr.HTTPStatusCode()]
+		return retryable
 	}
 	return false
 }
 
+// IsNonTerminalError returns true if err is an AWS API error that a retry can resolve on its own,
+// without a change to the request that produced it. Callers that would otherwise treat an error as
+// a terminal, user-visible failure should requeue on these instead.
+func IsNonTerminalError(err error) bool {
+	if err == nil {
+		return false
+	}
+	apiErr, ok := lo.ErrorsAs[smithy.APIError](err)
+	if !ok {
+		return false
+	}
+	if _, ok := retry.DefaultRetryableErrorCodes[apiErr.ErrorCode()]; ok {
+		return true
+	}
+	if _, ok := retry.DefaultThrottleErrorCodes[apiErr.ErrorCode()]; ok {
+		return true
+	}
+	return nonTerminalErrorCodes.Has(apiErr.ErrorCode())
+}
+
 func IgnoreServerError(err error) error {
 	if IsServerError(err) {
 		return nil
 	}
 	return err
 }
 
+// ToAPIErrorMessage returns a "<code>: <message>" summary of err when err is, or wraps, an AWS API
+// error. The second return value reports whether err was an AWS API error at all.
+func ToAPIErrorMessage(err error) (string, bool) {
+	if err == nil {
+		return "", false
+	}
+	apiErr, ok := lo.ErrorsAs[smithy.APIError](err)
+	if !ok {
+		return "", false
+	}
+	if apiErr.ErrorMessage() == "" {
+		return apiErr.ErrorCode(), true
+	}
+	return fmt.Sprintf("%s: %s", apiErr.ErrorCode(), apiErr.ErrorMessage()), true
+}
+
 // IsUnfulfillableCapacity returns true if the Fleet err means capacity is temporarily unavailable for launching. This
 // could be due to account limits, insufficient ec2 capacity, etc.
 func IsUnfulfillableCapacity(err ec2types.CreateFleetError) bool {