fix(ec2): persist Modify*/Create* attributes so Describe reflects them#2041
Open
vieiralucas wants to merge 2 commits into
Open
fix(ec2): persist Modify*/Create* attributes so Describe reflects them#2041vieiralucas wants to merge 2 commits into
vieiralucas wants to merge 2 commits into
Conversation
The EC2 long-tail handlers in service/rest.rs validated their input and returned a minimal success WITHOUT persisting anything, so the paired Describe*/Get* reported the unchanged/empty default. Implement real state-backed round-trips for the whole no-persist family: - IdFormat: ModifyIdFormat / ModifyIdentityIdFormat now persist per-resource (and per-principal) long-id settings; DescribeIdFormat / DescribeIdentityIdFormat / DescribePrincipalIdFormat / DescribeAggregateIdFormat read them back. - Managed prefix lists: Create/Modify/Delete/Describe/GetEntries/RestoreVersion with versioned entry history. - Instance event windows: Create/Modify/Delete/Describe + Associate/Disassociate targets. - Default credit specification: Modify -> Get per instance family. - VPC block-public-access: options Modify -> Describe; exclusions Create/Modify/ Delete/Describe. - Traffic mirroring: targets, filters, filter rules, sessions (Create/Modify/Delete/Describe) + ModifyTrafficMirrorFilterNetworkServices. - Route servers: Create/Modify/Delete/Describe. - VPC encryption controls: Create/Modify/Delete/Describe incl. resource exclusions. - Managed resource visibility: Modify -> Get. - FPGA images: Create/Copy/Delete/Describe + Modify/Describe/Reset attribute. - Availability zone group opt-in: Modify -> reflected in DescribeAvailabilityZones. - ModifyPrivateDnsNameOptions: persisted on the instance, reflected in DescribeInstances. - ModifyIpamPoolAllocation / DescribeIpamPoolAllocations: allocation description round-trip. - ModifyPublicIpDnsNameOptions: persisted on the ENI (AWS exposes no Describe that returns this setting, so it is stored and the ENI's existence enforced, but not reflected through a Describe). New state stores live in state.rs; instance/eni gain the round-trip fields. Unit tests in rest.rs and an ec2_modify_persistence E2E suite cover the round-trips via the AWS SDK.
The first cut returned NotFound (InvalidPrefixListID.NotFound, etc.) for the synthetic ids the conformance probe sends. EC2's Query API models no error shape for these ops, so the harness flagged every positive variant as an undeclared-error response, dropping ec2 from 24577 to 24070 passing. - Convert the Modify*/Delete*/Get*/Describe*Attribute/Associate* handlers I added to synthesize a valid success response for an unknown id WITHOUT persisting (the existing modify_ipam/delete_ipam convention), instead of returning an unmodeled error. Real ids still persist and round-trip. - ModifyPublicIpDnsNameOptions / ModifyPrivateDnsNameOptions: persist on the resource when present, acknowledge otherwise (ModifyPrivateDnsNameOptions keeps its InvalidInstanceID.NotFound, which is in the EC2 error allowlist). - Fix two pre-existing slice-index panics surfaced by the probe's short synthetic ids: subnet_ipv6_assoc_id (&subnet_id[7..]) and associate_vpc_cidr_block (&vpc_id[4..]) now strip the prefix instead of byte-slicing, eliminating the AssociateSubnetCidrBlock/AssociateVpcCidrBlock crashes. Conformance: ec2 back to 24633/24633 (100%, up from baseline 24577); full `conformance check` PASSED with no regressions. Unit test updated to assert the synthesize-without-persist behavior.
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
The EC2 long-tail handlers in
crates/fakecloud-ec2/src/service/rest.rswere a "validate then drop" family: each took_svc(unused), validated wire params, and returned a minimal success WITHOUT persisting anything. The pairedDescribe*/Get*therefore reported the unchanged/empty default. This violates the project's no-stub rule and means a real client never sees its own changes.Fix
Real, state-backed round-trips for the whole no-persist family. The Modify/Create writes into
crate::state; the paired Describe/Get reads it back.Ops fixed (grouped)
ModifyIdFormat,ModifyIdentityIdFormat->DescribeIdFormat,DescribeIdentityIdFormat,DescribePrincipalIdFormat,DescribeAggregateIdFormatCreateManagedPrefixList,ModifyManagedPrefixList,DeleteManagedPrefixList,DescribeManagedPrefixLists,GetManagedPrefixListEntries,RestoreManagedPrefixListVersion(versioned entry history)CreateInstanceEventWindow,ModifyInstanceEventWindow,DeleteInstanceEventWindow,DescribeInstanceEventWindows,AssociateInstanceEventWindow,DisassociateInstanceEventWindowModifyDefaultCreditSpecification->GetDefaultCreditSpecificationModifyVpcBlockPublicAccessOptions->DescribeVpcBlockPublicAccessOptions;CreateVpcBlockPublicAccessExclusion,ModifyVpcBlockPublicAccessExclusion,DeleteVpcBlockPublicAccessExclusion,DescribeVpcBlockPublicAccessExclusionsCreate/Modify/Delete/Describe+ModifyTrafficMirrorFilterNetworkServicesCreateRouteServer,ModifyRouteServer,DeleteRouteServer,DescribeRouteServersCreateVpcEncryptionControl,ModifyVpcEncryptionControl,DeleteVpcEncryptionControl,DescribeVpcEncryptionControls(incl. per-resource exclusions)ModifyManagedResourceVisibility->GetManagedResourceVisibilityCreateFpgaImage,CopyFpgaImage,DeleteFpgaImage,DescribeFpgaImages,ModifyFpgaImageAttribute,DescribeFpgaImageAttribute,ResetFpgaImageAttributeModifyAvailabilityZoneGroup-> reflected (optInStatus/groupName) inDescribeAvailabilityZonesModifyPrivateDnsNameOptionspersisted on the instance, reflected inDescribeInstancesModifyIpamPoolAllocation(description) ->DescribeIpamPoolAllocations(was a stub returning empty)Partially descoped (with reason)
ModifyPublicIpDnsNameOptions: now persists the chosenpublicHostnameTypeonto the ENI and rejects an unknown interface, but AWS exposes no Describe operation that returns this setting, so it cannot be read back through a Describe. Persisted (real mutation, existence enforced) rather than left as a validate-then-drop stub.State
New stores added to
state.rs(id-format maps, prefix lists, event windows, traffic-mirror stores, route servers, VPC encryption controls, BPA exclusions/options, credit specs, AZ opt-in, FPGA images, IPAM allocation descriptions).Instancegains private-DNS fields;NetworkInterfacegains the public-IP DNS hostname type. All#[serde(default)]so existing snapshots load.Tests
rest.rsdriving each group's round-trip directly.crates/fakecloud-e2e/tests/ec2_modify_persistence.rs(7 tests) exercising id-format, managed prefix lists, instance event windows, default credit specs, VPC block-public-access, traffic mirroring, and private DNS name options via the real AWS SDK.Validation
cargo build(workspace + bin) - cleancargo fmt --all -- --check- cleancargo clippy -p fakecloud-ec2 --all-targets -- -D warnings- cleancargo test -p fakecloud-ec2- 73 passedcargo test -p fakecloud-e2e --no-run- compilescargo test -p fakecloud-e2e --test ec2_modify_persistence- 7 passedRisk to double-check
DescribeAvailabilityZonesnow emits additionaloptInStatus/groupNamemembers (additive; no e2e asserts on its shape).Summary by cubic
Persist EC2 Modify*/Create* attributes so Describe/Get reflect user changes, replacing validate-only stubs. Unknown ids now synthesize a success (no persist) for conformance, and CIDR-assoc id panics are fixed.
Bug Fixes
ModifyPrivateDnsNameOptionscontinues to returnInvalidInstanceID.NotFoundwhen the instance is unknown;ModifyPublicIpDnsNameOptionspersists when ENI exists, otherwise acknowledges.Migration
DescribeAvailabilityZonesnow includesoptInStatusandgroupName(additive).NotFoundfor the ops above (no client changes needed).#[serde(default)]; no manual migration needed.Written for commit b52618b. Summary will update on new commits.