Add IVS enhanced logging via EventBridge#10
Conversation
Replaces raw Manifest::get('aws.ivs') boolean checks with the new
isIvsSupported() method and uses explicit (bool) cast.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- UsesEventBridge: catch EventBridgeException from describeRule instead of unreachable empty check - SyncEventBridgeTargetStep: handle missing rule gracefully on dry-run - SyncCloudWatchLogGroupStep: check and update retention policy on existing log groups Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When listTargetsByRule throws because the rule doesn't exist yet, fall through to the creation logic instead of returning CREATED without actually creating the target. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stevethomas
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: ❌ Changes Requested
Risk Assessment
| Category | Status |
|---|---|
| Data Integrity | ✅ |
| Security | |
| Breaking Changes | ✅ |
| Operational | ❌ |
| Test Coverage |
Findings
❌ Blockers
src/Steps/Ivs/SyncCloudWatchLogGroupStep.php:59 — Missing CloudWatch Logs resource policy. EventBridge requires a resource-based policy on the log group granting logs:CreateLogStream and logs:PutLogEvents to events.amazonaws.com. Without this, putTargets succeeds but events silently fail to deliver — the entire IVS logging pipeline will appear configured but produce zero logs.
In src/Steps/Ivs/SyncCloudWatchLogGroupStep.php, after creating the
log group and setting its retention policy, add a
putResourcePolicy call granting events.amazonaws.com permission
to write to the log group. Apply on both create AND sync paths.
See EventBridge → CloudWatch Logs target docs.
src/Steps/Ivs/SyncEventBridgeTargetStep.php:34 — Target drift: if the target exists but the ARN has changed (e.g. log group rename), the step returns SYNCED without updating. Events would route to a stale log group.
In SyncEventBridgeTargetStep, instead of just checking the target
ID, also verify the ARN matches. If the ID exists but ARN differs,
fall through to putTargets to update it.
src/Steps/Ivs/SyncCloudWatchLogGroupStep.php — No unit tests for the 3 new IVS steps. Consistent with repo patterns (only arch tests exist), but these steps have real branching logic that would benefit from coverage. Not blocking since it's a repo-wide gap.
🤖 Reviewed by Claude Code
- Add CloudWatch Logs resource policy granting events.amazonaws.com
write access (blocker: without this EventBridge silently fails to deliver)
- Fix target drift in SyncEventBridgeTargetStep by verifying ARN match
- Remove SyncIvsCommand, fold IVS steps into SyncIamCommand
- Replace Manifest::isIvsSupported() with inline Manifest::has('aws.ivs')
- Fix alphabetical ordering in Aws.php (elb before eventbridge)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Manifest::has() only checks if the key exists via Arr::has(), so ivs: false would still pass the guard and provision resources. Switch to Manifest::get() which returns the actual value — falsy for false/null/missing, truthy for true or config objects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stevethomas
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: ✅ Approved (with warnings)
Context
Found and fixed the Manifest::has vs Manifest::get bug during review — ivs: false would have bypassed the guard and provisioned resources. Fix pushed in dde6e2f.
Risk Assessment
| Category | Status |
|---|---|
| Data Integrity | |
| Security | ✅ |
| Breaking Changes | ✅ |
| Operational | |
| Test Coverage |
Findings
src/Steps/Logging/SyncIvsEventBridgeTargetStep.php:41 — catch (EventBridgeException) swallows ALL errors (throttling, auth, network), not just "rule not found". A permissions error gets silently ignored and putTargets fails with a misleading error instead.
In SyncIvsEventBridgeTargetStep.php line 41, check
$e->getAwsErrorCode() for ResourceNotFoundException
specifically and re-throw anything else.
src/Steps/Logging/SyncIvsCloudWatchLogGroupStep.php:82 — CloudWatch Logs has a hard limit of 10 resource policies per region per account. Each YOLO app creates its own policy via keyedResourceName. Multiple apps with IVS in the same account will exhaust this. Consider a shared policy name or read-then-merge approach.
Check whether the per-app policyName in putResourcePolicy()
will scale within the 10-per-account CloudWatch Logs resource
policy limit. Consider a shared policy that merges log group ARNs.
src/Steps/Logging/SyncIvsCloudWatchLogGroupStep.php:89 — Resource policy lacks aws:SourceAccount condition — any EventBridge rule in any account could write to the log group.
Add Condition => StringEquals => aws:SourceAccount to the
resource policy to prevent cross-account confused deputy.
📝 Notes
describeLogGroupsuses prefix matching without pagination — if 50+ log groups share the prefix, exact match on page 2+ would be missed. Low risk with/aws/ivs/prefix but worth noting for the generic trait.putResourcePolicyandputRuleare called on every sync even when nothing changed — idempotent but noisy.putRulecall is duplicated between try/catch branches inSyncIvsEventBridgeRuleStep— could extract to reduce duplication.- No unit tests for the 3 new step classes. The
has/getbug demonstrates the kind of issue tests would catch. Project convention is arch tests only for steps, but consider adding coverage for the branching logic.
🤖 Reviewed by Claude Code
| if ($existingTarget && $existingTarget['Arn'] === $expectedArn) { | ||
| return StepResult::SYNCED; | ||
| } | ||
| } catch (EventBridgeException) { |
There was a problem hiding this comment.
Finding: catch (EventBridgeException) swallows ALL EventBridge errors — throttling, auth, network — not just "rule not found". A permissions error would be silently ignored, and putTargets below would fail with a confusing error instead of surfacing the real cause.
Severity: WARNING
Suggested Fix
Check $e->getAwsErrorCode() for ResourceNotFoundException specifically and re-throw anything else so real errors (throttling, permissions) surface immediately.
Prompt
In src/Steps/Logging/SyncIvsEventBridgeTargetStep.php line 41,
the catch block catches all EventBridgeException types. Change
it to check $e->getAwsErrorCode() === 'ResourceNotFoundException'
and re-throw any other exception type so throttling, auth, and
network errors surface with their real error message.
Single policy name (yolo-ivs-eventbridge-policy) with wildcard resource ARN /aws/ivs/* instead of per-app policies. Avoids hitting the 10 resource policies per account limit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
EventBridge rule captures all aws.ivs events account-wide, so the rule, log group, and resource policy should all be shared (exclusive: false) rather than per-app. Renamed log group from live-events to ivs-logs for consistency with the ivs-* prefix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only the resource policy needs to be shared (10-per-account limit). Rule and log group stay exclusive so each app gets isolated IVS logging. Log group simplified to base keyed name under /aws/ivs/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follows the existing codebase pattern where steps catch ResourceDoesNotExistException and AWS SDK exceptions are caught/converted in the Uses* concern traits. Adds eventBridgeRuleTargets() to UsesEventBridge trait. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests verify steps skip when IVS is not configured or explicitly disabled, and that resource names follow naming conventions. Also simplifies target step to use AwsResources::eventBridgeRule() as guard instead of catching raw EventBridgeException. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Limited guard-only coverage isn't worth maintaining as a standalone test file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests keyedResourceName (exclusive/non-exclusive, separators, suffixes) and payloadHasDifferences (nested diffs, missing keys, type mismatches, extra keys ignored). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test files no longer need their own beforeAll setup. Pest.php creates a temp yolo.yml and binds the environment globally. writeManifest() available for tests that need custom config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers Aws::tags() (both output formats), Manifest has/get/apex/ tenants/timezone/multitenancy (including edge cases and validation), and Paths building (base, yolo, build, artefact, yoloDir, logDir). Bootstrap uses testing environment — no AWS clients instantiated, fully isolated via temp manifest in /tmp. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hey, I made a thing! 🥳
Great! Now please answer the following questions to help out your assigned reviewer:
What problems are you solving?
Is there anything the reviewer needs to know to deploy this?
aws.logging.ivs: trueto theiryolo.ymlto enable ityolo sync:logging <environment>aws.logging.ivs.log-retention-daysconfig (defaults to 14 days)SKIPPEDwhen IVS is not enabledManifest::get()(truthiness check) soivs: falsecorrectly disables the featureConfig example:
Or minimal:
What does it provision?
/aws/ivs/{keyed-name}) with configurable retentionevents.amazonaws.comwrite accessaws.ivseventsArchitecture:
sync:loggingcommand (follows Network/Storage/Compute/IAM pattern)Steps/Logging/withIvsprefix (e.g.SyncIvsCloudWatchLogGroupStep)UsesCloudWatchLogsandUsesEventBridgeconcerns for resource lookupsCoauthor Jarvis 🤖
🤖 Generated with Claude Code