🐛 ec2: instances: fix assigning public IP #4892

r4f4 · 2024-03-20T19:33:21Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

In the scenario where the user brings their own VPC, if no subnet ID is set in the machine spec and PublicIP is true, CAPA will choose one from the available public subnets. However, if the subnet doesn't have MapPublicIPOnLaunch == true, the instance will not be assigned a public IP. As a result, the instance will have no internet access, contrary to the user's expectation.

This change guarantees an instance will be assigned a public IP even if the subnet doesn't do it on instance launch. Instead, we set the option in the instance's network interface.

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 #4888

Special notes for your reviewer:

I still need to fix and add unit tests. I want to get feedback on the approach first.

Checklist:

Release note:

Fix machines getting a public IP even when user-supplied subnets don't have MapPublicIpOnLaunch.

k8s-ci-robot · 2024-03-20T19:33:31Z

Hi @r4f4. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

r4f4 · 2024-03-20T19:33:50Z

/hold
Until I fix the unit tests.

r4f4 · 2024-03-20T19:34:55Z

/cc @mtulio @patrickdillon

k8s-ci-robot · 2024-03-20T19:34:57Z

@r4f4: GitHub didn't allow me to request PR reviews from the following users: mtulio, patrickdillon.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mtulio @patrickdillon

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.

pkg/cloud/services/ec2/instances.go

damdo · 2024-03-22T09:47:49Z

/ok-to-test

damdo · 2024-03-22T10:07:29Z

/cc @vincepri

damdo · 2024-03-22T10:21:08Z

@r4f4 something needs fixing in API generation and units.
Could you PTAL? Thanks!

r4f4 · 2024-03-22T10:31:23Z

@r4f4 something needs fixing in API generation and units. Could you PTAL? Thanks!

Sure, I also need to add unit tests.

r4f4 · 2024-03-22T19:44:56Z

@damdo I don't know how to fix the fuzzy test. Would you mind giving some pointers?

damdo · 2024-03-25T17:43:58Z

@r4f4 The Fuzzy testing in this instance refers to the Conversion across API types conversion.
More specifically --- FAIL: TestFuzzyConversion/for_AWSCluster refers to the AWSCluster conversion.

Try and add

dst.Status.Bastion.PublicIPOnLaunch = restored.Status.Bastion.PublicIPOnLaunch

after this:

cluster-api-provider-aws/api/v1beta1/awscluster_conversion.go

Line 59 in daaa1e1

dst.Status.Bastion.PrivateDNSName = restored.Status.Bastion.PrivateDNSName

vincepri

/lgtm

vincepri · 2024-04-02T15:14:08Z

@r4f4 squash commits?

In the scenario where the user brings their own VPC, if no subnet ID is set in the machine spec and PublicIP is true, CAPA will choose one from the available public subnets. However, if the subnet doesn't have MapPublicIPOnLaunch == true, the instance will not be assigned a public IP. As a result, the instance will have no internet access, contrary to the user's expectation. This change guarantees an instance will be assigned a public IP even if the subnet doesn't do it on instance launch. Instead, we set the option in the instance's network interface.

The tests check that a NetworkInterface is defined with `AssociatePublicIpAddress` in the `RunInstances` input.

damdo

/lgtm

damdo · 2024-04-03T09:05:52Z

@vincepri @richardcase @dlipovetsky @Ankitasw this one needs an /approve

richardcase · 2024-04-04T11:11:41Z

/test ?

k8s-ci-robot · 2024-04-04T11:11:43Z

@richardcase: The following commands are available to trigger required jobs:

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

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass
/test pull-cluster-api-provider-aws-e2e-conformance
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e-eks-gc
/test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main
pull-cluster-api-provider-aws-build
pull-cluster-api-provider-aws-build-docker
pull-cluster-api-provider-aws-test
pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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.

richardcase · 2024-04-04T11:12:00Z

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

richardcase · 2024-04-04T11:12:18Z

Until the e2e pass:

/hold

richardcase · 2024-04-04T11:12:38Z

But from my side:

/approve

When the e2e passes feel free to unhold.

k8s-ci-robot · 2024-04-04T11:12:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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:

~~OWNERS~~ [richardcase]

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

richardcase · 2024-04-04T11:13:56Z

/cherry-pick release-2.4

k8s-infra-cherrypick-robot · 2024-04-04T11:13:58Z

@richardcase: once the present PR merges, I will cherry-pick it on top of release-2.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-2.4

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.

