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

Update the KEP for validation #2

Merged
merged 1 commit into from
Jul 29, 2021
Merged

Update the KEP for validation #2

merged 1 commit into from
Jul 29, 2021

Conversation

cici37
Copy link

@cici37 cici37 commented Jul 28, 2021

No description provided.

@cici37 cici37 force-pushed the crd-expression-lang-kep branch from 5f2f1e1 to 417f94f Compare July 28, 2021 20:25
jpbetz
jpbetz requested changes Jul 29, 2021
View reviewed changes
keps/sig-api-machinery/NNNN-crd-validation-defaulting-conversion-expressions/README.md Outdated Show resolved Hide resolved
keps/sig-api-machinery/NNNN-crd-validation-defaulting-conversion-expressions/README.md Outdated
@@ -261,7 +257,7 @@ kind: CustomResourceDefinition
properties:
spec:
x-kubernetes-validator:
- rule: "minReplicas <= maxReplicas"
- rule: this.properties.minReplicas <= this.properties.maxReplicas
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep this as minimal as possible for now. We might need a this keyword of equivalent, but I don't that's decided. And adding properties adds a lot of verbosity for unclear gains.

@cici37 cici37 force-pushed the crd-expression-lang-kep branch from 417f94f to a415112 Compare July 29, 2021 18:07
leilajal
leilajal reviewed Jul 29, 2021
View reviewed changes
keps/sig-api-machinery/NNNN-crd-validation-defaulting-conversion-expressions/README.md
@@ -246,10 +239,13 @@ expressions or webhooks.

## Proposal

