Skip to content

fix: surface EC2 request rejections on the EC2NodeClass validation condition - #9454

Open
somaz94 wants to merge 1 commit into
aws:mainfrom
somaz94:fix/nodeclass-surface-dryrun-client-errors
Open

fix: surface EC2 request rejections on the EC2NodeClass validation condition#9454
somaz94 wants to merge 1 commit into
aws:mainfrom
somaz94:fix/nodeclass-surface-dryrun-client-errors

Conversation

@somaz94

@somaz94 somaz94 commented Jul 27, 2026

Copy link
Copy Markdown

Fixes #8148

Description

validateRunInstancesAuthorization returns fmt.Errorf("validating ec2:RunInstances authorization, %w", err) for every error that isn't UnauthorizedOperation or DryRunOperation. But EC2 also rejects a dry run over the request itself, so an InvalidBlockDeviceMapping (root volume smaller than the AMI's snapshot) becomes a reconcile error that is only ever logged, and logged as if it were an authorization problem. The EC2NodeClass stays at ValidationSucceeded=Unknown/AwaitingReconciliation and retries forever, which is what #8148 reports. validateCreateFleetAuthorization and validateCreateLaunchTemplateAuthorization have the same shape.

All three now surface an AWS API error on ValidationSucceeded with the error's code and message instead of returning it:

ValidationSucceeded=False
reason:  RunInstancesValidationFailed
message: EC2 rejected the ec2:RunInstances dry run: InvalidBlockDeviceMapping: Volume of size 2GB is
         smaller than snapshot 'snap-0a8454f13dc7cb861', expect size>= 20GB

Two things worth flagging, since they go a little past the surfacing change:

  1. IsServerError never matched an EC2 error. It tested apiErr.ErrorFault() == smithy.FaultServer, but EC2's generated deserializers build a smithy.GenericAPIError without setting Fault, so it always read FaultUnknown. That is why the 503 Unavailable in the issue also landed in the authorization bucket. It now falls back to the response status code via retry.DefaultRetryableHTTPStatusCodes, so it tracks the SDK rather than hardcoding a range. IsServerError is only called from validation.go, so the blast radius stays in this file.

  2. A cached validation failure lives for ValidationTTL (30 minutes) and drives Ready=False, so only errors a retry cannot fix should reach it. Transient codes are filtered out first by a new IsNonTerminalError, which reuses the SDK's own throttle and retryable code sets and adds AuthFailure, RequestExpired, PendingVerification, plus the two launch template not-found codes (a template can be garbage collected between EnsureAll and the dry run). Those requeue exactly as before.

CreateLaunchTemplate isn't actually a dry run, so its message says "request" instead.

How was this change tested?

go test ./pkg/errors/... ./pkg/controllers/... and golangci-lint run (v2.12.2, the version hack/toolchain.sh pins) both pass locally against envtest 1.34.1.

New coverage:

  • three DescribeTable entries, one per validator, asserting the reason and the full condition message
  • a 503 and an EC2ThrottledException requeue without caching or failing validation
  • a non-AWS error still returns the wrapped error rather than being swallowed by the new branch
  • pkg/errors/errors_test.go, which the package didn't have, covering IsServerError, IsNonTerminalError and ToAPIErrorMessage

I also confirmed the two key specs fail when the fix is reverted.

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@somaz94

somaz94 commented Jul 30, 2026

Copy link
Copy Markdown
Author

Could a maintainer approve the workflow runs for this PR when you get a chance?

CI, CI-TEST, CodeQL and DryRunGenPR have all been sitting in action_required since the PR was opened, so no checks have executed yet and I have no way to show the change is green. The PR is out of draft and ready for review.

@somaz94
somaz94 force-pushed the fix/nodeclass-surface-dryrun-client-errors branch from b83bab7 to ed1e3ae Compare August 5, 2026 03:23
@somaz94

somaz94 commented Aug 5, 2026

Copy link
Copy Markdown
Author

Rebased onto main — the conflict was #9326's new CEL constants landing in the same const block, so this is up to date and mergeable again.

While rebasing I noticed this overlaps with #9360, which was opened about two weeks before mine and targets the same issue (#8148) and the same files. I'd rather not leave two PRs doing the same thing in your queue, so I'm happy to close this one if you'd prefer to take that one forward.

One difference worth knowing before you decide: the comment on #9360 raises cache poisoning on transient, non-rate-limited API errors. This PR guards that case explicitly — isTransientError also checks IsNonTerminalError, so a transient failure isn't cached as a validation result, and pkg/errors/errors_test.go covers it. If that's the behaviour you want, it may be easier to take here. If not, I'm glad to move it over as a review comment on #9360 instead.

Either way: the workflows on this PR have been at action_required since it was opened, so the repo's own suite has never actually run against it. An approval on the run would help whichever direction you pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InvalidBlockDeviceMapping (and other errors) are treated as validateRunInstancesAuthorization error, conditions do not expose error

1 participant