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.

Sign up for GitHub

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

Jump to bottom

When using an s3 bucket, add a vpc endpoint #4577

Merged
merged 1 commit into from
Nov 6, 2023
Merged

When using an s3 bucket, add a vpc endpoint #4577

merged 1 commit into from
Nov 6, 2023

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Oct 13, 2023 •
edited
Loading

What type of PR is this?

What this PR does / why we need it:

/kind feature

Attach the routing tables to an s3 regional vpc endpoint.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

ACTION REQUIRED
If you're using the S3 bucket functionality, the following permissions are required for the VPCEndpoint to be managed
`ec2:CreateVpcEndpoint`, `ec2:ModifyVpcEndpoint`, `ec2:DeleteVpcEndpoints`, `ec2:DescribeVpcEndpoints`. Rerun clusterawsadm to update IAM permissions, or update it manually.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2023
@vincepri
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e

@vincepri
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e

@vincepri vincepri force-pushed the s3-endpoint branch 6 times, most recently from b5a8386 to 9b5cd66 Compare October 16, 2023 16:17
@richardcase richardcase added this to the v2.3.0 milestone Oct 16, 2023
@vincepri
Copy link
Member Author

/retest

@vincepri
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 17, 2023
@vincepri
Copy link
Member Author

@richardcase wdyt of the release note?

@vincepri vincepri force-pushed the s3-endpoint branch 2 times, most recently from 92480c8 to 16d3851 Compare October 17, 2023 14:26
@vincepri
Copy link
Member Author

/test pull-cluster-api-provider-aws-build

@vincepri
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e

AndiDog
AndiDog reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@AndiDog AndiDog left a comment

Choose a reason for hiding this comment

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

Can we please describe in code (function docs for reconcileVPCEndpoints) and this PR why exactly this is helpful, required, etc.? Maybe link to https://docs.aws.amazon.com/vpc/latest/privatelink/vpc-endpoints-s3.html. Is this a difference in pricing? I guess requests then don't require a NAT/internet gateway anymore? The benefits should be substantial since we need quite a few new permissions.

pkg/cloud/services/network/network.go Outdated Show resolved Hide resolved
@vincepri vincepri force-pushed the s3-endpoint branch 2 times, most recently from 31637f8 to 7d416aa Compare October 18, 2023 15:18
AndiDog
AndiDog reviewed Oct 21, 2023
View reviewed changes
pkg/cloud/services/network/network.go Show resolved Hide resolved
api/v1beta2/conditions_consts.go Show resolved Hide resolved
pkg/cloud/services/network/vpc.go Outdated Show resolved Hide resolved
pkg/cloud/services/network/vpc.go Outdated Show resolved Hide resolved
pkg/cloud/services/network/vpc.go Show resolved Hide resolved
pkg/cloud/services/network/vpc.go Show resolved Hide resolved
test/e2e/suites/unmanaged/unmanaged_functional_test.go Show resolved Hide resolved
@vincepri
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e

AndiDog
AndiDog reviewed Oct 22, 2023
View reviewed changes
Copy link
Contributor

@AndiDog AndiDog left a comment

Choose a reason for hiding this comment

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

I'm okay with the change, but the tests are currently failing.

pkg/cloud/services/network/vpc.go Outdated
Values: []*string{&vpc.ID},
}),
}
// Create a map of existing endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor]

This comment seems outdated. You're returning a slice.

test/e2e/suites/unmanaged/unmanaged_functional_test.go Show resolved Hide resolved
@vincepri
Copy link
Member Author

@AndiDog comments have been addressed, ptal

@AndiDog
Copy link
Contributor

AndiDog commented Oct 24, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023
@richardcase
Copy link
Member

/test pull-cluster-api-provider-aws-e2e

richardcase
richardcase reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @vincepri . Lets get the e2e passing and then i think we are good to go on this.

@k8s-ci-robot
Copy link
Contributor

@vincepri: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-aws-e2e 4838af101c7b9364df08229b37697fdea95e941d link false /test pull-cluster-api-provider-aws-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2023
@AndiDog
Copy link
Contributor

AndiDog commented Nov 6, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2023
@vincepri
Copy link
Member Author

vincepri commented Nov 6, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2023
@k8s-ci-robot k8s-ci-robot merged commit 3176fea into kubernetes-sigs:main Nov 6, 2023
7 checks passed
@Ankitasw
Copy link
Member

Ankitasw commented Nov 15, 2023 •
edited
Loading

@vincepri E2E tests are continuously failing after this PR merge, please either fix the test or revert the change.

@philjb philjb mentioned this pull request Dec 26, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndiDog AndiDog AndiDog left review comments

@richardcase richardcase richardcase left review comments

@AverageMarcus AverageMarcus Awaiting requested review from AverageMarcus

@shivi28 shivi28 Awaiting requested review from shivi28

Assignees

@AndiDog AndiDog

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@vincepri @AndiDog @richardcase @k8s-ci-robot @Ankitasw