kubernetes/controller/internal/controller/deployer/blob_size.go (1)

1-61: Add unit tests for the blob_size.go module.

The limitedReadCloser and applyResourceSizeLimit functions have multiple edge cases that should be covered:

• Blob exactly at the size limit (should succeed)
• Blob exceeding the limit by one byte (should fail with clear error message)
• MaxResourceSizeMiB = 0 (should bypass size limiting entirely)
• SizeAware blob pre-check when declared size exceeds limit (should reject without reading data)
• Unknown blob size (SizeUnknown) falling through to runtime limit enforcement

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/controller/deployer/blob_size.go` around lines
1 - 61, Add unit tests covering limitedReadCloser.Read and
Reconciler.applyResourceSizeLimit: create tests for (1) blob exactly at the
limit using a reader of size exactly maxBytes and assert reads succeed and EOF,
(2) blob exceeding by one byte and assert the read returns the "resource exceeds
maximum allowed size" error from limitedReadCloser, (3)
Reconciler.MaxResourceSizeMiB == 0 where applyResourceSizeLimit returns the
original readerBlob unchanged, (4) a SizeAware mock blob whose Size() returns a
value > maxBytes and assert applyResourceSizeLimit returns an immediate error
without reading, and (5) a SizeAware mock that returns blob.SizeUnknown and a
reader larger than the limit to assert the runtime limitedReadCloser triggers
the overflow error; use the Reconciler.applyResourceSizeLimit function and
limitedReadCloser types to locate code paths and verify error messages and that
wrapped readers are closed/return expected bytes.

kubernetes/controller/internal/controller/deployer/blob_size_test.go (1)

115-194: Good coverage of applyResourceSizeLimit control flow. Consider adding a negative value test.

The tests thoroughly cover:

• Disabled limit (0)
• Size-aware blobs: under limit, over limit, unknown size
• Non-size-aware blobs

One gap: there's no test for negative MaxResourceSizeMiB values. Adding such a test would either document the expected behavior or highlight the need for validation.

🧪 Suggested test for negative limit

func TestApplyResourceSizeLimit_NegativeLimit(t *testing.T) {
	r := reconcilerWithLimit(-1)
	data := []byte("small")
	b := &mockBlob{data: data}
	rc := nopCloser{bytes.NewReader(data)}

	limited, err := r.applyResourceSizeLimit(b, rc)
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	// Document current behavior: negative limit creates io.LimitedReader with N < 0,
	// which returns EOF immediately on first read.
	buf := make([]byte, 10)
	n, readErr := limited.Read(buf)
	if n != 0 || readErr != io.EOF {
		t.Fatalf("expected immediate EOF with negative limit, got n=%d, err=%v", n, readErr)
	}
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/controller/deployer/blob_size_test.go` around
lines 115 - 194, Add a unit test that covers negative MaxResourceSizeMiB
behavior: create a Reconciler via reconcilerWithLimit(-1), build a mockBlob and
nopCloser, call applyResourceSizeLimit with those, and assert the observed
behavior (e.g., that the returned reader returns immediate EOF on first read or
document/expect the current behavior). Name the test
TestApplyResourceSizeLimit_NegativeLimit and include assertions that validate
both the returned reader type (if any) and the read behavior or returned error
so the negative-limit behavior is explicitly specified.

kubernetes/controller/internal/controller/deployer/deployer_controller.go (2)

535-547: Consider improving clarity and defensive checks in size limit enforcement.

A few observations on this logic:

1. Integer overflow (minor): r.MaxResourceSizeMiB * 1024 * 1024 could theoretically overflow for extremely large int64 values. In practice, reasonable values (1-10000 MiB) won't overflow, but a defensive approach could use constants or check bounds.

2. Variable naming: limitedReader is named before limiting is applied (line 521), which could be slightly confusing. Consider renaming to reader initially and limitedReader only after wrapping.

♻️ Optional: Clearer variable naming

-	limitedReader, err := resourceBlob.ReadCloser()
+	reader, err := resourceBlob.ReadCloser()
 	if err != nil {
 		status.MarkNotReady(r.EventRecorder, deployer, deliveryv1alpha1.GetOCMResourceFailedReason, err.Error())
 
 		return nil, fmt.Errorf("getting reader for resource blob: %w", err)
 	}
+	limitedReader := io.ReadCloser(reader)
 	defer func() {
 		err = errors.Join(err, limitedReader.Close())
 	}()
 
 	// Enforce resource size limit...
 	if r.MaxResourceSizeMiB > 0 {
 		maxBytes := r.MaxResourceSizeMiB * 1024 * 1024
 		if sizeAware, ok := resourceBlob.(blob.SizeAware); ok {
 			if size := sizeAware.Size(); size != blob.SizeUnknown && size > maxBytes {
 				err := fmt.Errorf("resource size %d bytes exceeds maximum allowed size of %d MiB", size, r.MaxResourceSizeMiB)
 				status.MarkNotReady(r.EventRecorder, deployer, deliveryv1alpha1.GetOCMResourceFailedReason, err.Error())
 
 				return nil, fmt.Errorf("failed to get limited reader: %w", err)
 			}
 		}
 
-		limitedReader = &limitedReadCloser{Closer: limitedReader, limited: &io.LimitedReader{R: limitedReader, N: maxBytes}}
+		limitedReader = &limitedReadCloser{Closer: reader, limited: &io.LimitedReader{R: reader, N: maxBytes}}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/controller/deployer/deployer_controller.go`
around lines 535 - 547, The size-enforcement block in deployer_controller.go can
be made more defensive and clearer: in the MaxResourceSizeMiB logic (seen around
r.MaxResourceSizeMiB usage and the limitedReadCloser wrapping), add a bounds
check to detect and guard against overflow when computing maxBytes (e.g.,
validate r.MaxResourceSizeMiB is within a safe range or clamp it before
multiplying), and rename the pre-existing generic limitedReader variable to
reader (or srcReader) and only create/assign limitedReader when you actually
wrap it with io.LimitedReader; ensure you use the new names consistently in the
Size-aware check (sizeAware.Size()) and when constructing &limitedReadCloser so
the code is clearer and avoids accidental misuse.

---

539-543: Misleading error message wrapper.

The wrapping error message "failed to get limited reader" doesn't accurately describe the actual failure (resource size exceeding the limit). Consider a more accurate message.

♻️ Suggested fix for clearer error message

-				return nil, fmt.Errorf("failed to get limited reader: %w", err)
+				return nil, err

Or if you prefer to add context:

-				return nil, fmt.Errorf("failed to get limited reader: %w", err)
+				return nil, fmt.Errorf("resource size check failed: %w", err)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/controller/deployer/deployer_controller.go`
around lines 539 - 543, Replace the misleading wrapper error "failed to get
limited reader" with a message that reflects the actual failure (resource size
exceeds limit) in the error returned from the block around status.MarkNotReady
and the return in deployer_controller.go; e.g., return a wrapped error like
"resource size exceeds maximum allowed limit: %w" or include the original err
when returning from the function that contains status.MarkNotReady, so callers
see the accurate condition reported by err (related symbols:
status.MarkNotReady, deployer, deliveryv1alpha1.GetOCMResourceFailedReason).