Skip to content

NE-955: IngressController - Add LoadBalancer AllowedSourceRanges API#1222

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
suleymanakbas91:allowed-source-ranges
Sep 30, 2022
Merged

NE-955: IngressController - Add LoadBalancer AllowedSourceRanges API#1222
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
suleymanakbas91:allowed-source-ranges

Conversation

@suleymanakbas91
Copy link
Contributor

@suleymanakbas91 suleymanakbas91 commented Jul 8, 2022 •
edited
Loading

  • operator/v1/types_ingress.go (CIDR): New type. Specify an IP address range in CIDR notation.
    (LoadBalancerStrategy): Add AllowedSourceRanges field of type slice of CIDR.
  • The rest is generated.

Enhancement Proposal: openshift/enhancements#1177

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 8, 2022

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

@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 Jul 8, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 8, 2022

Hello @suleymanakbas91! 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.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label command.

Who should apply these qe/docs/px labels?

  • For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
  • For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
  • For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

@openshift-ci openshift-ci bot requested review from bparees and eparis July 8, 2022 07:30
@suleymanakbas91
Copy link
Contributor Author

suleymanakbas91 commented Jul 8, 2022

/test all

@suleymanakbas91 suleymanakbas91 mentioned this pull request Jul 8, 2022
Merged
@suleymanakbas91 suleymanakbas91 force-pushed the allowed-source-ranges branch from e839707 to f16f4a2 Compare July 8, 2022 15:57
@suleymanakbas91
Copy link
Contributor Author

suleymanakbas91 commented Jul 8, 2022

/test all

jerpeter1
jerpeter1 reviewed Jul 19, 2022
View reviewed changes
@suleymanakbas91 suleymanakbas91 force-pushed the allowed-source-ranges branch from f16f4a2 to 584ef2b Compare July 26, 2022 13:35
@suleymanakbas91 suleymanakbas91 marked this pull request as ready for review July 26, 2022 16:51
@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 Jul 26, 2022
@openshift-ci openshift-ci bot requested review from derekwaynecarr and knobunc July 26, 2022 16:52
@suleymanakbas91 suleymanakbas91 force-pushed the allowed-source-ranges branch 2 times, most recently from 47ce43f to 5553a89 Compare July 27, 2022 10:57
@gcs278
Copy link
Contributor

gcs278 commented Jul 27, 2022

/assign @gcs278

@candita
Copy link
Contributor

candita commented Jul 27, 2022

/assign @Miciah

@suleymanakbas91 suleymanakbas91 changed the title NE-955: Add LoadBalancer AllowedSourceRanges API NE-955: IngressController - Add LoadBalancer AllowedSourceRanges API Jul 28, 2022
@gcs278
Copy link
Contributor

gcs278 commented Jul 29, 2022

No comments, looks good to me, but I'll wait for enhancement proposal to merge before lgtm.

Miciah
Miciah reviewed Aug 4, 2022
View reviewed changes
Miciah
Miciah reviewed Aug 4, 2022
View reviewed changes
Miciah
Miciah reviewed Aug 4, 2022
View reviewed changes
@suleymanakbas91 suleymanakbas91 force-pushed the allowed-source-ranges branch 3 times, most recently from 69a20a8 to aaa02c6 Compare August 17, 2022 11:39
@suleymanakbas91 suleymanakbas91 mentioned this pull request Aug 17, 2022
Merged
@suleymanakbas91
Copy link
Contributor Author

suleymanakbas91 commented Aug 22, 2022

Playground to test the CIDR range validation pattern: https://go.dev/play/p/kxt-x6LJknW

@suleymanakbas91
Copy link
Contributor Author

suleymanakbas91 commented Aug 22, 2022

Rebased over latest master. Run make generate-with-container again and it added more generated files to the PR.

Miciah
Miciah reviewed Aug 24, 2022
View reviewed changes
deads2k
deads2k reviewed Aug 26, 2022
View reviewed changes
operator/v1/types_ingress.go

// CIDR is an IP address range in CIDR notation (for example, "10.0.0.0/8"
// or "fd00::/8").
// +kubebuilder:validation:Pattern=`(^(([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/([0-9]|[12][0-9]|3[0-2])$)|(^s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:)))(%.+)?s*(\/(12[0-8]|1[0-1][0-9]|[1-9][0-9]|[0-9]))$)`
Copy link
Contributor

@deads2k deads2k Aug 26, 2022

Choose a reason for hiding this comment

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

🤯

deads2k
deads2k reviewed Aug 26, 2022
View reviewed changes
deads2k
deads2k reviewed Aug 26, 2022
View reviewed changes
@Miciah
Copy link
Contributor

Miciah commented Sep 30, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2022
@deads2k
Copy link
Contributor

deads2k commented Sep 30, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, Miciah, suleymanakbas91

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2022
@deads2k
Copy link
Contributor

deads2k commented Sep 30, 2022

if you want to test your regex, @JoelSpeed created a way to provide a test manifest and expected result files.

@jerpeter1
Copy link
Member

jerpeter1 commented Sep 30, 2022

/label qe-approved
/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added qe-approved Signifies that QE has signed off on this PR docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Sep 30, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 30, 2022

@suleymanakbas91: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit 199dc05 into openshift:master Sep 30, 2022
@suleymanakbas91 suleymanakbas91 deleted the allowed-source-ranges branch September 30, 2022 19:30
@JoelSpeed
Copy link
Contributor

JoelSpeed commented Oct 3, 2022

if you want to test your regex, @JoelSpeed created a way to provide a test manifest and expected result files.

Some examples of the most minimal tests can be found in #1305, from there we can add additional test cases to exercise various expected positive and negative validations, lmk if you want to try this out

@suleymanakbas91 suleymanakbas91 mentioned this pull request Oct 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deads2k deads2k deads2k left review comments

@jerpeter1 jerpeter1 jerpeter1 left review comments

@bparees bparees Awaiting requested review from bparees

@eparis eparis Awaiting requested review from eparis

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@knobunc knobunc Awaiting requested review from knobunc

+1 more reviewer

@Miciah Miciah Miciah left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Miciah Miciah

@gcs278 gcs278

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@suleymanakbas91 @gcs278 @candita @Miciah @deads2k @jerpeter1 @JoelSpeed @openshift-merge-robot