Skip to content

fix: honor --s3-root-uri and --conflict-resolution in attachment CLI - #1292

Open
crowecawcaw wants to merge 7 commits into
aws-deadline:mainlinefrom
crowecawcaw:review-fix/attachment-args
Open

fix: honor --s3-root-uri and --conflict-resolution in attachment CLI#1292
crowecawcaw wants to merge 7 commits into
aws-deadline:mainlinefrom
crowecawcaw:review-fix/attachment-args

Conversation

@crowecawcaw

@crowecawcaw crowecawcaw commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes:

What was the problem/requirement? (What/Why)

Two arg-plumbing bugs in the attachment CLI group:

  • attachment download raised a RuntimeError in the config-defaults workflow: --conflict-resolution was not stripped from the args in the else-branch cleanup of _apply_cli_options_to_config, so a leftover {"conflict_resolution": None} reached the if args: raise RuntimeError(...) guard.
  • --s3-root-uri was ignored/overwritten unless --profile was also passed, because the no-profile path unconditionally recomputed the S3 root URI.

What was the solution? (How)

  • Add conflict_resolution to the standard-option cleanup list in _apply_cli_options_to_config.
  • Guard the S3-root-URI recompute with if not s3_root_uri: in both attachment download and attachment upload (same shared option, same fix) so an explicit --s3-root-uri is honored regardless of --profile.

What is the impact of this change?

attachment download works in the config-defaults workflow, and --s3-root-uri is respected on its own.

How was this change tested?

Added red-green tests: the config-defaults download no longer raises, and --s3-root-uri is passed through without --profile for both download and upload.

  • Have you run the unit tests? Yes — new test_cli_attachment_args.py (3 tests) plus regression runs of test_cli_common.py/test_cli_job.py.
  • Have you run the integration tests? No.

Was this change documented?

  • No public-contract change; behavior now matches documented intent.
  • README.md not affected.

Does this PR introduce new dependencies?

  • This PR adds one or more new dependency Python packages.
  • This PR does not add any new dependencies.

Is this a breaking change?

No.

Does this change impact security?

No. (The separate manifest-upload disclosure / CAS-poisoning concern (C4) is out of scope and tracked separately.)

Testing

Automated coverage (test/unit/deadline_client/cli/test_cli_attachment_args.py, 9 tests, all invoking the real CLI via CliRunner):

  • --conflict-resolution omitted in the config-defaults workflow no longer trips the _apply_cli_options_to_config RuntimeError (download).
  • --conflict-resolution SKIP is threaded through to the _attachment_download call (not just consumed).
  • Explicit --s3-root-uri is honored without --profile for both download and upload, including when it would otherwise be overwritten by the queue's settings.
  • Explicit --s3-root-uri works even when the queue has no jobAttachmentSettings (both download and upload).
  • Failure mode preserved: with no --s3-root-uri and no queue attachment settings, the command still fails with MissingJobAttachmentSettingsError and never reaches the download.

Manual verification (scratch script driving deadline attachment download/upload end-to-end through click with mocked deadline/S3 clients) confirmed all of the above scenarios pass, including that the explicit URI (s3://manual-check-bucket/...) wins over the queue-provided bucket.

End-to-end S3 verification (moto): two additional tests run attachment download/upload with no job-attachments internals mocked — the queue-role credential path (AssumeQueueRoleForUser), real manifest decoding, and the real S3 transfer code all execute against moto S3 with both the queue's configured bucket and the explicitly-named bucket created. They prove the transfer actually touches the explicit bucket: download reads its content instead of a same-CAS-key decoy planted in the queue's bucket, and upload writes the CAS object + manifest to the explicit bucket while the queue's bucket stays empty.

The only thing still not covered by automation is real-AWS IAM: whether a given queue role's policy actually grants access to a caller-chosen bucket. That's an account-configuration concern rather than client-code behavior, so no hands-on testing of this change is required.

When 'deadline attachment download' runs without --profile and without an
explicit --conflict-resolution (relying on config defaults), all CLI options
passed to _apply_cli_options_to_config are None, so the else-branch cleanup
runs. That branch did not pop 'conflict_resolution', leaving it in args and
tripping the 'not standard AWS Deadline Cloud CLI options' RuntimeError guard.
Add 'conflict_resolution' to the cleanup list so it is consumed.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
'deadline attachment download'/'upload' unconditionally overwrote s3_root_uri
with the queue's job-attachment settings whenever --profile was not passed,
discarding any explicit --s3-root-uri. Only fall back to the queue settings
when --s3-root-uri was not provided.

Also adds arg-plumbing regression tests covering both the --conflict-resolution
config-defaults workflow and the --s3-root-uri handling.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@github-actions github-actions Bot added the waiting-on-maintainers Waiting on the maintainers to review. label Jul 21, 2026
Comment thread src/deadline/client/cli/_groups/attachment_group.py
Comment thread src/deadline/client/cli/_groups/attachment_group.py
Move the MissingJobAttachmentSettingsError guard inside the s3_root_uri
fallback so an explicitly-supplied URI is honored even when the queue has no
jobAttachmentSettings.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw
crowecawcaw marked this pull request as ready for review July 22, 2026 17:08
@crowecawcaw
crowecawcaw requested a review from a team as a code owner July 22, 2026 17:08
andychoquette
andychoquette previously approved these changes Jul 22, 2026
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
…e modes

Add coverage for:
- --conflict-resolution SKIP reaching the _attachment_download call
- explicit --s3-root-uri on upload when the queue lacks jobAttachmentSettings
- MissingJobAttachmentSettingsError still raised when neither an explicit
  --s3-root-uri nor queue attachment settings are available

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
# Only fall back to the queue's S3 settings when the caller did not provide an
# explicit --s3-root-uri. An explicitly-supplied value must always be honored,
# even when the queue has no attachment settings of its own.
if not s3_root_uri:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new comment says an explicit --s3-root-uri must be honored "even when the queue has no attachment settings of its own", but get_queue(...) (lines 126-130) is still called unconditionally before this check. So when a caller supplies --s3-root-uri precisely to operate without relying on queue settings, they still incur a GetQueue call — and if that call fails (e.g. AccessDenied, or the queue does not exist), the command errors out before ever reaching this branch, defeating the intent. Consider moving the get_queue(...) lookup inside the if not s3_root_uri: block so it is only performed when the fallback is actually needed.

# Only fall back to the queue's S3 settings when the caller did not provide an
# explicit --s3-root-uri. An explicitly-supplied value must always be honored,
# even when the queue has no attachment settings of its own.
if not s3_root_uri:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same as in attachment_download: get_queue(...) (lines 244-248) is still called unconditionally, so an explicit --s3-root-uri does not actually let a caller bypass the queue lookup. If GetQueue fails (AccessDenied, missing queue), upload errors out before reaching this fallback branch, contradicting the "must always be honored, even when the queue has no attachment settings" comment. Move the get_queue(...) call inside if not s3_root_uri:.

Add two end-to-end tests that run the attachment download/upload CLI with no
job-attachments internals mocked: the queue-role credential path
(AssumeQueueRoleForUser via the deadline mock), real manifest decoding, and
the real S3 transfer code all execute against moto S3. Both the queue's
configured bucket and the explicitly-named bucket exist; the tests prove the
transfer touches the explicit bucket (download reads its content instead of a
same-key decoy in the queue bucket, upload writes CAS data there and leaves
the queue bucket empty).

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-maintainers Waiting on the maintainers to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants