-
Notifications
You must be signed in to change notification settings - Fork 634
Use OpenAPI to validate ImageVolumeSource.Reference #4308
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,9 +133,11 @@ require ( | |
gopkg.in/yaml.v3 v3.0.1 // indirect | ||
k8s.io/apiextensions-apiserver v0.33.0 // indirect | ||
k8s.io/apiserver v0.33.0 // indirect | ||
k8s.io/code-generator v0.33.0 // indirect | ||
k8s.io/gengo/v2 v2.0.0-20250207200755-1244d31929d7 // indirect | ||
k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 // indirect | ||
sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.31.2 // indirect | ||
sigs.k8s.io/controller-tools v0.17.3 // indirect | ||
sigs.k8s.io/controller-tools v0.18.0 // indirect | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 v0.18 requires k8s.io/.. v0.33, but it wasn't bundled in the k8s.io updates last month: #4248 I don't know why! |
||
sigs.k8s.io/randfill v1.0.0 // indirect | ||
sigs.k8s.io/structured-merge-diff/v4 v4.6.0 // indirect | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import ( | |
"log/slog" | ||
"os" | ||
"path/filepath" | ||
"regexp" | ||
|
||
"github.com/itchyny/gojq" | ||
"sigs.k8s.io/yaml" | ||
|
@@ -44,8 +45,12 @@ func main() { | |
panic(err) | ||
} | ||
|
||
// Turn top-level strings that start with octothorpe U+0023 into YAML comments by removing their quotes. | ||
yamlData := need(yaml.Marshal(v)) | ||
yamlData = regexp.MustCompile(`(?m)^'(#[^']*)'(.*)$`).ReplaceAll(yamlData, []byte("$1$2")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we can't just capture the whole line as a single capture group because you want to exclude the quotes, yeah? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 It doesn't seem like I need the second capture... The line should end just after the quoted string, right? 🦆 I'll check this tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right. This finds lines starting with -'# controller-gen.kubebuilder.io/version': v0.18.0
+# controller-gen.kubebuilder.io/version: v0.18.0
I forgot that the quotes are around only the key of the flow mapping. The second capture is for the remainder of the line, if any. |
||
|
||
slog.Info("Writing", "file", yamlName) | ||
must(os.WriteFile(yamlPath, append([]byte("---\n"), need(yaml.Marshal(v))...), 0o644)) | ||
must(os.WriteFile(yamlPath, append([]byte("---\n"), yamlData...), 0o644)) | ||
} | ||
|
||
if _, ok := result.Next(); ok { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,26 @@ | |
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
# This [jq] filter modifies a Kubernetes CustomResourceDefinition. | ||
# Use the controller-gen "+kubebuilder:title" marker to identify schemas that need special manipulation. | ||
# | ||
# [jq]: https://jqlang.org | ||
|
||
# merge recursively combines a stream of objects. | ||
# https://jqlang.org/manual#multiplication-division-modulo | ||
def merge(stream): reduce stream as $i ({}; . * $i); | ||
|
||
# https://pkg.go.dev/k8s.io/api/core/v1#ImageVolumeSource | ||
reduce paths(try .title == "$corev1.ImageVolumeSource") as $path (.; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 This may very well be "too clever." I'm curious what others think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting a Seems reasonable to me, with the caveat that having the CRD be assembled from kubebuilder annotations and then post-process functions might make it a little harder to know easily where some element is coming from. Maybe that's a documentation issue, e.g., when we add the title in the struct, we could link to or reference this jq script at this point? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah. Smuggling information through a marker that does something else.
A comment on the marker is better than none. I'll add one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done. |
||
getpath($path) as $schema | | ||
setpath($path; $schema * { | ||
required: (["reference"] + ($schema.required // []) | sort), | ||
properties: { | ||
pullPolicy: { enum: ["Always", "Never", "IfNotPresent"] }, | ||
reference: { minLength: 1 } | ||
} | ||
} | del(.title)) | ||
) | | ||
|
||
# Kubernetes assumes the evaluation cost of an enum value is very large: https://issue.k8s.io/119511 | ||
# Look at every schema that has a populated "enum" property. | ||
reduce paths(try .enum | length > 0) as $path (.; | ||
|
@@ -64,4 +77,22 @@ reduce paths(try .["x-kubernetes-int-or-string"] == true) as $path (.; | |
end | ||
) | | ||
|
||
# Rename Kubebuilder annotations and move them to the top-level. | ||
# The caller can turn these into YAML comments. | ||
. += (.metadata.annotations | with_entries(select(.key | startswith("controller-gen.kubebuilder")) | .key = "# \(.key)")) | | ||
.metadata.annotations |= with_entries(select(.key | startswith("controller-gen.kubebuilder") | not)) | | ||
|
||
# Remove nulls and empty objects from metadata. | ||
# Some very old generators would set a null creationTimestamp. | ||
# | ||
# https://github.com/kubernetes-sigs/controller-tools/issues/402 | ||
# https://issue.k8s.io/67610 | ||
del(.metadata | .. | select(length == 0)) | | ||
|
||
# Remove status to avoid conflicts with the CRD controller. | ||
# Some very old generators would set this field. | ||
# | ||
# https://github.com/kubernetes-sigs/controller-tools/issues/456 | ||
del(.status) | | ||
|
||
. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,8 +313,7 @@ func (meta *Metadata) GetAnnotationsOrNil() map[string]string { | |
// +structType=atomic | ||
// | ||
// +kubebuilder:validation:XValidation:rule=`has(self.claimName) != has(self.image)`,message=`you must set only one of image or claimName` | ||
// +kubebuilder:validation:XValidation:rule=`!has(self.image) || !has(self.readOnly) || self.readOnly`,message=`readOnly cannot be set false when using an ImageVolumeSource` | ||
// +kubebuilder:validation:XValidation:rule=`!has(self.image) || (self.?image.reference.hasValue() && self.image.reference.size() > 0)`,message=`if using an ImageVolumeSource, you must set a reference` | ||
// +kubebuilder:validation:XValidation:rule=`!has(self.image) || !has(self.readOnly) || self.readOnly`,message=`image volumes must be readOnly` | ||
type AdditionalVolume struct { | ||
// Name of an existing PersistentVolumeClaim. | ||
// --- | ||
|
@@ -337,9 +336,11 @@ type AdditionalVolume struct { | |
// +optional | ||
Containers []DNS1123Label `json:"containers"` | ||
|
||
// Details for adding an image volume | ||
// Reference to an image or OCI artifact. | ||
// More info: https://kubernetes.io/docs/concepts/storage/volumes#image | ||
// --- | ||
// https://docs.k8s.io/concepts/storage/volumes#image | ||
// Use "title" to add more validation in [internal/crd/post-process.jq]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
// +kubebuilder:title=$corev1.ImageVolumeSource | ||
// | ||
// +optional | ||
Image *corev1.ImageVolumeSource `json:"image,omitempty"` | ||
|
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'm curious to hear your thinking behind this change
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 information is minimally interesting during development, but AFAIK has no use after that. As an annotation, it is deployed with the CRD but serves no purpose.
My impression has been that the generator is using
metadata.annotations
to push "stuff" through code that can only handle K8s objects.