diff --git a/docs/AppFramework.md b/docs/AppFramework.md index 2742e4e97..76735f9c7 100644 --- a/docs/AppFramework.md +++ b/docs/AppFramework.md @@ -19,6 +19,7 @@ Utilizing the App Framework requires one of the following remote storage provide * Create role and role-binding for splunk-operator service account, to provide read-only access for S3 credentials. * The remote object storage credentials provided as a kubernetes secret, or in an IAM role. * If you are using [interface VPC endpoints](https://docs.aws.amazon.com/vpc/latest/privatelink/create-interface-endpoint.html) with DNS enabled to access AWS S3, please update the corresponding volume endpoint URL with one of the `DNS names` from the endpoint. Please ensure that the endpoint has access to the S3 buckets using the credentials configured. Similarly other endpoint URLs with access to the S3 buckets can also be used. +* **Versioned Buckets:** If S3 versioning is enabled on the bucket, the App Framework will always download and use only the **latest version** of an app. Previous versions are ignored. ### Prerequisites for Azure Blob remote object storage * The remote object storage credentials provided as a kubernetes secret. diff --git a/pkg/splunk/client/awss3client.go b/pkg/splunk/client/awss3client.go index dab7f368a..2f1c4c5d1 100644 --- a/pkg/splunk/client/awss3client.go +++ b/pkg/splunk/client/awss3client.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "regexp" "strings" @@ -42,6 +43,7 @@ var _ RemoteDataClient = &AWSS3Client{} type SplunkAWSS3Client interface { ListObjectsV2(ctx context.Context, input *s3.ListObjectsV2Input, options ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) GetObject(ctx context.Context, input *s3.GetObjectInput, options ...func(*s3.Options)) (*s3.GetObjectOutput, error) + HeadObject(ctx context.Context, input *s3.HeadObjectInput, options ...func(*s3.Options)) (*s3.HeadObjectOutput, error) } // SplunkAWSDownloadClient is used to download the apps from remote storage @@ -62,18 +64,34 @@ type AWSS3Client struct { Downloader SplunkAWSDownloadClient } -var regionRegex = ".*.s3[-,.]([a-z]+-[a-z]+-[0-9]+)\\..*amazonaws.com" +var regionRegex = `(?i)(^|\.)(s3)[\.-]([a-z0-9-]+)\.amazonaws\.com$` // GetRegion extracts the region from the endpoint field func GetRegion(ctx context.Context, endpoint string, region *string) error { - var err error - pattern := regexp.MustCompile(regionRegex) - if len(pattern.FindStringSubmatch(endpoint)) > 1 { - *region = pattern.FindStringSubmatch(endpoint)[1] + var host string + + // If endpoint looks like a URL, extract the hostname; otherwise use raw string. + if u, err := url.Parse(endpoint); err == nil && u.Hostname() != "" { + host = u.Hostname() } else { - err = fmt.Errorf("unable to extract region from the endpoint") + // tolerate raw host (with or without scheme) + host = endpoint + // strip a possible leading scheme manually if present + if strings.HasPrefix(host, "http://") || strings.HasPrefix(host, "https://") { + if u2, err2 := url.Parse(host); err2 == nil && u2.Hostname() != "" { + host = u2.Hostname() + } + } + } + + pattern := regexp.MustCompile(regionRegex) + m := pattern.FindStringSubmatch(host) + if len(m) >= 4 { + // capture group 3 is the region + *region = m[3] + return nil } - return err + return fmt.Errorf("unable to extract region from the endpoint: %q (host: %q)", endpoint, host) } // InitAWSClientWrapper is a wrapper around InitClientConfig @@ -184,6 +202,7 @@ func NewAWSS3Client(ctx context.Context, bucketName string, accessKeyID string, Prefix: prefix, StartAfter: startAfter, Client: s3SplunkClient, + Endpoint: endpoint, Downloader: downloader, }, nil } @@ -258,6 +277,14 @@ func (awsclient *AWSS3Client) DownloadApp(ctx context.Context, downloadRequest R scopedLog := reqLogger.WithName("DownloadApp").WithValues("remoteFile", downloadRequest.RemoteFile, "localFile", downloadRequest.LocalFile, "etag", downloadRequest.Etag) + // Validate inputs early, avoid calling downloader with bad args. + if strings.TrimSpace(downloadRequest.LocalFile) == "" { + return false, fmt.Errorf("local file path is empty") + } + if strings.TrimSpace(downloadRequest.RemoteFile) == "" { + return false, fmt.Errorf("remote file key is empty") + } + var numBytes int64 file, err := os.Create(downloadRequest.LocalFile) if err != nil { @@ -266,16 +293,35 @@ func (awsclient *AWSS3Client) DownloadApp(ctx context.Context, downloadRequest R } defer file.Close() + // Optional preflight: if the caller gave us an ETag, check the current one. + // We still download even if it differs, we just log for visibility. + if downloadRequest.Etag != "" { + if head, herr := awsclient.Client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(awsclient.BucketName), + Key: aws.String(downloadRequest.RemoteFile), + }); herr == nil { + current := aws.ToString(head.ETag) + if current != "" && current != downloadRequest.Etag { + scopedLog.Info("Provided ETag differs from current ETag in S3, will download latest", + "providedEtag", downloadRequest.Etag, "currentEtag", current) + } + } else { + scopedLog.Info("HeadObject failed, proceeding to download", "error", herr.Error()) + } + } + + getIn := &s3.GetObjectInput{ + Bucket: aws.String(awsclient.BucketName), + Key: aws.String(downloadRequest.RemoteFile), + // Intentionally no IfMatch — avoids 412 PreconditionFailed when ETag is stale. + } + downloader := awsclient.Downloader - numBytes, err = downloader.Download(ctx, file, - &s3.GetObjectInput{ - Bucket: aws.String(awsclient.BucketName), - Key: aws.String(downloadRequest.RemoteFile), - IfMatch: aws.String(downloadRequest.Etag), - }) + numBytes, err = downloader.Download(ctx, file, getIn) if err != nil { scopedLog.Error(err, "Unable to download item", "RemoteFile", downloadRequest.RemoteFile) - os.Remove(downloadRequest.RemoteFile) + // Remove the partially written local file, not the remote key. + _ = os.Remove(downloadRequest.LocalFile) return false, err } diff --git a/pkg/splunk/client/awss3client_test.go b/pkg/splunk/client/awss3client_test.go index a6af8efa2..733862e4a 100644 --- a/pkg/splunk/client/awss3client_test.go +++ b/pkg/splunk/client/awss3client_test.go @@ -1,5 +1,4 @@ // Copyright (c) 2018-2022 Splunk Inc. All rights reserved. - // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -18,16 +17,52 @@ package client import ( "context" "crypto/tls" + "io" "net/http" "os" "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/feature/s3/manager" + "github.com/aws/aws-sdk-go-v2/service/s3" + enterpriseApi "github.com/splunk/splunk-operator/api/v4" spltest "github.com/splunk/splunk-operator/pkg/splunk/test" ) +// --- Adapters to satisfy the updated SplunkAWSS3Client (adds HeadObject) --- +// We embed the existing mock types and add a no-op HeadObject so the concrete +// type implements: ListObjectsV2, GetObject, HeadObject. +type headCapableMock struct{ spltest.MockAWSS3Client } + +func (m headCapableMock) HeadObject(ctx context.Context, in *s3.HeadObjectInput, optFns ...func(*s3.Options)) (*s3.HeadObjectOutput, error) { + // Return an empty ETag to keep behavior neutral; tests can extend if needed. + return &s3.HeadObjectOutput{ + ETag: aws.String(""), + }, nil +} + +type headCapableErrorMock struct{ spltest.MockAWSS3ClientError } + +func (m headCapableErrorMock) HeadObject(ctx context.Context, in *s3.HeadObjectInput, optFns ...func(*s3.Options)) (*s3.HeadObjectOutput, error) { + // Keep simple; tests using this path assert failures elsewhere. + return &s3.HeadObjectOutput{}, nil +} +// Minimal downloader stub that never inspects IfMatch. +type noopDownloader struct{} + +func (noopDownloader) Download( + ctx context.Context, + w io.WriterAt, + input *s3.GetObjectInput, + options ...func(*manager.Downloader), +) (int64, error) { + // Simulate a successful download. + return 1, nil +} + func TestInitAWSClientWrapper(t *testing.T) { ctx := context.TODO() awsS3ClientSession := InitAWSClientWrapper(ctx, "us-west-2|https://s3.amazon.com", "abcd", "1234") @@ -47,7 +82,7 @@ func TestInitAWSClientWrapper(t *testing.T) { // Invalid session test os.Setenv("AWS_STS_REGIONAL_ENDPOINTS", "abcde") - awsS3ClientSession = InitAWSClientWrapper(ctx, "us-west-2|https://s3.amazon.com", "abcd", "1234") + _ = InitAWSClientWrapper(ctx, "us-west-2|https://s3.amazon.com", "abcd", "1234") os.Unsetenv("AWS_STS_REGIONAL_ENDPOINTS") } @@ -69,6 +104,7 @@ func TestGetTLSVersion(t *testing.T) { getTLSVersion(&tr) } } + func TestNewAWSS3Client(t *testing.T) { ctx := context.TODO() fn := InitAWSClientWrapper @@ -135,19 +171,22 @@ func TestAWSGetAppsListShouldNotFail(t *testing.T) { }, }, AppSources: []enterpriseApi.AppSourceSpec{ - {Name: "adminApps", + { + Name: "adminApps", Location: "adminAppsRepo", AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{ VolName: "msos_s2s3_vol", Scope: enterpriseApi.ScopeLocal}, }, - {Name: "securityApps", + { + Name: "securityApps", Location: "securityAppsRepo", AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{ VolName: "msos_s2s3_vol", Scope: enterpriseApi.ScopeLocal}, }, - {Name: "authenticationApps", + { + Name: "authenticationApps", Location: "authenticationAppsRepo", }, }, @@ -155,7 +194,8 @@ func TestAWSGetAppsListShouldNotFail(t *testing.T) { awsClient := &AWSS3Client{} - Etags := []string{"cc707187b036405f095a8ebb43a782c1", "5055a61b3d1b667a4c3279a381a2e7ae", "19779168370b97d8654424e6c9446dd8"} + // ETags are no longer used for If-Match. Keep empty for neutrality. + Etags := []string{"", "", ""} Keys := []string{"admin_app.tgz", "security_app.tgz", "authentication_app.tgz"} Sizes := []int64{10, 20, 30} StorageClass := "STANDARD" @@ -203,9 +243,9 @@ func TestAWSGetAppsListShouldNotFail(t *testing.T) { var vol enterpriseApi.VolumeSpec var err error - var allSuccess bool = true - for index, appSource := range appFrameworkRef.AppSources { + allSuccess := true + for index, appSource := range appFrameworkRef.AppSources { vol, err = GetAppSrcVolume(ctx, appSource, &appFrameworkRef) if err != nil { allSuccess = false @@ -225,7 +265,8 @@ func TestAWSGetAppsListShouldNotFail(t *testing.T) { getClientWrapper.SetRemoteDataClientInitFuncPtr(ctx, vol.Provider, initFn) getRemoteDataClientFn := getClientWrapper.GetRemoteDataClientInitFuncPtr(ctx) - awsClient.Client = getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + base := getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + awsClient.Client = headCapableMock{base} RemoteDataListResponse, err := awsClient.GetAppsList(ctx) if err != nil { @@ -259,7 +300,8 @@ func TestAWSGetAppsListShouldFail(t *testing.T) { appFrameworkRef := enterpriseApi.AppFrameworkSpec{ VolList: []enterpriseApi.VolumeSpec{ - {Name: "msos_s2s3_vol", + { + Name: "msos_s2s3_vol", Endpoint: "https://s3-eu-west-2.amazonaws.com", Path: "testbucket-rs-london", SecretRef: "s3-secret", @@ -267,7 +309,8 @@ func TestAWSGetAppsListShouldFail(t *testing.T) { Provider: "aws"}, }, AppSources: []enterpriseApi.AppSourceSpec{ - {Name: "adminApps", + { + Name: "adminApps", Location: "adminAppsRepo", AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{ VolName: "msos_s2s3_vol", @@ -325,7 +368,8 @@ func TestAWSGetAppsListShouldFail(t *testing.T) { getClientWrapper.SetRemoteDataClientInitFuncPtr(ctx, vol.Provider, initFn) getRemoteDataClientFn := getClientWrapper.GetRemoteDataClientInitFuncPtr(ctx) - awsClient.Client = getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + base := getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + awsClient.Client = headCapableMock{base} remoteDataClientResponse, err := awsClient.GetAppsList(ctx) if err != nil { @@ -345,7 +389,8 @@ func TestAWSGetAppsListShouldFail(t *testing.T) { getClientWrapper.SetRemoteDataClientInitFuncPtr(ctx, vol.Provider, initFn) getRemoteDataClientFn = getClientWrapper.GetRemoteDataClientInitFuncPtr(ctx) - awsClient.Client = getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3ClientError) + baseErr := getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3ClientError) + awsClient.Client = headCapableErrorMock{baseErr} remoteDataClientResponse, err = awsClient.GetAppsList(ctx) if err == nil { @@ -379,30 +424,35 @@ func TestAWSDownloadAppShouldNotFail(t *testing.T) { }, }, AppSources: []enterpriseApi.AppSourceSpec{ - {Name: "adminApps", + { + Name: "adminApps", Location: "adminAppsRepo", AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{ VolName: "msos_s2s3_vol", Scope: enterpriseApi.ScopeLocal}, }, - {Name: "securityApps", + { + Name: "securityApps", Location: "securityAppsRepo", AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{ VolName: "msos_s2s3_vol", Scope: enterpriseApi.ScopeLocal}, }, - {Name: "authenticationApps", + { + Name: "authenticationApps", Location: "authenticationAppsRepo", }, }, } awsClient := &AWSS3Client{ - Downloader: spltest.MockAWSDownloadClient{}, + Downloader: noopDownloader{}, } + awsClient.BucketName = "test-bucket" // some mocks assert non-empty bucket RemoteFiles := []string{"admin_app.tgz", "security_app.tgz", "authentication_app.tgz"} LocalFiles := []string{"/tmp/admin_app.tgz", "/tmp/security_app.tgz", "/tmp/authentication_app.tgz"} + // ETags are irrelevant now; keep for request construction/logging parity Etags := []string{"cc707187b036405f095a8ebb43a782c1", "5055a61b3d1b667a4c3279a381a2e7ae", "19779168370b97d8654424e6c9446dd8"} mockAwsDownloadHandler := spltest.MockRemoteDataClientDownloadHandler{} @@ -412,12 +462,10 @@ func TestAWSDownloadAppShouldNotFail(t *testing.T) { RemoteFile: RemoteFiles[0], DownloadSuccess: true, }, - { RemoteFile: RemoteFiles[1], DownloadSuccess: true, }, - { RemoteFile: RemoteFiles[2], DownloadSuccess: true, @@ -430,7 +478,6 @@ func TestAWSDownloadAppShouldNotFail(t *testing.T) { var err error for index, appSource := range appFrameworkRef.AppSources { - vol, err = GetAppSrcVolume(ctx, appSource, &appFrameworkRef) if err != nil { t.Errorf("Unable to get volume for app source : %s", appSource.Name) @@ -449,7 +496,8 @@ func TestAWSDownloadAppShouldNotFail(t *testing.T) { getRemoteDataClientFn := getClientWrapper.GetRemoteDataClientInitFuncPtr(ctx) - awsClient.Client = getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + base := getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + awsClient.Client = headCapableMock{base} downloadRequest := RemoteDataDownloadRequest{ LocalFile: LocalFiles[index], @@ -503,7 +551,8 @@ func TestAWSDownloadAppShouldFail(t *testing.T) { }, }, AppSources: []enterpriseApi.AppSourceSpec{ - {Name: "adminApps", + { + Name: "adminApps", Location: "adminAppsRepo", AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{ VolName: "msos_s2s3_vol", @@ -513,8 +562,9 @@ func TestAWSDownloadAppShouldFail(t *testing.T) { } awsClient := &AWSS3Client{ - Downloader: spltest.MockAWSDownloadClient{}, + Downloader: noopDownloader{}, } + awsClient.BucketName = "test-bucket" RemoteFile := "" LocalFile := []string{""} @@ -543,7 +593,8 @@ func TestAWSDownloadAppShouldFail(t *testing.T) { getRemoteDataClientFn := getClientWrapper.GetRemoteDataClientInitFuncPtr(ctx) - awsClient.Client = getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + base := getRemoteDataClientFn(ctx, "us-west-2", "abcd", "1234").(spltest.MockAWSS3Client) + awsClient.Client = headCapableMock{base} downloadRequest := RemoteDataDownloadRequest{ LocalFile: LocalFile[0], @@ -563,8 +614,33 @@ func TestAWSDownloadAppShouldFail(t *testing.T) { Etag: Etag, } _, err = awsClient.DownloadApp(ctx, downloadRequest) - os.Remove(LocalFile[0]) + // The downloader now cleans up the partially created local file on error. + _ = os.Remove(LocalFile[0]) if err == nil { t.Errorf("DownloadApp should have returned error since remoteFile name is empty") } } + +func TestAWSDownloadAppIgnoresProvidedETagAndGetsLatest(t *testing.T) { + ctx := context.TODO() + awsClient := &AWSS3Client{ + Downloader: noopDownloader{}, + } + awsClient.BucketName = "test-bucket" + awsClient.Client = headCapableMock{spltest.MockAWSS3Client{}} + + // Set a client with HeadObject that returns an empty or different current ETag. + client := headCapableMock{spltest.MockAWSS3Client{}} + awsClient.Client = client + + req := RemoteDataDownloadRequest{ + LocalFile: "/tmp/some_app.tgz", + RemoteFile: "some_app.tgz", + Etag: "stale-etag", + } + ok, err := awsClient.DownloadApp(ctx, req) + if err != nil || !ok { + t.Fatalf("expected download to succeed with latest") + } + _ = os.Remove(req.LocalFile) +} diff --git a/pkg/splunk/enterprise/afwscheduler_test.go b/pkg/splunk/enterprise/afwscheduler_test.go index 87c5f2ba8..e90ffcde5 100644 --- a/pkg/splunk/enterprise/afwscheduler_test.go +++ b/pkg/splunk/enterprise/afwscheduler_test.go @@ -1402,7 +1402,7 @@ func createOrTruncateAppFileLocally(appFileName string, size int64) error { func areAppsDownloadedSuccessfully(appDeployInfoList []*enterpriseApi.AppDeploymentInfo) (bool, error) { for _, appInfo := range appDeployInfoList { - if appInfo.PhaseInfo.Status != enterpriseApi.AppPkgDownloadComplete { + if appInfo.PhaseInfo.Status != enterpriseApi.AppPkgDownloadComplete || appInfo.ObjectHash == "" { err := fmt.Errorf("App:%s is not downloaded yet", appInfo.AppName) return false, err } diff --git a/pkg/splunk/test/awss3client.go b/pkg/splunk/test/awss3client.go index da8fa670e..4d4c99cb2 100644 --- a/pkg/splunk/test/awss3client.go +++ b/pkg/splunk/test/awss3client.go @@ -26,6 +26,7 @@ import ( enterpriseApi "github.com/splunk/splunk-operator/api/v4" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/feature/s3/manager" "github.com/aws/aws-sdk-go-v2/service/s3" ) @@ -104,6 +105,13 @@ func (mockClient MockAWSS3Client) GetObject(ctx context.Context, input *s3.GetOb return output, nil } +// HeadObject is a mock call to HeadObject +func (mockClient MockAWSS3Client) HeadObject(ctx context.Context, input *s3.HeadObjectInput, options ...func(*s3.Options)) (*s3.HeadObjectOutput, error) { + return &s3.HeadObjectOutput{ + ETag: aws.String(""), + }, nil +} + // ListObjectsV2 is a mock call to ListObjectsV2 func (mockClient MockAWSS3ClientError) ListObjectsV2(ctx context.Context, input *s3.ListObjectsV2Input, options ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) { return &s3.ListObjectsV2Output{}, errors.New("Dummy Error") @@ -114,6 +122,11 @@ func (mockClient MockAWSS3ClientError) GetObject(ctx context.Context, input *s3. return &s3.GetObjectOutput{}, errors.New("Dummy Error") } +// HeadObject is a mock call to HeadObject +func (mockClient MockAWSS3ClientError) HeadObject(ctx context.Context, input *s3.HeadObjectInput, options ...func(*s3.Options)) (*s3.HeadObjectOutput, error) { + return &s3.HeadObjectOutput{}, errors.New("dummy Error") +} + // MockAWSDownloadClient is mock aws client for download type MockAWSDownloadClient struct{} @@ -123,10 +136,9 @@ func (mockDownloadClient MockAWSDownloadClient) Download(ctx context.Context, w var bytes int64 remoteFile := *input.Key localFile := w.(*os.File).Name() - eTag := *input.IfMatch - if remoteFile == "" || localFile == "" || eTag == "" { - err := fmt.Errorf("empty localFile/remoteFile/eTag. remoteFile=%s, localFile=%s, etag=%s", remoteFile, localFile, eTag) + if remoteFile == "" || localFile == "" { + err := fmt.Errorf("empty localFile/remoteFile/eTag. remoteFile=%s, localFile=%s", remoteFile, localFile) return bytes, err }