-
Notifications
You must be signed in to change notification settings - Fork 515
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
NE-1530: IngressController LB Subnet Selection in AWS #1841
Conversation
@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:
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. |
Hello @gcs278! Some important instructions when contributing to openshift/api: |
@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:
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: 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:
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: 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:
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. |
/hold |
I'm not sure whether this PR is waiting for my |
/test verify-crd-schema |
There is interest in refactoring |
@JoelSpeed @Miciah @candita updated API per @JoelSpeed suggestion in slack. The Sorry about mixing the rebase in the recent force push (I usually try to avoid that), but the changes are in the type_ingress.go diff and the Integration Tests diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we fix the list comment I've made in both places that list shows up, then I'm happy with the API fields as they are.
Are we only doing subnets in the spec now? I thought they were previously in spec and status?
// AWSSubnets contains a list of references to AWS subnets by | ||
// ID or name. | ||
// +kubebuilder:validation:XValidation:rule=`has(self.ids) && has(self.names) ? size(self.ids + self.names) <= 10 : true`,message="the total number of subnets cannot exceed 10" | ||
// +kubebuilder:validation:XValidation:rule=`has(self.ids) || has(self.names)`,message="must specify at least 1 subnet name or id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a min properties validation that means you must have at least one item in the struct for it to be valid, not sure if that's better than this (since you have a custom error message), but thought I'd let you know in case you wanted to try it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I appreciate the pointer. The error message with // +kubebuilder:validation:MinProperties=1
is:
spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.classicLoadBalancer.subnets in body should have at least 1 properties
I suppose my custom error message may be considered slightly more specific than that, so I'll keep the CEL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JoelSpeed
Allows users to specify subnets (i.e. Availability Zones) for IngressControllers using load balancers in AWS. Introduce under the `IngressControllerLBSubnetsAWS` FeatureGate.
/lgtm Feel free to cancel the hold if the network folks are happy now |
/override ci/prow/verify-crd-schema |
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema In response to this:
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. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita, gcs278, JoelSpeed, Miciah 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 |
@gcs278: all tests passed! 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. |
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-config-api-container-v4.17.0-202407100341.p0.g465787e.assembly.stream.el9 for distgit ose-cluster-config-api. |
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