cluster-api-provider-aws icon indicating copy to clipboard operation
cluster-api-provider-aws copied to clipboard

✨ Add support for additionalControlPlaneIngressRule on AWSManaged…

Open fad3t opened this issue 1 year ago • 32 comments
trafficstars

What type of PR is this?

/kind feature

What this PR does / why we need it:

This change adds support for additional control plane ingress rules when using the AWS managed control plane. Currently, the additional security group gets created but is empty, even if additional ingress rules are specified in the managed control plane specs.

Fixes #4788

Special notes for your reviewer:

Checklist:

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

Release note:

feat: add support for additionalControlPlaneIngressRules in AWSManagedControlPlane

fad3t avatar Feb 09 '24 07:02 fad3t

Welcome @fad3t!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot avatar Feb 09 '24 07:02 k8s-ci-robot

Hi @fad3t. 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 avatar Feb 09 '24 07:02 k8s-ci-robot

Hey there 👋 anything I can do to push this forward? Thanks!

fad3t avatar Feb 25 '24 13:02 fad3t

/lgtm

nrb avatar Apr 03 '24 18:04 nrb

when can this be merged since this a feature we want to use as well?

alagukannan avatar Apr 04 '24 18:04 alagukannan

Hey folks , looks like this PR is approved and ready to merge , when can we expect this to be merged and released ? It seems like lot of community contributors and consumers are waiting on this. Is there an alternative already in works and that's why this is not merged ?

mirnals avatar Apr 15 '24 15:04 mirnals

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

richardcase avatar Apr 19 '24 05:04 richardcase

This change looks good to me:

/approve

When the e2e passes lets unhold and get it merged.

/hold

richardcase avatar Apr 19 '24 05:04 richardcase

[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:

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 avatar Apr 19 '24 05:04 k8s-ci-robot

Hi @richardcase thx for the approval. I see I have a failing test, but it seems related to CUDA? Not sure this is related to my PR, is it?

fad3t avatar Apr 19 '24 11:04 fad3t

@fad3t No, I don't think this is related to your PR. I think there is some sort of condition that is causing GPU CI jobs to timeout.

I'm seeing other timeouts that I reported in #4937, but they're not the same.

/retest

nrb avatar Apr 19 '24 13:04 nrb

@fad3t: 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 eb671ff9a05ab43b7fa25e6b3c3514a97b8762a8 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 avatar Apr 19 '24 16:04 k8s-ci-robot