fix(ec2): render iamInstanceProfile on instances, fix IAM profile association state and checks - #2538
Merged
Conversation
…ociation state and checks DescribeInstances never emitted <iamInstanceProfile>. AssociateIamInstanceProfile, RunInstances --iam-instance-profile and the CloudFormation instance provisioner all recorded an association, but the instance render ignored it, so `describe-instances --query ...IamInstanceProfile.Arn` stayed null forever. RunInstances additionally rendered the instance before recording the association, so the launch response omitted it too. Render the profile from the association map at describe time (same pattern as security-group names and block-device mappings) and pass it into the instance render; build the association before rendering on RunInstances. Rendering ignores the association state so records persisted by earlier versions still show. Adjacent fixes in the same operation family, matching the AWS docs: - Associate and Replace answer `associating` on the wire and store `associated` (Associate used to store `associating` and never advance it; Replace answered `associated`). Same shape Disassociate already used for `disassociating`. - Replace retires the old association and mints a new id, as the AWS doc sample shows. - Associate rejects an unknown instance (InvalidInstanceID.NotFound) and a second profile on an instance that already has one (IncorrectState). Replace and Disassociate reject an unknown association id (InvalidAssociationID.NotFound). All three used to answer success with a fabricated record. - Associate and Replace require IamInstanceProfile (MissingParameter). Both used to store an association with an empty ARN. One constructor builds the association record for all four writers and one helper renders the <iamInstanceProfile> fragment for both instance and association responses. The conformance smoke tests for Associate, Replace and Disassociate now launch an instance (and associate a profile) first instead of using dummy ids.
Follow-ups on top of the iamInstanceProfile render, found reviewing it.
Conformance: EC2 declares no Smithy errors, so the probe treats any 4xx
on a Success variant as undeclared unless the code is in the service's
shared-error list. Add InvalidAssociationID.NotFound, IncorrectState and
InvalidIamInstanceProfileArn.Malformed there, so the new AWS-correct
errors read as handler responses instead of silently shedding ec2
variants (the per-service flake margin would have hidden the loss).
DescribeIamInstanceProfileAssociations honored only AssociationId.N and
dropped Filter.N. The terraform AWS provider reads an instance's profile
with Filter Name=instance-id and takes item [0], so with two instances in
an account it got the other instance's association and called Replace on
it. With Replace now retargeting the stored instance, that silently moved
instance A's profile onto B. Honor instance-id and state, sort the set by
association id so repeated describes agree, and return
InvalidAssociationID.NotFound for an explicitly named unknown id rather
than an empty set.
TerminateInstances left the association behind, so a terminated instance
kept reporting <iamInstanceProfile>, kept showing in the association
describe, and could never be associated again (the new one-profile check
saw the stale record). Drop the account's associations for the instances
a Terminate affects, as AWS does.
Validate the profile reference instead of fabricating one: an empty
Arn=/Name= (what the SDK builder sends for .name("")) now reads as absent
rather than storing an empty ARN, a name outside [\w+=,.@-]{1,128} is
InvalidParameterValue, and an ARN that is not an instance-profile ARN is
InvalidIamInstanceProfileArn.Malformed. Associate also rejects a
shutting-down or terminated instance with IncorrectInstanceState; AWS
associates with a running or stopped instance only.
The rendered profile id used gen_id("AIPA"), which is EC2's resource-id
shape (AIPA-<lower hex>); AWS unique ids are a 4-char prefix plus 17
uppercase alphanumerics with no separator, the shape fakecloud's own IAM
service mints. Add aws_unique_id and use it, now that this PR makes the
value visible on every DescribeInstances.
DescribeInstances gains the iam-instance-profile.arn and .id filters,
reading the same record the instance renders.
CloudFormation applied IamInstanceProfile on create but never on update,
so an UpdateStack that changed it reported UPDATE_COMPLETE with the old
profile still attached; AWS updates it in place. The update arm now
replaces, associates or disassociates to match the template. That path
also needed the actions it already issued -- ModifyInstanceAttribute and
CreateTags -- added to the EC2 provisioner dispatch, which they were
missing, so an in-place instance update failed the whole stack update
with action_not_implemented.
Tests: handler-level coverage for each error and for the filter, sort and
terminate behavior; a DescribeInstances profile-filter test; and an e2e
that creates a stack with an instance profile, updates the stack to a
different profile, and asserts the instance id is unchanged and
describe-instances reports the new profile.
Second review pass on the association contract. The rendered InstanceProfileId was minted per association, so two instances sharing one profile reported two different ids and the new iam-instance-profile.id filter could never select both; re-pointing at the same profile changed it again. AWS reports the profile's own id. EC2 cannot read IAM's store (no fakecloud-iam dependency), so derive the id from the profile ARN instead: stable across restarts, shared by every association with that profile, still AIPA + 17 uppercase alphanumerics. A name-based association synthesized arn:aws: regardless of region while IAM mints the ARN in the region's partition, so in cn-* / us-gov-* / us-iso* the two never compared equal. Use partition_for(region) in both the handler and the CloudFormation create path. The CloudFormation update arm replaced the association on every in-place update, because it never compared the template's profile to the attached one. An UpdateStack that only bumped InstanceType or added a tag retired the association id and the profile id under callers holding them; AWS leaves the association alone when the profile is unchanged. Compare first, and pull the id out with the file's existing xml_elem rather than a hand-rolled split. The CloudFormation create path built the association straight from the template with none of the validation Associate and Replace apply, so a template resolving IamInstanceProfile to a role ARN (the usual Fn::GetAtt mistake) created fine and then made every later UpdateStack fail when the stored value went back through the validating handler. Validate on create and update alike. Also drop the redundant sort in the association describe: the backing map is a BTreeMap keyed by association id, so its iteration order is already stable. Tests: one profile shared by two instances reports one id; a cn-north-1 association renders an aws-cn ARN; an unrelated stack update keeps the association id; a role ARN in the template fails the create.
`postgres_pod_exec_readiness_query_and_dump` failed with
psql: error: connection to server on socket
"/var/run/postgresql/.s.PGSQL.5432" failed: No such file or directory
right after its pg_isready loop reported ready. The postgres image runs a
temporary server on its own socket while initdb runs, then stops it and
starts the real one, so pg_isready can answer yes against the temporary
server and the socket is gone by the next exec. Wait for `psql -tAc
'SELECT 1'` to succeed instead, which is the only signal that outlives
the restart, and reuse that result as the query assertion.
The previous commit derived EC2's rendered IamInstanceProfile.Id from the profile ARN, which made it stable and shared across instances, but IAM still minted its own InstanceProfileId at random. So GetInstanceProfile and DescribeInstances reported two different ids for the same profile, and a describe-instances filter on the id IAM handed out matched nothing. Move the derivation into fakecloud-aws as arn::unique_id_for(prefix, arn) -- the AWS unique-id shape, a 4-char family prefix plus 17 uppercase base32 characters -- and have both services call it. IAM derives from the ARN it just built, EC2 from the ARN on the association, so the two agree without EC2 reading IAM's store. Also report the last real error when the RDS k8s postgres readiness loop times out, rather than a fixed string, so a renamed container or an auth failure is not indistinguishable from slow startup. Tests: an e2e that creates the profile through IAM, attaches it through EC2, and asserts DescribeInstances reports IAM's own InstanceProfileId and that the iam-instance-profile.id filter selects the instance; plus unit coverage of the id's shape, stability and partition sensitivity.
The last commit made IAM and EC2 agree. Three gaps remained. The CloudFormation AWS::IAM::InstanceProfile provisioner is a third constructor and still minted a random id, 20 characters rather than the 21 AWS returns, and built the ARN with a hardcoded arn:aws:. A CFN-created profile attached to an instance therefore reproduced exactly the bug this PR fixes: one id from GetInstanceProfile, a different one from DescribeInstances. It now derives from the ARN like the others, in the request's partition. IMDS iam/info returned a fourth value, the 22-character constant AIPAFAKECLOUDINSTPROF0, matching neither AWS's shape nor the other three. It builds the profile ARN already, so derive from that. partition_for was missing us-isof-* and eu-isoe-*, which IAM's and STS's private copies both had, so in those regions EC2's synthesized ARN (arn:aws:) and IAM's (arn:aws-iso-f:) disagreed and the derived ids came out different. Add them, and have the two copies delegate, so one lookup serves the workspace. Also: restore partition_for's doc comment, which the previous commit accidentally left attached to unique_id_for; note on the helper that a derived id is a pure function of the ARN, so a delete-then-recreate returns the same id where AWS mints a fresh one; and note on the EC2 side that a name-addressed profile on a non-default path is the one case the two ARNs cannot be made to agree, since EC2 cannot resolve the path. Tests: a CFN stack profile whose id matches what DescribeInstances renders for an instance it is attached to, the iam/info id derived from its own ARN, and partition coverage for the two added regions.
Widening arn::partition_for in the last commit exposed three places that had their own answer, and the id invariant still had a hole. IAM's policy validator rejected any partition outside a hardcoded five, so in us-isof-* / eu-isoe-* a policy naming an ARN fakecloud itself had just minted failed with MalformedPolicyDocument. Add the two partitions. CloudFormation's AWS::Partition resolver knew only cn- and us-gov-, and the IAM provisioner hardcoded arn:aws: for roles, policies, users, groups and OIDC providers while the instance profile had just become partition-aware. In an isolated region that left one stack holding ARNs in two partitions, and an Fn::Sub over AWS::Partition resolving to a profile that does not exist. Route both through arn::partition_for, and accept any partition where the Roles[] parser strips a role ARN. `Ref` on an AWS::IAM::InstanceProfile resolves to the profile NAME, and only IAM knows the Path that name's ARN carries, so a profile created with Path /app/ was stored at instance-profile/app/p while the instance rendered instance-profile/p -- two ARNs and two ids for one profile, which is the divergence this PR set out to remove, reachable from a template with no ARN to pass instead. The provisioner now resolves the name through IAM on both the create and the update path, falling back to a name-addressed association when IAM holds no such profile. IMDS iam/info synthesized its profile ARN from the credentials role name, so it agreed with IAM only for a profile named after the role on the default path. Prefer the ARN of a profile IAM actually holds for that role. Tests: a stack whose pathed profile has one ARN and one id on the instance and in GetInstanceProfile; iam/info preferring the stored profile's ARN.
…gion Widening the partition lookup reached CreateStack but not the change-set path, and left the two pseudo-parameters disagreeing. CreateChangeSet and ExecuteChangeSet both seeded AWS::Partition with a literal "aws" and AWS::URLSuffix with "amazonaws.com". So a template resolving AWS::Partition previewed one value and created another, and a stack provisioned through ExecuteChangeSet in a non-default partition minted ARNs in the wrong one -- the divergence this branch just removed from the IAM provisioner. Both now resolve from the request's region. AWS::URLSuffix knew only cn-, so an isolated region got the newly correct AWS::Partition next to amazonaws.com, and an Fn::Sub building an endpoint from the pair produced a host that matches nothing. Derive it from the same partition lookup: c2s.ic.gov, sc2s.sgov.gov, cloud.adc-e.uk and csp.hci.ic.gov for the four isolated partitions. The don't-churn-the-association guard compared on the profile name only when the request was name-addressed on the wire, which stopped being the case once a name is resolved to IAM's ARN. A template naming a profile IAM did not yet hold got a path-less ARN stored, and the next unrelated UpdateStack then saw IAM's pathed ARN as a change and retired the association id. Compare on the profile name whenever the template addressed the profile by name. IMDS picked any profile carrying the role when several do, which AWS allows and IMDS cannot disambiguate without the EC2 association. Answer only when the choice is unambiguous -- a profile named after the role, or the single one carrying it -- and otherwise keep the synthesized ARN. Also move partition()'s doc comment back onto partition(), where the last commit's insertion had displaced it. Tests: AWS::Partition and AWS::URLSuffix agree across all seven partitions.
Member
|
Thanks a lot for this, @jakshi. The write-up made the review easy: you found the real bug ( I pushed a few follow-ups before merging:
Merged. Really appreciate the contribution, and the level of detail in the PR description. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Attach an IAM instance profile to an instance and
describe-instancesstill reportsIamInstanceProfile: null. The association exists (describe-iam-instance-profile-associationslists it), but the instance render never emits<iamInstanceProfile>. Same forrun-instances --iam-instance-profile: the launch response omits it because RunInstances renders the instance before writing the association.Smaller issues in the same family, found while fixing this:
associate-iam-instance-profilestores the association asassociatingand nothing ever advances it. The other two writers storeassociated.replace-iam-instance-profile-associationkeeps the old association id and answersassociated. AWS answersassociatingwith a new id (see the doc sample).associate-iam-instance-profileaccepts an unknown instance, and a second profile on an instance that already has one.replace-iam-instance-profile-associationanddisassociate-iam-instance-profileaccept an unknown association id and answer with a fabricated record.IamInstanceProfileand store an association with an empty ARN.What changed
DescribeInstancesresolves the profile from the association map once per request and passes it into the instance render, as it already does for security-group names and block-device mappings. No new field onInstance, no snapshot change. Records with stateassociatingfrom older versions still render; they stay untouched, sodescribe-iam-instance-profile-associationskeeps reporting them asassociatinguntil replaced or disassociated.RunInstancesbuilds the association before rendering, so the launch response carries the element.associatingon the wire and storeassociated, mirroring what Disassociate already does withdisassociating. Replace retires the old record and mints a newiip-assoc-id.InvalidInstanceID.NotFoundfor an unknown instance andIncorrectStatefor a second profile ("You cannot associate more than one IAM instance profile with an instance", per the AssociateIamInstanceProfile docs). Replace and Disassociate returnInvalidAssociationID.NotFoundfor an unknown association id. Associate and Replace returnMissingParameterwhenIamInstanceProfileis absent (the model marks it required). Error codes are from the EC2 error-code list; the message texts are ours.<iamInstanceProfile>fragment for both instance and association responses.Test plan
fakecloud-ec2): describe shows the profile after associate and drops it after disassociate;run-instancesresponse carries it; CloudFormation-provisioned instance shows it; describe shows the new profile after replace; Associate answersassociatingand Describe readsassociated; Replace answersassociatingwith a new id and the old id is gone; Associate on unknown instance ->InvalidInstanceID.NotFound; second Associate ->IncorrectState; Replace and Disassociate on unknown association id ->InvalidAssociationID.NotFound; Associate and Replace withoutIamInstanceProfile->MissingParameter, nothing stored (Replace keeps the old association).aws-sdk-ec2,ec2_instance_control_plane): run -> associate -> describe asserts the exactiam_instance_profile().arn()and.id()-> disassociate -> describe assertsNone.None, the describe regression test fails.associating(written by earlier versions) still renders on the instance.cargo test --workspace --no-fail-fast,cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --check. Locally, 14801 pass; the only failure iskinesis_reports_millis_behind_latest_when_limit_truncates, whose two localhost writes land in the same millisecond. It does not touch EC2. All 94fakecloud-tfaccshards pass locally with terraform 1.16.3.Not in this PR
iam-instance-profile.arn/iam-instance-profile.idfilters onDescribeInstances.describe-iam-instance-profile-associations).idmatch theInstanceProfileIdthe IAM service assigned; EC2 mints its own.