### Validation
[Common Expression Language (CEL)](https://github.com/google/cel-go) is an
excellent supplement to the current validation mechanism because it is sufficiently expressive to
Copy link

Choose a reason for hiding this comment

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

Just trying to understand more, what's the advantage of CEL over other languages for defining unified policy?
For example why not we use OPA by CNCF?

Copy link
Author

Choose a reason for hiding this comment

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

OPA would be used as admission controller which shares very similar pro/cons as webhooks.

Copy link
Owner

@jpbetz jpbetz Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There are two orthogonal aspects to this:

  1. The idea of using a general purpose webhook for validation
  • pro: Doesn't require we make a change to Kubernetes core
  • con: webhooks are only called for types that they are registered for, so each CRD needs to be registered
  • con: requires introducing the webhook in the first place (and getting everyone to install it and deal with upgrades/roll-backs, ...)
  • con: doesn't promote a unified ecosystem for validation like 1st class support does
  1. The OPA rule grammar vs. CEL
  • designed for multiline programs (both a con and a pro)
  • does not have the same sandboxing constraints as CEL (clearly a con)
  • doesn't typecheck (con)
  • has a full package and module system (a con for when writing readable and compose-able validation rules)
  • integrates tightly with Kubernetes types (a significant pro)
  • opinionated about the language being for policy declarations, this bleeds into the grammar quite a bit (con, but could be overcome by working with OPA community to generalize grammar)

We really a section in the KEP that captures the details of these types of comparisons. I also think we need to be clear that we're still in the early phases of experimenting with all the various available grammars and that CEL is currently our leading candidate but that it is very possible that we will switch to something else if we find an alternative that is objectively better.

keps/sig-api-machinery/NNNN-crd-validation-defaulting-conversion-expressions/README.md Outdated
@@ -261,7 +257,7 @@ kind: CustomResourceDefinition
properties:
spec:
x-kubernetes-validator:
- rule: "minReplicas <= maxReplicas"
- rule: this.properties.minReplicas <= this.properties.maxReplicas
Copy link

Choose a reason for hiding this comment

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

this is an expression that has direct access to all the fields of this object (in spec). do we really need to scope to the fields using 'this'?

Copy link
Author

Choose a reason for hiding this comment

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

It has been updated. We could talk over it later

keps/sig-api-machinery/NNNN-crd-validation-defaulting-conversion-expressions/README.md
- The 'field' is optional and defaults to the scope if not explicitly set.

If the expression evaluates to null, the field is left unset.
TODO: Any downsides to using null this way?
Copy link

Choose a reason for hiding this comment

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

leaving the field unset makes our work extra hard to get the kind of data if we need to. Currently I can not think of any usecases where we need this. From a related angle, leaving the fields unset to represent null value reduce our convictions about the data.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the idea here was to make it possible to conditionally default some field value only is some condition is true, otherwise don't do any defaulting. But the specifics of how we do that is a bit of an incomplete thought here.

Let's drop this from the KEP for now since this has been moved to future work and is an incomplete thought. We can circle back when we get to defaulting.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll clean this up in a separate PR actually, since this PR just moved the text.

@jpbetz jpbetz merged commit 4b731b4 into jpbetz:crd-expression-lang-kep Jul 29, 2021
jpbetz pushed a commit that referenced this pull request Sep 29, 2021
@ffromani
Merge pull request #2 from swatisehgal/minor-fixes
730c7dd 
Minor formatting/typo fixes and disambiguate explanation of smtaware policy
jpbetz pushed a commit that referenced this pull request Jul 27, 2022
@ritazh
Merge pull request #2 from aramase/3299-kms-v2
6d3d847 
chore: use snake case for non-generated proto API
jpbetz pushed a commit that referenced this pull request Feb 9, 2023
@KnVerey @justinsb
KEP-3659: ApplySet: kubectl apply --prune redesign and graduation str…
01da7ba 
…ategy (kubernetes#3661)

* Initial KEP for improving pruning in kubectl apply

* Add design details

Co-authored-by: Katrina Verey <katrina.verey@shopify.com>

* Add another open question

* Links, clarifications, ownerRef and GKNN explanations

* Follow-on to initial feedback, address some unresolved blocks

* Fix lint errors

* Add more detail about reference implementation (#2)

* Apply prune jan25 (#3)

* More clearly delineate specification vs kubectl details

* Move design details of spec to Design Details section

* Updates from synchronous conversation

* Remove leftover paragraph (#5)

Not an alternative rejected any more, given applyset.k8s.io/inventory

* Justin has always been coauthor

* KEP-3659: production readiness etc (#4)

Fill in the testing/ PRR sections.

* Fix test failures

* Prune: document confused deputy attack and mitigations

Likely pushes us to GKNN-derived IDs.

* Constrain applyset id

We just choose the constrained applyset id to prevent "applyset ID
impersonation".

* Update KEP and PRR metadata

* Enhance testing description

* ID vs name fixes

* Fixes from soltysh's review

---------

Co-authored-by: Justin Santa Barbara <justinsb@google.com>
jpbetz pushed a commit that referenced this pull request Oct 5, 2023
@nabokihms
Merge pull request #2 from aramase/aramase/f/kep_3331_review_comments_1
5204f7e 
address API review comments for extra mappings
jpbetz pushed a commit that referenced this pull request Sep 3, 2024
@ihcsim @carlbraganza @PrasadG193
KEP-3314: CSI Changed Block Tracking (kubernetes#4082)
cf708c9 
* Add draft of CSI CBT KEP

Signed-off-by: Ivan Sim <ivan.sim@dell.com>

* Update KEP status

Signed-off-by: Ivan Sim <ivan.sim@dell.com>

* Initial structure.
Filled in the Proposal, Caveats and Risks.
Put in the CSI spec in the Details section.

* Removed distracting links to common K8s definitions.
Clarified the proposal.

* More caveats.  Better grammar.

* Use "snapshot access session".

* addressed most of the feedback in the PR.

* Updated role figure.

* More refinements.

* Session figure. Renamed figure files.

* Fix background of session figure.

* Updated figures and roles.

* Propose a new role for session data.

* GRPC spec

* Don't propose roles.

* Add user stories in the proposal (#2)

* Add user stories in the proposal

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>

* Remove acceptance criteria for the user stories

* Make changes suggested by Carl

---------

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>

* Added details to the manager, sidecar and SP service sections.
Fixed session figure errors and rewrote the client gRPC
description in the risks section.

* Called out UNRESOLVED issues.
More on the SP service and sidecar.

* Resolved issues with expiry and advertising.

* Updated TOC

* Fixed typo and svg space rendering.

* Fixed typo in perms figure.

* Typo in session figure. More detail in user stories.

* Add SnapshotSession CRDs (#5)

* Add SnapshotSession CRDs

* Add CR descriptions

* Address review comments

* Address review comments

* Remove typo

* Remove unnecessary new line

* Added image of the flow when the TokenRequest and TokenReview APIs are used.

* Fixed figure spacing

* Updated permissions svg; removed session.

* Updated figures. Removed session figure.

* Added explanation of permissions.

* Updated overview and risks.

* Updated RPC and components.

* Completed remaining rewrite.

* Updated to CSI spec to reflect container-storage-interface/spec#551

* Removed the security_token and namespace from the gRPC spec.
Pass the security token via the metadata authorization key.
Pass the namespace as part of the K8s snapshot id string.

* Update sections on test plan, PRR and graduation criteria

Signed-off-by: Ivan Sim <ihcsim@gmail.com>

* More neutral language on passing the auth token.

* Updated to reflect changes in the CSI spec PR.

* Use a separate gRPC API for the sidecar.

* Replaced authorization gRPC metadata with a security_token field in request messages.

* Fixed typo.

* Updated CSI spec; downplayed similarity between the K8s and CSI gRPC services.

* Add beta and GA graduation criteria

Signed-off-by: Ivan Sim <ihcsim@gmail.com>

* Updated CSI spec again - no unsigned numbers used.

* Update KEP milestone to v1.30

Signed-off-by: Ivan Sim <ihcsim@gmail.com>

* Update 'Scalability' section

Signed-off-by: Ivan Sim <ihcsim@gmail.com>

* Add sig-auth as participating sigs

Signed-off-by: Ivan Sim <ihcsim@gmail.com>

* Require that the CR be named for the driver.

* Removed the label requirement for the CR.

* Replaced johnbelamaric with soltysh for PRR approver.

* Bump up milestone to v1.31

* Change KEP status to implementable

---------

Signed-off-by: Ivan Sim <ivan.sim@dell.com>
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
Signed-off-by: Ivan Sim <ihcsim@gmail.com>
Co-authored-by: Carl Braganza <carl@kasten.io>
Co-authored-by: Prasad Ghangal <prasad.ghangal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leilajal leilajal leilajal left review comments

@jpbetz jpbetz jpbetz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cici37 @jpbetz @leilajal