-
Notifications
You must be signed in to change notification settings - Fork 817
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
Add CRD validation via OpenAPIv3 Schema #100
Conversation
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.
Cool stuff - I had a question, and found some typos to fix.
name: | ||
type: string | ||
minLength: 0 | ||
maxLength: 63 |
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.
Curious where 63 comes from -
Looking at the names doc
By convention, the names of Kubernetes resources should be up to maximum length of 253 characters and consist of lower case alphanumeric characters, -, and ., but certain resources have more specific restrictions.
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 was also surprised.
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.
const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
var dns1123LabelRegexp = regexp.MustCompile("^" + dns1123LabelFmt + "$")
const dns1123LabelMaxLength int = 63
// IsDNS1123Label tests for a string that conforms to the definition of a label in
// DNS (RFC 1123).
func IsDNS1123Label(value string) bool {
return len(value) <= dns1123LabelMaxLength && dns1123LabelRegexp.MatchString(value)
}
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.
Found the latest version here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L2854
https://github.com/kubernetes/apimachinery/blob/master/pkg/api/validation/objectmeta.go
But looks like the docs are wrong. Wow, 63 chars is not a huge amount.
type: string | ||
minLength: 0 | ||
maxLength: 63 | ||
pattern: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" |
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.
a .
is also allowed in a name
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.
You'll be surprised it's not, I have just tried with a .
and it gives me this:
The Pod "nginx-apparmor" is invalid:
* metadata.annotations[container.apparmor.security.beta.kubernetes.io/nginx]: Invalid value: "nginx": container not found
* spec.containers[0].name: Invalid value: "nginx.test": a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for val
idation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')
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.
Apparently so! Sounds like it's time to file a PR on the Kubernetes documentation.
description: if there is more than one container, specify which one is the game server | ||
type: string | ||
minLength: 0 | ||
maxLength: 63 |
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.
Same questions as above
portPolicy: | ||
title: the port policy that will be applied to the game server | ||
description: | | ||
portPolicy has two options: |
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.
Love all these titles and descriptions!! 👍
install.yaml
Outdated
initialDelaySeconds: | ||
title: Number of seconds after the container has started before health check is initiated. Defaults to 5 seconds | ||
type: integer | ||
minimun: 0 |
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.
Typo: minimum
(looks like this got copied pasted around)
Oh, also realised - this should also be in the (Another reason for #101 or similar) |
62ca856
to
6c00c4d
Compare
Applied to |
4032cc2
to
a9f9d9a
Compare
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.
LGTM!
This will prevent bad usage.
There is still some validations that we should add to #10 and fix via webhook.
For the health I think we can live with default values.