LOG-7913: Add S3 functional tests with minIO mock

[vparfonov]

Description

Implement comprehensive functional tests for S3 output forwarding using minIO as a containerized S3-compatible mock.

Architecture:

• minIO runs as a sidecar container in the same pod as Vector collector
• Test client accesses minIO via port-forwarding
• Vector batch timeout overridden to 10s via config visitor (default 300s too slow for tests)

Test Coverage:

• Add test suite covering application, audit, and infrastructure log types
• Support 5 compression formats: none, gzip, snappy, zlib, zstd

SDK Updates:

• Upgraded aws-sdk-go-v2 core: v1.9.0 → v1.41.5 (required for S3 support)
• Added aws-sdk-go-v2/service/s3 v1.98.0
• Upgraded cloudwatchlogs: v1.7.0 → v1.68.0 (fixes ComputePayloadHash middleware with newer core SDK)
• Upgraded smithy-go: v1.8.0 → v1.24.2

Testing:

• All S3 test passing
• CloudWatch tests fixed and passing with upgraded SDK versions
• No regressions to existing functional tests

/cc
/assign

Links

• Depending on PR(s):
• GitHub issue:
• JIRA: https://redhat.atlassian.net/browse/LOG-7913
• Enhancement proposal:

[openshift-ci-robot]

@vparfonov: This pull request references LOG-7913 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

Implement comprehensive functional tests for S3 output forwarding using minIO as a containerized S3-compatible mock.

Architecture:

• minIO runs as a sidecar container in the same pod as Vector collector
• Containers communicate via localhost:9000 (shared network namespace)
• Test client accesses minIO via oc port-forward on 127.0.0.1:19000
• Vector batch timeout overridden to 10s via config visitor (default 300s too slow for tests)

Test Coverage:

• Application log test: basic S3 output and log retrieval
• Audit log test: audit log type handling and key-prefix routing
• Compression table tests: 5 compression variants with decompression validation (none, gzip, zlib, snappy, zstd)
• JSON array parsing: Vector writes S3 objects as JSON arrays, properly unmarshaled

SDK Updates:

• Upgraded aws-sdk-go-v2 core: v1.9.0 → v1.41.5 (required for S3 support)
• Added aws-sdk-go-v2/service/s3 v1.98.0
• Upgraded cloudwatchlogs: v1.7.0 → v1.68.0 (fixes ComputePayloadHash middleware with newer core SDK)
• Upgraded smithy-go: v1.8.0 → v1.24.2

Testing:

• All S3 test passing
• CloudWatch tests fixed and passing with upgraded SDK versions
• No regressions to existing functional tests

/cc
/assign

Links

• Depending on PR(s):
• GitHub issue:
• JIRA: https://redhat.atlassian.net/browse/LOG-7913
• Enhancement proposal:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[vparfonov]

/hold

[qodo-code-review]

Review Summary by Qodo

Add S3 functional tests with minIO mock and AWS SDK upgrades

✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Implement S3 functional tests using minIO containerized mock
• Add port-forward mechanism for minIO service access
• Support multiple compression formats (gzip, zlib, snappy, zstd)
• Upgrade AWS SDK dependencies for S3 and CloudWatch compatibility

Diagram

flowchart LR
  A["Test Framework"] -->|"AddS3Output"| B["minIO Container"]
  B -->|"Port-forward 9000→19000"| C["S3 Client"]
  C -->|"Create Bucket"| D["SetupS3Bucket"]
  D -->|"Write Logs"| E["Vector Collector"]
  E -->|"Batch & Upload"| F["S3 Objects"]
  F -->|"Decompress & Parse"| G["ReadLogsFromS3"]
  G -->|"JSON Array Parsing"| H["Test Assertions"]

Loading

File Changes

1. test/framework/functional/framework.go ✨ Enhancement +6/-4

Enable S3 output and add port-forward cleanup

• Add S3 port-forward cleanup in Cleanup() method
• Uncomment and enable S3 output handling in addOutputContainers()
• Call cleanupS3PortForward() to terminate port-forward process

test/framework/functional/framework.go

---

2. test/framework/functional/output_s3.go ✨ Enhancement +140/-51

Implement S3 output with port-forward and decompression

• Uncomment entire S3 output implementation
• Replace Route-based access with port-forward mechanism (localhost:19000)
• Implement SetupS3Bucket() with port-forward setup and readiness polling
• Enhance ReadLogsFromS3() with decompression support for gzip, zlib, snappy, zstd
• Add JSON array parsing for Vector S3 object format
• Implement cleanupS3PortForward() and tryDecompress() helper functions
• Add compression format detection and handling

test/framework/functional/output_s3.go

---

3. test/functional/outputs/aws/s3/forward_to_s3_test.go 🧪 Tests +59/-32

Enable S3 functional tests with compression variants

• Uncomment all S3 functional tests
• Add setupS3Output() helper function for test configuration
• Configure Vector batch timeout to 10s via config visitor
• Move bucket creation to after deployment for proper initialization
• Add compression format test table (none, gzip, snappy, zlib, zstd)
• Increase wait time from 15s to 20s for batch processing
• Test application logs, audit logs, and compression variants

test/functional/outputs/aws/s3/forward_to_s3_test.go

---

View more (3)

4. test/runtime/observability/cluster_log_forwarder.go ✨ Enhancement +1/-1

Update S3 endpoint URL for port-forward access

• Change S3 endpoint URL from HTTPS to HTTP for port-forward access
• Update URL format to use localhost:port instead of route hostname

test/runtime/observability/cluster_log_forwarder.go

---

5. go.mod Dependencies +14/-5

Upgrade AWS SDK and add S3 dependencies

• Upgrade aws-sdk-go-v2 from v1.9.0 to v1.41.5
• Upgrade aws-sdk-go-v2/service/cloudwatchlogs from v1.7.0 to v1.68.0
• Add aws-sdk-go-v2/service/s3 v1.98.0 for S3 support
• Upgrade smithy-go from v1.8.0 to v1.24.2
• Add klauspost/compress v1.17.9 for compression support
• Add pelletier/go-toml v1.9.5 to direct dependencies

go.mod

---

6. go.sum Dependencies +24/-11

Update dependency checksums for AWS SDK upgrade

