Skip to content

Conversation

sandydoo
Copy link
Member

@sandydoo sandydoo commented Aug 9, 2025

dynamodb requires auth headers.
Using the awscli is the best I could come up with

Fixes #2084.

@sandydoo sandydoo added the bug Something isn't working label Aug 9, 2025
Copy link
Contributor

github-actions bot commented Aug 9, 2025

🔍 Suggested Reviewers

Based on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:

  • @shyim - 62.5% of changed lines (5 lines)
  • @domenkozar - 37.5% of changed lines (3 lines)

Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏

This comment was automatically generated by git-blame-auto-reviewer

Last updated: 2025-08-11T12:05:48.530Z

Copy link
Contributor

claude bot commented Aug 9, 2025

Claude finished @sandydoo's task —— View job


PR Review: DynamoDB Health Check Fix

I've analyzed this PR that fixes DynamoDB health check issues by replacing curl with AWS CLI calls that include proper authentication headers.

Todo List

  • Read and analyze all changed files
  • Review the DynamoDB health check implementation
  • Check test coverage and example updates
  • Provide specific feedback on code quality and potential issues

Code Review Findings

What was fixed:

The DynamoDB Local health check was previously using a simple HTTP curl request, but DynamoDB requires proper AWS authentication headers even for local instances. This fix switches to using AWS CLI with dummy credentials.

Issues and areas for improvement:

1. Authentication Security (Low Priority)

  • Lines 43-44 in src/modules/services/dynamodb-local.nix: Using hardcoded dummy credentials is acceptable for local development, but consider using a more obvious placeholder like fake-access-key-id to make it clearer these are dummy values.

2. Timeout Configuration

  • Line 48 in src/modules/services/dynamodb-local.nix: The timeout increased from 2 to 5 seconds. This seems reasonable given AWS CLI has more overhead than curl, but consider documenting why this increase was needed.

3. Error Handling

  • Line 44 in src/modules/services/dynamodb-local.nix: The >/dev/null 2>&1 suppression is appropriate for readiness probes, but it may make debugging harder when the health check fails. Consider logging failed attempts to a separate log file for troubleshooting.

4. Dependency Management

  • Line 44 in src/modules/services/dynamodb-local.nix: Adding awscli2 as a dependency increases the closure size. This is unavoidable given DynamoDB's authentication requirements, but it's worth noting the impact.

5. Consistency Issues

  • Lines 10 vs 44: The test script uses http://127.0.0.1:8000 while the old health check used http://127.0.0.1:${toString cfg.port}. Both are now consistent, which is good.
  • Lines 10 vs 44: The test script doesn't suppress output while the health check does. This inconsistency is probably fine since they serve different purposes.

6. Test Coverage Gap

  • The example test script in examples/dynamodb-local/.test.sh now properly tests the actual DynamoDB functionality instead of just port availability, which is an improvement. However, it doesn't test failure scenarios.

7. Process Dependency

  • Line 8 in examples/dynamodb-local/.test.sh: Changed from wait_for_port 8000 to wait_for_processes. This is more robust since it waits for the actual readiness probe to pass rather than just the port to be open.

Performance Considerations

  • AWS CLI has more startup overhead than curl, but this is acceptable for health checks that run every 10 seconds
  • The increased timeout from 2 to 5 seconds acknowledges this overhead appropriately

Security Assessment

  • Using dummy credentials for local DynamoDB is standard practice and secure
  • No actual security credentials are exposed

Overall Assessment

This is a solid fix that addresses the core issue correctly. The approach of using AWS CLI with dummy credentials is the standard solution for local DynamoDB health checks. The changes are minimal, focused, and improve reliability.


Copy link

cloudflare-workers-and-pages bot commented Aug 9, 2025

Deploying devenv with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8c6e37e
Status:🚫  Build failed.

View logs

@sandydoo sandydoo changed the title dynamodb: fix dynamodb health-check dynamodb: fix dynamodb health check Aug 9, 2025
@domenkozar domenkozar marked this pull request as ready for review August 9, 2025 20:23
@domenkozar
Copy link
Member

Waiting for process-compose processes to be ready (timeout: 120 seconds)...

  • readiness_probes='{"dynamodb":true}'
  • process_compose_wait=/nix/store/jmq0qn4mp2j30p97jzgi9qklcpbnwyr1-process-compose-wait
  • timeout 120 /nix/store/jmq0qn4mp2j30p97jzgi9qklcpbnwyr1-process-compose-wait
    ✓ All processes are ready
  • aws dynamodb list-tables --endpoint-url http://localhost:8000

Could not connect to the endpoint URL: "http://localhost:8000/"

@domenkozar domenkozar merged commit 5481802 into main Aug 11, 2025
263 of 278 checks passed
sandydoo pushed a commit that referenced this pull request Aug 21, 2025
dynamodb: fix dynamodb health check
sandydoo pushed a commit that referenced this pull request Aug 21, 2025
dynamodb: fix dynamodb health check
sandydoo pushed a commit that referenced this pull request Aug 21, 2025
dynamodb: fix dynamodb health check
sandydoo pushed a commit that referenced this pull request Aug 21, 2025
dynamodb: fix dynamodb health check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamodb local health check always fails
3 participants