damdo · 2024-04-04T13:06:20Z

It timed out
/test pull-cluster-api-provider-aws-e2e

r4f4 · 2024-04-04T15:08:38Z

Are we good to remove the hold?

nrb · 2024-04-04T15:12:32Z

/unhold

k8s-infra-cherrypick-robot · 2024-04-04T15:17:02Z

@richardcase: new pull request created: #4908

In response to this:

/cherry-pick release-2.4

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.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. 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. labels Mar 20, 2024

k8s-ci-robot requested review from fiunchinho and luthermonson March 20, 2024 19:33

k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 20, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2024

mtulio reviewed Mar 20, 2024

View reviewed changes

pkg/cloud/services/ec2/instances.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 20, 2024

r4f4 mentioned this pull request Mar 22, 2024

🐛 Add validation when PublicIP == true but user-supplied public subnet has MapPublicIPOnLauch == false #4889

Closed

5 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 22, 2024

k8s-ci-robot requested a review from vincepri March 22, 2024 10:07

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2024

vincepri approved these changes Apr 2, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2024

r4f4 added 2 commits April 2, 2024 17:15

🌱ec2: instances: add unit tests for MapPublicIpOnLaunch=false

46abb5b

The tests check that a NetworkInterface is defined with `AssociatePublicIpAddress` in the `RunInstances` input.

r4f4 force-pushed the shared-vpc-associate-public-ip branch from 6bbfda0 to 46abb5b Compare April 2, 2024 15:15

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2024

damdo reviewed Apr 3, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 3, 2024

damdo requested a review from vincepri April 3, 2024 09:05

mtulio mentioned this pull request Apr 3, 2024

✨ Support BYO Public IPv4 Pool when provision infrastructure #4905

Merged

7 tasks

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024

k8s-ci-robot merged commit d3c59d9 into kubernetes-sigs:main Apr 4, 2024
22 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Apr 4, 2024

[release-2.4] 🐛 ec2: instances: fix assigning public IP #4908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 ec2: instances: fix assigning public IP #4892

🐛 ec2: instances: fix assigning public IP #4892

r4f4 commented Mar 20, 2024 •

edited

Loading

k8s-ci-robot commented Mar 20, 2024

r4f4 commented Mar 20, 2024

r4f4 commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

damdo commented Mar 22, 2024

damdo commented Mar 22, 2024

damdo commented Mar 22, 2024 •

edited

Loading

r4f4 commented Mar 22, 2024

r4f4 commented Mar 22, 2024

damdo commented Mar 25, 2024

vincepri left a comment

vincepri commented Apr 2, 2024

damdo left a comment

damdo commented Apr 3, 2024

richardcase commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

richardcase commented Apr 4, 2024

richardcase commented Apr 4, 2024

richardcase commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

richardcase commented Apr 4, 2024

k8s-infra-cherrypick-robot commented Apr 4, 2024

damdo commented Apr 4, 2024

r4f4 commented Apr 4, 2024

nrb commented Apr 4, 2024

k8s-infra-cherrypick-robot commented Apr 4, 2024

🐛 ec2: instances: fix assigning public IP #4892

🐛 ec2: instances: fix assigning public IP #4892

Conversation

r4f4 commented Mar 20, 2024 • edited Loading

k8s-ci-robot commented Mar 20, 2024

r4f4 commented Mar 20, 2024

r4f4 commented Mar 20, 2024

k8s-ci-robot commented Mar 20, 2024

damdo commented Mar 22, 2024

damdo commented Mar 22, 2024

damdo commented Mar 22, 2024 • edited Loading

r4f4 commented Mar 22, 2024

r4f4 commented Mar 22, 2024

damdo commented Mar 25, 2024

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Apr 2, 2024

damdo left a comment

Choose a reason for hiding this comment

damdo commented Apr 3, 2024

richardcase commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

richardcase commented Apr 4, 2024

richardcase commented Apr 4, 2024

richardcase commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

richardcase commented Apr 4, 2024

k8s-infra-cherrypick-robot commented Apr 4, 2024

damdo commented Apr 4, 2024

r4f4 commented Apr 4, 2024

nrb commented Apr 4, 2024

k8s-infra-cherrypick-robot commented Apr 4, 2024

r4f4 commented Mar 20, 2024 •

edited

Loading

damdo commented Mar 22, 2024 •

edited

Loading