[bulk][split-218 #82] Warn if custom role lacks minimum permissions (bu-bu4) - #35
Closed
relentlesscol wants to merge 20 commits into
Closed
[bulk][split-218 #82] Warn if custom role lacks minimum permissions (bu-bu4)#35relentlesscol wants to merge 20 commits into
relentlesscol wants to merge 20 commits into
Conversation
Add a full CI pipeline that runs on every push to main and on PRs: ## Workflow chain (sequential via workflow_run triggers) unit tests (push+PR) → connector smoke → e2e fill/copy/update/delete/diff - Unit tests: Python 3.11, --ignore=tests/e2e, runs on PRs for fast feedback - Connector smoke: read-path validation (count/find/sql/load) via OIDC auth - E2e commands: one workflow per command, fan out in parallel after smoke - Flake retry: auto-reruns a failed e2e once; alerts only on 2nd failure ## Infrastructure scripts - e2e-bootstrap.sh: create OIDC provider + IAM role + secrets in one command - e2e-teardown.sh: clean removal of all CI infrastructure from an account - e2e-switch-account.sh: interactive teardown-old + bootstrap-new ## IAM permissions (least-privilege for the github-actions-e2e-runner role) - DynamoDB: CRUD on named test tables + Create/Delete for bulk-e2e-* transient tables - Glue: Start/Get job runs on bulk_dynamodb - CloudWatch Logs: DescribeLogGroups + StartLiveTail (CLI streams Glue output) - S3: read/write on aws-glue-bulk-dynamodb-* bucket - IAM: PassRole to glue.amazonaws.com for AWSGlueServiceRole* - STS: GetCallerIdentity (account guard) ## Security - All AWS credentials via OIDC (no long-lived keys) - Account ID and table names stored as GitHub Secrets - OIDC trust scoped to specific repos + main branch only - Security suite excluded from CI (requires admin, mutates shared job) Tested end-to-end on relentlesscol/amazon-dynamodb-tools fork — full pipeline green including all 5 e2e command tests.
When using --s3 mode, diff_segment() was unconditionally writing a file per segment to S3 via put_object, even when no differences were found in that segment. This produced many empty 0-byte files cluttering the output bucket and adding unnecessary S3 API calls. Guard the put_object call with an emptiness check on the diff list. The return value (len(diff) == 0) is preserved so the aggregated count in run() still correctly reports "No differences found" when all segments are empty. Closes awslabs#183.
…dules.zip (#17) * test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * Exclude __pycache__, .DS_Store, and dev cruft from python_modules.zip The module_zipper bundled everything under the source tree including bytecode caches, OS metadata files, and egg-info directories. Add exclusion lists for directories (__pycache__, .pytest_cache, .git, *.egg-info) and files (.DS_Store, Thumbs.db, .pyc, .pyo) that should never ship in the deployment zip. Fixes awslabs#174
The load command mutates table data but did not verify that Point-In-Time Recovery was enabled before proceeding. Other table-mutating commands (copy, update, fill) already pass pitr_enabled=True to validate_tables(), making this an oversight rather than a design choice. Without PITR, a failed or incorrect load has no recovery path — the user loses data with no automatic backup to restore from. Adding the flag ensures validate_tables() checks PITR status and exits with a clear error if it's disabled, matching the safety behavior of peer commands. Closes awslabs#179.
#19) * test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * Warn if custom --XRole lacks minimum required permissions (awslabs#82) When a custom role is specified via --XRole, use simulate_principal_policy to check a representative set of required actions (DynamoDB, S3, CloudWatch, pricing, service quotas). Emit a warning listing any denied actions without blocking execution — the user may have grants we cannot detect (inline policies, resource-based policies, etc.).
) * test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * feat: add poison-pill early-exit for non-recoverable systemic errors (bu-i83b) When a Spark worker encounters a non-recoverable systemic error (AccessDeniedException, ValidationException, ExpiredTokenException, ResourceNotFoundException, ModuleNotFoundError, OutOfMemoryError), it writes a poison-pill marker to S3. All other workers check for this marker between scan pages and abort early instead of continuing to scan the entire table — which previously wasted minutes of compute and DynamoDB read capacity against a job that was guaranteed to fail. The mechanism uses the existing S3 bucket (already available for rate limiting) with a per-job-run key under server/poison-pill/. Workers rate-limit the HEAD check to once per 5 seconds to avoid S3 cost. The driver cleans up the marker in its finally block. The poison_pill_config parameter defaults to None in worker functions so existing test call-sites remain backwards-compatible. When None, a no-op implementation is used that never checks or signals. Affected verbs: copy, update, scancount, find (delete path).
…#181) (#24) The log.info call in RateLimiterAggregator.__init__ was confusing because it printed at INFO level during normal operation. Downgrade to debug. Also fix the format string which was printing bucket for both Bucket and Prefix fields. Fixes awslabs#181
When the load command begins writing, log the write rate (WCU/s) being used and whether it was explicitly set by the user via --XMaxWriteRate or automatically determined by the DynamoDB connector. This addresses issue awslabs#182 — users had no visibility into what write throughput the load operation would consume. The message appears after the cost estimate and before the actual write begins, so operators can abort if the rate is unexpected.
The find verb previously serialized records through an intermediate
spark.read.json(records.toJSON()) step before writing to S3. This
re-inferred the schema from JSON strings, losing type fidelity for
DynamoDB complex types (maps, lists, sets, binary, boolean, null).
Numbers could also lose precision during the re-inference pass.
Replace the toJSON→read.json→write.json pipeline with a direct
records.write.mode('overwrite').json(location) call, which serializes
the DataFrame using its existing Spark schema — exactly the schema the
Glue DynamoDB connector produced on read. This preserves all attribute
types through the find→S3→load round trip.
Remove the now-unused SparkSession import (it was only needed for the
intermediate spark.read.json() call).
Tested: 1252 unit tests pass (3 new tests validate the direct-write
path and absence of the re-inference step).
Closes awslabs#184
* test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * Remove redundant TableName from scan_kwargs in update module Table.scan() is called on a boto3 Table resource that already knows its table name. Passing TableName is dead code that could mask bugs if the variable and resource ever diverge.
) * test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * Show clean error messages for bad parameters instead of stack traces Wrap the driver's main execution in try/except for ClientError and BotoCoreError so users see "Error: AccessDeniedException — <message>" rather than a full Python traceback when e.g. they pass an S3 bucket they don't own or a table they can't access. Fixes awslabs#137
* test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * feat(bootstrap): force built-in role refresh on version mismatch (bu-i84) Previously, _add_glue_job_role returned early when a role already existed, never updating its policies. New tool versions that add required permissions (e.g. quotas_policy) were silently ignored for users who had already bootstrapped, causing permission-missing errors after upgrades. Now built-in roles are tagged with a BulkDynamoDBVersion tag. On each bootstrap run, the stored tag is compared against __version__; on mismatch (or missing tag), all managed policies and inline policies are re-applied and the tag is updated. This makes policy state converge to what the current version expects without requiring users to manually delete and recreate roles. Custom roles (--XRole <name>) are exempt from version tracking and policy management since they are user-controlled.
* test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * feat: add --XIdleTimeout for Glue cost optimization (bu-i88) AWS Glue charges by DPU-minute. When workers finish processing but the job stays alive until the overall Timeout expires, the idle workers still incur cost. The IdleTimeout parameter (added to Glue's start_job_run API) causes the job to auto-terminate when all workers have been idle for the specified duration. Adds --XIdleTimeout (1-10080 minutes, default 5) as a configurable parameter following the existing XTimeout pattern. The 5-minute default balances cost savings against premature termination during inter-phase gaps in multi-step ETL jobs. Users processing small tables will see significant cost reduction since jobs no longer idle for the remaining 55 minutes of the default 60-minute timeout. The parameter is passed to start_job_run only (not create_job/update_job) because IdleTimeout is a run-level setting in the Glue API.
* test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * Add rate validation warnings for XMaxReadRate/XMaxWriteRate Warn users when their configured read/write rate is: - Too high: exceeds table provisioned capacity or on-demand limit - Too low: below minimum recommended or would make job unreasonably slow Includes suggested rate ranges based on actual table capacity. Fixes awslabs#89
* test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * perf: install faker only for fill verb to speed worker startup (bu-i91) Worker logs showed 8-15 seconds spent installing faker via pip on every Glue job run, even for verbs (copy, find, delete, etc.) that never use it. The faker dependency was baked into the Glue job's DefaultArguments as --additional-python-modules, causing unconditional installation at worker spin-up. Move faker out of the global _THIRD_PARTY_PYTHON_MODULES list (which populates DefaultArguments at bootstrap time) and into a new VERB_PYTHON_MODULES dict keyed by verb name. The runner now injects --additional-python-modules into per-run Arguments only when the action matches a verb with extra dependencies (currently only 'fill'). This eliminates the pip install overhead for all other verbs while preserving fill's access to faker. Validated: 479 existing client+fill tests pass; manual assertions confirm faker is injected per-run for fill and omitted for copy/find.
* test: cover JSON-array and nested-braces cases for _jsonify_message Addresses review feedback on PR awslabs#171 (coverage was thin — one happy-path test). Adds the second regex alternation branch ([...]) and a nested-object case. Full suite: 1275 passed, 99.7% line / 96.8% branch coverage. Co-authored-by: dark-factory-agent * feat: add --XExistingBucket parameter to bootstrap (GH#130) Allow users who cannot grant bucket-creation permissions to pass an existing S3 bucket to bootstrap. The bucket is validated for existence and accessibility before proceeding. Bucket creation is skipped but policy application and file uploads still occur.
Adds a --per-segment mode to the scancount command that prints item counts per DynamoDB scan segment instead of only the total. This makes it easy to spot partition skew — e.g. one segment holding 5M items while others have 1K. When --per-segment is set, after the normal total-count run completes, a second pass uses rdd.map().collect() to gather (segment, count) tuples. Results are printed sorted by count descending with percentage-of-total and basic statistics (mean, skew ratio). A warning is emitted when the max/mean ratio exceeds 5x, indicating significant hot-partition risk. The implementation adds a _count_segment() helper (lighter than _count_data — no accumulator side-effects) used by the per-segment collection path. The existing _count_data path and accumulator-based total are unchanged.
… count Addresses PR awslabs#190 review feedback: 1. Add --segments parameter (default 200) so users can control parallelism 2. Document --per-segment and --segments in README.md Refs: awslabs#92
…s#82) The validate_role argparse validator previously rejected any role name that didn't start with "AWSGlueServiceRole", making custom roles only reachable through the interactive menu. Move validate_role to utils/ (alongside other validators) and accept any non-empty string — the actual existence check and permission warning happen downstream in _get_role_name and _check_custom_role_permissions.
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
1 participant
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.
Summary
Issue awslabs#82. Warn if custom --XRole lacks required perms. Files: client/src/utils/. Must pass make test.
Implementation notes
Implemented: allow custom role names via --XRole CLI flag; moved validate_role to utils/, accept any non-empty string (existence+perm check is downstream)
Refinery handoff
bu-bu4(task, P2)polecat/bu-bu4mainmainvia Gastown Refinery.