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

Move config v1 IP validation to CEL #1802

Merged
merged 1 commit into from Mar 14, 2024
Merged

Move config v1 IP validation to CEL #1802

merged 1 commit into from Mar 14, 2024

Conversation

JoelSpeed
Copy link
Contributor

Now that we have CEL validation for IP and CIDR in openshift 4.16, this PR changes to using CEL for two newly introduced APIs in 4.16/4.15.

The regex being removed attempted to support IPv4 addresses mapped in IPv6 addresses, however, the regex was broken and only actually accepted 255.255.255.255 as a valid v4 address as part of the 4in6 mapping.
The new CEL explicitly forbids 4in6.

I've spoken to @mkowalski and @sadasu and we believe this change should be ok.
For both use cases, we do not support IP 4in6 addresses, this is noted in the product docs

While using dual-stack networking, you cannot use IPv4-mapped IPv6 addresses, such as ::FFFF:198.51.100.1, where IPv6 is required.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 12, 2024

Hello @JoelSpeed! 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 openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 12, 2024
@openshift-ci openshift-ci bot requested review from jkyros and mfojtik March 12, 2024 15:42
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024
@deads2k
Copy link
Contributor

deads2k commented Mar 12, 2024

die regex, die!

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD ad26456 and 2 for PR HEAD a040f75 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD c691737 and 1 for PR HEAD a040f75 in total

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2024
@mkowalski
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed, mkowalski

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4785ede and 2 for PR HEAD ae74cd4 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 5f1498a and 1 for PR HEAD ae74cd4 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD f41c09c and 0 for PR HEAD ae74cd4 in total

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 13, 2024 •
edited

@JoelSpeed: 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/e2e-azure ae74cd4 link false /test e2e-azure

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

@openshift-ci-robot
Copy link

/hold

Revision ae74cd4 was retested 3 times: holding

@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 13, 2024
@JoelSpeed
Copy link
Contributor Author

/hold cancel

Master keeps merging, not actually an issue with this PR

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2024
@JoelSpeed
Copy link
Contributor Author

/retest-required

@JoelSpeed
Copy link
Contributor Author

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Mar 14, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 3cbbe4f into openshift:master Mar 14, 2024
16 of 17 checks passed
@dgoodwin dgoodwin mentioned this pull request Mar 15, 2024
Closed
dgoodwin added a commit to dgoodwin/api that referenced this pull request Mar 15, 2024
@dgoodwin
Revert "Merge pull request openshift#1802 from JoelSpeed/cel-ip-cidr"
f5264cb 
This reverts commit 3cbbe4f, reversing
changes made to 4caef7f.
dgoodwin added a commit to dgoodwin/api that referenced this pull request Mar 15, 2024
@dgoodwin
Revert "Merge pull request openshift#1802 from JoelSpeed/cel-ip-cidr"
a8d9711 
This reverts commit 3cbbe4f, reversing
changes made to 4caef7f.
@dgoodwin dgoodwin mentioned this pull request Mar 15, 2024
Merged
@JoelSpeed JoelSpeed mentioned this pull request Mar 15, 2024
Closed
openshift-merge-bot bot added a commit that referenced this pull request Mar 15, 2024
@openshift-merge-bot
Merge pull request #1810 from dgoodwin/revert-1802-1710503688947
094bb9e 
TRT-1563: Revert #1802 "Move config v1 IP validation to CEL"
JoelSpeed added a commit to JoelSpeed/api that referenced this pull request Mar 19, 2024
@JoelSpeed
Revert "Revert "Merge pull request openshift#1802 from JoelSpeed/cel-…
f993244 
…ip-cidr""

This reverts commit a8d9711.
JoelSpeed added a commit to JoelSpeed/api that referenced this pull request Mar 19, 2024
@JoelSpeed
Revert "Revert "Merge pull request openshift#1802 from JoelSpeed/cel-…
e3484eb 
…ip-cidr""

This reverts commit a8d9711.
@JoelSpeed JoelSpeed mentioned this pull request Mar 19, 2024
Merged
JoelSpeed added a commit to JoelSpeed/api that referenced this pull request Apr 16, 2024
@JoelSpeed
Revert "Revert "Merge pull request openshift#1802 from JoelSpeed/cel-…
e866820 
…ip-cidr""

This reverts commit a8d9711.
JoelSpeed added a commit to JoelSpeed/api that referenced this pull request Apr 16, 2024
@JoelSpeed
Revert "Revert "Merge pull request openshift#1802 from JoelSpeed/cel-…
5c5e8a5 
…ip-cidr""

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

@jkyros jkyros Awaiting requested review from jkyros

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@mkowalski mkowalski

@deads2k deads2k

Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JoelSpeed @deads2k @openshift-ci-robot @mkowalski