diff --git a/.github/workflows/functional-sidecar.yml b/.github/workflows/functional-sidecar.yml new file mode 100644 index 00000000..5cf0a896 --- /dev/null +++ b/.github/workflows/functional-sidecar.yml @@ -0,0 +1,31 @@ +name: functional tests (sidecar) +permissions: {} +on: pull_request + +jobs: + build: + name: RunTests + runs-on: ubuntu-latest + steps: + + - name: Checkout + uses: actions/checkout@v6 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version: 'stable' + id: go + + - name: Get Dependencies + run: | + go mod download + + - name: Build and Run + run: | + make testbin + ./runtests.sh --sidecar + + - name: Coverage Report + run: | + go tool covdata percent -i=/tmp/covdata diff --git a/.github/workflows/system.yml b/.github/workflows/system.yml index 75909d3b..5cfb41c9 100644 --- a/.github/workflows/system.yml +++ b/.github/workflows/system.yml @@ -50,10 +50,11 @@ jobs: sudo apt-get update sudo apt-get install s3cmd - - name: Install mc - run: | - curl https://dl.min.io/client/mc/release/linux-amd64/mc --create-dirs -o /usr/local/bin/mc - chmod 755 /usr/local/bin/mc +# disable mc tests due to dl.min.io instability +# - name: Install mc +# run: | +# curl https://dl.min.io/client/mc/release/linux-amd64/mc --create-dirs -o /usr/local/bin/mc +# chmod 755 /usr/local/bin/mc - name: Install xml libraries (for rest) run: | diff --git a/Makefile b/Makefile index 0250e001..6da6d45f 100644 --- a/Makefile +++ b/Makefile @@ -100,5 +100,10 @@ up-app: # Run the host-style tests in docker containers .PHONY: test-host-style test-host-style: - docker compose -f tests/host-style-tests/docker-compose.yml up --build --abort-on-container-exit --exit-code-from test + @compose_file=tests/host-style-tests/docker-compose.yml; \ + COMPOSE_MENU=false docker compose -f "$$compose_file" down -v --remove-orphans >/dev/null 2>&1 || true; \ + COMPOSE_MENU=false docker compose -f "$$compose_file" up --build --abort-on-container-exit --exit-code-from test; \ + status=$$?; \ + COMPOSE_MENU=false docker compose -f "$$compose_file" down -v --remove-orphans; \ + exit $$status diff --git a/README.md b/README.md index a7fa2f6b..ff3bc57a 100644 --- a/README.md +++ b/README.md @@ -90,7 +90,6 @@ Our multi-layered testing strategy includes: - **System Tests** - Protocol-level validation using industry-standard S3 clients: - AWS CLI - Official AWS command-line tools - s3cmd - Popular S3 client - - MinIO mc - Modern S3-compatible client - Direct REST API testing with curl for request/response validation - **Security Testing** - Both HTTP and HTTPS configurations tested. Vulnerability scanning with govulncheck. And regular dependency updates with dependabot. - **Compatibility Testing** - Multiple backends, versioning scenarios, static bucket modes, and various authentication methods. diff --git a/backend/meta/sidecar.go b/backend/meta/sidecar.go index f121815c..98082af8 100644 --- a/backend/meta/sidecar.go +++ b/backend/meta/sidecar.go @@ -145,15 +145,24 @@ func (s SideCar) ListAttributes(bucket, object string) ([]string, error) { } // DeleteAttributes removes all attributes for an object or a bucket. +// When object is empty the entire bucket sidecar directory is removed, +// cleaning up any orphaned object or multipart metadata within it. func (s SideCar) DeleteAttributes(bucket, object string) error { - metadir := filepath.Join(s.dir, bucket, object, sidecarmeta) if object == "" { - metadir = filepath.Join(s.dir, bucket, sidecarmeta) + // Remove the entire bucket sidecar directory so that orphaned + // object/multipart metadata does not accumulate after DeleteBucket. + bucketDir := filepath.Join(s.dir, bucket) + err := os.RemoveAll(bucketDir) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to remove bucket attributes: %w", err) + } + return nil } + metadir := filepath.Join(s.dir, bucket, object, sidecarmeta) err := os.RemoveAll(metadir) if err != nil && !errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("failed to remove attributes: %v", err) + return fmt.Errorf("failed to remove attributes: %w", err) } s.cleanupEmptyDirs(metadir, bucket, object) return nil diff --git a/backend/posix/posix.go b/backend/posix/posix.go index d2c8a8ab..680dfa38 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -641,6 +641,13 @@ func (p *Posix) DeleteBucket(ctx context.Context, bucket string) error { if err != nil { return fmt.Errorf("remove bucket: %w", err) } + // Bucket data is already removed; a metadata cleanup failure orphans only + // the sidecar directory, which is not user-visible. Log and continue rather + // than returning an error that would mislead callers into thinking the + // bucket still exists. + if err = p.meta.DeleteAttributes(bucket, ""); err != nil { + debuglogger.Logf("failed to delete bucket sidecar attributes (%q): %v", bucket, err) + } // Remove the bucket from versioning directory if p.versioningEnabled() { err = os.RemoveAll(filepath.Join(p.versioningDir, bucket)) @@ -882,8 +889,12 @@ func (p *Posix) deleteNullVersionIdObject(bucket, key string) error { if errors.Is(err, fs.ErrNotExist) { return nil } + if err != nil { + return err + } - return err + _ = p.meta.DeleteAttributes(versionPath, "") + return nil } func isRemovableAttr(attr string) bool { @@ -1042,6 +1053,18 @@ func (p *Posix) ensureNotDeleteMarker(bucket, object, versionId string) error { return nil } + // With path-based metadata backends (e.g. sidecar), RetrieveAttribute + // returns ErrNoSuchKey whether the sidecar attribute is absent OR the + // data file simply doesn't exist — the two cases are indistinguishable + // from metadata alone. Verify the data file directly so callers + // receive the correct NoSuchVersion / NoSuchKey error. + if _, statErr := os.Stat(filepath.Join(bucket, object)); errors.Is(statErr, fs.ErrNotExist) || errors.Is(statErr, syscall.ENOTDIR) { + if versionId != "" { + return s3err.GetAPIError(s3err.ErrNoSuchVersion) + } + return s3err.GetAPIError(s3err.ErrNoSuchKey) + } + _, err := p.meta.RetrieveAttribute(nil, bucket, object, deleteMarkerKey) if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) { if versionId != "" { @@ -1518,6 +1541,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // cleanup object if returning error os.RemoveAll(filepath.Join(tmppath, uploadID)) os.Remove(tmppath) + _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, err } } @@ -1536,6 +1560,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // cleanup object if returning error os.RemoveAll(filepath.Join(tmppath, uploadID)) os.Remove(tmppath) + _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, err } @@ -1549,6 +1574,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // cleanup object if returning error os.RemoveAll(filepath.Join(tmppath, uploadID)) os.Remove(tmppath) + _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, err } } @@ -1564,6 +1590,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // cleanup object if returning error os.RemoveAll(filepath.Join(tmppath, uploadID)) os.Remove(tmppath) + _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, fmt.Errorf("parse object lock retention: %w", err) } err = p.PutObjectRetention(withCtxNoSlot(ctx), bucket, filepath.Join(objdir, uploadID), "", retParsed) @@ -1574,6 +1601,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // cleanup object if returning error os.RemoveAll(filepath.Join(tmppath, uploadID)) os.Remove(tmppath) + _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, err } } @@ -1588,6 +1616,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // cleanup object if returning error _ = os.RemoveAll(filepath.Join(tmppath, uploadID)) _ = os.Remove(tmppath) + _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, fmt.Errorf("store mp checksum algorithm: %w", err) } } @@ -2051,6 +2080,10 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C if err != nil { return res, "", fmt.Errorf("create object version: %w", err) } + // Clean up object-lock attrs that may have leaked from the previous + // version's path-based metadata into this (new) version's sidecar. + _ = p.meta.DeleteAttribute(bucket, object, objectLegalHoldKey) + _ = p.meta.DeleteAttribute(bucket, object, objectRetentionKey) } // if the versioning is enabled, generate a new versionID for the object @@ -2488,6 +2521,11 @@ func (p *Posix) AbortMultipartUpload(ctx context.Context, mpu *s3.AbortMultipart } os.Remove(objdir) + // Clean up sidecar metadata for the aborted upload. With xattr this is + // a no-op; with sidecar the metadata directory would otherwise be orphaned. + uploadMetaPath := filepath.Join(MetaTmpMultipartDir, fmt.Sprintf("%x", sum), uploadID) + _ = p.meta.DeleteAttributes(bucket, uploadMetaPath) + return nil } @@ -3556,6 +3594,13 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje if err != nil { return s3response.PutObjectOutput{}, fmt.Errorf("create object version: %w", err) } + // With path-based metadata backends (e.g. sidecar), object-lock + // attributes written on the previous version persist at this path + // after createObjVersion because metadata is not replaced atomically + // the way xattrs are on file rename. Delete them so they do not + // bleed into the new version. + _ = p.meta.DeleteAttribute(*po.Bucket, *po.Key, objectLegalHoldKey) + _ = p.meta.DeleteAttribute(*po.Bucket, *po.Key, objectRetentionKey) } } if errors.Is(err, syscall.ENAMETOOLONG) { @@ -3644,6 +3689,11 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje return s3response.PutObjectOutput{}, err } versionID = nullVersionId + // Clear any stale versionId sidecar attribute left from a previous + // versioned object at this path. With xattr this is implicit (the + // new file carries only the attrs set on the tmpfile), but with + // path-based metadata the old attr persists until explicitly deleted. + _ = p.meta.DeleteAttribute(*po.Bucket, *po.Key, versionIdKey) } var sum string @@ -3921,6 +3971,16 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return nil, fmt.Errorf("get obj versionId: %w", err) } if errors.Is(err, meta.ErrNoSuchKey) { + // With sidecar, ErrNoSuchKey means "attribute absent" regardless of + // whether the data file exists. If the file is absent the object + // does not exist at all → AWS returns success for DeleteObject. + // Also handle ENOTDIR: when a key such as "foo/bar" is requested + // but "foo" is a regular file (not a directory), the path cannot + // contain any object. + _, statErr := os.Stat(filepath.Join(bucket, object)) + if errors.Is(statErr, fs.ErrNotExist) || errors.Is(statErr, syscall.ENOTDIR) { + return &s3.DeleteObjectOutput{VersionId: input.VersionId}, nil + } vId = []byte(nullVersionId) } @@ -3994,6 +4054,15 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return nil, fmt.Errorf("link tmp file: %w", err) } + // With path-based metadata (sidecar) the live object's attrs are + // not replaced atomically. The restored version may not have all + // the attrs that the deleted version had (e.g. a null version has + // no versionIdKey). Clear attrs that belong to the deleted version + // before copying the restored version's attrs so that the restored + // version presents a clean state. + _ = p.meta.DeleteAttribute(bucket, object, versionIdKey) + _ = p.meta.DeleteAttribute(bucket, object, deleteMarkerKey) + attrs, err := p.meta.ListAttributes(versionPath, srcVersionId) if err != nil { return nil, fmt.Errorf("list object attributes: %w", err) @@ -4016,6 +4085,7 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return nil, fmt.Errorf("remove obj version %w", err) } + _ = p.meta.DeleteAttributes(versionPath, srcVersionId) p.removeParents(filepath.Join(p.versioningDir, bucket), filepath.Join(genObjVersionKey(object), *input.VersionId)) return &s3.DeleteObjectOutput{ @@ -4042,6 +4112,7 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return nil, fmt.Errorf("delete object: %w", err) } + _ = p.meta.DeleteAttributes(versionPath, *input.VersionId) p.removeParents(filepath.Join(p.versioningDir, bucket), filepath.Join(genObjVersionKey(object), *input.VersionId)) return &s3.DeleteObjectOutput{ @@ -5553,6 +5624,19 @@ func (p *Posix) GetObjectTagging(ctx context.Context, bucket, object, versionId return nil, err } + if versionId == "" { + _, err = os.Stat(filepath.Join(bucket, object)) + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) { + return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) + } + if errors.Is(err, syscall.ENAMETOOLONG) { + return nil, s3err.GetAPIError(s3err.ErrKeyTooLong) + } + if err != nil { + return nil, fmt.Errorf("stat object: %w", err) + } + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? @@ -5630,6 +5714,19 @@ func (p *Posix) PutObjectTagging(ctx context.Context, bucket, object, versionId return err } + if versionId == "" { + _, err = os.Stat(filepath.Join(bucket, object)) + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) { + return s3err.GetAPIError(s3err.ErrNoSuchKey) + } + if errors.Is(err, syscall.ENAMETOOLONG) { + return s3err.GetAPIError(s3err.ErrKeyTooLong) + } + if err != nil { + return fmt.Errorf("stat object: %w", err) + } + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index 3688a656..b4523674 100644 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -32,7 +32,15 @@ case "$backend" in ;; esac -set -- "$backend" +# Global flags must precede the backend subcommand. +if [ -n "${VGW_ARGS:-}" ]; then + # shellcheck disable=SC2086 + set -- ${VGW_ARGS} +else + set -- +fi + +set -- "$@" "$backend" if [ -n "${VGW_BACKEND_ARG:-}" ]; then set -- "$@" "$VGW_BACKEND_ARG" @@ -43,9 +51,4 @@ if [ -n "${VGW_BACKEND_ARGS:-}" ]; then set -- "$@" ${VGW_BACKEND_ARGS} fi -if [ -n "${VGW_ARGS:-}" ]; then - # shellcheck disable=SC2086 - set -- "$@" ${VGW_ARGS} -fi - exec "$BIN" "$@" diff --git a/runtests.sh b/runtests.sh index 02cf0a24..f0afff56 100755 --- a/runtests.sh +++ b/runtests.sh @@ -1,5 +1,21 @@ #!/bin/bash +# parse options +USE_SIDECAR=false +for arg in "$@"; do + case "$arg" in + --sidecar) USE_SIDECAR=true ;; + esac +done + +# build sidecar flag for versitygw invocations +SIDECAR_FLAG="" +if $USE_SIDECAR; then + rm -rf /tmp/sidecar + mkdir /tmp/sidecar + SIDECAR_FLAG="--sidecar /tmp/sidecar" +fi + # make temp dirs rm -rf /tmp/gw mkdir /tmp/gw @@ -26,7 +42,7 @@ openssl req -new -x509 -key key.pem -out cert.pem -days 365 -subj "/C=US/ST=Cali ECHO "Running the sdk test over http" # run server in background not versioning-enabled # port: 7070(default) -GOCOVERDIR=/tmp/covdata ./versitygw -a user -s pass --iam-dir /tmp/gw posix /tmp/gw & +GOCOVERDIR=/tmp/covdata ./versitygw -a user -s pass --iam-dir /tmp/gw posix $SIDECAR_FLAG /tmp/gw & GW_PID=$! sleep 1 @@ -63,7 +79,7 @@ ECHO "Running the sdk test over https" # run server in background with TLS certificate # port: 7071(default) -GOCOVERDIR=/tmp/https.covdata ./versitygw --cert "$PWD/cert.pem" --key "$PWD/key.pem" -p :7071 -a user -s pass --iam-dir /tmp/gw posix /tmp/gw & +GOCOVERDIR=/tmp/https.covdata ./versitygw --cert "$PWD/cert.pem" --key "$PWD/key.pem" -p :7071 -a user -s pass --iam-dir /tmp/gw posix $SIDECAR_FLAG /tmp/gw & GW_HTTPS_PID=$! sleep 1 @@ -99,7 +115,7 @@ kill $GW_HTTPS_PID ECHO "Running the sdk test over http against the versioning-enabled gateway" # run server in background versioning-enabled # port: 7072 -GOCOVERDIR=/tmp/versioning.covdata ./versitygw -p :7072 -a user -s pass --iam-dir /tmp/gw posix --versioning-dir /tmp/versioningdir /tmp/gw & +GOCOVERDIR=/tmp/versioning.covdata ./versitygw -p :7072 -a user -s pass --iam-dir /tmp/gw posix $SIDECAR_FLAG --versioning-dir /tmp/versioningdir /tmp/gw & GW_VS_PID=$! # wait a second for server to start up @@ -131,7 +147,7 @@ kill $GW_VS_PID ECHO "Running the sdk test over https against the versioning-enabled gateway" # run server in background versioning-enabled # port: 7073 -GOCOVERDIR=/tmp/versioning.https.covdata ./versitygw --cert "$PWD/cert.pem" --key "$PWD/key.pem" -p :7073 -a user -s pass --iam-dir /tmp/gw posix --versioning-dir /tmp/versioningdir /tmp/gw & +GOCOVERDIR=/tmp/versioning.https.covdata ./versitygw --cert "$PWD/cert.pem" --key "$PWD/key.pem" -p :7073 -a user -s pass --iam-dir /tmp/gw posix $SIDECAR_FLAG --versioning-dir /tmp/versioningdir /tmp/gw & GW_VS_HTTPS_PID=$! # wait a second for server to start up @@ -163,7 +179,7 @@ kill $GW_VS_HTTPS_PID ECHO "Running No ACL integration tests" # run server in background versioning-enabled # port: 7073 -GOCOVERDIR=/tmp/noacl.covdata ./versitygw -p :7074 -a user -s pass -noacl --iam-dir /tmp/gw posix /tmp/gw & +GOCOVERDIR=/tmp/noacl.covdata ./versitygw -p :7074 -a user -s pass -noacl --iam-dir /tmp/gw posix $SIDECAR_FLAG /tmp/gw & GW_NO_ACL_PID=$! # wait a second for server to start up diff --git a/tests/generate_matrix.sh b/tests/generate_matrix.sh index 72ee9ab4..f0d110a5 100755 --- a/tests/generate_matrix.sh +++ b/tests/generate_matrix.sh @@ -20,15 +20,36 @@ source ./tests/drivers/params.sh set -euo pipefail +# Tests to exclude from the matrix (matched against file basename without extension) +skip_list=( + "mc" + "mc_file_count" +) + files=() iam_types=() regions=() idx=0 +is_skipped() { + local basename="${1##*/}" + local name="${basename%.sh}" + name="${name#test_}" + for skip in "${skip_list[@]}"; do + if [[ "$name" == "$skip" ]]; then + return 0 + fi + done + return 1 +} + check_for_and_load_test_file_and_params() { if ! check_param_count_v2 "file name" 1 $#; then exit 1 fi + if is_skipped "$1"; then + return 0 + fi if grep -q '@test' "$1"; then if [ $(( idx % 8 )) -eq 0 ]; then iam="s3" diff --git a/tests/integration/concurrency.go b/tests/integration/concurrency.go index f9b0f5f3..9e6c5d7c 100644 --- a/tests/integration/concurrency.go +++ b/tests/integration/concurrency.go @@ -37,8 +37,12 @@ func NewTestState(ctx context.Context, conf *S3Conf, parallel bool) *TestState { parallel: parallel, } - // Start background test processor (only used in parallel mode) - go ts.process() + // Start background test processor (only used in parallel mode). + // Track it in the WaitGroup so Wait() doesn't return until process() + // has drained mainCh and all launched goroutines have finished. + ts.wg.Go(func() { + ts.process() + }) return ts } @@ -99,9 +103,12 @@ func (ct *TestState) process() { // Wait blocks until all queued parallel tests complete, then runs all // synchronous tests. It also ensures proper cleanup of the test channel. func (ct *TestState) Wait() { - // Wait for all parallel tests to finish - ct.wg.Wait() + // Close the channel first so process() drains remaining items and exits. + // This must happen before wg.Wait() to avoid a race where wg.Wait() + // returns while process() still has buffered tests yet to start. close(ct.mainCh) + // Wait for process() goroutine and all test goroutines to finish. + ct.wg.Wait() // Run all synchronous tests sequentially for _, fn := range ct.syncTests { diff --git a/tests/integration/utils.go b/tests/integration/utils.go index dd992453..f30d847b 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -127,9 +127,10 @@ func teardown(s *S3Conf, bucket string) error { for attempts < maxRetryAttempts { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ - Bucket: bucket, - Key: key, - VersionId: versionId, + Bucket: bucket, + Key: key, + VersionId: versionId, + BypassGovernanceRetention: aws.Bool(true), }) cancel() if err == nil { diff --git a/webui/web/buckets.html b/webui/web/buckets.html index 70dfa275..ab0366e3 100644 --- a/webui/web/buckets.html +++ b/webui/web/buckets.html @@ -221,7 +221,7 @@ under the License. -