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.

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

CRD Validation Expression Language #2876

Open
9 of 11 tasks
jpbetz opened this issue Aug 18, 2021 · 38 comments
Open
9 of 11 tasks

CRD Validation Expression Language #2876

jpbetz opened this issue Aug 18, 2021 · 38 comments
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. stage/beta Denotes an issue tracking an enhancement targeted for Beta status tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team
Milestone

Comments

@jpbetz
Copy link
Contributor

jpbetz commented Aug 18, 2021

Enhancement Description

Please keep this description up to date. This will help the Enhancement Team to track the evolution of the enhancement efficiently.

@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 18, 2021
@mysunshine92
Copy link

mysunshine92 commented Aug 25, 2021

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 25, 2021
@kevindelgado
Copy link
Contributor

kevindelgado commented Sep 3, 2021

Hi @jpbetz and @cici37! 1.23 Enhancements team here. Just checking in as we approach enhancements freeze on Thursday 09/09. Here's where this enhancement currently stands:

  • KEP file using the latest template has been merged into the k/enhancements repo (PR Open).
  • KEP status is marked as implementable
  • KEP has a test plan section filled out.
  • KEP has up to date graduation criteria (Only has criteria for beta).
  • KEP has a production readiness review that has been completed and merged into k/enhancements.

If you can, please add a test plan and alpha graduation criteria to the KEP and be sure the KEP PR merges by enhancements freeze :)

Thanks!

@kevindelgado
Copy link
Contributor

kevindelgado commented Sep 3, 2021

/milestone 1.23
/stage alpha

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 3, 2021

@kevindelgado: The provided milestone is not valid for this repository. Milestones in this repository: [keps-beta, keps-ga, v1.17, v1.18, v1.19, v1.20, v1.21, v1.22, v1.23, v1.24, v1.25, v1.26]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.23
/stage alpha

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.

@k8s-ci-robot k8s-ci-robot added the stage/alpha Denotes an issue tracking an enhancement targeted for Alpha status label Sep 3, 2021
@kevindelgado
Copy link
Contributor

kevindelgado commented Sep 3, 2021

/milestone v1.23

@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 3, 2021
@kevindelgado kevindelgado added the tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team label Sep 3, 2021
@kevindelgado
Copy link
Contributor

kevindelgado commented Sep 8, 2021

Hi @cici37

