Skip to content

Conversation

@vivekr-splunk
Copy link
Collaborator

@vivekr-splunk vivekr-splunk commented Aug 26, 2025

This PR fixes region parsing in the AWS S3 client and ensures downloads always fetch the latest version of objects from S3. It also improves input validation, error handling, and cleanup behavior in the download workflow.


Key Changes

  • awss3client.go

    • Updated GetRegion to correctly parse full S3 URLs and broadened regex for both path-style and virtual-hosted endpoints.
    • Fixed NewAWSS3Client to properly set Endpoint and handle region derivation.
    • Removed IfMatch in DownloadApp to avoid stale ETag errors.
    • Added optional HeadObject preflight check to log differences when provided ETag does not match current.
    • Added input validation for empty remote/local file paths.
    • Fixed cleanup logic to remove partially written local files only.
  • Tests

    • Strengthened TestNewAWSS3Client to cover valid, backward-compatibility, and invalid scenarios.
    • Updated TestAWSDownloadAppShouldNotFail, TestAWSDownloadAppShouldFail, and TestAWSDownloadAppIgnoresProvidedETagAndGetsLatest to validate latest-version behavior and error handling.

Testing and Verification

  • Ran unit tests locally:

    • TestNewAWSS3Client passes for valid endpoints, fails for invalid ones.
    • TestAWSDownloadAppShouldNotFail confirms successful downloads.
    • TestAWSDownloadAppShouldFail removes partial files on failure.
    • TestAWSDownloadAppIgnoresProvidedETagAndGetsLatest validates latest object is always downloaded.
  • Verified log messages for mismatched ETags and error conditions.

Validation results (S3 versioned bucket):

Before:
  description = MyTestApp v1

After pushing new content to same key and triggering App Framework:
  description = MyTestApp v2
  previous version preserved at default.old.<timestamp>

No 412/PreconditionFailed observed in operator logs.
Operator consistently fetches latest object version for the key.

Related Issues

  • Fix region parsing and enforce latest version download for S3 apps
  • GitHub: Internal bug report for failing TestAWSDownloadAppShouldNotFail

PR Checklist

  • Code changes adhere to the project's coding standards.
  • Relevant unit and integration tests are included.
  • Documentation has been updated accordingly.
  • All tests pass locally.
  • The PR description follows the project's guidelines.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the AWS S3 client implementation by fixing region parsing to handle both path-style and virtual-hosted S3 URLs, and enforces downloading the latest version of objects by removing ETag-based conditional downloads. The changes also add better input validation and error handling.

  • Enhanced region extraction with more robust regex pattern for S3 endpoints
  • Removed IfMatch parameter to always download latest object versions instead of conditional downloads
  • Added input validation for empty file paths and improved cleanup of partial downloads

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
awss3client.go Updated region parsing logic, removed IfMatch conditional downloads, added input validation and HeadObject interface
awss3client_test.go Added test adapters for HeadObject method, updated existing tests to work without IfMatch, added new test for latest version behavior

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}

var regionRegex = ".*.s3[-,.]([a-z]+-[a-z]+-[0-9]+)\\..*amazonaws.com"
var regionRegex = `(?i)(^|\.)(s3)[\.-]([a-z0-9-]+)\.amazonaws\.com$`
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The regex pattern uses a case-insensitive flag (?i) but then only matches lowercase characters in the region group [a-z0-9-]+. Consider either removing the case-insensitive flag or updating the character class to include uppercase letters [a-zA-Z0-9-]+ for consistency.

Suggested change
var regionRegex = `(?i)(^|\.)(s3)[\.-]([a-z0-9-]+)\.amazonaws\.com$`
var regionRegex = `(?i)(^|\.)(s3)[\.-]([a-zA-Z0-9-]+)\.amazonaws\.com$`

Copilot uses AI. Check for mistakes.
Comment on lines 66 to 70
// Ensure IfMatch is non-nil AND non-empty for older mocks that expect it.
if input.IfMatch == nil || aws.ToString(input.IfMatch) == "" {
val := "test-ifmatch" // any non-empty sentinel keeps older mocks happy
input.IfMatch = &val
}
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

This compatibility wrapper artificially sets IfMatch to satisfy legacy mocks, but the production code no longer uses IfMatch. This creates a disconnect between test and production behavior. Consider updating the legacy mocks instead of working around them with artificial values.

Suggested change
// Ensure IfMatch is non-nil AND non-empty for older mocks that expect it.
if input.IfMatch == nil || aws.ToString(input.IfMatch) == "" {
val := "test-ifmatch" // any non-empty sentinel keeps older mocks happy
input.IfMatch = &val
}

Copilot uses AI. Check for mistakes.
@coveralls
Copy link
Collaborator

coveralls commented Aug 27, 2025

Pull Request Test Coverage Report for Build 17265453835

Details

  • 36 of 51 (70.59%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.2%) to 86.421%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/splunk/client/awss3client.go 36 51 70.59%
Files with Coverage Reduction New Missed Lines %
pkg/splunk/enterprise/cp.go 1 33.33%
pkg/splunk/enterprise/util.go 2 90.38%
pkg/splunk/client/awss3client.go 6 86.61%
Totals Coverage Status
Change from base Build 17239583201: -0.2%
Covered Lines: 10724
Relevant Lines: 12409

💛 - Coveralls

Copy link
Collaborator

@patrykw-splunk patrykw-splunk left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

5 participants