Object locking need to persist the tags and set the headers #6994

chrislusf · 2025-07-17T00:03:08Z

What problem are we solving?

How are we solving the problem?

How is the PR tested?

Checks

I have added unit tests if possible.
I will add related wiki document changes and link to this PR after merging.

No logic to include object lock metadata in HEAD/GET response headers No logic to extract object lock metadata from PUT request headers

gemini-code-assist

Summary of Changes

Hello @chrislusf, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical gap in S3 object locking functionality by ensuring that object lock metadata (mode, retention date, legal hold) is correctly persisted when objects are uploaded and subsequently returned in GET and HEAD responses. This change enables proper S3 object lock compliance and management within the system. Additionally, I've added extensive tests to validate this new behavior across various scenarios, including versioned objects and error handling.

Highlights

Object Lock Metadata Persistence: I've implemented logic to extract S3 object lock headers (X-Amz-Object-Lock-Mode, X-Amz-Object-Lock-Retain-Until-Date, X-Amz-Object-Lock-Legal-Hold) from PutObject requests. This metadata is now correctly stored as extended attributes on the object entries in the filer, ensuring it persists with the object.
Object Lock Metadata Retrieval: The GetObject and HeadObject handlers have been enhanced to read the stored object lock metadata from the extended attributes. This ensures that the corresponding S3 object lock headers are correctly included in their responses, providing clients with the object's retention and legal hold status.
New S3 Header Constants: New constants for the S3 object lock headers (AmzObjectLockMode, AmzObjectLockRetainUntilDate, AmzObjectLockLegalHold) have been added to s3_constants/header.go for consistency and clarity.
Comprehensive Testing: I've introduced two new test suites (s3_object_lock_headers_test.go and s3_versioning_object_lock_test.go) to thoroughly validate the persistence and retrieval of object lock metadata. These tests cover various scenarios, including different object lock modes (GOVERNANCE, COMPLIANCE), legal holds, retention dates, versioned objects, and error conditions.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request implements S3 object lock functionality by persisting lock metadata from request headers and adding them to response headers for GET/HEAD requests. The changes are well-supported by a comprehensive set of new integration tests.

I've identified a critical compilation error in one of the new test files and a medium-severity issue in the response handler logic that could be improved for better readability and error logging. Addressing these points will improve the quality and robustness of the new feature.

gemini-code-assist · 2025-07-17T00:04:30Z

test/s3/versioning/s3_versioning_object_lock_test.go

