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.

Sign up for GitHub

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

Jump to bottom

NE-705: IngressController subnet selection in AWS #1595

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

NE-705: IngressController subnet selection in AWS #1595

wants to merge 2 commits into from

Conversation

gcs278
Copy link
Contributor

@gcs278 gcs278 commented Mar 13, 2024 •
edited

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets. This enhancement introduces spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.subnets which allows cluster admins to specify the subnets for the load balancer.

Epic: https://issues.redhat.com/browse/NE-705
RFE: https://issues.redhat.com/browse/RFE-1717

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 13, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 13, 2024 •
edited by openshift-ci bot

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

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets.

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 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 Mar 13, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 13, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch from 8a2586a to 8bc4b3d Compare March 27, 2024 22:41
@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch 2 times, most recently from 8dbb992 to 1a5f6b7 Compare March 29, 2024 20:06
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 29, 2024 •
edited by openshift-ci bot

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

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets.

Epic: https://issues.redhat.com/browse/NE-705
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.

@gcs278 gcs278 marked this pull request as ready for review March 29, 2024 20:06
@openshift-ci openshift-ci bot requested review from frobware and Miciah March 29, 2024 20:07
@gcs278 gcs278 changed the title [WIP] NE-705: IngressController subnet selection in AWS NE-705: IngressController subnet selection in AWS Mar 29, 2024
@openshift-ci 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 Mar 29, 2024
@gcs278
Copy link
Contributor Author

gcs278 commented Mar 29, 2024

Ready for initial review, but will keep hold on until I feel it has consensus.

Currently missing install-time design, but I have stubbed out the sections that need updating for that.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 29, 2024 •
edited by openshift-ci bot

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

Adds lb-subnet-selection-aws.md enhancement for specifying IngressController's load balancer type service subnets. This enhancement introduces spec.endpointPublishingStrategy.loadBalancer.providerParameters.aws.subnets which allows cluster admins to specify the subnets for the load balancer.

Epic: https://issues.redhat.com/browse/NE-705
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.

@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch from 1a5f6b7 to 565156e Compare March 30, 2024 03:17
@gcs278 gcs278 mentioned this pull request Apr 1, 2024
Open
@gcs278
Copy link
Contributor Author

gcs278 commented Apr 2, 2024

Updating the Enhancement for the API to be immutable. Temporarily WIP
/wip

@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch from 565156e to 51a7c4d Compare April 3, 2024 03:00
@gcs278
Copy link
Contributor Author

gcs278 commented Apr 3, 2024

/wip cancel

@Miciah
Copy link
Contributor

Miciah commented Apr 3, 2024

/assign

@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch 2 times, most recently from 03fe7d0 to 27a63e9 Compare May 7, 2024 16:16
gcs278
gcs278 commented May 7, 2024
View reviewed changes
Copy link
Contributor Author

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

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

Thank for another review @candita

Updates here https://github.com/openshift/enhancements/compare/f6ada5a04048efcaf75b90349e36edfa83a0ea09..27a63e90db1231e58f452506405fddafa72da546. PTAL when you can.

enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Outdated
Comment on lines 275 to 276
Note that updating the status with admin instructions is consistent with the approach in [LoadBalancer Allowed Source Ranges](/enhancements/ingress/lb-allowed-source-ranges.md),
but diverges with [Ingress Mutable Publishing Scope](/enhancements/ingress/mutable-publishing-scope.md).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, the suggestion doesn't make sense to me. It says "For both of these options...this design...is consistent with LB Allowed Sourcer Ranges....but diverges with Ingress Mutable Publishing Scope". Adding "For both these options" confuses me, not sure what the "options" are.

Updated to something similar, let me know if it makes sense.

enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
@gcs278 gcs278 mentioned this pull request May 8, 2024
Open
@candita
Copy link
Contributor

candita commented May 9, 2024

No major concerns, just a few nits.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2024
@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch from 27a63e9 to 1d13c6d Compare May 10, 2024 02:56
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 10, 2024
gcs278
gcs278 commented May 10, 2024
View reviewed changes
Copy link
Contributor Author

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

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

Thanks for another review @candita

Changes here: https://github.com/openshift/enhancements/compare/27a63e90db1231e58f452506405fddafa72da546..1d13c6dc106eaf018d32163c42453787249ab423

enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Outdated Show resolved Hide resolved
enhancements/ingress/lb-subnet-selection-aws.md Show resolved Hide resolved
@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch from 1d13c6d to 5132daf Compare May 12, 2024 18:35
@gcs278
Copy link
Contributor Author

gcs278 commented May 12, 2024

@candita Updated to reflect Joel API design suggestion for openshift/api#1841. May update a few more times to keep enhancement and API in sync.

@candita
Copy link
Contributor

candita commented May 13, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2024
@gcs278
NE-705: IngressController subnet selection in AWS
a4e19e7 
Adds lb-subnet-selection-aws.md enhancement for specifying
IngressController's load balancer type service subnets.
@gcs278
Add auto-delete to IngressController Subnet Selection
c69aee5 
Add ingress.operator.openshift.io/auto-delete-load-balancer
functionality to the lb-subnet-selection-aws.md enhancement.
This allows for configuration management tooling to seamlessly update
subnets on an IngressController.
@gcs278 gcs278 force-pushed the ingresscontroller-subnets-aws branch from 5132daf to c69aee5 Compare May 14, 2024 15:10
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 14, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 14, 2024

New changes are detected. LGTM label has been removed.

@gcs278
Copy link
Contributor Author

gcs278 commented May 14, 2024

@candita Sorry, another update to keep in sync with the API: https://github.com/openshift/enhancements/compare/5132daf966f01988aec29c9202b505c8aaf32aef..c69aee540871a86f6209bd35701af85e7b654dd5

Joel has LGTM'ed the API PR now.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@mtulio mtulio mtulio left review comments

@candita candita candita left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@alebedev87 alebedev87 alebedev87 left review comments

@frobware frobware Awaiting requested review from frobware

Assignees

@Miciah Miciah

@candita candita

@alebedev87 alebedev87

Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@gcs278 @openshift-ci-robot @Miciah @candita @mtulio @JoelSpeed @alebedev87