Skip to content

WIP: add S3Control native provider #22

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

lizard-boy
Copy link

Motivation

Currently fully implemented in Moto, it would be easier to have it native for better compatibility with the S3 v3 provider. It would also allow us to support Access Point in the v3 provider (Moto just added partial support, but it's not fully in parity with AWS).
This is a pretty small provider, we can aim for same features as Moto for now.

Implementation of S3 Access Point compatibility will come in a next PR.

Resources:

Changes

Operations to implement (currently implemented in Moto):

PublicAccessBlock
PutPublicAccessBlock
GetPublicAccessBlock
DeletePublicAccessBlock
AccessPoint (still lacking VPC config validation)
CreateAccessPoint
GetAccessPoint
ListAccessPoints (still need pagination)
DeleteAccessPoint
AccessPointPolicy
PutAccessPointPolicy
DeleteAccessPointPolicy
GetAccessPointPolicy
GetAccessPointPolicyStatus

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces a native S3Control provider for LocalStack, focusing on improving compatibility with the S3 v3 provider and supporting Access Point functionality. Key changes include:

  • New S3ControlProvider class implementing core S3 Control operations
  • S3ControlStore model for managing S3 control-related data
  • Custom S3ControlResponseSerializer for handling S3 Control API responses
  • Configuration updates to enable the native S3Control provider
  • Extensive test suite covering public access block and access point operations
  • API specification patches to align with actual S3 and S3Control service behavior

The implementation is still a work in progress, with several TODO comments indicating areas needing further development, particularly around VPC configuration validation and pagination for ListAccessPoints.

10 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings

access_points: dict[AccessPointName, GetAccessPointResult] = LocalAttribute(
default=dict
) # TODO: check locality
# TODO: check for accross-region accesses
Copy link

Choose a reason for hiding this comment

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

syntax: Fix typo: 'accross' should be 'across'

Comment on lines +136 to +138
# TODO: support VpcConfiguration
# TODO: support PublicAccessBlockConfiguration
# TODO: check bucket_account_id
Copy link

Choose a reason for hiding this comment

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

logic: Implement VpcConfiguration and PublicAccessBlockConfiguration support, and bucket_account_id check

Comment on lines +159 to +168
# TODO: what are the permissions to needed to create an AccessPoint to a bucket?
try:
connect_to(region_name=context.region).s3.head_bucket(Bucket=bucket)
except ClientError as e:
if e.response.get("Error", {}).get("Code") == "404":
raise InvalidRequest(
"Amazon S3 AccessPoint can only be created for existing bucket",
)
# TODO: find AccessDenied exception?
raise
Copy link

Choose a reason for hiding this comment

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

logic: Implement proper error handling for AccessDenied cases

Comment on lines +228 to +230
# TODO: implement pagination
# TODO: implement filter with Bucket name
# TODO: implement ordering
Copy link

Choose a reason for hiding this comment

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

logic: Implement pagination, bucket filtering, and ordering for list_access_points

Comment on lines +252 to +255
def put_access_point_policy(
self, context: RequestContext, account_id: AccountId, name: AccessPointName, policy: Policy
) -> None:
pass
Copy link

Choose a reason for hiding this comment

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

logic: Implement put_access_point_policy method

Comment on lines +257 to +260
def get_access_point_policy(
self, context: RequestContext, account_id: AccountId, name: AccessPointName
) -> GetAccessPointPolicyResult:
pass
Copy link

Choose a reason for hiding this comment

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

logic: Implement get_access_point_policy method

Comment on lines +262 to +265
def delete_access_point_policy(
self, context: RequestContext, account_id: AccountId, name: AccessPointName
) -> None:
pass
Copy link

Choose a reason for hiding this comment

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

logic: Implement delete_access_point_policy method

Comment on lines +267 to +270
def get_access_point_policy_status(
self, context: RequestContext, account_id: AccountId, name: AccessPointName
) -> GetAccessPointPolicyStatusResult:
pass
Copy link

Choose a reason for hiding this comment

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

logic: Implement get_access_point_policy_status method

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.

2 participants