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

🐛 fix: additional control plane LBs must use correct schema/subnets #4840

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

mtulio
Copy link
Contributor

@mtulio mtulio commented Mar 1, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

Secondary control plane load balancer is created using same schema of primary, failing in some scenarios like primary is created as private (internal) and secondary external (internet-facing) - it will result in the external LB w/ private subnets.

Which issue(s) this PR fixes

Special notes for your reviewer:

Checklist:

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

Release note:

Fix discovered subnets for additional Load Balancers ("SecondaryControlPlaneLoadBalancer") when it is internet-facing.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 1, 2024
@k8s-ci-robot k8s-ci-robot requested a review from AndiDog March 1, 2024 13:04
@k8s-ci-robot k8s-ci-robot requested a review from vincepri March 1, 2024 13:04
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 1, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @mtulio. 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.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 1, 2024
@mtulio mtulio changed the title 🐛 fix: additional control plane LBs must use correct schema/subnets WIP | 🐛 fix: additional control plane LBs must use correct schema/subnets Mar 1, 2024
@mtulio
Copy link
Contributor Author

mtulio commented Mar 1, 2024

/test ?

@k8s-ci-robot
Copy link
Contributor

@mtulio: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

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.

@mtulio
Copy link
Contributor Author

mtulio commented Mar 1, 2024

/assign @vincepri

@mtulio mtulio changed the title WIP | 🐛 fix: additional control plane LBs must use correct schema/subnets 🐛 fix: additional control plane LBs must use correct schema/subnets Mar 1, 2024
@mtulio mtulio marked this pull request as ready for review March 1, 2024 14:22
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 1, 2024
@k8s-ci-robot k8s-ci-robot requested review from cnmcavoy and faiq March 1, 2024 14:23
@mtulio
Copy link
Contributor Author

mtulio commented Mar 1, 2024

Cluster installed successfully w/ second LB in the correct subnets (public). This PR is ready for review.

/cc @patrickdillon @vincepri @r4f4

@k8s-ci-robot
Copy link
Contributor

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

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

In response to this:

Cluster installed successfully w/ second LB in the correct subnets (public). This PR is ready for review.

/cc @patrickdillon @vincepri @r4f4

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 k8s-ci-robot requested a review from nrb March 1, 2024 14:32
@patrickdillon
Copy link

/assign @nrb

@nrb
Copy link
Contributor

nrb commented Mar 1, 2024

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 1, 2024
@nrb
Copy link
Contributor

nrb commented Mar 1, 2024

/retest

Unrelated infra errors

@mtulio mtulio force-pushed the CORS-3288-fix-lb-subnets branch 2 times, most recently from 88ad248 to 6422b40 Compare March 1, 2024 20:44
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @nrb @richardcase

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2024
@mtulio
Copy link
Contributor Author

mtulio commented Mar 6, 2024

@mtulio: 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-apidiff-main 6422b40 link false /test pull-cluster-api-provider-aws-apidiff-main

Ignoring the apidiff as it is informational.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024
@mtulio mtulio force-pushed the CORS-3288-fix-lb-subnets branch from 6422b40 to dc2daa1 Compare March 6, 2024 14:54
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2024
@mtulio mtulio force-pushed the CORS-3288-fix-lb-subnets branch 2 times, most recently from 8b91067 to 48e2f12 Compare March 6, 2024 15:30
@nrb
Copy link
Contributor

nrb commented Mar 6, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2024
@mtulio
Copy link
Contributor Author

mtulio commented Mar 6, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from richardcase.

@richardcase would you mind to taking a look at this PR?

@nrb
Copy link
Contributor

nrb commented Mar 6, 2024

Not sure why the needs-rebase label is still applied.

/tide refresh

pkg/cloud/scope/cluster.go Outdated Show resolved Hide resolved
@mtulio mtulio force-pushed the CORS-3288-fix-lb-subnets branch from 48e2f12 to 9eb9b22 Compare March 6, 2024 18:39
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2024
The additional (non-primary) Control planes with schema different
than the primary are getting incorrect subnets.

The fix makes sure that the schema for the lbSpec is checked, instead
of the primary.

Considering we can access the lbSpec locally, we no longer need the
function ControlPlaneLoadBalancerScheme.
@mtulio mtulio force-pushed the CORS-3288-fix-lb-subnets branch from 9eb9b22 to 0714d36 Compare March 6, 2024 18:40
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024
@nrb
Copy link
Contributor

nrb commented Mar 6, 2024

/lgtm

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

vincepri commented Mar 6, 2024

/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 Mar 6, 2024
@k8s-ci-robot k8s-ci-robot merged commit baf8d59 into kubernetes-sigs:main Mar 6, 2024
19 checks passed
@mtulio mtulio deleted the CORS-3288-fix-lb-subnets branch March 6, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants