Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEW API: GetObjectAttributes #18732

Merged
merged 16 commits into from
Jan 5, 2024
Merged

Conversation

zveinn
Copy link
Contributor

@zveinn zveinn commented Jan 2, 2024

Related PRs

About

GetObjectAttributes is a combination of HeadObject and ListParts.
Within minio-go HeadObject is represented by StatObject and ListParts is represented by ListObjectParts.
We however did not need to use ListObjectParts since StatObject functions like a combination of HeadObject and ListParts.
We were able to extract all required information by just using StatObject.

Notes

This is my first time integrating an S3 API into minio, so I would appriciate a high level of scrutiny.
There are a few things that are present in the headObjectHandler that were not implemented in the GetObjectAttributes API:

  • Caching layer
  • Archive support which can be seen in HeadObjectHandler
  • Proxy layer calls during active-active replication
    If any of these are relevant for this API then I would appriciate some assistance from someone that understands these
    mechanisms better.

Things to consider

Some error returns are not compatible with Amazons GetObjectAttributes API, but should be consistent with other minio APIs.

Post PR work

Testing

The testing for this new API is done using the functional test suite in the minio-go repository.
Running the suite will use the play.min.io credentials by default, which will not work since this API has not been deployed on that environment.

Including or not including s3:ListBucket can change the error response when objects/versions are not found.
Details can be found under "permissions" within the S3 documentation.

How to test

Make sure you have the right code versions checked out

Set up minio with HTTPS

To run all the tests related to GetObjectAttributes we need HTTPS enabled for minio.
The best way to do that locally is to use certgen and the --certs-dir flag.

Create Policies

You need to create a policy which contains S3:GetObjectAttributes and S3:GetObjectVersionAttributes.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "s3:ListBucket",
                "s3:GetObject",
                "s3:GetObjectVersion",
                "s3:GetObjectAttributes",
                "s3:GetObjectVersionAttributes"
            ],
            "Resource": [
                "arn:aws:s3:::*"
            ]
        }
    ]
}

Modify the functional_tests.go file

Modification of the functional_test.go file is required since it is designed to run against an internal testing site.

Modify the main function so that the three functions related to GetObjectAttributes API are placed above the isfullMode() check.

Remember to include an os.Exit(1) so you don't run the entire suite.

	testGetObjectAttributes()
	testGetObjectAttributesErrorCases()
	testGetObjectAttributesSSECEncryption()
	os.Exit(1)

	// execute tests
	if isFullMode() {

Set environment variables and run the tests

export SERVER_ENDPOINT=127.0.0.1:9000
export ACCESS_KEY=[user_with_relevant_policies]
export SECRET_KEY=[user_secret_key]
export ENABLE_HTTPS=true
export MINT_MODE=true
export SKIP_CERT_VALIDATION=true

go run functional_tests.go

Types of changes

  • [ x ] New feature (non-breaking change which adds functionality)

@zveinn zveinn changed the title Initial commit NEW API: GetObjectAttributes Jan 2, 2024
Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

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

Minor stuff...

cmd/api-errors.go Outdated Show resolved Hide resolved
cmd/object-api-interface.go Outdated Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
cmd/object-api-datatypes.go Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
cmd/object-handlers.go Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
zveinn and others added 9 commits January 2, 2024 16:23
Co-authored-by: Klaus Post <klauspost@gmail.com>
Co-authored-by: Klaus Post <klauspost@gmail.com>
Co-authored-by: Klaus Post <klauspost@gmail.com>
Co-authored-by: Klaus Post <klauspost@gmail.com>
Co-authored-by: Klaus Post <klauspost@gmail.com>
Co-authored-by: Klaus Post <klauspost@gmail.com>
cmd/object-handlers.go Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Few comments

cmd/object-handlers.go Show resolved Hide resolved
cmd/object-handlers.go Outdated Show resolved Hide resolved
internal/hash/checksum.go Show resolved Hide resolved
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Few comments

cmd/object-handlers.go Show resolved Hide resolved
internal/hash/checksum.go Show resolved Hide resolved
Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana merged commit 9b8ba97 into minio:master Jan 5, 2024
19 checks passed
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.

None yet

4 participants