NE-1530: IngressController LB Subnet Selection in AWS #1841

gcs278 · 2024-04-01T00:10:54Z

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers using load balancers in AWS. Introduce
under the IngressControllerLBSubnetsAWS FeatureGate. Works for both CLB and NLBs.

Enhancement: openshift/enhancements#1595
Epic: https://issues.redhat.com/browse/NE-705

openshift-ci-robot · 2024-04-01T00:10:57Z

@gcs278: This pull request references NE-705 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.16." or "openshift-4.16.", but it targets "openshift-4.13" instead.

In response to this:

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers in AWS. Introduce under the
AWSLoadBalancerSubnetSelection FeatureGate. Works for both CLB and NLBs.

Enhancement: openshift/enhancements#1595
RFE: https://issues.redhat.com/browse/RFE-1717.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-04-01T00:10:59Z

Hello @gcs278! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci-robot · 2024-04-01T00:11:43Z

@gcs278: This pull request references NE-1530 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers in AWS. Introduce under the
AWSLoadBalancerSubnetSelection FeatureGate. Works for both CLB and NLBs.

Enhancement: openshift/enhancements#1595
RFE: https://issues.redhat.com/browse/RFE-1717.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-04-01T00:12:07Z

@gcs278: This pull request references NE-1530 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers in AWS. Introduce under the
AWSLoadBalancerSubnetSelection FeatureGate. Works for both CLB and NLBs.

Enhancement: openshift/enhancements#1595
Epic: https://issues.redhat.com/browse/NE-705

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-04-03T21:50:27Z

@gcs278: This pull request references NE-1530 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers using load balancers in AWS. Introduce
under the IngressControllerLBSubnetsAWS FeatureGate. Works for both CLB and NLBs.

Enhancement: openshift/enhancements#1595
Epic: https://issues.redhat.com/browse/NE-705

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 openshift-eng/jira-lifecycle-plugin repository.

gcs278 · 2024-04-03T21:53:28Z

/hold
EP is still under review and it's likely we will add a ingresses.config.openshift.io.spec.loadBalancer.platform.aws.subnets field as well.

JoelSpeed · 2024-05-03T08:26:53Z

operator/v1/types_ingress.go

