Skip to content

Fix AWS Parity Audit Bugs: Event Delivery, Tagging, Describe NotFound & More (#319) - #320

Merged
thzgajendra merged 45 commits into
stackshy:developmentfrom
thzgajendra:fix/aws-parity-audit-319
Aug 4, 2026
Merged

Fix AWS Parity Audit Bugs: Event Delivery, Tagging, Describe NotFound & More (#319)#320
thzgajendra merged 45 commits into
stackshy:developmentfrom
thzgajendra:fix/aws-parity-audit-319

Conversation

@thzgajendra

@thzgajendra thzgajendra commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Objective / Issue

Fixes the AWS parity E2E audit findings in #319 — the systemic themes plus the High/Med per-service gaps. Every fix was verified end-to-end by driving the real aws-sdk-go-v2 / AWS CLI against the running cloudemu serve binary, and each carries a regression test. 44 commits.

Review round (ead94bb)

Addressed every finding from the code review (including the ones rated Low / out-of-scope):

  • EventBridge id determinism — folded the PutEvents loop index into generateEventID so byte-identical events in one call get unique ids under FakeClock.
  • S3 notification completeness — notify on CompleteMultipartUpload/CopyObject, emit ObjectRemoved:Delete on DeleteObject; made the ObjectRemoved test non-vacuous (copy + delete + assert delivery).
  • Lambda parity — mirrored the no-handler 200+echo invoke stub to Azure Functions and GCP Cloud Functions (CLAUDE.md mirror rule).
  • RDS tag-Matches — scope-gated AddTags/RemoveTags/ListTags behind the rds SigV4 scope, like redshift.
  • SNS MessageAttributes — now carried end-to-end through the Publish → SQS envelope.
  • ECR — implemented Set/Get/DeleteRepositoryPolicy (SDK round-trip test).
  • ENI — added a CreateNetworkInterface unknown-SubnetId negative test.
  • Lint hygienestateRunning const, ec2/metadata.go receivers, wsl fixes.
  • ssm/runcommand.go "dead code" — could not reproduce (staticcheck + unused linters find nothing; the writeErr fallthrough is reachable for InvalidArgument); asked the reviewer for the specific line.

Per-service: bug found → fix

Service Bug / gap found in the audit Fix
CloudWatch Every op via the AWS CLI → InvalidAction (handler only matched rpc-v2-cbor; the CLI sends the query protocol, which EC2 then stole) Serve the classic query protocol; disambiguate by the monitoring SigV4 credential scope
SNS Publishes never delivered to SQS subscriptions; TagResource/UntagResourceInvalidAction; SetTopicAttributesInvalidAction SNS→SQS fan-out delivery (real envelope); Tag/Untag; SetTopicAttributes (DisplayName)
EventBridge Matched rule events never delivered to SQS targets; TagResource missing; RuleArn malformed (arn:aws:events:::rule/..., no region/account) Rule→SQS target delivery; Tag/Untag/ListTagsForResource; well-formed rule ARNs
EC2 Describe{Vpcs,Instances,Volumes,SecurityGroups} by missing ID returned empty (not Invalid*.NotFound); CreateTags/DeleteTagsInvalidAction; DescribeRegions/DescribeInstanceTypes/MonitorInstances/DescribeInstanceStatus/CreateNetworkInterface missing Invalid*.NotFound; CreateTags/DeleteTags routed by resource-ID prefix; all 5 ops implemented
SQS GetQueueAttributes/SetQueueAttributes/PurgeQueue not served (couldn't read QueueArn for DLQ/ESM/S3-notify wiring); TagQueue missing All three served; queue Tag/Untag/ListQueueTags
Lambda UpdateFunctionConfiguration/PublishVersion/CreateAlias/AddPermission → 404; TagResource → 405+HTML; invoke returned a FunctionError for every function; CreateEventSourceMapping → 405 Routed configuration/versions/aliases/policy/tags/event-source-mappings; invoke returns a testable stub when no Go handler is registered
ECR GetAuthorizationToken missing (no docker login); TagResource missing Auth token (base64 AWS:<pw> + proxy endpoint + expiry); repository Tag/Untag/List
SecretsManager UpdateSecret missing; TagResource missing UpdateSecret (metadata + optional new version); Tag/Untag
DynamoDB Query ignored FilterExpression (silent wrong data); TagResource/DescribeTimeToLive missing Query applies the filter after the key match; tagging + TTL served
S3 HeadBucket → 405; PUT ?tagging mis-routed to CreateBucket; ListObjectsV2 ignored MaxKeys (no pagination); PUT ?notification mis-routed + object events never delivered HeadBucket; bucket tagging; max-keys + continuation pagination; bucket notifications with S3→SQS delivery
IAM PutRolePolicy (inline policies) missing; TagRole missing Inline role policies (Put/Get/Delete/List); role Tag/Untag/List
SSM AddTagsToResource missing Parameter Add/Remove/ListTagsForResource
CloudWatch Logs PutRetentionPolicy missing; TagResource missing Retention via UpdateLogGroup; log-group tagging (ARN + legacy name forms)
ElastiCache ModifyCacheCluster missing Implemented (node type / engine)
Redshift CreateClusterParameterGroup/CreateClusterSubnetGroup missing; CreateTags missing Parameter & subnet groups; CreateTags/DeleteTags/DescribeTags (SigV4-scope-gated to avoid EC2/ELBv2 collisions)
ResourceExplorer2 CreateIndex/GetDefaultView → 405+HTML (SDK deserialize error) Both served (idempotent index; first-view default)
ELBv2 AddTags/RemoveTags missing (tags only settable at create) Implemented against LB/target-group by ARN
EKS TagResource missing; cluster version returned null Cluster Tag/Untag/List; default Kubernetes version
Route 53 ChangeTagsForResource/ListTagsForResource missing Implemented (generic ID-keyed tag store)
Bedrock (no product bug) ConverseStream CI test flaked on connection teardown Disable keep-alives on the streaming test client

Systemic themes (the table above, grouped)

  • Event delivery: SNS→SQS, EventBridge→SQS, S3→SQS, CloudWatch-via-CLI.
  • Theme C: EC2 Describe Invalid*.NotFound.
  • Theme A — tagging (13 services): EC2, SNS, DynamoDB, Lambda, SQS, SSM, SecretsManager, CloudWatch Logs, ECR, EventBridge, IAM, Redshift, ELBv2, EKS, Route 53.
  • Theme D — 405+HTML wire bugs: Lambda tagging, resourceexplorer2.
  • Silent-wrong-data: DynamoDB Query filter, S3 ListObjectsV2 MaxKeys.

How we fixed it

  • Provider-owned behavior, thin wire: several gaps were driver-implemented but not dispatched (SQS attributes, DynamoDB TTL, cloudwatchlogs retention).
  • AWS-specific surfaces stay AWS-local: ops that don't belong on a shared cross-cloud driver grow the AWS provider and are reached via a type-asserted AWS-local interface, leaving Azure/GCP providers untouched.
  • Query-protocol disambiguation: Redshift's generic tag verbs collide with EC2/ELBv2, so they're claimed only under the redshift SigV4 scope.

Regressions caught in-branch

  • Split CreateNetworkInterface out of the shared NetworkInterfaces interface (resourcediscovery's read-only subset assertion must keep working).
  • Redshift tag-verb collision fixed via SigV4-scope gating.

Test plan

  • go build/vet/gofmt/mod tidy clean; go test ./... green
  • Full CI gate incl. CodeQL — only the 8 pre-existing baseline alerts (middleware.go, azure/databricks, aws/sts, gcp/cloudsql); zero new
  • 28 regression test files (~1,443 lines); each fix E2E-verified via the real AWS CLI/SDK
  • All 11 GitHub checks green

Docs

Inline Go package doc comments updated (sqs, lambda). docs/services.md documents portable-driver operations only and explicitly excludes provider-specific surfaces; the wired ops were already listed there and the new AWS-local ops are out of its scope, so no change was required.

Deferred follow-ups (Low-value long tail)

Field/ARN cosmetics (ECR repositoryArn, IAM CreateDate, SecretsManager ARN suffix, S3 ETag MD5, SQS localhost queue URL, SNS SubscriptionArn) and niche ops (DynamoDB UpdateTable/Streams, AutoScaling CreateLaunchConfiguration, Route 53 CreateHealthCheck, ELBv2 ModifyTargetGroup, SecretsManager Rotate/RestoreSecret, EC2 ModifySubnetAttribute/VPC endpoints/extra instance fields, IAM CreateServiceLinkedRole, SQS batch/visibility, SNS ListTagsForResource). Several need per-service region/account or driver-struct plumbing; the audit flags these as diminishing-returns.

Closes #319

…shy#319)

The CloudWatch handler only matched rpc-v2-cbor requests, so the AWS CLI —
which sends CloudWatch as the classic query protocol (form-encoded POST,
Action=..., XML responses) — was stolen by the EC2 handler and every op
returned InvalidAction.

Add a query-protocol path to the CloudWatch handler, disambiguated from EC2
by the SigV4 credential scope service ("monitoring"). Implements PutMetricData,
ListMetrics, GetMetricStatistics, PutMetricAlarm, DescribeAlarms, DeleteAlarms,
and SetAlarmState (the last previously a gap). PutMetricData now defaults an
absent timestamp to now (so GetMetricStatistics returns datapoints).

Verified end-to-end with the real aws CLI; EC2 form-POST routing unaffected.
Adds TestQueryProtocol regression test.
SNS Publish stored the message but never delivered it to subscribers, so the
SNS->SQS fan-out pattern silently dropped messages. Add an SQSDeliverer hook
(satisfied by the SQS mock's new DeliverExternal, which enqueues by queue ARN)
and fan out on Publish to every sqs-protocol subscription, wrapping the payload
in the standard SNS Notification envelope. Wired via SetSQSDeliverer in aws.go.

Verified end-to-end with the real aws CLI (subscribe + publish + receive).
Adds TestSNSToSQSDelivery.
…y#319)

PutEvents matched rules against patterns but never propagated events to
their targets, so SQS-target subscribers received nothing. Wire an
injected SQSDeliverer and deliver the standard EventBridge event
envelope to every matched target whose ARN is an SQS queue.
)

Describe{Vpcs,Instances,Volumes,SecurityGroups} with an explicit ID that
does not exist returned an empty success instead of the resource-specific
Invalid*.NotFound error real EC2 emits. That silently broke existence
checks, Terraform refresh/drift detection, and wait-until-deleted polls.
Providers now return NotFound for a missing explicit ID; the server
handlers already map it to InvalidVpcID/InvalidInstanceID/InvalidVolume/
InvalidGroup.NotFound.
…ackshy#319)

The provider already implemented these, but the JSON-RPC handler didn't
dispatch them, so callers couldn't read a queue's ARN (required for
DLQ wiring, Lambda event-source mappings, and S3->SQS notifications),
resize a queue, or drain it. Wire the three operations through to the
existing driver methods.
…kshy#319)

UpdateFunctionConfiguration, PublishVersion/ListVersionsByFunction, and
the alias lifecycle (create/get/list/update/delete) returned 404
"unsupported Lambda path" — the driver implemented them but the REST
handler only dispatched the collection, resource, and invoke shapes. Add
routing for /{name}/configuration, /{name}/versions, /{name}/aliases, and
/{name}/aliases/{alias}. Resource policies (AddPermission), tagging, and
event-source mappings remain follow-ups.
…shy#319)

Terraform's aws_lambda_permission and the invoke grants S3/SNS/
EventBridge create were unreachable (404). Add a per-function resource-
policy store in the AWS provider and route /{name}/policy(/{sid}).
Resource policies are Lambda-specific, so the handler type-asserts an
AWS-local policyManager rather than widening the portable Serverless
driver (which Azure Functions and GCP Cloud Functions also implement).
The emulator stores statements without evaluating them.
docker login and image push/pull authenticate via GetAuthorizationToken,
which was unimplemented. The AWS ECR provider now returns a base64
"AWS:<password>" credential, the registry proxy endpoint, and a 12h
expiry. Registry auth is ECR-specific, so the handler type-asserts an
AWS-local authTokenProvider rather than widening the shared
ContainerRegistry driver (Azure ACR and GCP Artifact Registry also
implement it).
stackshy#319)

UpdateSecret (routine metadata/value change) and secret tagging were
unimplemented. Add UpdateSecret (description + optional new value
version) plus Tag/Untag to the AWS provider, routed via an AWS-local
secretMutator assertion so the shared Secrets driver (also implemented by
Azure Key Vault and GCP Secret Manager) stays untouched.
CreateTags/DeleteTags returned InvalidAction, so every IaC tool that
tags EC2 resources (nearly all of them) failed. Route the calls by
resource-ID prefix: VPC-family IDs to the networking provider's existing
tag methods, and instance/volume/snapshot/image IDs to a new AWS-local
compute tagger. Unknown IDs return InvalidID.NotFound.
SNS TagResource returned InvalidAction, blocking IaC tag-on-create flows.
Add TagTopic/UntagTopic to the AWS provider, routed via an AWS-local
topicTagger assertion (the shared Notification driver is also implemented
by Azure Notification Hubs and GCP FCM). ListTagsForResource is deferred:
its action name collides with RDS in the query protocol and needs SigV4
credential-scope routing to disambiguate.
…tackshy#319)

The database driver already implemented table tagging, but the DynamoDB
JSON-RPC handler didn't dispatch the three tag operations, so they
returned UnknownOperationException. Wire them through, resolving the
ResourceArn to the table name.
…s) (stackshy#319)

The Lambda tagging API lives at the /2017-03-31/tags prefix, which the
handler didn't match, so requests fell through to the S3 catch-all and
returned a 405 + HTML body the SDK couldn't deserialize. Match that
prefix and route POST/DELETE/GET to new AWS-local function-tagger
methods.
SQS tag operations returned UnknownOperationException. Add queue tagging
to the AWS provider (stored on QueueInfo.Tags), routed via an AWS-local
queueTagger assertion so the portable MessageQueue driver stays
untouched.
…stackshy#319)

SSM AddTagsToResource returned UnknownOperationException. Add a tags map
to parameter storage plus Tag/Untag/List provider methods, routed via an
AWS-local parameterTagger assertion.
…shy#319)

HEAD /{bucket} returned 405 (no HeadBucket), and PUT /{bucket}?tagging
mis-routed to CreateBucket, failing with BucketAlreadyOwnedByYou. Add a
HEAD case backed by a bucket-exists check and a ?tagging dispatch to the
provider's existing Put/Get/DeleteBucketTagging methods.
…tackshy#319)

Two integration tests still asserted the pre-fix empty-success behavior
for DescribeInstances/DescribeVolumes by a missing ID; align them with
the theme-C InvalidInstanceID/InvalidVolume.NotFound contract.
…y#319)

Both operations were unmatched, so they fell through to the S3 catch-all
and returned a 405 + HTML body the SDK couldn't deserialize (theme D).
Route them: CreateIndex idempotently returns the bootstrapped local
index; GetDefaultView returns the first-created view (AWS auto-associates
it as the account default).
PutRetentionPolicy returned UnknownOperationException. Route it through
the driver's existing UpdateLogGroup, which applies the retention to the
log group.
…ies) (stackshy#319)

PutRolePolicy and the inline-role-policy operations returned
InvalidAction. Store inline policies on the role in the AWS IAM provider
and route the four query-protocol actions via an AWS-local
rolePolicyManager assertion.
ModifyCacheCluster returned InvalidAction. Add ModifyCache to the AWS
provider (updates node type/engine) routed via an AWS-local cacheModifier
assertion so the shared Cache driver (Azure Cache, GCP Memorystore) stays
untouched.
Query applied only the key condition and ignored FilterExpression, so it
returned the full key-matched set — silently wrong data (Scan applied the
same filter correctly). Thread a Filters field through QueryInput, parse
FilterExpression in the query handler, and apply it after the key match.
The list handler never read the max-keys query param and hardcoded MaxKeys
in the response, so ListObjectsV2 returned every key with IsTruncated=false
and no continuation token. Parse max-keys (and the v1 marker) and pass them
to the driver, which already paginated correctly.
…ackshy#319)

The streaming (eventstream) runtime client reused pooled connections
against the httptest server; once a stream was fully consumed, the
reader could observe 'use of closed network connection' instead of a
clean EOF, flaking the Test CI job under load. Disable keep-alives on the
streaming test client so each request gets a fresh, server-closed
connection and the stream ends deterministically.
…shy#319)

Both returned InvalidAction, breaking region and instance-type validation
calls that bootstrap tooling makes. Serve DescribeRegions from a common
region set (or the requested subset) and DescribeInstanceTypes with vCPU/
memory specs for the common types (defaulting for unknown types).
…anceStatus (stackshy#319)

All three returned InvalidAction. CreateNetworkInterface makes a
standalone available ENI in a subnet (VPC resolved from the subnet);
Monitor/UnmonitorInstances validate the instances exist and echo the
monitoring state; DescribeInstanceStatus reports running instances with
passing system/instance checks (IncludeAllInstances covers other states).
…erSubnetGroup (stackshy#319)

Both returned InvalidAction, blocking IaC that provisions a warehouse
with a custom parameter or subnet group. Store the groups in the AWS
Redshift provider and route the two actions via an AWS-local
clusterGroupManager assertion (they're not part of the shared
relationaldb driver).
…hy#319)

The provider already implemented TTL, but the handler didn't dispatch
the two TTL operations, so they returned UnknownOperationException. Wire
them through to the driver's UpdateTTL/DescribeTTL.
…shy#319)

TestDDBTypedErrors asserted UpdateTimeToLive returns UnknownOperation,
which no longer holds now that TTL is routed. Point the assertion at
DescribeContinuousBackups, which remains unimplemented.
SetTopicAttributes returned InvalidAction. Route the DisplayName
attribute through the driver's UpdateTopic; other attribute names
(Policy, DeliveryPolicy) are accepted but not modeled, since the emulator
doesn't evaluate topic policies.
PUT /{bucket}?notification mis-routed to CreateBucket, and object-create
events never propagated. Route the ?notification sub-resource (Put/Get
BucketNotificationConfiguration), store QueueConfigurations on the
bucket, and deliver an S3 ObjectCreated:Put event to matching SQS targets
on upload via an injected SQSDeliverer (same pattern as SNS/EventBridge).
Event-name selectors support exact and ':*' wildcard matches.
…tackshy#319)

Two gaps: invoke returned a FunctionError ('no handler registered') for
every uploaded function since the emulator can't run arbitrary zip code;
and CreateEventSourceMapping (SQS/DDB-stream -> Lambda) returned 405.

Invoke now returns a 200 stub echoing the payload when no Go handler is
registered, so invoke control flow is testable. Route the
/2015-03-31/event-source-mappings collection and per-UUID paths through
the driver's existing ESM methods.
TagResource/UntagResource/ListTagsForResource (and their legacy
TagLogGroup/UntagLogGroup/ListTagsLogGroup aliases) were unimplemented.
Store tags on the log group in the provider and route both the modern
ARN-based and legacy name-based operations via an AWS-local
logGroupTagger assertion.
…stackshy#319)

ECR tag operations returned UnknownOperation. Store tags on the
repository in the provider and route the three ARN-based tag operations
via an AWS-local repositoryTagger assertion.
…rce (stackshy#319)

EventBridge tag operations returned UnknownOperation. Rules carry no tag
field, so back tagging with a generic ARN-keyed store on the provider and
route the three operations via an AWS-local resourceTagger assertion.
…ckshy#319)

IAM role tagging returned InvalidAction. Store tags on the role in the
provider (the field already existed for tag-on-create) and route the
three query-protocol actions via an AWS-local roleTagManager assertion.
)

Redshift resource tagging returned InvalidAction. Back it with an
ARN-keyed store on the provider and route the three query-protocol
actions via an AWS-local resourceTagger assertion.
…s interface (stackshy#319)

Adding CreateNetworkInterface to the NetworkInterfaces driver interface
broke resourcediscovery's read-only ENI walker: its subset type-assertion
(and the test's failingInterfaces fake) no longer satisfied the widened
interface, so interface-listing errors were silently swallowed. Split the
creation method into a separate NetworkInterfaceCreator interface and
type-assert it in the EC2 handler instead.
…ons (stackshy#319)

Adding CreateTags/DeleteTags/DescribeTags to redshiftActions made the
Redshift handler (registered before EC2 and ELBv2) steal those verbs from
EC2 (CreateTags/DeleteTags) and ELBv2 (DescribeTags) on the shared query
protocol. Claim the ambiguous verbs only when the SigV4 credential scope
names 'redshift'; otherwise fall through to the owning handler.
Tags could only be set at create time; AddTags/RemoveTags returned
InvalidAction. Add tag-mutation methods to the ELB provider (updating the
load balancer or target group by ARN) routed via an AWS-local tagMutator
assertion. The empty <FooResult/> wrapper is included so the SDK
deserializes the response.
…tackshy#319)

EKS tag operations at /tags/{arn} fell through to the S3 catch-all.
Match the /tags/ prefix and route the three operations to new provider
methods (resolving the cluster from the ARN) via an AWS-local
clusterTagger assertion.
…tackshy#319)

Route 53 tagging at /2013-04-01/tags/{type}/{id} fell through to the S3
catch-all. Match that prefix and route the two operations to a generic
ID-keyed tag store on the provider via an AWS-local resourceTagger
assertion.
…tackshy#319)

PutRule/DescribeRule/ListRules returned 'arn:aws:events:::rule/<bus>/<name>'
with empty region and account. Thread accountID/region into the handler
so rule ARNs are complete (arn:aws:events:<region>:<account>:rule/...).
…y#319)

CreateCluster left Version empty when the caller omitted it, so
Create/DescribeCluster returned a null version. Default to the latest
supported version (1.29), matching real EKS.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review — closes #319 (comment)

Reviewed the whole PR at 192b938 against the #319 checklist (isolated worktree, whole-repo go test, -race, real-SDK verification, adversarial fan-out). This is a strong, well-tested PR — it delivers the entire High/Med checklist and its dispatch changes are collision-free.

Gate matrix

build / vet / go test ./... (whole repo, 100 files) / -race (all AWS pkgs) / gofmt / go mod tidy — all green. golangci-lint has a few Low PR-introduced hits on the new files (goconst running, two unused-receivers in ec2/metadata.go, two nolintlint) — worth cleaning for the local 0-issue gate.

#319 checklist — delivered

Every High/Med item is implemented and backed by a real-SDK/CLI regression test asserting field values: Theme A tagging (13 services), Theme B (SQS attrs/purge, SetTopicAttributes, ModifyCacheCluster, CreateClusterParameterGroup, PutRetentionPolicy, SetAlarmState, UpdateSecret), Theme C (EC2 Invalid*.NotFound via a shared helper — and Describe-all still returns the full set, no cascade), Theme D (lambda TagResource + resourceexplorer2 now return JSON errors, not HTML 405), event delivery (SNS→SQS, EB→SQS, S3→SQS), silent-data (DynamoDB FilterExpression applied; S3 MaxKeys honored), and all per-service ops. Only ecr SetRepositoryPolicy (rated Low) is deferred.

Dispatch / cascade / duplicate — clean

Cross-checked every query-protocol/JSON handler's Matches + registration order: redshift's SigV4-scope gating is correct and fails closed; CreateNetworkInterface was correctly split off the shared NetworkInterfaces interface (resourcediscovery unaffected); rgt is X-Amz-Target-prefix isolated; no new collisions, no duplicate cases, no delivery double-fire, no lock-ordering deadlock. Event delivery works end-to-end (SNS/EB/S3 → SQS land real messages; tests assert body shape).

Findings (none break #319's stated bugs — fidelity gaps / follow-ups)

Medium

  1. EventBridge event IDs collide for byte-identical events under FakeClockgenerateEventID (eventbridge.go:533, pre-existing) hashes source:detailType:detail:eventBus:UnixNano, so two identical events in one PutEvents under config.FakeClock (which the suite relies on for determinism) get the same EventId — reproduced. Real EventBridge always issues unique IDs; any consumer using EventId as an idempotency/history key misbehaves. This PR newly delivers those events (deliverToTargets), which is what surfaces it. Fold the atomic idgen counter (or the loop index) into the id. (Not inline — the line isn't in this PR's diff.)
  2. S3 notifications fire only on PutObjectCompleteMultipartUpload/CopyObject/DeleteObject never notify, so s3:ObjectRemoved:* can never fire; the PR's own test registers an ObjectRemoved queue but never deletes, so it gives false confidence. The S3→SQS item is only partially delivered. (inline)
  3. Lambda invoke-no-handler stub not mirrored to Azure/GCP — AWS now returns 200+echo while Azure Functions / GCP Cloud Functions still return 500 (CLAUDE.md mirror rule). Mirror it or track a follow-up. (inline)
  4. (Pre-existing, out of scope) RDS's tag-Matches is unscoped and claims generic AddTagsToResource/ListTagsForResource (already worked around from SNS in this PR). Not introduced here and ElastiCache tagging isn't in scope, but a follow-up to scope-gate RDS like redshift would let those services add tagging cleanly.

Low — SNS MessageAttributes dropped end-to-end (not in #319); ecr SetRepositoryPolicy deferred; CreateNetworkInterface missing an unknown-SubnetId negative test; dead code in ssm/runcommand.go after the NotFound fix; the lint hygiene above.

Overall: the checklist is solidly delivered with real regression tests and clean dispatch. Worth addressing before merge, in my view: the S3-notification completeness + the vacuous ObjectRemoved test, and the EventBridge id determinism. The rest are fine as tracked follow-ups.

Comment thread providers/aws/s3/s3.go
m.emitMetric("PutRequests", 1, "Count", dims)
m.emitMetric("BytesUploaded", float64(len(data)), "Bytes", dims)

m.notifyObjectCreated(bkt, bucket, key, int64(len(data)))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — S3 notifications only fire for PutObject. notifyObjectCreated is called here in PutObject and nowhere else — CompleteMultipartUpload (s3.go:702) and CopyObject (:464) create/overwrite objects but never notify, and no delete path fires an ObjectRemoved:* event. So the s3:ObjectRemoved:* selector can never receive anything, and multipart/copy uploads produce no events. The regression test registers an ObjectRemoved queue but never actually deletes, so it doesn't catch this — it only proves the ObjectCreated selector excludes the ObjectRemoved queue. Consider wiring notifyObjectCreated into multipart/copy and adding an ObjectRemoved emit on delete (or explicitly scope S3→SQS to PutObject in the docs + fix the test to delete).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ead94bb. Wired notifyObjectCreated into CompleteMultipartUpload and CopyObject, and added an ObjectRemoved:Delete emit on DeleteObject (refactored the notify path into a generic notify(bkt, bucket, key, size, eventName)). The s3:ObjectRemoved:* selector now actually receives events, and TestBucketNotificationDelivery was made non-vacuous: it now copies (asserts a 2nd ObjectCreated delivery) and deletes (asserts a 3rd delivery carrying "eventName":"ObjectRemoved:Delete" to the deletes queue). Thanks — this was a real completeness gap.

}

return &driver.InvokeOutput{StatusCode: 500, Error: "no handler registered"}, nil
return &driver.InvokeOutput{StatusCode: 200, Payload: payload}, nil

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — parity divergence. The no-handler invoke stub now returns StatusCode: 200 + echoed payload (good for AWS testability), but providers/azure/functions/functions.go:209 and providers/gcp/cloudfunctions/functions.go:208 still return 500 / "no handler registered". CLAUDE.md requires the 3 providers to mirror behavior — identical cross-provider test code now gets different outcomes. Port the same stub to Azure/GCP or track a follow-up.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ead94bb. Ported the same stub to both other providers: the no-handler if h == nil path in providers/azure/functions/functions.go and providers/gcp/cloudfunctions/functions.go now emits success metrics and returns StatusCode: 200 with the echoed payload (defaulting to {} when empty), matching AWS. Updated both provider tests to assert the 200/echo outcome (GCP's table case flipped from wantStatus: 500 to 200). Cross-provider invoke tests now get identical outcomes. Thanks for catching the mirror-rule divergence.

… parity gaps (stackshy#319)

- eventbridge: fold PutEvents index into generateEventID so byte-identical
  events in one call get unique ids under FakeClock
- s3: notify on CompleteMultipartUpload/CopyObject and emit ObjectRemoved:Delete
  on DeleteObject; make the ObjectRemoved test actually delete and assert
- lambda parity: mirror the no-handler 200+echo invoke stub to Azure Functions
  and GCP Cloud Functions
- rds: scope-gate AddTags/RemoveTags/ListTags Matches to the rds SigV4 scope
- sns: carry MessageAttributes through Publish -> SQS envelope
- ecr: implement Set/Get/DeleteRepositoryPolicy
- ec2: lint hygiene (stateRunning const, receiver/wsl cleanups) and a
  CreateNetworkInterface unknown-SubnetId negative test
@thzgajendra

Copy link
Copy Markdown
Collaborator Author

Thanks for the deep review. Addressed every finding (including the ones you rated Low / out-of-scope) in ead94bb — full local gate green (build/vet/go test ./.../tidy/golangci-lint on the new files/CodeQL at the 8-alert baseline, no new alerts):

Medium

  1. EventBridge id collisionsgenerateEventID now takes the PutEvents loop index and folds it into the hash (source:detailType:detail:eventBus:UnixNano:index), so byte-identical events in one call get unique ids under FakeClock. Added a subtest asserting EventIDs[0] != EventIDs[1].
  2. S3 notification completeness — see inline reply (multipart/copy notify + ObjectRemoved:Delete on delete + non-vacuous test).
  3. Lambda parity — see inline reply (stub mirrored to Azure/GCP).
  4. RDS unscoped tag-Matches — scope-gated AddTagsToResource/RemoveTagsFromResource/ListTagsForResource behind sigV4ScopeService(...) == "rds", same fail-closed pattern as redshift.

Low

  • SNS MessageAttributes — now carried end-to-end through the Publish → SQS envelope as {name: {"Type":"String","Value":v}}; regression test asserts they survive the hop.
  • ECR SetRepositoryPolicy — implemented Set/Get/DeleteRepositoryPolicy (provider + wire handler + dispatch), verified with an SDK round-trip test.
  • CreateNetworkInterface negative test — added TestCreateNetworkInterfaceUnknownSubnet asserting InvalidSubnetID.NotFound.
  • Lint hygienestateRunning const (goconst), two ec2/metadata.go receivers → (*Handler), two wsl range-cuddle fixes.
  • ssm/runcommand.go "dead code" — I couldn't reproduce this one. staticcheck + golangci-lint's unused/deadcode linters find nothing, and the writeErr(w, err) fallthrough after the IsNotFound branch in sendCommand is genuinely reachable — it handles the provider's InvalidArgument errors (empty InstanceIds/DocumentName), which the NotFound branch doesn't cover. If you meant a specific line, could you point me at it? Happy to remove it if I'm missing something.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review — closes #319 (approve)

Re-reviewed at ead94bb, the follow-up commit that addresses my earlier review. It resolves every finding I raised, each with a real regression test, and the full deep-test matrix is green. Approving.

Gates — all green

Pass Result
go build / go vet / gofmt clean
go test ./... (whole repo) 225 pkgs ok, 0 fail
go test -race ./... (whole repo) 89 pkgs, 0 data races, 0 fail
-count=3 on FakeClock-sensitive pkgs no flakes
golangci-lint no PR-introduced issues (prior ec2 flags now fixed via a stateRunning const + (*Handler) receivers)

Prior findings — all fixed, each with a passing test

  • EventBridge id collisiongenerateEventID now folds in the batch index; TestPutEvents/byte-identical_events_in_one_call_get_unique_ids asserts distinct EventIds under FakeClock. (generateEventID 100% cov)
  • S3 notifications only on PutObject → refactored to a notify() helper; DeleteObjectObjectRemoved:Delete, CopyObject/CompleteMultipartUploadObjectCreated. Test asserts a real "eventName":"ObjectRemoved:Delete" delivery to the :deletes queue. (notify* 100% cov)
  • Lambda stub not mirrored → Azure Functions + GCP Cloud Functions now return 200+echo with metrics; both providers' no_handler sub-tests flipped to the success stub.
  • RDS unscoped tag-Matches → now scope-gated by sigV4ScopeService(...) == "rds" (mirrors redshift, fails closed); TestSDKRDSTagging green.
  • SNS MessageAttributes dropped → parsed server-side and carried into the SQS envelope end-to-end; TestPublish/with_attributes + TestSNSToSQSDelivery green.
  • ECR SetRepositoryPolicy (deferred Low) → implemented; server honestly type-asserts an AWS-specific repoPolicyManager (not the portable driver); persistence is correct (repos stores *repoData, mutated in place like PutImage). TestSDKECRRepositoryPolicy (real SDK roundtrip) green.

Dispatch is collision-free (RDS gating is strictly safer than before; ECR policy verbs dispatch by X-Amz-Target with an Unimplemented fallback; driver additions are additive and compile across all 3 providers). No cascade, no duplicate delivery.

Optional follow-ups (all Low, non-blocking)

  • providers/aws/s3/s3.go:911 DeleteObjectVersion (versioned delete) doesn't fire ObjectRemoved — the one object path left uncovered.
  • CopyObject/CompleteMultipartUpload emit ObjectCreated:Put rather than real S3's ObjectCreated:Copy / :CompleteMultipartUpload — only matters for buckets filtering on those exact sub-types; ObjectCreated:* matches fine.
  • Test-placement: provider-level ecr repo-policy methods are 0% in their own package (covered only via the server roundtrip), pulling provider ecr to 85.5%; parseMessageAttributes (27.3%) and sigV4ScopeService fail-closed branches are lightly covered. A couple of small direct unit tests would close these.

Solid work — the #319 checklist is fully delivered end-to-end with real SDK-level regression tests. LGTM.

@thzgajendra
thzgajendra merged commit 51f2490 into stackshy:development Aug 4, 2026
16 of 17 checks passed
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.

AWS parity E2E audit: bugs, gaps & enhancements across all 30 services

2 participants