Additional ingress rules for control plane #4359

alexander-demicev · 2023-06-26T12:47:22Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR fixes mistake from #4228, initially, the intention was to allow adding custom ingress rules for the control plane itself and not for the load balancer.

cc @richardcase @fiunchinho

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:

Additional ingress rules for control plane

richardcase · 2023-06-30T10:58:38Z

/test ?

k8s-ci-robot · 2023-06-30T10:58:40Z

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

/test pull-cluster-api-provider-aws-build
/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-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 · 2023-06-30T10:59:06Z

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

enxebre · 2023-06-30T12:02:51Z

api/v1beta2/network_types.go

+
+	// AdditionalControlPlaneIngressRules is an optional set of ingress rules to add to the control plane
+	// +optional
+	AdditionalControlPlaneIngressRules []IngressRule `json:"additionalControlPlaneIngressRules,omitempty"`


how is this different from AWSLoadBalancerSpec.IngressRules?
You mention in the PR desc it was a mistake but it seems we are keeping it?

enxebre · 2023-06-30T12:03:21Z

docs/book/src/topics/bring-your-own-aws-infrastructure.md

@@ -175,6 +175,19 @@ spec:
 > 
 >An incorrectly configured Classic ELB can easily lead to a non-functional cluster. We strongly recommend you let Cluster API create the Classic ELB.

+### Control Plane ingress rules
+
+It's possible to specify custom ingress rules for the control plane itself. To do so, add this to the AWSCluster specification:


Can you clarify/define what is "the control plane itself" in this context?

There are couple of places where we can customize ingress rules. One of them is the control plane load balancer, which is responsible for external traffic, these 2 PRs are introducing the possibility to set custom rules for it #4228, #4304. Current PR makes it possible to customize ingress rules on the control plane machine directly, in our case we want worker machines to talk with the control plane through a specific port, it's a requirement for the RKE2.

makes sense thanks! so this would be control plane machines/ec2 instances. Just to make sure I'm clear this could also be satisfied by setting your AdditionalSecurityGroups in those machines, correct? (Not saying that's the way to go and I have no objections to this change, just making sure we put all context together)

AdditionalSecurityGroups is not the best option here because we want to leverage CAPA ability to manage the underlying infrastructure. AdditionalSecurityGroups requires SGs to be pre-created.

Ideally, we need a field in AWS machine template for defining security groups but I don't know how this can be combined with already existing AdditionalSecurityGroups cc @richardcase

Indeed AdditionalSecurityGroups []string in AWSCluster is not flexible enough, while other code locations have AdditionalSecurityGroups []AWSResourceReference which I believe would allow searching for CAPA-managed security groups. I'll take this into account for networking code improvements.

enxebre · 2023-06-30T12:04:09Z

Dropped some questions Alex, it'd be good to provide some context on the PR desc to elaborate the use case which is driven this change and how it differs from #4228

alexander-demicev · 2023-07-03T09:46:15Z

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

enxebre · 2023-07-03T11:12:12Z

pkg/cloud/services/securitygroup/securitygroups.go