• Add checksums for upgraded AWS SDK packages
• Add checksums for new S3 service and internal packages
• Add checksums for compression library dependencies
• Remove obsolete jmespath and old go-cmp checksums

go.sum

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (12)   📘 Rule violations (0)   📎 Requirement gaps (0)   🎨 UX Issues (0)

🐞\ ≡ Correctness (3) ☼ Reliability (8) ⚙ Maintainability (1) ⭐ New (1)

1. Duplicate MinIO wiring 🐞 ≡

Description

AddS3Output always creates a "minio-server" Service and appends a "minio" container for each S3
output, so a CLF with multiple S3 outputs will produce duplicate container names and/or
AlreadyExists errors on service creation. This can make framework.Deploy() fail for otherwise valid
CLF specs.

Code

test/framework/functional/output_s3.go[R50-68]

+	// Initialize S3 state if not already done
+	if _, ok := s3States[f]; !ok {
+		s3States[f] = &s3State{}
}
-	if err := f.createMinIORoute(); err != nil {
+	state := s3States[f]
+	if err := f.createMinIOService(state); err != nil {
  return err
}
-	// initialize the client
-	s3Client = s3.NewFromConfig(
+	// S3 client is initialized in SetupS3Bucket after port-forward is ready
+
+	// add the minIO container
+	b.AddContainer("minio", minioImage).
+		WithCmdArgs([]string{minioCmd, minioDataPath}).
+		AddEnvVar("MINIO_ROOT_USER", AwsAccessKeyID).
+		AddEnvVar("MINIO_ROOT_PASSWORD", AwsSecretAccessKey).
+		AddContainerPort("minio-port", 9000).
+		End()

Evidence

The framework loops through all outputs and calls AddS3Output for each S3 output; AddS3Output
unconditionally creates the same Service name and adds a container with the same name.
PodBuilder.End() always appends containers (no dedupe), so a second S3 output yields an invalid pod
spec with duplicate container names.

test/framework/functional/framework.go[421-463]
test/framework/functional/output_s3.go[49-83]
internal/runtime/pod.go[39-43]
internal/runtime/pod.go[130-141]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`AddS3Output` currently adds a `minio` sidecar container and creates a `minio-server` Service every time it is invoked. Because `addOutputContainers` calls `AddS3Output` once per S3 output, any CLF with >1 S3 output will end up with duplicate container names and repeated service creation attempts.
### Issue Context
- `PodBuilder.AddContainer(...).End()` always appends a new container; it does not dedupe by name.
- The Service name is hard-coded (`minio-server`) and is created unconditionally.
### Fix Focus Areas
- test/framework/functional/output_s3.go[49-83]
- test/framework/functional/framework.go[421-463]
- internal/runtime/pod.go[39-43]
### Suggested fix
- Guard MinIO setup with state, e.g. in `s3State` add `minioInitialized bool` and return early if already initialized.
- Only create the Service once (or use `client.IgnoreAlreadyExists(...)` / `Test.IgnoreAlreadyExists(...)` style helper if available).
- Only add the `minio` container once; alternatively, check existing `b.Pod.Spec.Containers` for name `minio` before appending.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Hardcoded port-forward port 🐞 ☼

Description

SetupS3Bucket hard-codes local port 19000 and the S3 client endpoint is also hard-coded to
127.0.0.1:19000, so tests will fail whenever that port is already in use. This makes the S3
functional suite unreliable across concurrent/overlapping test runs on the same host.

Code

test/framework/functional/output_s3.go[R112-114]

+	localPort := "19000"
+	portForwardCmd = exec.Command("oc", "port-forward", "-n", minioNamespace, "svc/minio-server", localPort+":9000")
+

Evidence

Both the port-forward command and the AWS S3 client endpoint are pinned to the same fixed local port
(19000), so a simple local port collision will prevent the port-forward from starting and/or prevent
the client from connecting.

test/framework/functional/output_s3.go[54-76]
test/framework/functional/output_s3.go[109-141]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
S3 functional tests hard-code the port-forward local port (`19000`) and also hard-code the S3 client endpoint to `http://127.0.0.1:19000`. If that port is occupied, `oc port-forward` fails and the tests cannot reach minIO.
### Issue Context
The code currently selects `localPort := "19000"` inside `SetupS3Bucket` and separately bakes `http://127.0.0.1:19000` into the S3 client configuration.
### Fix Focus Areas
- test/framework/functional/output_s3.go[54-76]
- test/framework/functional/output_s3.go[109-141]
### Suggested direction
- Allocate a free local port dynamically (e.g., `net.Listen("tcp", "127.0.0.1:0")` to discover a free port, then close it and use that port for `oc port-forward`).
- Build the S3 client endpoint URL from the chosen port (or initialize the S3 client after the port is chosen).
- Ensure the chosen port is stored per test/framework instance, not as a constant.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Global port-forward state 🐞 ⚙

Description

S3 state is stored in package-level globals (portForwardCmd, minioNamespace, s3Client) and Cleanup
kills a single global process, which breaks test isolation between framework instances.
SetupS3Bucket also depends on AddS3Output having already set minioNamespace, creating a fragile
ordering dependency.

Code

test/framework/functional/output_s3.go[R39-53]

var (
-	s3Client *s3.Client
+	s3Client       *s3.Client
+	portForwardCmd *exec.Cmd
+	minioNamespace string
+	minioService   *corev1.Service
)
func (f *CollectorFunctionalFramework) AddS3Output(b *runtime.PodBuilder, spec obs.OutputSpec) error {
if err := f.createMinIOService(); err != nil {
return err
}
-	if err := f.createMinIORoute(); err != nil {
-		return err
-	}
+	// Store namespace for later port-forward setup
+	minioNamespace = f.Namespace
-	// initialize the client

Evidence

The port-forward process handle and namespace are stored globally in the functional package and are
mutated in AddS3Output, while framework cleanup unconditionally calls cleanupS3PortForward() which
acts on the same global. This design couples unrelated tests/framework instances and makes the
behavior depend on execution order.

test/framework/functional/output_s3.go[39-53]
test/framework/functional/output_s3.go[109-127]
test/framework/functional/framework.go[131-149]
test/framework/functional/output_s3.go[237-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
S3 test infrastructure uses package-level global variables (`s3Client`, `portForwardCmd`, `minioNamespace`) and a package-level cleanup function. This breaks isolation across `CollectorFunctionalFramework` instances and introduces fragile ordering (SetupS3Bucket assumes AddS3Output already populated `minioNamespace`).
### Issue Context
- `minioNamespace` is set in `AddS3Output`, but consumed later in `SetupS3Bucket`.
- `CollectorFunctionalFramework.Cleanup()` always calls `cleanupS3PortForward()`, which operates on the global `portForwardCmd`.
### Fix Focus Areas
- test/framework/functional/output_s3.go[39-53]
- test/framework/functional/output_s3.go[109-127]
- test/framework/functional/output_s3.go[237-246]
- test/framework/functional/framework.go[131-149]
### Suggested direction
- Add per-framework fields (e.g., `f.s3Client`, `f.s3PortForwardCmd`, `f.minioNamespace`, `f.s3LocalPort`).
- Use `f.Namespace` directly instead of a global `minioNamespace`.
- Convert `cleanupS3PortForward()` into a receiver method (e.g., `f.cleanupS3PortForward()`), and only stop the port-forward started by that framework.
- Optionally guard against calling `SetupS3Bucket` multiple times by reusing an existing port-forward or cleaning up before starting a new one.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. PortForwarder stop non-idempotent 🐞 ☼ ⭐

Description

PortForwarder.Stop signals API-based forwarding by sending on stopCh, which can deadlock if Stop is
called more than once (buffer size 1) and can panic if stopCh was closed elsewhere. This makes the
port-forward helper brittle for reuse beyond the current call sites.

Code

test/framework/functional/portforward.go[R30-38]

+// Stop terminates the port-forward connection.
+func (pf *PortForwarder) Stop() {
+	if pf.cmd != nil && pf.cmd.Process != nil {
+		_ = pf.cmd.Process.Kill()
+		_ = pf.cmd.Wait()
+	} else {
+		pf.stopCh <- struct{}{}
+	}
+}

Evidence

Stop uses pf.stopCh <- struct{}{} for the channel-based port-forward path, but the channel is
created with buffer size 1 and is not protected against multiple Stop calls or closed-channel sends;
this can block forever on the second Stop call or panic if stopCh is closed by a caller.

test/framework/functional/portforward.go[30-38]
test/framework/functional/portforward.go[53-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PortForwarder.Stop()` is not safe to call multiple times for the channel-based port-forward path because it sends into a buffered channel (size 1). A second call can deadlock, and sending after a close can panic.

### Issue Context
Even if current call sites don’t hit the edge cases today, this helper is now shared and likely to be reused; making Stop idempotent prevents hard-to-debug hangs in functional tests.

### Fix Focus Areas
- test/framework/functional/portforward.go[18-38]
- test/framework/functional/portforward.go[53-80]

### Suggested fix
- Replace the send with a channel close (which is the conventional stop signal for client-go portforward), and guard it with `sync.Once` to make it idempotent.
- Optionally, add a non-blocking fallback (e.g., `select { case <-alreadyClosed: default: }`) or store a `stopped` flag to avoid panics.
- Keep the cmd-based stop path, but also make it safe if called multiple times (ignore `os.ErrProcessDone`, etc.).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. MinIO create errors masked 🐞 ☼

Description

createMinIOService falls back to Get for any Service Create error (not just AlreadyExists), which
can hide real creation failures and lead to using an unexpected pre-existing Service object.

Code

test/framework/functional/output_s3.go[R86-92]

+	if err := f.Test.Create(state.minioService); err != nil {
+		// Service may already exist if there are multiple S3 outputs
+		// Try to get it instead to ensure we have the reference
+		if err := f.Test.Get(state.minioService); err != nil {
+			return fmt.Errorf("unable to create or retrieve minio service: %v", err)
+		}
+	}

Evidence

The code treats every Create error as “maybe already exists” and attempts Get, but the client Create
call returns raw API errors and should be checked for AlreadyExists explicitly before falling back.

/test/framework/functional/output_s3.go[80-93]
/test/client/client.go[95-108]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createMinIOService` currently retries with `Get` on *any* error from `Create`, which can mask genuine failures (RBAC, invalid object, transient API issues) and make debugging test failures harder.
### Issue Context
Only `AlreadyExists` should trigger a fallback to `Get`. For any other error, return the original `Create` error.
### Fix Focus Areas
- /test/framework/functional/output_s3.go[80-93]
- /test/client/client.go[95-108]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Nil S3 client dereference 🐞 ≡

Description

ReadLogsFromS3 verifies the framework state exists but does not verify state.client is
initialized, so calling ReadLogsFromS3 before SetupS3Bucket can panic via nil client usage.

Code

test/framework/functional/output_s3.go[R214-225]

+	state, ok := s3States[f]
+	if !ok || state == nil {
+		return nil, fmt.Errorf("S3 not initialized")
+	}
+
  var allMessages []string
  var listOutput *s3.ListObjectsV2Output

-	// wait until files are visible
  log.V(2).Info("waiting for logs in s3 bucket", "bucket", bucketName, "keyPrefix", keyPrefix)
  err := wait.PollUntilContextTimeout(context.TODO(), defaultRetryInterval, f.GetMaxReadDuration(), true, func(ctx context.Context) (done bool, err error) {
-		listOutput, err = s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{
+		listOutput, err = state.client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{
  		Bucket: aws.String(bucketName),

Evidence

state.client is only initialized inside SetupS3Bucket, but ReadLogsFromS3 uses it
unconditionally after only checking that state exists.

/test/framework/functional/output_s3.go[115-163]
/test/framework/functional/output_s3.go[213-250]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ReadLogsFromS3` can dereference a nil `state.client` if `SetupS3Bucket` was not invoked, causing a panic.
### Issue Context
The state map is initialized in `AddS3Output`/`SetupS3Bucket`, but the S3 client itself is created only in `SetupS3Bucket`.
### Fix Focus Areas
- /test/framework/functional/output_s3.go[115-163]
- /test/framework/functional/output_s3.go[213-250]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (5)

7. Free port selection race 🐞 ☼

Description

findFreePort closes the listener before starting oc port-forward, so another process can claim the
port between selection and use, causing flaky address-in-use failures.

Code

test/framework/functional/output_s3.go[R101-113]

+// findFreePort discovers an available local port by listening on port 0
+func findFreePort() (string, error) {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		return "", err
+	}
+	defer func() {
+		_ = listener.Close()
+	}()
+
+	addr := listener.Addr().(*net.TCPAddr)
+	return fmt.Sprintf("%d", addr.Port), nil
+}

Evidence

The implementation explicitly releases the chosen port, and only later starts port-forward bound to
that same port, creating a TOCTOU window.

/test/framework/functional/output_s3.go[101-113]
/test/framework/functional/output_s3.go[115-171]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The current `findFreePort` approach can be flaky because the chosen port is released before `oc port-forward` binds to it.
### Issue Context
This is a classic time-of-check/time-of-use race. In CI with concurrent processes/tests, this can intermittently fail.
### Fix Focus Areas
- /test/framework/functional/output_s3.go[101-113]
- /test/framework/functional/output_s3.go[115-171]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Port-forward process leak 🐞 ☼

Description

SetupS3Bucket overwrites state.portForwardCmd on each call without stopping any existing
port-forward process, which can leak processes and/or fail on subsequent calls due to port
conflicts.

Code

test/framework/functional/output_s3.go[R164-165]

+	state.portForwardCmd = exec.Command("oc", "port-forward", "-n", f.Namespace, "svc/minio-server", state.localPort+":9000")

-	return nil
-}
-
-func (f *CollectorFunctionalFramework) createMinIOService() error {
-	service = runtime.NewService(f.Namespace, "minio-server")
-	runtime.NewServiceBuilder(service).
-		AddServicePort(minioPort, minioPort).
-		WithSelector(map[string]string{"testname": "functional"})

Evidence

A new exec.Cmd is assigned each time SetupS3Bucket runs; the only termination path is the explicit
CleanupS3PortForward call, so repeated SetupS3Bucket calls within a test can orphan the prior
process.

/test/framework/functional/output_s3.go[115-172]
/test/framework/functional/output_s3.go[297-312]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SetupS3Bucket` can start multiple `oc port-forward` processes if called more than once, because it overwrites `state.portForwardCmd` without terminating any prior process.
### Issue Context
Even if current tests call it once, this is a sharp edge for future tests (multiple buckets, retries, etc.) and can cause resource leaks.
### Fix Focus Areas
- /test/framework/functional/output_s3.go[115-172]
- /test/framework/functional/output_s3.go[297-312]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. S3 region mismatch 🐞 ≡

Description

The S3 tests configure the output region as "us-east-1", but the MinIO bootstrap client in
SetupS3Bucket hard-codes "us-east-2" and creates buckets with LocationConstraintUsEast2. This
inconsistency can cause failures with stricter S3-compatible implementations and makes the test less
representative of the configured output.

Code

test/functional/outputs/aws/s3/forward_to_s3_test.go[R21-44]

const (

Evidence

The test explicitly sets output.S3.Region to TestRegion which is us-east-1, while the
framework S3 client used to create the bucket signs for awsRegion (us-east-2) and uses a
LocationConstraintUsEast2. These values diverge within the same test flow.

test/functional/outputs/aws/s3/forward_to_s3_test.go[21-44]
test/framework/functional/output_s3.go[40-47]
test/framework/functional/output_s3.go[137-166]
test/framework/functional/output_s3.go[198-214]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The S3 output region configured in the CLF (`us-east-1`) does not match the region used by the framework’s S3 client (`us-east-2`) nor the hard-coded `CreateBucketConfiguration.LocationConstraintUsEast2`.
### Issue Context
Even if MinIO is permissive today, this can become a source of flaky failures or hide real misconfiguration issues. It also makes the test setup inconsistent with the output being tested.
### Fix Focus Areas
- test/functional/outputs/aws/s3/forward_to_s3_test.go[21-44]
- test/framework/functional/output_s3.go[40-47]
- test/framework/functional/output_s3.go[198-205]
### Suggested fix
Pick a single canonical test region and use it everywhere:
- Option A (minimal change): change `TestRegion` to `us-east-2` and stop overriding to `us-east-1`.
- Option B (more correct for `us-east-1`): change `awsRegion` to `us-east-1` and omit `CreateBucketConfiguration` entirely when region is `us-east-1` (AWS behavior), while using the matching location constraint for other regions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Port-forward errors hidden 🐞 ☼

Description

SetupS3Bucket tries to detect immediate oc port-forward exits by checking ProcessState after
Start(), but ProcessState is not populated unless Wait() runs, so this branch won’t catch early
failures. When the TCP readiness poll times out, the returned error omits captured oc stderr, making
port-forward failures hard to debug.

Code

test/framework/functional/output_s3.go[R168-196]

+	state.portForwardCmd = exec.Command("oc", "port-forward", "-n", f.Namespace, "svc/minio-server", state.localPort+":9000")
-func (f *CollectorFunctionalFramework) createMinIOService() error {
-	service = runtime.NewService(f.Namespace, "minio-server")
-	runtime.NewServiceBuilder(service).
-		AddServicePort(minioPort, minioPort).
-		WithSelector(map[string]string{"testname": "functional"})
+	var stderr bytes.Buffer
+	state.portForwardCmd.Stderr = &stderr
-	if err := f.Test.Client.Create(service); err != nil {
-		return fmt.Errorf("unable to create minio service: %v", err)
+	if err := state.portForwardCmd.Start(); err != nil {
+		return fmt.Errorf("unable to start port-forward: %v", err)
}
-	return nil
-}
+	log.V(2).Info("Port-forward process started", "pid", state.portForwardCmd.Process.Pid)
-func (f *CollectorFunctionalFramework) createMinIORoute() error {
-	log.V(2).Info("Creating route for minio-server")
-	// Re-using the global route variable defined in the functional package
-	route = runtime.NewRoute(f.Namespace, "minio-server", "minio-server", fmt.Sprintf("%d", minioPort))
-	route.Spec.TLS = &openshiftv1.TLSConfig{
-		Termination:                   openshiftv1.TLSTerminationPassthrough,
-		InsecureEdgeTerminationPolicy: openshiftv1.InsecureEdgeTerminationPolicyNone,
+	time.Sleep(2 * time.Second)
+
+	if state.portForwardCmd.ProcessState != nil && state.portForwardCmd.ProcessState.Exited() {
+		return fmt.Errorf("port-forward process exited immediately: %s", stderr.String())
}
-	if err := f.Test.Client.Create(route); err != nil {
-		return fmt.Errorf("unable to create minio route: %v", err)
+
+	log.V(2).Info("waiting for port-forward to be ready")
+	pollErr := wait.PollUntilContextTimeout(context.TODO(), 500*time.Millisecond, 10*time.Second, true, func(ctx context.Context) (done bool, err error) {
+		conn, err := net.DialTimeout("tcp", "127.0.0.1:"+state.localPort, 1*time.Second)
+		if err != nil {
+			return false, nil
+		}
+		_ = conn.Close()
+		log.V(2).Info("Port-forward is ready")
+		return true, nil
+	})
+	if pollErr != nil {
+		return fmt.Errorf("port-forward failed to be ready: %v", pollErr)
}
-	return nil
-}

Evidence

The code captures stderr into a buffer but only uses it in the ProcessState-based early-exit branch;
it then returns a generic poll timeout error if readiness isn’t reached. There is no
Wait()/monitoring goroutine to populate ProcessState or surface the stderr when the process exits
during the readiness window.

test/framework/functional/output_s3.go[168-196]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Port-forward startup failures are difficult to diagnose because:
- the `ProcessState` early-exit check won’t work without a `Wait()` path, and
- stderr is captured but not included in the common failure path (readiness poll timeout).
### Issue Context
This increases functional test flakiness/debug time when `oc port-forward` fails (RBAC, service not ready, oc missing, port binding issues, etc.).
### Fix Focus Areas
- test/framework/functional/output_s3.go[168-196]
### Suggested fix
- Start a goroutine that calls `cmd.Wait()` and signals completion/error over a channel.
- During readiness polling, also select on that channel; if the process exits, return an error that includes `stderr.String()`.
- If readiness polling times out, include `stderr.String()` in the returned error message as well.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Brittle batch timeout patch 🐞 ☼

Description

The S3 test overrides Vector's batch timeout via strings.Replace against an exact TOML snippet; if
the generated config formatting changes, the replacement silently no-ops and the test ends up
waiting on Vector’s default 300s batch timeout. This can cause timeouts/flaky failures without a
clear error.

Code

test/functional/outputs/aws/s3/forward_to_s3_test.go[R46-55]

+		// Override the generated Vector config to set a short batch timeout.
+		// Vector's default batch timeout for S3 is 300s which is too long for tests.
+		// https://vector.dev/docs/reference/configuration/sinks/aws_s3/#batch.timeout_secs
+		framework.VisitConfig = func(conf string) string {
+			conf = strings.Replace(conf,
+				"\n[sinks.output_s3.encoding]",
+				"\n[sinks.output_s3.batch]\ntimeout_secs = 10\n\n[sinks.output_s3.encoding]",
+				1)
+			return conf
+		}

Evidence

The test performs a single string replacement on a specific section header, and does not assert that
the replacement occurred. Any upstream change to sink naming or TOML layout would leave the config
unmodified while the test continues assuming a 10s timeout.

test/functional/outputs/aws/s3/forward_to_s3_test.go[36-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The functional S3 tests rely on a fragile `strings.Replace` to inject `[sinks.output_s3.batch] timeout_secs = 10` before `[sinks.output_s3.encoding]`. If the generated Vector config changes, the replace may not match and the test will silently keep the default (slow) batch timeout.
### Issue Context
The code currently does not validate that the replacement happened.
### Fix Focus Areas
- test/functional/outputs/aws/s3/forward_to_s3_test.go[46-55]
### Suggested direction
- Parse and modify the TOML instead of doing a raw string replace (you already added go-toml in go.mod):
- Parse config into a TOML tree.
- Set `sinks.output_s3.batch.timeout_secs = 10` (create intermediate tables if needed).
- Serialize back to string.
- At minimum, assert the config contains the anchor substring before/after mutation (fail fast if it doesn’t).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. GetObject lacks per-call timeout 🐞 ☼

Description

ReadLogsFromS3 uses context.TODO() for GetObject calls, so an individual GetObject can stall much
longer than necessary even though the overall read loop is bounded. This can unnecessarily slow or
intermittently hang a test until the outer timeout triggers.

Code

test/framework/functional/output_s3.go[R189-202]

getFileOutput, err := s3Client.GetObject(context.TODO(), &s3.GetObjectInput{
  Bucket: aws.String(bucketName),
  Key:    object.Key,
})
if err != nil {
  return nil, fmt.Errorf("failed to get object %s: %w", *object.Key, err)
}
-		defer getFileOutput.Body.Close()
-
-		// read and concat
-		reader := bufio.NewReader(getFileOutput.Body)
-		for {
-			line, err := reader.ReadString('\n')
-			if err != nil && err != io.EOF {
-				return nil, fmt.Errorf("error reading file: %w", err)
+
+		body, err := io.ReadAll(getFileOutput.Body)
+		getFileOutput.Body.Close()
+		if err != nil {
+			return nil, fmt.Errorf("failed to read object %s: %w", *object.Key, err)
+		}
+

Evidence

GetObject is invoked with an unbounded context (context.TODO) rather than a per-call timeout; while
list polling is bounded by GetMaxReadDuration, each object fetch itself has no explicit deadline.

test/framework/functional/output_s3.go[161-202]
test/framework/functional/framework.go[151-156]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ReadLogsFromS3` calls `s3Client.GetObject(context.TODO(), ...)` without a per-call deadline. If the request stalls, the test can be delayed until the outer polling/overall test timeout.
### Issue Context
The framework already has `GetMaxReadDuration()` used to bound other waits.
### Fix Focus Areas
- test/framework/functional/output_s3.go[186-202]
### Suggested direction
Wrap each GetObject call with a bounded context, e.g.:
- `ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)`
- `defer cancel()`
- Use `ctx` for `GetObject` and optionally for `io.ReadAll` if you switch to streaming with a reader that honors context.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

Previous review results

Review updated until commit 953a54e

Results up to commit 2b088ec

🐞 Bugs (11)  
📘 Rule violations (0)  
📎 Requirement gaps (0)  
🎨 UX Issues (0)

🐞\ ≡ Correctness (3) ☼ Reliability (7) ⚙ Maintainability (1) ⭐ New (4)

1. Duplicate MinIO wiring 🐞 ≡

Description

AddS3Output always creates a "minio-server" Service and appends a "minio" container for each S3
output, so a CLF with multiple S3 outputs will produce duplicate container names and/or
AlreadyExists errors on service creation. This can make framework.Deploy() fail for otherwise valid
CLF specs.

Code

test/framework/functional/output_s3.go[R50-68]

+	// Initialize S3 state if not already done
+	if _, ok := s3States[f]; !ok {
+		s3States[f] = &s3State{}
  }

-	if err := f.createMinIORoute(); err != nil {
+	state := s3States[f]
+	if err := f.createMinIOService(state); err != nil {
  	return err
  }

-	// initialize the client
-	s3Client = s3.NewFromConfig(
+	// S3 client is initialized in SetupS3Bucket after port-forward is ready
+
+	// add the minIO container
+	b.AddContainer("minio", minioImage).
+		WithCmdArgs([]string{minioCmd, minioDataPath}).
+		AddEnvVar("MINIO_ROOT_USER", AwsAccessKeyID).
+		AddEnvVar("MINIO_ROOT_PASSWORD", AwsSecretAccessKey).
+		AddContainerPort("minio-port", 9000).
+		End()

Evidence

The framework loops through all outputs and calls AddS3Output for each S3 output; AddS3Output
unconditionally creates the same Service name and adds a container with the same name.
PodBuilder.End() always appends containers (no dedupe), so a second S3 output yields an invalid pod
spec with duplicate container names.

test/framework/functional/framework.go[421-463]
test/framework/functional/output_s3.go[49-83]
internal/runtime/pod.go[39-43]
internal/runtime/pod.go[130-141]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`AddS3Output` currently adds a `minio` sidecar container and creates a `minio-server` Service every time it is invoked. Because `addOutputContainers` calls `AddS3Output` once per S3 output, any CLF with >1 S3 output will end up with duplicate container names and repeated service creation attempts.
### Issue Context
- `PodBuilder.AddContainer(...).End()` always appends a new container; it does not dedupe by name.
- The Service name is hard-coded (`minio-server`) and is created unconditionally.
### Fix Focus Areas
- test/framework/functional/output_s3.go[49-83]
- test/framework/functional/framework.go[421-463]
- internal/runtime/pod.go[39-43]
### Suggested fix
- Guard MinIO setup with state, e.g. in `s3State` add `minioInitialized bool` and return early if already initialized.
- Only create the Service once (or use `client.IgnoreAlreadyExists(...)` / `Test.IgnoreAlreadyExists(...)` style helper if available).
- Only add the `minio` container once; alternatively, check existing `b.Pod.Spec.Containers` for name `minio` before appending.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Hardcoded port-forward port 🐞 ☼

Description

SetupS3Bucket hard-codes local port 19000 and the S3 client endpoint is also hard-coded to
127.0.0.1:19000, so tests will fail whenever that port is already in use. This makes the S3
functional suite unreliable across concurrent/overlapping test runs on the same host.

Code

test/framework/functional/output_s3.go[R112-114]

+	localPort := "19000"
+	portForwardCmd = exec.Command("oc", "port-forward", "-n", minioNamespace, "svc/minio-server", localPort+":9000")
+

Evidence

Both the port-forward command and the AWS S3 client endpoint are pinned to the same fixed local port
(19000), so a simple local port collision will prevent the port-forward from starting and/or prevent
the client from connecting.

test/framework/functional/output_s3.go[54-76]
test/framework/functional/output_s3.go[109-141]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
S3 functional tests hard-code the port-forward local port (`19000`) and also hard-code the S3 client endpoint to `http://127.0.0.1:19000`. If that port is occupied, `oc port-forward` fails and the tests cannot reach minIO.
### Issue Context
The code currently selects `localPort := "19000"` inside `SetupS3Bucket` and separately bakes `http://127.0.0.1:19000` into the S3 client configuration.
### Fix Focus Areas
- test/framework/functional/output_s3.go[54-76]
- test/framework/functional/output_s3.go[109-141]
### Suggested direction
- Allocate a free local port dynamically (e.g., `net.Listen("tcp", "127.0.0.1:0")` to discover a free port, then close it and use that port for `oc port-forward`).
- Build the S3 client endpoint URL from the chosen port (or initialize the S3 client after the port is chosen).
- Ensure the chosen port is stored per test/framework instance, not as a constant.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Global port-forward state 🐞 ⚙

Description

S3 state is stored in package-level globals (portForwardCmd, minioNamespace, s3Client) and Cleanup
kills a single global process, which breaks test isolation between framework instances.
SetupS3Bucket also depends on AddS3Output having already set minioNamespace, creating a fragile
ordering dependency.

Code

test/framework/functional/output_s3.go[R39-53]

var (
-	s3Client *s3.Client
+	s3Client       *s3.Client
+	portForwardCmd *exec.Cmd
+	minioNamespace string
+	minioService   *corev1.Service
)
func (f *CollectorFunctionalFramework) AddS3Output(b *runtime.PodBuilder, spec obs.OutputSpec) error {
if err := f.createMinIOService(); err != nil {
  return err
}
-	if err := f.createMinIORoute(); err != nil {
-		return err
-	}
+	// Store namespace for later port-forward setup
+	minioNamespace = f.Namespace
-	// initialize the client

Evidence

The port-forward process handle and namespace are stored globally in the functional package and are
mutated in AddS3Output, while framework cleanup unconditionally calls cleanupS3PortForward() which
acts on the same global. This design couples unrelated tests/framework instances and makes the
behavior depend on execution order.

test/framework/functional/output_s3.go[39-53]
test/framework/functional/output_s3.go[109-127]
test/framework/functional/framework.go[131-149]
test/framework/functional/output_s3.go[237-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
S3 test infrastructure uses package-level global variables (`s3Client`, `portForwardCmd`, `minioNamespace`) and a package-level cleanup function. This breaks isolation across `CollectorFunctionalFramework` instances and introduces fragile ordering (SetupS3Bucket assumes AddS3Output already populated `minioNamespace`).
### Issue Context
- `minioNamespace` is set in `AddS3Output`, but consumed later in `SetupS3Bucket`.
- `CollectorFunctionalFramework.Cleanup()` always calls `cleanupS3PortForward()`, which operates on the global `portForwardCmd`.
### Fix Focus Areas
- test/framework/functional/output_s3.go[39-53]
- test/framework/functional/output_s3.go[109-127]
- test/framework/functional/output_s3.go[237-246]
- test/framework/functional/framework.go[131-149]
### Suggested direction
- Add per-framework fields (e.g., `f.s3Client`, `f.s3PortForwardCmd`, `f.minioNamespace`, `f.s3LocalPort`).
- Use `f.Namespace` directly instead of a global `minioNamespace`.
- Convert `cleanupS3PortForward()` into a receiver method (e.g., `f.cleanupS3PortForward()`), and only stop the port-forward started by that framework.
- Optionally guard against calling `SetupS3Bucket` multiple times by reusing an existing port-forward or cleaning up before starting a new one.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. MinIO create errors masked 🐞 ☼ ⭐

Description

createMinIOService falls back to Get for any Service Create error (not just AlreadyExists), which
can hide real creation failures and lead to using an unexpected pre-existing Service object.

Code

test/framework/functional/output_s3.go[R86-92]

+	if err := f.Test.Create(state.minioService); err != nil {
+		// Service may already exist if there are multiple S3 outputs
+		// Try to get it instead to ensure we have the reference
+		if err := f.Test.Get(state.minioService); err != nil {
+			return fmt.Errorf("unable to create or retrieve minio service: %v", err)
+		}
+	}

Evidence

The code treats every Create error as “maybe already exists” and attempts Get, but the client Create
call returns raw API errors and should be checked for AlreadyExists explicitly before falling back.

/test/framework/functional/output_s3.go[80-93]
/test/client/client.go[95-108]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`createMinIOService` currently retries with `Get` on *any* error from `Create`, which can mask genuine failures (RBAC, invalid object, transient API issues) and make debugging test failures harder.

### Issue Context
Only `AlreadyExists` should trigger a fallback to `Get`. For any other error, return the original `Create` error.

### Fix Focus Areas
- /test/framework/functional/output_s3.go[80-93]
- /test/client/client.go[95-108]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Nil S3 client dereference 🐞 ≡ ⭐

Description

ReadLogsFromS3 verifies the framework state exists but does not verify state.client is
initialized, so calling ReadLogsFromS3 before SetupS3Bucket can panic via nil client usage.

Code

test/framework/functional/output_s3.go[R214-225]

+	state, ok := s3States[f]
+	if !ok || state == nil {
+		return nil, fmt.Errorf("S3 not initialized")
+	}
+
	var allMessages []string
	var listOutput *s3.ListObjectsV2Output

-	// wait until files are visible
	log.V(2).Info("waiting for logs in s3 bucket", "bucket", bucketName, "keyPrefix", keyPrefix)
	err := wait.PollUntilContextTimeout(context.TODO(), defaultRetryInterval, f.GetMaxReadDuration(), true, func(ctx context.Context) (done bool, err error) {
-		listOutput, err = s3Client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{
+		listOutput, err = state.client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{
			Bucket: aws.String(bucketName),

Evidence

state.client is only initialized inside SetupS3Bucket, but ReadLogsFromS3 uses it
unconditionally after only checking that state exists.

/test/framework/functional/output_s3.go[115-163]
/test/framework/functional/output_s3.go[213-250]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ReadLogsFromS3` can dereference a nil `state.client` if `SetupS3Bucket` was not invoked, causing a panic.

### Issue Context
The state map is initialized in `AddS3Output`/`SetupS3Bucket`, but the S3 client itself is created only in `SetupS3Bucket`.

### Fix Focus Areas
- /test/framework/functional/output_s3.go[115-163]
- /test/framework/functional/output_s3.go[213-250]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Free port selection race 🐞 ☼ ⭐

Description

findFreePort closes the listener before starting oc port-forward, so another process can claim the
port between selection and use, causing flaky address-in-use failures.

Code

test/framework/functional/output_s3.go[R101-113]

+// findFreePort discovers an available local port by listening on port 0
+func findFreePort() (string, error) {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		return "", err
+	}
+	defer func() {
+		_ = listener.Close()
+	}()
+
+	addr := listener.Addr().(*net.TCPAddr)
+	return fmt.Sprintf("%d", addr.Port), nil
+}

Evidence

The implementation explicitly releases the chosen port, and only later starts port-forward bound to
that same port, creating a TOCTOU window.

/test/framework/functional/output_s3.go[101-113]
/test/framework/functional/output_s3.go[115-171]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The current `findFreePort` approach can be flaky because the chosen port is released before `oc port-forward` binds to it.

### Issue Context
This is a classic time-of-check/time-of-use race. In CI with concurrent processes/tests, this can intermittently fail.

### Fix Focus Areas
- /test/framework/functional/output_s3.go[101-113]
- /test/framework/functional/output_s3.go[115-171]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (4)

7. Port-forward process leak 🐞 ☼ ⭐

Description

SetupS3Bucket overwrites state.portForwardCmd on each call without stopping any existing
port-forward process, which can leak processes and/or fail on subsequent calls due to port
conflicts.

Code

test/framework/functional/output_s3.go[R164-165]

+	state.portForwardCmd = exec.Command("oc", "port-forward", "-n", f.Namespace, "svc/minio-server", state.localPort+":9000")

-	return nil
-}
-
-func (f *CollectorFunctionalFramework) createMinIOService() error {
-	service = runtime.NewService(f.Namespace, "minio-server")
-	runtime.NewServiceBuilder(service).
-		AddServicePort(minioPort, minioPort).
-		WithSelector(map[string]string{"testname": "functional"})

Evidence

A new exec.Cmd is assigned each time SetupS3Bucket runs; the only termination path is the explicit
CleanupS3PortForward call, so repeated SetupS3Bucket calls within a test can orphan the prior
process.

/test/framework/functional/output_s3.go[115-172]
/test/framework/functional/output_s3.go[297-312]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SetupS3Bucket` can start multiple `oc port-forward` processes if called more than once, because it overwrites `state.portForwardCmd` without terminating any prior process.

### Issue Context
Even if current tests call it once, this is a sharp edge for future tests (multiple buckets, retries, etc.) and can cause resource leaks.

### Fix Focus Areas
- /test/framework/functional/output_s3.go[115-172]
- /test/framework/functional/output_s3.go[297-312]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. S3 region mismatch 🐞 ≡

Description

The S3 tests configure the output region as "us-east-1", but the MinIO bootstrap client in
SetupS3Bucket hard-codes "us-east-2" and creates buckets with LocationConstraintUsEast2. This
inconsistency can cause failures with stricter S3-compatible implementations and makes the test less
representative of the configured output.

Code

test/functional/outputs/aws/s3/forward_to_s3_test.go[R21-44]

  const (

Evidence

The test explicitly sets output.S3.Region to TestRegion which is us-east-1, while the
framework S3 client used to create the bucket signs for awsRegion (us-east-2) and uses a
LocationConstraintUsEast2. These values diverge within the same test flow.

test/functional/outputs/aws/s3/forward_to_s3_test.go[21-44]
[test/framework/functional/output_s3.go[40-47]](https://github.com/openshift/cluster-logging-operator/blob/2...

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vparfonov
Once this PR has been reviewed and has the lgtm label, please assign alanconway for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vparfonov]

/review

[qodo-code-review]

Persistent review updated to latest commit f8aa050

[openshift-ci-robot]

@vparfonov: This pull request references LOG-7913 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

Implement comprehensive functional tests for S3 output forwarding using minIO as a containerized S3-compatible mock.

Architecture:

• minIO runs as a sidecar container in the same pod as Vector collector
• Containers communicate via localhost:9000 (shared network namespace)
• Test client accesses minIO via oc port-forward on 127.0.0.1:19000
• Vector batch timeout overridden to 10s via config visitor (default 300s too slow for tests)

Test Coverage:

• Application log test: basic S3 output and log retrieval
• Audit log test: audit log type handling and key-prefix routing
• Infrastructure logs: routing to infrastructure/ prefix
• Compression table tests: 5 compression variants with decompression validation (none, gzip, zlib, snappy, zstd)
• JSON array parsing: Vector writes S3 objects as JSON arrays, properly unmarshaled

SDK Updates:

• Upgraded aws-sdk-go-v2 core: v1.9.0 → v1.41.5 (required for S3 support)
• Added aws-sdk-go-v2/service/s3 v1.98.0
• Upgraded cloudwatchlogs: v1.7.0 → v1.68.0 (fixes ComputePayloadHash middleware with newer core SDK)
• Upgraded smithy-go: v1.8.0 → v1.24.2

Testing:

• All S3 test passing
• CloudWatch tests fixed and passing with upgraded SDK versions
• No regressions to existing functional tests

/cc
/assign

Links

• Depending on PR(s):
• GitHub issue:
• JIRA: https://redhat.atlassian.net/browse/LOG-7913
• Enhancement proposal:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[vparfonov]

/review

[qodo-code-review]

Persistent review updated to latest commit 2b088ec

[openshift-ci-robot]

@vparfonov: This pull request references LOG-7913 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

Implement comprehensive functional tests for S3 output forwarding using minIO as a containerized S3-compatible mock.

Architecture:

• minIO runs as a sidecar container in the same pod as Vector collector
• Containers communicate via localhost:9000 (shared network namespace)
• Test client accesses minIO via oc port-forward
• Vector batch timeout overridden to 10s via config visitor (default 300s too slow for tests)

Test Coverage:

• Application log test: basic S3 output and log retrieval
• Audit log test: audit log type handling and key-prefix routing
• Infrastructure logs: routing to infrastructure/ prefix
• Compression table tests: 5 compression variants with decompression validation (none, gzip, zlib, snappy, zstd)
• JSON array parsing: Vector writes S3 objects as JSON arrays, properly unmarshaled

SDK Updates:

• Upgraded aws-sdk-go-v2 core: v1.9.0 → v1.41.5 (required for S3 support)
• Added aws-sdk-go-v2/service/s3 v1.98.0
• Upgraded cloudwatchlogs: v1.7.0 → v1.68.0 (fixes ComputePayloadHash middleware with newer core SDK)
• Upgraded smithy-go: v1.8.0 → v1.24.2

Testing:

• All S3 test passing
• CloudWatch tests fixed and passing with upgraded SDK versions
• No regressions to existing functional tests

/cc
/assign

Links

• Depending on PR(s):
• GitHub issue:
• JIRA: https://redhat.atlassian.net/browse/LOG-7913
• Enhancement proposal:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@vparfonov: This pull request references LOG-7913 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.8.0" version, but no target version was set.

Details

In response to this:

Description

Implement comprehensive functional tests for S3 output forwarding using minIO as a containerized S3-compatible mock.

Architecture:

• minIO runs as a sidecar container in the same pod as Vector collector
• Test client accesses minIO via port-forwarding
• Vector batch timeout overridden to 10s via config visitor (default 300s too slow for tests)

Test Coverage:

• Add test suite covering application, audit, and infrastructure log types
• Support 5 compression formats: none, gzip, snappy, zlib, zstd

SDK Updates:

• Upgraded aws-sdk-go-v2 core: v1.9.0 → v1.41.5 (required for S3 support)
• Added aws-sdk-go-v2/service/s3 v1.98.0
• Upgraded cloudwatchlogs: v1.7.0 → v1.68.0 (fixes ComputePayloadHash middleware with newer core SDK)
• Upgraded smithy-go: v1.8.0 → v1.24.2

Testing:

• All S3 test passing
• CloudWatch tests fixed and passing with upgraded SDK versions
• No regressions to existing functional tests

/cc
/assign

Links

• Depending on PR(s):
• GitHub issue:
• JIRA: https://redhat.atlassian.net/browse/LOG-7913
• Enhancement proposal:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[vparfonov]

/test functional-target

[qodo-code-review]

PR-Agent: could not fine a component named functional-target in a supported language in this PR.

[vparfonov]

/test functional-target

[qodo-code-review]

PR-Agent: could not fine a component named functional-target in a supported language in this PR.

[vparfonov]

/review

[qodo-code-review]

Persistent review updated to latest commit 953a54e

[vparfonov]

/hold cancel

[openshift-ci]

@vparfonov: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.