Add AWS App Runner (Container App Hosting) Service Emulation - #391
Add AWS App Runner (Container App Hosting) Service Emulation#391thzgajendra wants to merge 3 commits into
Conversation
Full aws-sdk-go-v2/service/apprunner parity — all 37 operations — over the AWS JSON 1.0 (X-Amz-Target: AppRunner.<Op>) wire protocol: - Services (create RUNNING, describe/update/delete/list, pause/resume state machine, StartDeployment) with per-service operation history (ListOperations) - Auto-scaling / observability / VPC-connector configs (revision-managed), connections, VPC ingress connections, custom domains, and tags (routed by ARN across all six taggable resource kinds) - SetIfAbsent atomic creates, deep-copied reads, per-op typed exceptions matching each op's SDK error model, optimistic lifecycle guards, deterministic Clock/idgen Includes a proactive review-lessons pass: per-op error fidelity (CreateVpcIngressConnection/AssociateCustomDomain -> InvalidRequestException, not NotFound), Tags deep-copy on auto-scaling-config and connection reads, and required-field/enum validation. docs/services.md (service stackshy#40), architecture.md, and generated coverage.
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Thorough review of the new App Runner service. This is a strong PR and the proactive review-lessons pass clearly worked — credit up front:
- Concurrency is fully clean. All four sibling-PR bug classes are structurally prevented: creates are atomic test-and-set; auto-scaling / observability configs are revision-managed under one lock with exactly-one-Latest (12 concurrent same-name creates → 12 distinct revisions, one Latest); all six taggable kinds do single-lock read-modify-write (no lost updates, probe-verified); reads are fully deep-copied (including the previously-shallow ASC/connection Tags, now fixed); lifecycle transitions are atomically guarded; and the orphan class is unreachable (soft-delete + nested children).
- The op set is exactly faithful — all 37 real
apprunner@v1.39.14operations, nothing invented, and correctly noDescribeConnection(App Runner has only Create/List/Delete for connections). - Error taxonomy is correct for 36 of 37 ops — the 24 RNF-modeling ops keep RNF; ops that don't model RNF (AssociateCustomDomain, CreateVpcIngressConnection) return InvalidRequestException. Routing (
X-Amz-Target: AppRunner.prefix, unique across handlers), casing (nojson:"...ID"bug), epoch-seconds timestamps, pagination, and the service state machine are all faithful.
Requesting changes on one wire-fidelity High plus a Medium (details inline).
High
StartDeployment emits InvalidStateException, which the op does not model. Verified against the SDK: StartDeployment's error deserializer models only InternalServiceErrorException, InvalidRequestException, and ResourceNotFoundException — no InvalidStateException case. So start-deployment on a non-RUNNING service sends an exception a real smithy client can't deserialize as typed (errors.As(&*types.InvalidStateException) fails → generic untyped APIError); real App Runner returns InvalidRequestException. The other three state-guards (PauseService/ResumeService, DeleteService, DisassociateCustomDomain) all correctly use InvalidStateException because those ops model it — StartDeployment is the outlier. Fix: emit invalidRequest at services.go:232. This path is also untested (see coverage), which is why it slipped.
Medium
- Delete-in-use is not rejected.
DeleteAutoScalingConfigurationandDeleteVpcConnector(andDeleteObservabilityConfiguration) succeed even when a service still references them; real App Runner returnsInvalidStateExceptionfor an in-use config/connector. The in-use query already exists and is genuinely implemented (ListServicesForAutoScalingConfigurationscans services byAutoScalingConfigArn) — delete just doesn't consult it. - SDK round-trip coverage is 26/37.
StartDeploymentis untested (a round-trip asserting the typed error on start-deployment-of-paused would have caught the High), along with the Update/Delete config ops and the VPC connector/ingress describes. Recommend adding StartDeployment (happy + wrong-state) and the config-revision delete/describe ops.
Low
DisassociateCustomDomainreturnsInvalidRequestExceptionfor a missing service, but the op modelsResourceNotFoundException(its siblingDescribeCustomDomainscorrectly uses RNF) — not wire-breaking (IRE is also modeled), but inconsistent with real AWS.DeleteServiceis a soft-delete that doesn't disassociate its per-service children (custom domains / VPC ingress connections linger), and operations still succeed against aDELETEDservice (tag / associate-custom-domain / create-vpc-ingress) where real App Runner rejects them.SourceConfigurationone-of (CodeRepository XOR ImageRepository) and instance cpu/memory combos aren't validated (only non-empty is checked);ProviderTypeGITHUB/BITBUCKET is validated.ExServiceQuotaExceededis declared but never emitted (dead constant) — wire it to a quota guard or drop it.
On "full parity"
Wire/operation parity essentially holds (faithful op set, taxonomy correct bar the one op, correct protocol/casing/timestamps); behavioral parity is best-effort — immediate-RUNNING is documented, but the delete-in-use and deleted-service state guards are missing. Recommend the wording reflect that.
| defer sd.mu.Unlock() | ||
|
|
||
| if sd.svc.Status != driver.ServiceStatusRunning { | ||
| return "", invalidState("service %q is %s; StartDeployment requires RUNNING", arn, sd.svc.Status) |
There was a problem hiding this comment.
High — StartDeployment does not model InvalidStateException. Verified against apprunner@v1.39.14: this op's error deserializer has cases only for InternalServiceErrorException, InvalidRequestException, and ResourceNotFoundException. Emitting invalidState here means start-deployment on a non-RUNNING service returns an exception a real client can't deserialize as typed (errors.As(&*types.InvalidStateException) fails → generic APIError); real App Runner returns InvalidRequestException. Use invalidRequest(...) instead. (Pause/Resume/Delete/Disassociate correctly use invalidState because those ops DO model it — StartDeployment is the exception.)
There was a problem hiding this comment.
Fixed. StartDeployment now returns InvalidRequestException for a non-RUNNING service. Verified against apprunner@v1.39.14 that its error deserializer models only Internal/InvalidRequest/ResourceNotFound (not InvalidState). Added TestStartDeploymentIllegalStateIsInvalidRequest + E2E.
| // DeleteAutoScalingConfiguration marks a revision INACTIVE (or, when | ||
| // deleteAllRevisions is set, every revision sharing its name). It returns the | ||
| // (highest) affected revision. | ||
| func (m *Mock) DeleteAutoScalingConfiguration( |
There was a problem hiding this comment.
Medium — DeleteAutoScalingConfiguration deactivates the revision unconditionally. Real App Runner rejects deleting a configuration that a service still uses with InvalidStateException. The in-use query is already implemented right here (ListServicesForAutoScalingConfiguration scans services by AutoScalingConfigArn) — consult it before deleting and return InvalidStateException when non-empty.
There was a problem hiding this comment.
Fixed — with one correction. DeleteAutoScalingConfiguration now rejects an in-use config (any service referencing it via AutoScalingConfigArn, reusing the ListServicesForAutoScalingConfiguration scan; a delete-all-revisions checks every revision of the name). One nuance: I verified this op does NOT model InvalidStateException (only Internal/InvalidRequest/ResourceNotFound), so the in-use rejection is InvalidRequestException, not InvalidState — emitting InvalidState here would be a type a real client cannot deserialize for this op (the same class as the StartDeployment finding). Added TestDeleteAutoScalingConfigurationInUse + E2E.
| } | ||
|
|
||
| // DeleteVpcConnector marks a connector INACTIVE and stamps its deletion time. | ||
| func (m *Mock) DeleteVpcConnector(_ context.Context, arn string) (*driver.VpcConnector, error) { |
There was a problem hiding this comment.
Medium — same delete-in-use gap: DeleteVpcConnector succeeds even when a service references this connector via its NetworkConfiguration. Real App Runner returns InvalidStateException for an in-use VPC connector. Gate the delete on whether any service references it.
There was a problem hiding this comment.
Fixed — same nuance as the ASC delete. DeleteVpcConnector now rejects a connector referenced by any service via its egress NetworkConfiguration.EgressConfiguration.VpcConnectorArn. This op does NOT model InvalidStateException either (only Internal/InvalidRequest/ResourceNotFound), so the in-use rejection is InvalidRequestException. Added TestDeleteVpcConnectorInUse + E2E.
| ) (*driver.CustomDomain, string, error) { | ||
| sd, ok := m.services.Get(serviceArn) | ||
| if !ok { | ||
| return nil, "", invalidRequest("no App Runner service found for ARN %q", serviceArn) |
There was a problem hiding this comment.
Low — DisassociateCustomDomain returns InvalidRequestException for a missing service, but this op models ResourceNotFoundException (and the sibling DescribeCustomDomains correctly uses RNF via getService). Not wire-breaking since IRE is also modeled, but it diverges from real AWS and from its sibling — prefer notFound here.
There was a problem hiding this comment.
Fixed. DisassociateCustomDomain now returns ResourceNotFoundException for a missing service (it models RNF, matching the DescribeCustomDomains sibling). AssociateCustomDomain keeps InvalidRequestException since it does NOT model RNF. Added TestDisassociateCustomDomainMissingServiceIsNotFound + E2E.
| ExInvalidRequest = "InvalidRequestException" | ||
| ExInvalidState = "InvalidStateException" | ||
| ExResourceNotFound = "ResourceNotFoundException" | ||
| ExServiceQuotaExceeded = "ServiceQuotaExceededException" |
There was a problem hiding this comment.
Low — ExServiceQuotaExceeded is declared but never emitted anywhere (the six Create ops model it, but the emulator has no quotas). Dead code per the no-unused-symbols rule — wire it to a quota guard or remove it.
There was a problem hiding this comment.
Fixed. Removed the dead ExServiceQuotaExceeded constant — the emulator models no quotas, so nothing emits it. (Went with removal rather than inventing an arbitrary quota limit that would diverge from real per-region defaults.)
- StartDeployment on an illegal state returns InvalidRequestException (the op does NOT model InvalidStateException; only Pause/Resume/Delete/Disassociate do) - DeleteAutoScalingConfiguration and DeleteVpcConnector reject an in-use resource with InvalidRequestException (neither op models InvalidStateException, so the in-use rejection is a 400 InvalidRequest, not InvalidState) — in-use is detected by scanning services' AutoScalingConfigArn / egress VpcConnectorArn - DisassociateCustomDomain returns ResourceNotFoundException for a missing service (it models RNF, unlike AssociateCustomDomain) - Removed the dead ExServiceQuotaExceeded constant (no quotas modeled) - Regression tests for each fix
|
Thanks @NitinKumar004 — all 5 addressed (pushed on the latest commit).
Each fix has a regression test. Verified: gofmt / vet / build / |
NitinKumar004
left a comment
NitinKumar004
left a comment
There was a problem hiding this comment.
Review notes
Real data-plane engine (per #427): MISSING — App Runner is ContainerEngine-eligible (it HOSTS a container image as a long-running service, exactly like ECS services / ACI / Cloud Run, all of which route through services/container/containerengine per #427). The PR wires NO engine: CreateService returns a synthetic Status=RUNNING + ServiceURL, StartDeployment only records a SUCCEEDED operation, DeleteService just flips status to DELETED — none call containerengine.Run/Stop.
Staleness / integration: CONFLICTS(docs/architecture.md) — docs-only, trivial. The PR is 29 commits behind development; merging origin/development into the PR head conflicts ONLY in docs/architecture.md (PR appended "+ App Runner" to a prose list; development rewrote the whole file to a mermaid layout).
Terraform compat: GAP.
coveragegen / docs autogen: OK.
Findings
High · real-engine — App Runner is fully synthetic — no ContainerEngine wiring, diverges from #427 (ECS/ACI/Cloud Run)
providers/aws/apprunner/services.go:44
config.WithContainerEngine has no effect on App Runner: a user who opts into the real data plane gets a real container for ECS/ACI/Cloud Run but a fake RUNNING service from App Runner, so integration tests that exercise the hosted app (hit ServiceURL, read container logs, observe crash/restart) pass against nothing. The engine seam exists and is plumbed (opts reaches New) but is unused, so this is a silent behavioral gap, not a compile error — the PR's own green CI cannot catch it.
CreateService sets Status: driver.ServiceStatusRunning + a fabricated ServiceURL and returns immediately; StartDeployment (services.go:222) only records a SUCCEEDED operation; DeleteService (services.go:92) flips status to DELETED. None route through services/container/containerengine.Run/Stop. apprunner.New(o) (apprunner.go:96) stores config.Options o — which carries o.ContainerEngine — but the Mock never reads it. ECS (providers/aws/ecs/engine.go:30 backTaskWithEngine) and Cloud Run (providers/gcp/cloudrun/engine
Medium · wire-fidelity — ListServices returns DELETED services forever
providers/aws/apprunner/services.go:168
If a user calls DeleteService(arn) and then ListServices(), the deleted service still appears in ServiceSummaryList (Status DELETED) permanently, because the store keeps the tombstone and ListServices does not filter it out. Real aws apprunner list-services never returns deleted services, so client code that enumerates live services via ListServices double-counts and may operate on a deleted service's ARN.
ListServices iterates m.services.SortedValues() with no status filter, and DeleteService (line 92-115) intentionally retains the record with Status=DELETED rather than removing it. No test deletes-then-lists (TestServiceCRUD deletes but never re-lists; TestSDKListServices/TestListServicesPagination never delete).
Medium · wire-fidelity — List{AutoScaling,Observability}Configurations return INACTIVE (deleted) revisions, contradicting docstring and real API
providers/aws/apprunner/autoscaling.go:175
If a user creates ASC 'hi' rev1, deletes it (rev1 -> INACTIVE), then calls ListAutoScalingConfigurations(name='hi', latestOnly=false), the deleted rev1 is still returned. Real App Runner returns only active configurations, and the function's own docstring promises ACTIVE-only, so a caller enumerating live/deletable revisions acts on an already-deleted revision.
ListAutoScalingConfigurations (autoscaling.go:175) and ListObservabilityConfigurations (observability.go:96) filter only on name and latestOnly, never on Status, yet both carry the doc comment 'lists ACTIVE revisions'. Delete sets Status=INACTIVE but leaves the revision in the map. TestObservabilityRevisions deletes rev1 then lists with latestOnly=true, which already excludes rev1 (it was never Latest), so the INACTIVE-still-listed path is untested.
Medium · staleness — Stale by 29 commits — docs/architecture.md conflicts on merge with development
docs/architecture.md:60
PR cannot be merged as-is; needs a rebase onto development. Resolution is trivial (re-insert 'App Runner' into the rewritten containers list), but until rebased the branch is red on merge and the App Runner mention would be lost if 'theirs' is taken blindly.
merge-base 6ff27cf; git merge origin/development into the PR head fails only in docs/architecture.md. The PR appended '+ App Runner' to the server/ prose list; development rewrote the entire file (three-layer prose -> mermaid flowchart). Code files auto-merge cleanly and build/vet/test pass after resolving.
Low · wire-fidelity — HasAssociatedService wire field is never populated (always false)
services/apprunner/driver/types.go:150
If a user creates an ASC, creates a service referencing it, then calls DescribeAutoScalingConfiguration/ListAutoScalingConfigurations, HasAssociatedService comes back false despite an active service using the config. SDK consumers that gate deletion or reporting on HasAssociatedService are misled into thinking the config is unused.
HasAssociatedService is defined on the driver type and emitted in both wireASC (types.go:180) and ascSummaryItem (types.go:195), but grep shows it is never assigned anywhere in providers/aws/apprunner — the provider has ascInUse()/ListServicesForAutoScalingConfiguration to compute association but never sets this field.
Low · wire-fidelity — ListOperations ordered by random operation Id, not reverse-chronological
providers/aws/apprunner/operations.go:26
If a user performs Create then Pause then Update on a service and calls ListOperations, OperationSummaryList is ordered by an arbitrary random id rather than newest-first, so a client reading OperationSummaryList[0] to obtain the latest operation gets an arbitrary one. (Pagination itself stays consistent since the token is the same Id key.)
ListOperations sorts sd.ops by o.ID (idgen.GenerateID random) ascending. Real App Runner returns operations in reverse chronological order (most recent first).
Low · correctness — CreateService silently accepts an unknown AutoScalingConfigurationArn; ASC min>max validation bypassable
providers/aws/apprunner/autoscaling.go:252
If a user calls CreateService with AutoScalingConfigurationArn set to a non-existent config, the service is created RUNNING with an empty AutoScalingConfigurationSummary name/revision instead of the InvalidRequestException real App Runner returns, so the SDK response is internally inconsistent and the misconfiguration goes undetected.
attachDefaultASC looks up svc.AutoScalingConfigArn in ascByArn; on miss it simply returns, leaving AutoScalingConfigName/Revision empty, and CreateService never validates the ARN exists. Separately, CreateAutoScalingConfiguration validation (autoscaling.go:70) is
minSize>0 && maxSize>0 && minSize>maxSize, so passing minSize=30 with maxSize unset (defaulted to 25) skips the check and stores an invalid min>max config.
Low · terraform — App Runner absent from SDK compat matrix and terraform harness
compat/aws/containerregistry_compat_test.go:1
App Runner's real-cloud fidelity is asserted only by the PR's own SDK test, not by the repo's cross-cutting compat matrix or terraform suite, so a future wire-shape regression (e.g. a source_configuration field that stops round-tripping and would show a terraform perpetual diff) would go uncaught by the shared harnesses.
No compat/aws/apprunner_compat_test.go and no apprunner entry in docs/compat/coverage — App Runner will not appear in the generated compatibility matrix. contrib/terraform/fixtures/ has only basic/networking/wrapper, so there is no terraform apply -> post-apply-plan(no changes) round-trip for aws_apprunner_service et al. The real-SDK roundtrip in server/aws/apprunner/sdk_roundtrip_test.go covers the shapes but is not the terraform/compat harness.
2 participants
Objective
Add AWS App Runner to cloudemu at full
aws-sdk-go-v2wire parity — all 37 operations — as part of #295 Workstream B. Realaws-sdk-go-v2/service/apprunnerclients and theaws apprunnerCLI work against the SDK-compat server by only changing the endpoint.What's implemented
App Runner is AWS JSON 1.0 with
X-Amz-Target: AppRunner.<Op>dispatch (same protocol as Step Functions, the structural template). Four-layer architecture (services/apprunner/driver→providers/aws/apprunner→server/aws/apprunner→ registration).CreateServicemints an ARN + ID and returns an immediately-RUNNINGservice with a synthesizedServiceUrl; describe/update/delete/list;PauseService/ResumeService/StartDeploymentenforce the state machine (InvalidStateExceptionfrom an illegal state); every mutation records anOperationSummarysurfaced byListOperations.Latest, atomic under a lock).Quality:
SetIfAbsentatomic creates; deep-copied reads (race-clean); per-op typed exceptions; optimistic lifecycle guards; deterministicClock/idgen.Proactive review-lessons pass
Before opening, I ran an adversarial audit against the accumulated review lessons with the exact SDK per-op error models, and fixed what it found:
ResourceNotFoundException(e.g.CreateVpcIngressConnection,AssociateCustomDomain) returnInvalidRequestExceptionfor a missing service, not RNF; the 24 ops that do model RNF keep it.Tagsdeep-copy on auto-scaling-config and connection reads (were shallow-aliased); regression test under-race.CreateServicerequiresSourceConfiguration; auto-scaling rejectsMinSize > MaxSize;ProviderType/enum validation.Docs
docs/services.md(service #40, operation families, Grand Total → 2786),docs/architecture.md, and regenerateddocs/coverage/(addsaws/apprunner.md). The coverage generator's determinism fix (from the MSK PR) held — no unrelated page churn.Tests
Provider unit tests (CRUD, get-missing → RNF, pause illegal-state → InvalidState, ASC revision increments,
-raceno-alias for SourceConfiguration + Tags, concurrent-tag no-lost-update) and SDK-roundtrip tests using the real client.Verification (locally)
gofmt ·
go build ./...·go build -mod=readonly ./...·go vet·go test -race ./...·go mod tidy(no diff) · golangci-lint--new-from-rev0 issues · CodeQL (CI'sgo-code-scanningsuite) 0 findings in the service path · CLI + wire E2E (create RUNNING → pause → illegal-pause→InvalidState → resume → 3 recorded operations → tag routing → ghost-service create→InvalidRequest → missing describe→RNF).Risk & rollback
Additive: a new service package + registration (X-Amz-Target dispatched, unambiguous with S3). No shared-code changes. Revert by dropping the branch.