@@ -545,6 +545,20 @@ type AWSLoadBalancerParameters struct {
 	//
 	// +optional
 	NetworkLoadBalancerParameters *AWSNetworkLoadBalancerParameters `json:"networkLoadBalancer,omitempty"`
+
+	// subnets specifies the list of subnets for the load balancer to
+	// route traffic to. The values can be either a subnet ID or name.


Can you find anything that suggests only specific characters are allowed in either the name or ID?

JoelSpeed · 2024-05-03T08:27:14Z

operator/v1/types_ingress.go

+	//
+	// +optional
+	// +openshift:enable:FeatureGate=IngressControllerLBSubnetsAWS
+	Subnets []AWSSubnetReference `json:"subnets,omitempty"`


Needs a listType tag, its this atomic?

I've read the docs (https://github.com/kubernetes/kube-openapi/blob/master/pkg/idl/doc.go and https://kubernetes.io/docs/reference/using-api/server-side-apply/), but I'm struggling to totally grasp the difference between an atomic and set listType. I don't totally understand the implications as it relates to my field. I see set:

// Sets are lists that must not have multiple times the same value. Each // value must be a scalar (or another atomic type).

And atomic:

// Atomic lists will be entirely replaced when updated. This tag may be // used on any type of list (struct, scalar, ...).

Sounds like atomic only allows one "manager" for server side apply. That seems acceptable for this field.

I also see atomic is the default, so I'll go with that.

candita · 2024-05-08T16:21:19Z

/assign

gcs278 · 2024-05-08T20:43:11Z

Yikes, I messed up this rebase bad. Fixing.

candita · 2024-05-09T02:07:36Z

An example failure from verify-crd-schema. Is it in development?

error running generator schemacheck on group operator.openshift.io:
could not run schemacheck generator for group/version operator.openshift.io/v1:
error in operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml: ListsMustHaveSSATags: crd/ingresscontrollers.operator.openshift.io version/v1 field/^.spec.namespaceSelector.matchExpressions must set x-kubernetes-list-type
error in operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml: ListsMustHaveSSATags: crd/ingresscontrollers.operator.openshift.io version/v1 field/^.spec.namespaceSelector.matchExpressions[*].values must set x-kubernetes-list-type

/test verify-crd-schema

gcs278 · 2024-05-09T02:49:10Z

An example failure from verify-crd-schema. Is it in development?

error running generator schemacheck on group operator.openshift.io:
could not run schemacheck generator for group/version operator.openshift.io/v1:
error in operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml: ListsMustHaveSSATags: crd/ingresscontrollers.operator.openshift.io version/v1 field/^.spec.namespaceSelector.matchExpressions must set x-kubernetes-list-type
error in operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yaml: ListsMustHaveSSATags: crd/ingresscontrollers.operator.openshift.io version/v1 field/^.spec.namespaceSelector.matchExpressions[*].values must set x-kubernetes-list-type

/test verify-crd-schema

@candita I saw that too, but at the end of the logs is:

This verifier checks all files that have changed. In some cases you may have changed or
renamed a file that already contained api violations, but you are not introducing a new
violation. In such cases it is appropriate to /override the failing CI job.

And I reviewed the logs and don't see any issue with my added subnets API. Instead, all of the errors are from existing fields. So I think I will require an override when the time comes to it.

candita · 2024-05-09T18:56:37Z

/retest

candita · 2024-05-09T19:03:33Z

features/features.go

+
+    FeatureGateIngressControllerLBSubnetsAWS = newFeatureGate("IngressControllerLBSubnetsAWS").
+                        reportProblemsToJiraComponent("Routing").
+                        contactPerson("gspence").


nit: For network edge features, the contact person is @Miciah , but I don't know if there are rules about it having to be the team lead or not. Otherwise, it should be your github handle.

Sure. I'll put "miciah" down.

candita · 2024-05-09T19:19:40Z

operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerLBSubnetsAWS.yaml

+                type: AWS
+                aws:
+                  type: Classic
+    - name: Subnets should be able to add after IC creation.


nit:

Suggested change

- name: Subnets should be able to add after IC creation.

- name: Should be able to add subnets after IC creation.

candita · 2024-05-09T19:49:20Z

operator/v1/types_ingress.go

+	// In order for the load balancer to be provisioned with subnets:
+	// * Each subnet must exist.
+	// * Each subnet must be from a different availability zone.
+	// * The load balancer service must be deleted.


Suggested change

// * The load balancer service must be deleted.

// * The load balancer service must be recreated to pick up new values.

candita · 2024-05-09T19:51:34Z

operator/v1/types_ingress.go

+	// * Each subnet must be from a different availability zone.
+	// * The load balancer service must be deleted.
+	//
+	// When omitted, the subnets will be auto-discovered per availability zone.


If I understand this correctly:

Suggested change

// When omitted, the subnets will be auto-discovered per availability zone.

// If omitted, the subnets will be auto-discovered per availability zone.

// When subnets are auto-discovered, they do not propagate to this list.

Sure. Done.

candita · 2024-05-09T19:54:05Z

operator/v1/types_ingress.go

+	// +optional
+	// +listType=atomic
+	// +openshift:enable:FeatureGate=IngressControllerLBSubnetsAWS
+	// +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x == y))`,message="subnets cannot contain duplicates"


Do you know if this validation is case-sensitive? The subnet names can be, so there could correctly be both subnetB and subnetb in the list. You could just add a test to the test yaml.

I don't know how much it matters, but I tested this with two empty subnets and that returns false. https://playcel.undistro.io/?content=H4sIAAAAAAAAA6tWSk7NUbJSKk7NSdNLzMnRqNBRALNTKzKLS4rj8%2FNSNSp1FCoUbG0VKjU1lXSUUhJLEj3zCkpLoLqsYvIUFHQVICRQPjc%2FJRUoBTK2FgCZjmKCXgAAAA%3D%3D

Do you know if this validation is case-sensitive? The subnet names can be, so there could correctly be both subnetB and subnetb in the list. You could just add a test to the test yaml.

Good question. It is indeed case-sensitive. Good idea, I will do that.

I don't know how much it matters, but I tested this with two empty subnets and that returns false.

Oh nice, I didn't know about this CEL playground website.

Good point. The CEL is working as expected with two empty subnets, but an empty length subnet is not a valid subnet reference. Adding // +kubebuilder:validation:MinLength=1

candita · 2024-05-09T19:56:11Z

operator/v1/types_ingress.go

@@ -556,6 +572,11 @@ const (
 	AWSNetworkLoadBalancer AWSLoadBalancerType = "NLB"
 )

+// AWSSubnetReference is a reference to an AWS subnet. It can be either
+// be a subnet ID or name.


Suggested change

// be a subnet ID or name.

// an AWS subnet ID or a generic name.

candita · 2024-05-09T20:37:30Z

No major concerns on my part, just a few comments/nits.
/lgtm

openshift-ci · 2024-05-09T20:37:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: candita, gcs278
Once this PR has been reviewed and has the lgtm label, please assign spadgett for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers using load balancers in AWS. Introduce under the `IngressControllerLBSubnetsAWS` FeatureGate.

openshift-ci · 2024-05-10T01:19:24Z

New changes are detected. LGTM label has been removed.

gcs278

Thanks for the review @candita

Changes here: https://github.com/openshift/api/compare/81aaf82a567be6f4b42e236e2276d65569c7f056..e8783c84b59e430682265cbbe441614b32ee8ee6

gcs278 · 2024-05-09T23:19:30Z

features/features.go

+
+    FeatureGateIngressControllerLBSubnetsAWS = newFeatureGate("IngressControllerLBSubnetsAWS").
+                        reportProblemsToJiraComponent("Routing").
+                        contactPerson("gspence").


Sure. I'll put "miciah" down.

gcs278 · 2024-05-09T23:25:26Z

operator/v1/types_ingress.go

@@ -545,6 +545,20 @@ type AWSLoadBalancerParameters struct {
 	//
 	// +optional
 	NetworkLoadBalancerParameters *AWSNetworkLoadBalancerParameters `json:"networkLoadBalancer,omitempty"`
+
+	// subnets specifies the list of subnets for the load balancer to
+	// route traffic to. The values can be either a subnet ID or name.


Thanks. That's consistent with my experiments that allowed me to create a subnet name tag with every character my keyboard has. I don't think we can restrict characters on the basis of subnet names.

However, after some tedious experimenting, I found that though , is a valid in a subnet name, but is impossible to specify in the annotation since it uses that as the delimiter. I will add a restriction for that.

gcs278 · 2024-05-10T00:49:28Z

operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerLBSubnetsAWS.yaml

+                type: AWS
+                aws:
+                  type: Classic
+    - name: Subnets should be able to add after IC creation.


gcs278 · 2024-05-10T00:50:52Z

operator/v1/types_ingress.go

+	// In order for the load balancer to be provisioned with subnets:
+	// * Each subnet must exist.
+	// * Each subnet must be from a different availability zone.
+	// * The load balancer service must be deleted.


gcs278 · 2024-05-10T00:53:43Z

operator/v1/types_ingress.go

+	// * Each subnet must be from a different availability zone.
+	// * The load balancer service must be deleted.
+	//
+	// When omitted, the subnets will be auto-discovered per availability zone.


Sure. Done.

gcs278 · 2024-05-10T01:03:21Z

operator/v1/types_ingress.go

+	// +optional
+	// +listType=atomic
+	// +openshift:enable:FeatureGate=IngressControllerLBSubnetsAWS
+	// +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x == y))`,message="subnets cannot contain duplicates"


Do you know if this validation is case-sensitive? The subnet names can be, so there could correctly be both subnetB and subnetb in the list. You could just add a test to the test yaml.

Good question. It is indeed case-sensitive. Good idea, I will do that.

I don't know how much it matters, but I tested this with two empty subnets and that returns false.

Oh nice, I didn't know about this CEL playground website.

Good point. The CEL is working as expected with two empty subnets, but an empty length subnet is not a valid subnet reference. Adding // +kubebuilder:validation:MinLength=1

gcs278 · 2024-05-10T01:07:16Z

operator/v1/types_ingress.go

@@ -556,6 +572,11 @@ const (
 	AWSNetworkLoadBalancer AWSLoadBalancerType = "NLB"
 )

+// AWSSubnetReference is a reference to an AWS subnet. It can be either
+// be a subnet ID or name.


openshift-ci · 2024-05-10T04:06:24Z

@gcs278: 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
ci/prow/verify-crd-schema	`e8783c8`	link	true	`/test verify-crd-schema`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 1, 2024

openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 1, 2024

openshift-ci bot requested review from deads2k and JoelSpeed April 1, 2024 00:11

gcs278 force-pushed the NE705-subnets branch from 10900de to 0fa9b22 Compare April 1, 2024 00:11

gcs278 changed the title ~~NE-705: Add API for AWS LB Subnet Selection~~ NE-1530: Add API for AWS LB Subnet Selection Apr 1, 2024

gcs278 force-pushed the NE705-subnets branch from 0fa9b22 to f4b084d Compare April 3, 2024 02:54

gcs278 changed the title ~~NE-1530: Add API for AWS LB Subnet Selection~~ [WIP] NE-1530: Add API for AWS LB Subnet Selection Apr 3, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2024

gcs278 force-pushed the NE705-subnets branch from f4b084d to 2e7c6f8 Compare April 3, 2024 21:21

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

gcs278 force-pushed the NE705-subnets branch from 2e7c6f8 to d661c33 Compare April 3, 2024 21:24

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

gcs278 force-pushed the NE705-subnets branch from d661c33 to 49e4cab Compare April 3, 2024 21:48

gcs278 changed the title ~~[WIP] NE-1530: Add API for AWS LB Subnet Selection~~ NE-1530: IngressController LB Subnet Selection in AWS Apr 3, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2024

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

gcs278 force-pushed the NE705-subnets branch from 49e4cab to 061b1e3 Compare April 4, 2024 14:31

gcs278 changed the title ~~NE-1530: IngressController LB Subnet Selection in AWS~~ [WIP] NE-1530: IngressController LB Subnet Selection in AWS Apr 6, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2024

gcs278 force-pushed the NE705-subnets branch from 061b1e3 to 127fb08 Compare April 7, 2024 17:29

gcs278 changed the title ~~[WIP] NE-1530: IngressController LB Subnet Selection in AWS~~ NE-1530: IngressController LB Subnet Selection in AWS Apr 7, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2024

gcs278 force-pushed the NE705-subnets branch 2 times, most recently from 19737cb to c906aa1 Compare May 2, 2024 23:12

JoelSpeed reviewed May 3, 2024

View reviewed changes

gcs278 force-pushed the NE705-subnets branch from c906aa1 to 5047f89 Compare May 3, 2024 19:36

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2024

gcs278 force-pushed the NE705-subnets branch from 5047f89 to d4cada8 Compare May 6, 2024 13:52

openshift-ci bot assigned candita May 8, 2024

gcs278 force-pushed the NE705-subnets branch from d4cada8 to 1ac387a Compare May 8, 2024 20:54

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2024

gcs278 force-pushed the NE705-subnets branch from 1ac387a to 81aaf82 Compare May 9, 2024 16:48

candita reviewed May 9, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2024

NE-1530: IngressController LB Subnet Selection in AWS

e8783c8

Allows users to specify subnets (i.e. Availability Zones) for IngressControllers using load balancers in AWS. Introduce under the `IngressControllerLBSubnetsAWS` FeatureGate.

gcs278 force-pushed the NE705-subnets branch from 81aaf82 to e8783c8 Compare May 10, 2024 01:19

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 10, 2024

gcs278 commented May 10, 2024

View reviewed changes

	- name: Subnets should be able to add after IC creation.
	- name: Should be able to add subnets after IC creation.

	// * The load balancer service must be deleted.
	// * The load balancer service must be recreated to pick up new values.

	// When omitted, the subnets will be auto-discovered per availability zone.
	// If omitted, the subnets will be auto-discovered per availability zone.
	// When subnets are auto-discovered, they do not propagate to this list.

	// be a subnet ID or name.
	// an AWS subnet ID or a generic name.

NE-1530: IngressController LB Subnet Selection in AWS #1841

Are you sure you want to change the base?

NE-1530: IngressController LB Subnet Selection in AWS #1841

Conversation

gcs278 commented Apr 1, 2024 • edited

openshift-ci-robot commented Apr 1, 2024 • edited by openshift-ci bot

openshift-ci bot commented Apr 1, 2024

openshift-ci-robot commented Apr 1, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Apr 1, 2024 • edited by openshift-ci bot

openshift-ci-robot commented Apr 3, 2024 • edited by openshift-ci bot

gcs278 commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita commented May 8, 2024

gcs278 commented May 8, 2024

candita commented May 9, 2024

gcs278 commented May 9, 2024 • edited

candita commented May 9, 2024

candita May 9, 2024 • edited

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

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

candita May 9, 2024 • edited

Choose a reason for hiding this comment

candita May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita commented May 9, 2024

openshift-ci bot commented May 9, 2024

openshift-ci bot commented May 10, 2024

gcs278 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

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

openshift-ci bot commented May 10, 2024

gcs278 commented Apr 1, 2024 •

edited

openshift-ci-robot commented Apr 1, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Apr 1, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Apr 1, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Apr 3, 2024 •

edited by openshift-ci bot

gcs278 commented May 9, 2024 •

edited

candita May 9, 2024 •

edited

candita May 9, 2024 •

edited

candita May 9, 2024 •

edited

candita May 9, 2024 •

edited