search

kubernetes / enhancements

Enhancements tracking repo for Kubernetes
Apache License 2.0
3.33k stars 1.44k forks source link

CRD Validation Expression Language #2876

Closed jpbetz closed 6 months ago

jpbetz commented 2 years ago

Enhancement Description

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

mysunshine92 commented 2 years ago

/sig api-machinery

kevindelgado commented 2 years ago

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:

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 commented 2 years ago

/milestone 1.23 /stage alpha

k8s-ci-robot commented 2 years ago

@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](https://github.com/kubernetes/enhancements/issues/2876#issuecomment-912829269): >/milestone 1.23 >/stage alpha Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kevindelgado commented 2 years ago

/milestone v1.23

kevindelgado commented 2 years ago

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 commented 2 years ago

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

mehabhalodiya commented 2 years ago

Hi @jpbetz :wave: 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 commented 2 years ago

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:

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

Thanks!!

kikisdeliveryservice commented 2 years ago

Related: https://github.com/kubernetes/enhancements/pull/3039

cici37 commented 2 years ago

@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 commented 2 years ago

I have updated the documentation PR to https://github.com/kubernetes/website/pull/30626. Sorry for any inconvenient.

deads2k commented 2 years ago

related enhancement https://github.com/kubernetes/enhancements/pull/3160

hosseinsalahi commented 2 years ago

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:

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!

gracenng commented 2 years ago

The Enhancements Freeze is now in effect and this enhancement is removed from the release. Missing beta PRR Please feel free to file an exception.

/milestone clear

jpbetz commented 2 years ago

The Enhancements Freeze is now in effect and this enhancement is removed from the release. Missing beta PRR Please feel free to file an exception.

/milestone clear

We are keeping this feature in alpha in 1.24. This was planned. This release contained API breaking changes and so a promotion to beta would have been problematic from an API stability perspective. I've updated the description of this issue to try to make that more clear.

gracenng commented 2 years ago

Hi @jpbetz, 1.24 Enhancements Lead here. Looks like the latest-milestone in the KEP has not been updated to 1.24.

Assuming there's no code and doc change, please file an exception request for Enhancements Freeze. Thanks!

cici37 commented 2 years ago

Hi Joe Betz, 1.24 Enhancements Lead here. Looks like the latest-milestone in the KEP has not been updated to 1.24.

Assuming there's no code and doc change, please file an exception request for Enhancements Freeze. Thanks!

@gracenng Hi Grace, This KEP update is not for 1.24. We are updating the issue for 1.25 release. So it should be out of the 1.24 track. Thanks for reaching out^

cici37 commented 2 years ago

For better tracking the process of promoting the feature to beta: https://github.com/kubernetes/enhancements/issues/3297

kikisdeliveryservice commented 2 years ago

This issue should remain open until the feature is delivered to GA. Please continue to use this to track beta (including https://github.com/kubernetes/enhancements/pull/3266) and GA @cici37 .

cici37 commented 2 years ago

/milestone v1.25 Thanks for the clarification!

Priyankasaggu11929 commented 2 years ago

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)

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

didicodes commented 1 year ago

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 commented 1 year ago

Thanks @didicodes for the reminder. The doc placeholder PR is created here: https://github.com/kubernetes/website/pull/35029

didicodes commented 1 year ago

Well recieved, @cici37. Thank you 🙌

jasonbraganza commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jasonbraganza commented 1 year ago

Hello @cici37 & @jpbetz 👋🏿

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

Thank you!

tylergu commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

cc @cici37

jpbetz commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Hi @tylergu , Thanks for raising the suggestion.

We intensively changed to return type instead of the object in https://github.com/kubernetes/kubernetes/pull/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 commented 1 year ago

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

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

m-yosefpor commented 1 year ago

/remove-lifecycle stale

lavalamp commented 1 year ago

/label lead-opted-in /milestone v1.27

cici37 commented 1 year ago

Note: Talked with Enhancement lead @marosset offline earlier, this one is remaining in beta without stage change in v1.27 hence it is ok to not track it for release team. We would love it to be tracked to make sure code is in before deadline and documentation is updated properly. The required KEP update has been merged earlier. No PRR update is needed for this one.

cici37 commented 1 year ago

/tracked yes

cici37 commented 1 year ago

/assign

marosset commented 1 year ago

Hi @cici37 & @jpbetz :wave:,

Checking in as we approach 1.27 code freeze at 17:00 PDT on Tuesday 14th March 2023.

Please ensure the following items are completed:

Please let me know if there are any other PRs in k/k I should be tracking for this KEP.

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

cici37 commented 1 year ago

@marosset Thanks for reaching out! The PR we would like to merge in 1.27 regarding with this KEP is: https://github.com/kubernetes/kubernetes/pull/115969. It is currently on track. Thanks

taniaduggal commented 1 year ago

Hi @jpbetz 👋 , I’m reaching out from the 1.27 Release Docs team. This enhancement is marked as ‘Needs Docs’ for the 1.27 release. Please follow the steps detailed in the documentation to open a PR against dev-1.27 branch in the k/website repo. This PR can be just a placeholder at this time, and must be created by March 16. For more information, please take a look at Documenting for a release to familiarize yourself with the documentation requirements for the release. Please feel free to reach out with any questions. Thanks!

cici37 commented 1 year ago

Hi @jpbetz 👋 , I’m reaching out from the 1.27 Release Docs team. This enhancement is marked as ‘Needs Docs’ for the 1.27 release. Please follow the steps detailed in the documentation to open a PR against dev-1.27 branch in the k/website repo. This PR can be just a placeholder at this time, and must be created by March 16. For more information, please take a look at Documenting for a release to familiarize yourself with the documentation requirements for the release. Please feel free to reach out with any questions. Thanks!

@taniaduggal Thanks for reaching out! The doc placeholder PR for this kep in 1.27 is opened here: https://github.com/kubernetes/website/pull/40019

cici37 commented 1 year ago

/milestone v1.28

jpbetz commented 1 year ago

/label lead-opted-in