+func createBucketWithObjectLock(t *testing.T, client *s3.Client, bucketName string) {
+	_, err := client.CreateBucket(context.TODO(), &s3.CreateBucketInput{
+		Bucket:                     aws.String(bucketName),
+		ObjectLockEnabledForBucket: aws.Bool(true),


The ObjectLockEnabledForBucket field in s3.CreateBucketInput is of type bool, but aws.Bool(true) returns a *bool. This type mismatch will cause a compilation error.

Suggested change

ObjectLockEnabledForBucket: aws.Bool(true),

ObjectLockEnabledForBucket: true,

gemini-code-assist · 2025-07-17T00:04:31Z

weed/s3api/s3api_object_handlers.go

+	// Add object lock mode header if present
+	if modeBytes, exists := entry.Extended[s3_constants.ExtObjectLockModeKey]; exists {
+		mode := string(modeBytes)
+		if mode != "" {
+			w.Header().Set(s3_constants.AmzObjectLockMode, mode)
+		}
+	}
+
+	// Add retention until date header if present
+	if dateBytes, exists := entry.Extended[s3_constants.ExtRetentionUntilDateKey]; exists {
+		dateStr := string(dateBytes)
+		if dateStr != "" {
+			// Convert Unix timestamp to ISO8601 format for S3 compatibility
+			if timestamp, err := strconv.ParseInt(dateStr, 10, 64); err == nil {
+				retainUntilDate := time.Unix(timestamp, 0).UTC()
+				w.Header().Set(s3_constants.AmzObjectLockRetainUntilDate, retainUntilDate.Format(time.RFC3339))
+			}
+		}
+	}
+
+	// Add legal hold header if present
+	if legalHoldBytes, exists := entry.Extended[s3_constants.ExtLegalHoldKey]; exists {
+		legalHold := string(legalHoldBytes)
+		if legalHold != "" {
+			w.Header().Set(s3_constants.AmzObjectLockLegalHold, legalHold)
+		}
+	}


This function can be improved for readability and robustness:

The nested if checks can be flattened by combining the existence check with a length check on the byte slice.

When parsing the retention date, an error should be logged if strconv.ParseInt fails. This would make the behavior consistent with extractObjectLockMetadataFromRequest and aid in debugging potential data corruption issues.

// Add object lock mode header if present if modeBytes, exists := entry.Extended[s3_constants.ExtObjectLockModeKey]; exists && len(modeBytes) > 0 { w.Header().Set(s3_constants.AmzObjectLockMode, string(modeBytes)) } // Add retention until date header if present if dateBytes, exists := entry.Extended[s3_constants.ExtRetentionUntilDateKey]; exists && len(dateBytes) > 0 { dateStr := string(dateBytes) // Convert Unix timestamp to ISO8601 format for S3 compatibility if timestamp, err := strconv.ParseInt(dateStr, 10, 64); err == nil { retainUntilDate := time.Unix(timestamp, 0).UTC() w.Header().Set(s3_constants.AmzObjectLockRetainUntilDate, retainUntilDate.Format(time.RFC3339)) } else { glog.Errorf("addObjectLockHeadersToResponse: failed to parse retention until date '%s': %v", dateStr, err) } } // Add legal hold header if present if legalHoldBytes, exists := entry.Extended[s3_constants.ExtLegalHoldKey]; exists && len(legalHoldBytes) > 0 { w.Header().Set(s3_constants.AmzObjectLockLegalHold, string(legalHoldBytes)) }

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…t lock-enabled buckets fix --- FAIL: TestAddObjectLockHeadersToResponse/Handle_entry_with_no_object_lock_metadata (0.00s)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

This PR enhances S3 object locking by persisting lock metadata on PUT and exposing it via GET/HEAD responses, and it updates tests to cover these behaviors and adapt to SDK changes.

Added header validation, extraction into Extended attributes, and error mapping in PutObjectHandler
Implemented inclusion of object lock headers in GET/HEAD responses
Expanded unit and integration tests; updated test code to use aws.Bool

Reviewed Changes

Copilot reviewed 17 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
weed/s3api/s3api_object_retention.go	Removed outdated `checkObjectLockPermissionsForPut` helper
weed/s3api/s3api_object_lock_headers_test.go	New unit tests for extracting, validating, and round‐tripping lock headers
weed/s3api/s3api_object_handlers_put.go	Added `validateObjectLockHeaders`, `extractObjectLockMetadataFromRequest`, and error mapping
weed/s3api/s3api_object_handlers.go	Invoke `addObjectLockHeadersToResponse` in GET/HEAD
weed/s3api/s3_constants/header.go	Defined constants for object lock header names
weed/notification/webhook/webhook_queue.go	Improved logging to handle nil dead‐letter messages
test/s3/versioning, retention, cors (multiple files)	Updated integration tests to use `aws.Bool(...)` for boolean parameters

Comments suppressed due to low confidence (2)

weed/s3api/s3api_object_handlers_put.go:103

The previous permission check (checkObjectLockPermissionsForPut) was removed, which may allow unauthorized overwrites of locked objects. Consider reapplying object lock permission enforcement or integrating governance bypass logic after header validation to ensure locked objects cannot be overwritten.

		if err := s3a.validateObjectLockHeaders(r, versioningEnabled); err != nil {

weed/notification/webhook/webhook_queue.go:200

[nitpick] For static log messages without formatting directives, prefer glog.Error over glog.Errorf to clarify intent and avoid suggesting formatting behavior.

					glog.Errorf("received nil message from dead letter channel")

chrislusf added 2 commits July 16, 2025 16:35

fix object locking read and write

c6f4a5a

No logic to include object lock metadata in HEAD/GET response headers No logic to extract object lock metadata from PUT request headers

add tests for object locking

eac427e

gemini-code-assist bot reviewed Jul 17, 2025

View reviewed changes

chrislusf requested a review from Copilot July 17, 2025 00:04

gemini-code-assist bot reviewed Jul 17, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

chrislusf and others added 5 commits July 16, 2025 17:08

Update weed/s3api/s3api_object_handlers_put.go

d15fb7c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_object_handlers.go

355f3ec

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

refactor

138bf8e

add unit tests

9eca7e5

sync versions

6744866

chrislusf requested a review from Copilot July 17, 2025 00:36

This comment was marked as outdated.

Sign in to view

chrislusf added 2 commits July 16, 2025 17:43

Update s3_worm_integration_test.go

3658316

fix legal hold values

85d8b2c

chrislusf requested a review from Copilot July 17, 2025 01:12

This comment was marked as outdated.

Sign in to view

chrislusf added 8 commits July 16, 2025 18:28

lint

096e546

fix tests

4009260

racing condition when enable versioning

7dd457b

fix tests

5168062

validate put object lock header

f13208d

allow check lock permissions for PUT

649ceca

default to OFF legal hold

87cd265

only set object lock headers for objects that are actually from objec…

fafd279

…t lock-enabled buckets fix --- FAIL: TestAddObjectLockHeadersToResponse/Handle_entry_with_no_object_lock_metadata (0.00s)

chrislusf requested a review from Copilot July 17, 2025 04:00

This comment was marked as outdated.

Sign in to view

address comments

0068e13

chrislusf requested a review from Copilot July 17, 2025 04:11

This comment was marked as outdated.

Sign in to view

chrislusf added 2 commits July 16, 2025 21:24

fix tests

5424897

purge

2819c14

chrislusf requested a review from Copilot July 17, 2025 04:26

This comment was marked as outdated.

Sign in to view

chrislusf added 2 commits July 16, 2025 21:32

fix

f9c9a17

refactoring

e53af82

chrislusf requested a review from Copilot July 17, 2025 04:54

This comment was marked as outdated.

Sign in to view

address comment

fbee44d

chrislusf requested a review from Copilot July 17, 2025 05:00

This comment was marked as outdated.

Sign in to view

address comment

1bcec2d

chrislusf requested a review from Copilot July 17, 2025 05:18

This comment was marked as outdated.

Sign in to view

chrislusf and others added 4 commits July 16, 2025 22:19

Update weed/s3api/s3api_object_handlers_put.go

ceddd31

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_object_handlers_put.go

d7e2a09

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update weed/s3api/s3api_object_handlers.go

0fb1b14

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

avoid nil

f9807d8

chrislusf requested a review from Copilot July 17, 2025 05:44

Copilot AI reviewed Jul 17, 2025

View reviewed changes

ensure locked objects cannot be overwritten

a7c0e67

chrislusf merged commit a524b4f into master Jul 17, 2025
18 checks passed

chrislusf deleted the object-locking-improvements branch July 17, 2025 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Object locking need to persist the tags and set the headers #6994

Object locking need to persist the tags and set the headers #6994

Uh oh!

chrislusf commented Jul 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Jul 17, 2025

Uh oh!

gemini-code-assist bot Jul 17, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

	ObjectLockEnabledForBucket: aws.Bool(true),
	ObjectLockEnabledForBucket: true,

Uh oh!

Object locking need to persist the tags and set the headers #6994

Object locking need to persist the tags and set the headers #6994

Uh oh!

Conversation

chrislusf commented Jul 17, 2025

What problem are we solving?

How are we solving the problem?

How is the PR tested?

Checks

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!