-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fn framework: Enable validation using openAPI schema for functionConfig #4467
fn framework: Enable validation using openAPI schema for functionConfig #4467
Conversation
7454a75
to
0c483db
Compare
|
||
// SchemaFromFunctionDefinition extracts the schema for a particular GVK from the provided KRMFunctionDefinition | ||
// Since the relevant fields of KRMFunctionDefinition exactly match the ones in CustomResourceDefinition, | ||
// this helper can also load CRDs (e.g. produced by KubeBuilder) transparently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fact useful to users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with the PR while trying to use KubeBuilder to generate the schemas for me, so maybe? It's pretty neat: you annotate your types with Kubebuilder meta, run go generate, point an embed comment to the file, then point these new helpers to the embedded schema, and boom, super easy very thorough validations for your types.
` | ||
|
||
func (a v1alpha1JavaSpringBoot) Schema() (*spec.Schema, error) { | ||
schema, err := framework.SchemaFromFunctionDefinition(resid.NewGvk("example.com", "v1alpha1", "JavaSpringBoot"), javaSpringBootDefinition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support reading from a Catalog kind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can have another helper for that once Catalog is in here / somewhere. I'd like to keep that out of scope of this PR though.
// Standard KRM object metadata | ||
yaml.ObjectMeta `yaml:"metadata,omitempty" json:"metadata,omitempty"` | ||
// APIVersion and Kind of the object. Must be config.kubernetes.io/v1alpha1 and KRMFunctionDefinition respectively. | ||
yaml.TypeMeta `yaml:",inline" json:",inline"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we usually have this field on the top.
// The maintainers for this version of the function, if different from the primary maintainers. | ||
Maintainers []string `yaml:"maintainers,omitempty" json:"maintainers,omitempty"` | ||
// The runtime information describing how to execute this function. | ||
Runtime runtimeutil.FunctionSpec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json and yaml tags are missing.
And FunctionSpec
is not well aligned with the KRM fn schema. We should reconcile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. That's part of the Catalog-in-Kustomize work though, right? We never want two versions of this that are different, so that's why I just went ahead and used what is already available, for now. (this field isn't relevant to my PR and could also be left off)
kyaml/fn/framework/processors.go
Outdated
"sigs.k8s.io/kustomize/kyaml/errors" | ||
"sigs.k8s.io/kustomize/kyaml/kio" | ||
"sigs.k8s.io/kustomize/kyaml/kio/filters" | ||
"sigs.k8s.io/kustomize/kyaml/openapi" | ||
"sigs.k8s.io/kustomize/kyaml/yaml" | ||
jyaml "sigs.k8s.io/yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be aliased k8syaml
for consistency
0c483db
to
cfbcf64
Compare
/lgtm CI is complaining about a missing go.sum entry. I can re-LGTM when you fix that. |
cfbcf64
to
c90504a
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KnVerey 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 |
I was really confused until I finally realized the missing entry was in pluginator, not kyaml 😅 |
/lgtm |
// this helper can also load CRDs (e.g. produced by KubeBuilder) transparently. | ||
func SchemaFromFunctionDefinition(gvk resid.Gvk, data string) (*spec.Schema, error) { | ||
var def KRMFunctionDefinition | ||
// need to use sigs yaml because spec.Schema type only has json tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I'm expecting this to be fixed soon kubernetes/kube-openapi#279
So we can go back and clean up the places we're using sigs yaml after that PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great! There will still be a few places we need to do it, to support decoding into k/k types and types using metav1 metadata. But the fewer, the better!
This introduces a new interface,
ValidationSchemaProvider
, and a new helper,SchemaFromFunctionDefinition(gvk resid.Gvk, data string) (*spec.Schema, error)
. Together, they enable a function author to use OpenAPI they have written to validate their inputs instead of manually writing the rules in theValidate()
hook we already have (they can be used together, as the test show).I used
k8s.io/kube-openapi/pkg/validation/spec
for this (instead of say, kubeval) for two reasons: we already depend on that package, and it is what the extensions API server appears to be using.See also kubernetes/enhancements#3220, which makes the one change required for this to work for both KRMFunctionDefinition and CustomResourceDefinition. The advantage, as illustrated in one of the tests, is that you can use a tool like Kubebuilder to generate a CRD from your type structs, and then consume it directly with these new tools to get validation. Ideally we'd provide a bespoke KRMFunctionDefinition generator some day, but this should work nicely in the meantime.
This PR also defines the
KRMFunctionDefinition
type proposed in the KEP, since I need it for this feature. Alternatively, I could define only the subset of fields I actually need for this PR and/or make the type private for now--LMK if you want that./kind feature
/triage accepted
/cc @natasha41575 @mengqiy @jeremyrickard