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

feat(s3control): Add S3 AccessPoint and AccessPointPolicy #1784

Conversation

kbujanecki-dt
Copy link
Contributor

@kbujanecki-dt kbujanecki-dt commented Jun 19, 2023

Description of your changes

Fixes #1782

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

apiVersion: s3control.aws.crossplane.io/v1alpha1
kind: AccessPoint
metadata:
  name: uber-access-point
spec:
  deletionPolicy: Orphan
  forProvider:
    accountID: "123456789012"
    bucket: "test-bucket"
    region: "eu-central-1"
    publicAccessBlockConfiguration:
      blockPublicACLs: true
      blockPublicPolicy: true
      ignorePublicACLs: true
      restrictPublicBuckets: true
    vpcConfiguration:
      vpcID: "vpc-XXXXXXXXXXX"
  providerConfigRef:
    name: "example"
apiVersion: s3control.aws.crossplane.io/v1alpha1
kind: AccessPointPolicy
metadata:
  name: uber-access-point-policy
spec:
  providerConfigRef:
    name: "example"
  forProvider:
    accessPointNameSelector:
      name: "uber-access-point"
    region: "eu-central-1"
    accountID: "123456789012"
    policy: >
      {
          "Version": "2012-10-17",
          "Statement": [
              {
                  "Effect": "Allow",
                  "Principal": {
                      "AWS": "arn:aws:iam::123456789012:role/sso/accessRole"
                  },
                  "Action": "s3:*",
                  "Resource": "arn:aws:s3:eu-central-1:123456789012:accesspoint/uber-access-point/object/*"
              }
          ]
      } 

@kbujanecki-dt kbujanecki-dt force-pushed the feature/s3control-access-points branch 2 times, most recently from 44af070 to 6dd0820 Compare June 20, 2023 07:55
@przysiadZeSztanga
Copy link

fyi, we plan to add policy for s3 access point in another PR

@kbujanecki-dt kbujanecki-dt changed the title Add S3 AccessPoint from s3control API Add S3 AccessPoint and AccessPointPolicy from s3control API Jun 21, 2023
@przysiadZeSztanga
Copy link

S3 access poit policies and selectors were added.

@przysiadZeSztanga
Copy link

@MisterMX I saw that you did several reviews, could you have a look?

apis/s3control/v1alpha1/zz_access_point_policy.go Outdated Show resolved Hide resolved
pkg/controller/s3control/accesspoint/setup.go Outdated Show resolved Hide resolved
@kbujanecki-dt
Copy link
Contributor Author

@MisterMX, thx you for the feedback. I implemented the requested changes.

CR for AccessPoint after changes:

kind: AccessPoint
metadata:
  name: access-point
spec:
  deletionPolicy: Delete
  forProvider:
    accountID: "123456789012"
    bucketName: example-bucket
    region: "eu-central-1"
    publicAccessBlockConfiguration:
      blockPublicACLs: true
      blockPublicPolicy: true
      ignorePublicACLs: true
      restrictPublicBuckets: true
    policy:
      statements:
        - notAction:
            - s3:DeleteBucket
          effect: Allow
          principal:
            awsPrincipals:
              - iamRoleArn: "arn:aws:iam::123456789012:role/sso/localRoleGitOpsArgoCDDeploy"
          resource:
            - "arn:aws:s3:eu-central-1:123456789012:accesspoint/access-point/object/*"
      version: 2012-10-17
    vpcConfiguration:
      vpcID: "vpc-1234567890123456"
  providerConfigRef:
    name: "example"

The structure for the access point policy body was copied from BucketPolicyBody.

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

@kbujanecki-dt thank you very much for your contribution. In order to avoid duplicate code it would be great if you could reuse the implementation from s3 which is already tested.

apis/s3control/v1alpha1/access_point_policy_types.go Outdated Show resolved Hide resolved
apis/s3control/v1alpha1/custom_types.go Outdated Show resolved Hide resolved
pkg/controller/s3control/accesspoint/hooks_test.go Outdated Show resolved Hide resolved
pkg/controller/s3control/accesspoint/policy.go Outdated Show resolved Hide resolved
@MisterMX
Copy link
Collaborator

@kbujanecki-dt this looks good to me now. Thank you very much for your contribution! Can you please squash your changes into a single commit? Then I can merge it.

@MisterMX MisterMX changed the title Add S3 AccessPoint and AccessPointPolicy from s3control API feat(s3control): Add S3 AccessPoint and AccessPointPolicy Jul 26, 2023
@kbujanecki-dt kbujanecki-dt force-pushed the feature/s3control-access-points branch from 36d36e3 to c8e9f65 Compare July 31, 2023 10:42
@kbujanecki-dt
Copy link
Contributor Author

kbujanecki-dt commented Jul 31, 2023

@kbujanecki-dt this looks good to me now. Thank you very much for your contribution! Can you please squash your changes into a single commit? Then I can merge it.

I squashed commits and pushed. On my fork I see a single commit but PR view looks unrefreshed (13 commits)
It looks ok now.
@MisterMX thank you for your time, really appreciate that. We are waiting for merge.

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for your contribution @kbujanecki-dt!

@MisterMX MisterMX merged commit c9f619a into crossplane-contrib:master Aug 3, 2023
8 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.

Add S3 AccessPoint and AccessPointPolicy from s3control API
3 participants