Ping! As a reminder your PR (#2877) needs to merge by EOD PST tomorrow September 9th to be included in the 1.23 Release. After that time you will need to request an exception.

Lmk if you need anything,
Kevin

@cici37
Copy link
Contributor

cici37 commented Sep 9, 2021

@kevindelgado PR has been merged. Updated the checkbox. Thanks^^

@mehabhalodiya
Copy link

mehabhalodiya commented Sep 18, 2021

Hi @jpbetz 👋 1.23 Docs shadow here.

This enhancement is marked as 'Needs Docs' for the 1.23 release.

Please follow the steps detailed in the documentation to open a PR against the dev-1.23 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before Thu November 18, 11:59 PM PDT.

Also, if needed take a look at Documenting for a release to familiarize yourself with the docs requirement for the release.

Thanks!

@kevindelgado
Copy link
Contributor

kevindelgado commented Nov 8, 2021

Hi @cici37 and @jpbetz 👋

Checking in once more as we approach 1.23 code freeze at 6:00 pm PST on Tuesday, November 16.

Please ensure the following items are completed:

  • All PRs to the Kubernetes repo that are related to your enhancement are linked in the above issue description (for tracking purposes).
  • All PRs are fully merged by the code freeze deadline.
  • Have a documentation placeholder PR open by Thursday, November 18.

As always, we are here to help should questions come up.

Thanks!!

@kikisdeliveryservice
Copy link
Member

kikisdeliveryservice commented Nov 11, 2021

Related: #3039

@cici37
Copy link
Contributor

cici37 commented Nov 16, 2021

@kevindelgado @mehabhalodiya
Thanks for the reminder. All PRs to k/k related with this enhancement have been merged and we have a documentation placeholder PR here. Updated the description.

@cici37
Copy link
Contributor

cici37 commented Nov 24, 2021

I have updated the documentation PR to kubernetes/website#30626. Sorry for any inconvenient.

@gracenng gracenng added tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team and removed tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team labels Jan 9, 2022
@gracenng gracenng removed this from the v1.23 milestone Jan 9, 2022
@liggitt liggitt added this to the v1.24 milestone Jan 19, 2022
@deads2k
Copy link
Contributor

deads2k commented Jan 21, 2022

related enhancement #3160

@gracenng gracenng added tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team stage/beta Denotes an issue tracking an enhancement targeted for Beta status and removed tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team stage/alpha Denotes an issue tracking an enhancement targeted for Alpha status labels Jan 22, 2022
@hosseinsalahi
Copy link

hosseinsalahi commented Jan 28, 2022

Hello @jpbetz

v1.24 Enhancements team here.

Just checking in as we approach enhancements freeze on 18:00pm PT on Thursday Feb 3rd, 2022. This enhancement is targeting beta for v1.24, is this correct?

Here’s where this enhancement currently stands:

  • Updated KEP file using the latest template has been merged into the k/enhancements repo.
  • KEP status is marked as implementable for this release
  • KEP has a test plan section filled out.
  • KEP has up to date graduation criteria.
  • KEP has a production readiness review that has been completed and merged into k/enhancements.

In the kep.yaml latest-milestone should point to the correct release.

The status of this enhancement is marked as at risk. Please update the merged kep.yaml to reflect the stage as current release cycle 1.24.
Thanks!

@kikisdeliveryservice
Copy link
Member

kikisdeliveryservice commented May 13, 2022

This issue should remain open until the feature is delivered to GA. Please continue to use this to track beta (including #3266) and GA @cici37 .

@cici37
Copy link
Contributor

cici37 commented May 13, 2022

/milestone v1.25
Thanks for the clarification!

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone May 13, 2022
@Priyankasaggu11929 Priyankasaggu11929 added tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team and removed tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team labels Jun 3, 2022
@Priyankasaggu11929
Copy link
Member

Priyankasaggu11929 commented Jun 9, 2022

Hello @jpbetz @cici37 👋, 1.25 Enhancements team here.

Just checking in as we approach enhancements freeze on 18:00 PST on Thursday June 16, 2022.

For note, This enhancement is targeting for stage beta for 1.25 (correct me, if otherwise)

Here's where this enhancement currently stands: (updated on June 9, 2022)

  • KEP file using the latest template has been merged into the k/enhancements repo.
  • KEP status is marked as implementable
  • KEP has a updated detailed test plan section filled out
  • KEP has up to date graduation criteria
  • KEP has a production readiness review that has been completed and merged into k/enhancements.

For note, the status of this enhancement is marked as tracked. Thank you for keeping the issue description up-to-date!

@didicodes
Copy link

didicodes commented Jul 13, 2022

Hello @jpbetz, @cici37 👋, 1.25 Release Docs Shadow here.

This enhancement is marked as ‘Needs Docs’ for the 1.25 release. Please follow the steps detailed in the documentation to open a PR against the dev-1.25 branch in the k/website repo. This PR can be just a placeholder at this time and must be created by August 4.


Also, take a look at Documenting for a release to familiarize yourself with the docs requirement for the release. Thank you!

@cici37
Copy link
Contributor

cici37 commented Jul 14, 2022

Thanks @didicodes for the reminder. The doc placeholder PR is created here: kubernetes/website#35029

@didicodes
Copy link

didicodes commented Jul 15, 2022

Well recieved, @cici37. Thank you 🙌

@jasonbraganza
Copy link
Member

jasonbraganza commented Jul 25, 2022

Hi @cici37, @jpbetz 👋

Checking in once more as we approach 1.25 code freeze at 01:00 UTC on Wednesday, 3rd August 2022.

Please ensure the following items are completed:

Please verify, if there are any additional k/k PRs besides the ones listed above.
Please plan to get the open k/k merged by the code freeze deadline. The status of the enhancement is currently marked as at-risk.
Please also update the issue description with the relevant links for tracking purpose. Thank you so much!

@jasonbraganza
Copy link
Member

jasonbraganza commented Jul 29, 2022

Hello @cici37 & @jpbetz 👋

Just a gentle reminder from the enhancement team as we approach 1.25 code freeze at 01:00 UTC on Wednesday, 3rd August 2022. (less than a week to go)
Please plan to have the k/k PR merged before then.

The status of this enhancement is currently marked as at risk

Thank you.

@cici37
Copy link
Contributor

cici37 commented Jul 29, 2022

Thanks for reaching out. We have all required PR into k/k. Will work on doc shortly. Thank you

@jasonbraganza
Copy link
Member

jasonbraganza commented Aug 2, 2022

Hello @cici37 & @jpbetz 👋🏿

All k/k code PRs are now merged, so the status is now marked as tracked

Thank you!

@tylergu
Copy link

tylergu commented Aug 10, 2022

Hi all, I am not sure if this is the appropriate place to discuss some potential improvement for this feature, I would appreciate if someone could point me to if there is a more appropriate way to discuss this, (slack or opening an issue?)

I was using the CustomResourceValidationExpressions feature with the latest Kubernetes, I wanted to specify immutability through the rule self == oldSelf using CEL expression. The feature works properly, when I mutate the field, the CR update gets rejected with the error message: The RabbitmqCluster "test-cluster" is invalid: spec.persistence.storageClassName: Invalid value: "string": cannot change StorageClass.

I think the error message could be improved by showing the actual value of the field rather than the type of the field. In the transition rule case, the error message could show the values of self -> oldSelf.

I looked into this KEP and found that the errors constructed here where the value simply uses the sts.Type: code
I think the error could be constructed using the function parameter obj, oldObj if that's possible. We could have a new error type instead of the ErrorTypeInvalid if that helps.

I am happy to contribute if you think this is a reasonable and feasible improvement.

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 10, 2022

Showing values is error messages is more problematic than might be immediately obvious.

  1. The value might be a small scalar, in which case it is easy to include in a message, but it also might be a complex object multiple MB in size, or anything in between.
  2. The value might contain sensitive information, in which case including it in an error message can result it being logged in various places, making it inconvenient from a security perspective.

We had originally printed values in the errors but changed to types primary due to (1).

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 10, 2022

cc @cici37

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 10, 2022

One option to consider here: Extend the functionality of the message field of each rule to support formatting. E.g. `message: "Field is immutable but was changed from {{oldSelf}} to {{self}}"

@tylergu
Copy link

tylergu commented Aug 10, 2022

@jpbetz Thanks for the explanation. I was suggesting this because I saw other validators also include the values in the error messages, e.g. EnumFailCode Why should the x-kubernetes-validations should be a special case here?

One option to consider here: Extend the functionality of the message field of each rule to support formatting. E.g. `message: "Field is immutable but was changed from {{oldSelf}} to {{self}}"

I think this option makes sense as an alternative

@jpbetz
Copy link
Contributor Author

jpbetz commented Aug 10, 2022

Why should the x-kubernetes-validations should be a special case here?

Because it is very general, it can be used on any part of a CRDs schema and so to reason about what is safe and reasonable, you have to consider all possible uses. Enums are much simpler, they have a set of possible values all of which would typically be safe to be included in a message without causing either of the problems I mentioned.

@tylergu
Copy link

tylergu commented Aug 10, 2022

Sorry, let me clarify with some examples.
I think the two problems you mentioned exist for some existing validators, e.g. Enum validator, String's Pattern validation. Both these validation prints the value of the field in the error message:

  1. The RabbitmqCluster "test-cluster" is invalid: spec.service.type: Unsupported value: "veryLooooooooooooooooooooooooooooooooooooogString": supported values: "ClusterIP", "LoadBalancer", "NodePort"
  2. spec.persistence.storage: Invalid value: "5000000000000000000000000000000000GGGGGGGGGGGGGGGi": spec.persistence.storage in body should match '^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$'

Both of these validations could contain very large data (problem 1), and contain sensitive data if the field is meant for some authentication purposes (problem 2). When the validation fails, the values will be included in the error message. The pattern validation could be used for any String field, the enum validation can be used for any part of a CRD schema too.

Could you give me some counter-examples where the x-kubernetes-validations causes problems, but enum validation does not cause problems?

Much Thanks!

@cici37
Copy link
Contributor

cici37 commented Aug 10, 2022

Hi @tylergu , Thanks for raising the suggestion.

We intensively changed to return type instead of the object in kubernetes/kubernetes#107090 to address the concern of returning too large object back to the user.

The topic was brought up in one of the sig meetings as well and get the agreement of not aligning with some of the existing validations. I believe it is a valid concern since the chance to return back a large object for validation rules is larger(based on the possible use cases we investigated) and change the returned object for existing validator could be considered as a API breaking change which makes it harder to adjust.

Hope the additional info helped.

@tylergu
Copy link

tylergu commented Aug 10, 2022

@cici37 Thanks for the additional info!

In this case I think the solution proposed by @jpbetz makes sense to let user have some control whether to include the value or not.

The current error message is really confusing, to some extent it doesn't make sense: The RabbitmqCluster "test-cluster" is invalid: spec.persistence.storageClassName: Invalid value: "string": cannot change StorageClass. The error message says that Invalid value: "string", where the "string" is the type, but not Invalid value.

@rhockenbury rhockenbury added tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team and removed tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team labels Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. stage/beta Denotes an issue tracking an enhancement targeted for Beta status tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team
Projects
No open projects
Development

No branches or pull requests