-				}
+		ingressRules := s.scope.AdditionalControlPlaneIngressRules()
+		for i := range ingressRules {
+			if ingressRules[i].SourceSecurityGroupIDs == nil && ingressRules[i].SourceSecurityGroupRoles == nil { // if the rule doesn't have a source security group, use the control plane security group


This is called AdditionalControlPlaneIngressRules because it defaults to the controlPlane SG as the source, but as an API consumer I could set SourceSecurityGroupIDs to anything I want and so bypassing the intended API scope?

Currently, there is no override, just adding new rules. The reason is that most likely we always want control plane to have rules for the API server and etcd.

richardcase · 2023-07-10T16:43:57Z

/milestone v2.3.0

api/v1beta2/awscluster_webhook.go

docs/book/src/topics/bring-your-own-aws-infrastructure.md

pkg/cloud/services/securitygroup/securitygroups_test.go

richardcase · 2023-07-12T14:27:03Z

/test ?

k8s-ci-robot · 2023-07-12T14:27:06Z

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

/test pull-cluster-api-provider-aws-build
/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-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 · 2023-07-12T14:27:30Z

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

fiunchinho · 2023-07-13T08:48:54Z

lgtm

alexander-demicev · 2023-07-20T20:55:05Z

@richardcase all tests are passing

api/v1beta2/awscluster_webhook.go

docs/book/src/topics/bring-your-own-aws-infrastructure.md

pkg/cloud/services/securitygroup/securitygroups.go

AndiDog · 2023-08-28T08:02:44Z

docs/book/src/topics/bring-your-own-aws-infrastructure.md

@@ -175,6 +175,19 @@ spec:
 > 
 >An incorrectly configured Classic ELB can easily lead to a non-functional cluster. We strongly recommend you let Cluster API create the Classic ELB.

+### Control Plane ingress rules
+
+It's possible to specify custom ingress rules for the control plane itself. To do so, add this to the AWSCluster specification:


Indeed AdditionalSecurityGroups []string in AWSCluster is not flexible enough, while other code locations have AdditionalSecurityGroups []AWSResourceReference which I believe would allow searching for CAPA-managed security groups. I'll take this into account for networking code improvements.

richardcase · 2023-08-29T09:26:38Z

@alexander-demicev - apart from the comments from @AndiDog is there anything outstanding on this?

alexander-demicev · 2023-09-05T13:47:59Z

@AndiDog I addressed all comments, thanks for the review

AndiDog

Mostly LGTM apart from some minor things

api/v1beta2/awscluster_webhook_test.go

pkg/cloud/services/securitygroup/securitygroups.go

JoelSpeed

I've been testing this today and found a couple of bugs while using it, have marked these with comments, pretty easy to fix I believe

pkg/cloud/services/securitygroup/securitygroups.go

JoelSpeed · 2023-09-11T14:51:20Z

pkg/cloud/services/securitygroup/securitygroups.go

+			for _, sourceSGRole := range ingressRules[i].SourceSecurityGroupRoles {
+				ingressRules[i].SourceSecurityGroupIDs = append(ingressRules[i].SourceSecurityGroupIDs, s.scope.SecurityGroups()[sourceSGRole].ID)


Should use a set style for this, else over time, the list of SourceSecurityGroupIDs grows and becomes invalid as far as AWS is concerned.

Suggested change

for _, sourceSGRole := range ingressRules[i].SourceSecurityGroupRoles {

ingressRules[i].SourceSecurityGroupIDs = append(ingressRules[i].SourceSecurityGroupIDs, s.scope.SecurityGroups()[sourceSGRole].ID)

securityGroupIDs := sets.New[string](ingressRules[i].SourceSecurityGroupIDs...)

for _, sourceSGRole := range ingressRules[i].SourceSecurityGroupRoles {

securityGroupIDs.Insert(s.scope.SecurityGroups()[sourceSGRole].ID)

}

ingressRules[i].SourceSecurityGroupIDs = sets.List[string](securityGroupIDs)

alexander-demicev · 2023-09-12T14:45:45Z

@JoelSpeed @AndiDog all should be fixed, thanks for reviews

JoelSpeed · 2023-09-12T14:47:30Z

Bugs I picked up look to be fixed now, thanks @alexander-demicev

richardcase · 2023-09-18T12:41:19Z

/test ?

k8s-ci-robot · 2023-09-18T12:41:21Z

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

/test pull-cluster-api-provider-aws-build
/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-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 · 2023-09-18T12:41:40Z

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

richardcase · 2023-09-18T12:41:56Z

@alexander-demicev - could you squash your commits?

richardcase · 2023-09-18T16:42:50Z

e2e test passed.

Fix setting additional ingress rules for CP Update documentation with CP ingress rules

JoelSpeed

Seems there's possibly an unresolved comment in the documentation added within the PR, is that resolved now? Otherwise LGTM

Ankitasw · 2023-09-21T12:29:23Z

pkg/cloud/services/securitygroup/securitygroups_test.go

@@ -760,7 +779,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) {
 						t.Fatalf("Expected to port %d, got %d", tc.expectedAdditionalIngresRule.ToPort, r.ToPort)
 					}

-					if !reflect.DeepEqual(r.SourceSecurityGroupIDs, tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs) {
+					if !sets.New[string](tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs...).Equal(sets.New[string](tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs...)) {


small nit:
why set comparison and not cmp.Equal (if you do not intend to use reflect library)?

actually no reason :) I just felt like I could use a method provided by "sets" package since we make security groups a set in the controller logic.

ok 🙂 just wanted to check if this has added benefit

Ankitasw · 2023-09-21T15:24:28Z

/lgtm

cc @richardcase for final approval

richardcase · 2023-09-21T21:50:30Z

/approve

k8s-ci-robot · 2023-09-21T21:50:40Z

[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 · 2023-09-26T16:11:00Z

/cherrypick release-2.2

k8s-infra-cherrypick-robot · 2023-09-26T16:11:35Z

@richardcase: #4359 failed to apply on top of branch "release-2.2":

Applying: Add additional ingress rules for CP to API
Using index info to reconstruct a base tree...
M	api/v1beta1/awscluster_conversion.go
M	api/v1beta1/conversion.go
M	api/v1beta1/zz_generated.conversion.go
M	api/v1beta2/awscluster_webhook.go
M	api/v1beta2/awscluster_webhook_test.go
M	api/v1beta2/network_types.go
M	api/v1beta2/zz_generated.deepcopy.go
M	config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml
M	config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
M	config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
Falling back to patching base and 3-way merge...
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
Auto-merging config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
Auto-merging config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml
Auto-merging api/v1beta2/zz_generated.deepcopy.go
Auto-merging api/v1beta2/network_types.go
Auto-merging api/v1beta2/awscluster_webhook_test.go
CONFLICT (content): Merge conflict in api/v1beta2/awscluster_webhook_test.go
Auto-merging api/v1beta2/awscluster_webhook.go
CONFLICT (content): Merge conflict in api/v1beta2/awscluster_webhook.go
Auto-merging api/v1beta1/zz_generated.conversion.go
Auto-merging api/v1beta1/conversion.go
CONFLICT (content): Merge conflict in api/v1beta1/conversion.go
Auto-merging api/v1beta1/awscluster_conversion.go
CONFLICT (content): Merge conflict in api/v1beta1/awscluster_conversion.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add additional ingress rules for CP to API
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-2.2

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 requested review from dthorsen and pydctw June 26, 2023 12:47

enxebre reviewed Jun 30, 2023

View reviewed changes

enxebre reviewed Jul 3, 2023

View reviewed changes

k8s-ci-robot added this to the v2.3.0 milestone Jul 10, 2023

fiunchinho reviewed Jul 11, 2023

View reviewed changes

api/v1beta2/awscluster_webhook.go Outdated Show resolved Hide resolved

fiunchinho reviewed Jul 11, 2023

View reviewed changes

docs/book/src/topics/bring-your-own-aws-infrastructure.md Outdated Show resolved Hide resolved

fiunchinho reviewed Jul 11, 2023

View reviewed changes

pkg/cloud/services/securitygroup/securitygroups_test.go Show resolved Hide resolved

alexander-demicev force-pushed the cpingress branch from 8b0a9f4 to da9142d Compare July 12, 2023 12:34

AndiDog reviewed Aug 28, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023

AndiDog mentioned this pull request Sep 5, 2023

REQUEST: New membership for AndiDog kubernetes/org#4437

Closed

9 tasks

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023

AndiDog reviewed Sep 8, 2023

View reviewed changes

api/v1beta2/awscluster_webhook_test.go Show resolved Hide resolved

pkg/cloud/services/securitygroup/securitygroups.go Show resolved Hide resolved

JoelSpeed reviewed Sep 11, 2023

View reviewed changes

alexander-demicev force-pushed the cpingress branch from 796086d to 6c18a53 Compare September 12, 2023 14:45

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2023

alexander-demicev force-pushed the cpingress branch from 6c18a53 to 110f45d Compare September 18, 2023 12:12

Add additional ingress rules for CP to API

80dbe3a

Fix setting additional ingress rules for CP Update documentation with CP ingress rules

alexander-demicev force-pushed the cpingress branch from 110f45d to 80dbe3a Compare September 19, 2023 09:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2023

JoelSpeed reviewed Sep 21, 2023

View reviewed changes

fiunchinho mentioned this pull request Sep 21, 2023

Save Nat Gateways IPs to status on every reconciliation #4508

Merged

4 tasks

Ankitasw reviewed Sep 21, 2023

View reviewed changes

k8s-ci-robot assigned Ankitasw Sep 21, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023

k8s-ci-robot merged commit b90c18c into kubernetes-sigs:main Sep 21, 2023
3 checks passed

fiunchinho mentioned this pull request Sep 27, 2023

[release-2.2] Add additional ingress rules for CP to API #4524

Merged

4 tasks

		for _, sourceSGRole := range ingressRules[i].SourceSecurityGroupRoles {
		ingressRules[i].SourceSecurityGroupIDs = append(ingressRules[i].SourceSecurityGroupIDs, s.scope.SecurityGroups()[sourceSGRole].ID)

Additional ingress rules for control plane #4359

Additional ingress rules for control plane #4359

Conversation

alexander-demicev commented Jun 26, 2023 • edited by Ankitasw Loading

richardcase commented Jun 30, 2023

k8s-ci-robot commented Jun 30, 2023

richardcase commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Jun 30, 2023 • edited Loading

alexander-demicev commented Jul 3, 2023

enxebre Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase commented Jul 10, 2023

richardcase commented Jul 12, 2023

k8s-ci-robot commented Jul 12, 2023

richardcase commented Jul 12, 2023

fiunchinho commented Jul 13, 2023

alexander-demicev commented Jul 20, 2023

Choose a reason for hiding this comment

richardcase commented Aug 29, 2023

alexander-demicev commented Sep 5, 2023

AndiDog left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev commented Sep 12, 2023

JoelSpeed commented Sep 12, 2023

richardcase commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

richardcase commented Sep 18, 2023

richardcase commented Sep 18, 2023

richardcase commented Sep 18, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ankitasw commented Sep 21, 2023

richardcase commented Sep 21, 2023

k8s-ci-robot commented Sep 21, 2023

richardcase commented Sep 26, 2023

k8s-infra-cherrypick-robot commented Sep 26, 2023

alexander-demicev commented Jun 26, 2023 •

edited by Ankitasw

Loading

enxebre Jul 3, 2023 •

edited

Loading

enxebre commented Jun 30, 2023 •

edited

Loading

enxebre Jul 3, 2023 •

edited

Loading