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.

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

Merged
merged 1 commit into from
Feb 22, 2018

Conversation

cyriltovena
Copy link
Collaborator

This will prevent bad usage.

There is still some validations that we should add to #10 and fix via webhook.

  • Container should be required if multiple containers are specified in the PodSpecTemplate.
  • HostPort should only be passed if PortPolicy is static. (there is a typo in GameServer definition validation #10 )

For the health I think we can live with default values.

Copy link
Member

@markmandel markmandel left a 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
Copy link
Member

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.

Copy link
Collaborator Author

@cyriltovena cyriltovena Feb 22, 2018

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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)
}

Copy link
Member

Choose a reason for hiding this comment

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

type: string
minLength: 0
maxLength: 63
pattern: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$"
Copy link
Member

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

Copy link
Collaborator Author

@cyriltovena cyriltovena Feb 22, 2018

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])?')

Copy link
Member

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
Copy link
Member

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:
Copy link
Member

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
Copy link
Member

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)

@markmandel
Copy link
Member

Oh, also realised - this should also be in the build/install.yaml as well.

(Another reason for #101 or similar)

@cyriltovena cyriltovena force-pushed the feature/openapi-validation branch from 62ca856 to 6c00c4d Compare February 22, 2018 18:13
@cyriltovena
Copy link
Collaborator Author

Applied to build/install.yaml and fix typos

@cyriltovena cyriltovena force-pushed the feature/openapi-validation branch from 4032cc2 to a9f9d9a Compare February 22, 2018 18:30
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

LGTM!

@markmandel markmandel merged commit 4409235 into master Feb 22, 2018
@markmandel markmandel deleted the feature/openapi-validation branch February 22, 2018 18:46
@markmandel markmandel added this to the 0.1 milestone May 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants