Skip to content

fix: [CI-22408]: allow PLUGIN_ACL=none to disable x-amz-acl on S3 cache uploads#181

Open
hemanthmantri wants to merge 1 commit into
drone-plugins:masterfrom
hemanthmantri:fix/CI-22408
Open

fix: [CI-22408]: allow PLUGIN_ACL=none to disable x-amz-acl on S3 cache uploads#181
hemanthmantri wants to merge 1 commit into
drone-plugins:masterfrom
hemanthmantri:fix/CI-22408

Conversation

@hemanthmantri

@hemanthmantri hemanthmantri commented May 15, 2026

Copy link
Copy Markdown

Fixes CI-22408

Proposed Changes

  • storage/backend/s3/s3.go: add isACLDisabled helper. In New, when PLUGIN_ACL matches the sentinel values none, disabled, or off (case-insensitive, trimmed), leave backend.acl empty so the Put path does not attach x-amz-acl to the S3 request.
  • main.go, DOCS.md, README.md: document the new sentinel values on the --acl / PLUGIN_ACL flag.
  • storage/backend/s3/s3_unit_test.go: add TestIsACLDisabled table test covering empty, sentinel, and real canned-ACL values.
  • CHANGELOG.md: add Unreleased entry.

Description

When a customer's S3 bucket has Object Ownership set to BucketOwnerEnforced (the AWS default for new buckets since April 2023), AWS rejects any canned ACL header other than bucket-owner-full-control with HTTP 400 AccessControlListNotSupported: The bucket does not allow ACLs. The Harness Cache Intelligence cache step hit this on CreateMultipartUpload because:

  1. The plugin sends x-amz-acl whenever PLUGIN_ACL is non-empty.
  2. The upstream Harness CI manager injects PLUGIN_ACL=private by default (originally added in harness-core PR #56087 to address CI-10547 for OVH), so simply removing acl from the pipeline YAML does not stop the header from being sent — the upstream default still wins unless the step explicitly overrides it.

This change gives customers an in-pipeline opt-out: setting PLUGIN_ACL=none (or disabled / off) at the step level overrides the upstream private injection and tells the plugin not to send the header at all, which is what BucketOwnerEnforced requires.

Existing canned-ACL values (private, public-read, bucket-owner-full-control, ...) continue to be forwarded to S3 unchanged.

Note: customers can also unblock themselves today, before this PR ships, by setting PLUGIN_ACL=bucket-owner-full-control at the step level — AWS documents that value as accepted on BucketOwnerEnforced buckets. The sentinel-based opt-out introduced here is the cleaner long-term option.

Existing pattern in this same file (S3 Express directory buckets — CI-13695, CI-21463, CI-22058) already skips the ACL header conditionally; this change extends that pattern with a customer-controllable knob for the regular-bucket case.

I considered two alternative approaches and rejected both:

  • Auto-retry without ACL on AccessControlListNotSupported: the upload uses non-seekable pipe readers, so the body cannot be replayed safely.
  • Probe GetBucketOwnershipControls at startup: requires an additional IAM permission that many cross-account roles don't grant.

Checklist

  • Read the CONTRIBUTING document.
  • Read the CODE OF CONDUCT document.
  • Add tests to cover changes.
  • Ensure your code follows the code style of this project.
  • Code compiles correctly.
  • All new and existing tests passed.
  • Add your changes to Unreleased section of CHANGELOG.
  • Improve and update the README (if necessary).
  • Ensure documentation is up-to-date.

…he uploads

Customers writing the cache to S3 buckets with Object Ownership =
BucketOwnerEnforced (ACLs disabled) hit AccessControlListNotSupported
on CreateMultipartUpload because the plugin always sends x-amz-acl when
PLUGIN_ACL is non-empty, and the upstream Harness CI manager injects
PLUGIN_ACL=private by default (see CI-10547). Removing PLUGIN_ACL from
the pipeline YAML alone does not clear the upstream injection.

Treat PLUGIN_ACL values "none", "disabled", or "off" (case-insensitive,
trimmed) as a sentinel meaning "do not send the x-amz-acl header" so
customers can override the upstream default with a step env var. Real
canned ACL values continue to flow through unchanged.

Fixes CI-22408.
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.

1 participant