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

GameServerSpec: Add PodTemplateSpec validation #1298

Closed
aLekSer opened this issue Jan 28, 2020 · 7 comments
Closed

GameServerSpec: Add PodTemplateSpec validation #1298

aLekSer opened this issue Jan 28, 2020 · 7 comments
Labels
kind/feature New features for Agones obsolete wontfix Sorry, but we're not going to do that.

Comments

@aLekSer
Copy link
Collaborator

aLekSer commented Jan 28, 2020

Is your feature request related to a problem? Please describe.
There were 2 validation of PodTemplateSpec added recently - for Labels and Annotations. This approach could be generalised for all fields in a PodTemplateSpec.
This could run system into a failure state without informing user that he choose wrong pod template.
For instance fleet mentioned below with resources requests.memory > limits.memory is a mistake and no pod could be created from such template.
Fleet would start to fire up new GameServers infinitely as in aforementioned bugs with Labels.

Describe the solution you'd like
Use a standard Validation logic as could be found here for a ReplicaSet:
https://github.com/kubernetes/kubernetes/blob/52d7614a8ca5b8aebc45333b6dc8fbf86a5e7ddf/pkg/apis/apps/validation/validation.go#L674

Describe alternatives you've considered

Additional context
Fleet which does not signalise of a failure config:

apiVersion: "agones.dev/v1"
kind: Fleet
metadata:
  name: simple-udp-failure
spec:
  replicas: 2
  template:
    spec:
      ports:
      - name: default
        containerPort: 7654
      template:
        spec:
          containers:
          - name: simple-udp
            image: gcr.io/agones-images/udp-server:0.17
            resources:
              requests:
                memory: "66Mi"
                cpu: "20m"
              limits:
                memory: "64Mi"
                cpu: "20m"
@aLekSer aLekSer added the kind/feature New features for Agones label Jan 28, 2020
@aLekSer
Copy link
Collaborator Author

aLekSer commented Feb 19, 2020

Well, I have tried to reuse the version of ValidatePodTemplateSpec()
https://github.com/kubernetes/kubernetes/blob/52d7614a8ca5b8aebc45333b6dc8fbf86a5e7ddf/pkg/apis/apps/validation/validation.go#L674
But there are lots of issues with adding dependency into go modules: go get k8s.io/kubernetes/pkg/apis/core/validation@v1.13, go get don't work in this case.
There is a comment on how to reuse this code:
kubernetes/kubernetes#80316 (comment)

ValidatePodSpec contains lots of useful validations, and the idea of this ticket was to add once and support all these validations inline with kubernetes version updates.
https://github.com/kubernetes/kubernetes/blob/36981002246682ed7dc4de54ccc2a96c1a0cbbdb/pkg/apis/core/validation/validation.go#L3169

@aLekSer
Copy link
Collaborator Author

aLekSer commented Mar 20, 2020

What could cause errors in creating Pods, as could be found in https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L3194 :

  • nodeAffinity, podAffinity and podAntiAffinity
  • dnsPolicy: "None" ("Required value: must provide dnsConfig when dnsPolicy is None")
  • container fields like: lifecycle, startupProbe
    ...

@markmandel
Copy link
Member

Worth exploring:
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#rawextension

Another trick I saw (but now can't find again), that I thought was clever - run a dry run of a creation of a pod with the same details, and return any validation issues that it found.

What we might want to do though is keep a cache of of specs that have been marked valid, so we can skip hitting the API all at once for the dry run steps.

@markmandel
Copy link
Member

Another trick @luna-duclos put me onto:

Using https://github.com/kubernetes-sigs/controller-tools/tree/master/cmd/controller-gen to generate the schema for the Pod:

Docs:
https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html

Which can then be included into the schema for Game Server template. This actually sounds very promising!

markmandel added a commit to markmandel/agones that referenced this issue Jan 20, 2021
This PR provides the mechanisms to pull core Kubernetes resource OpenAPI
schemas out of the API, and customise and convert them into a format
that is embeddable in our own CRD specifications.

This has been used to extract OpenAPI schemas for `ObjectMeta`, and
`PodTemplateSpec`, and then embedded in `GameServer` as well as
parent CRDs.

Also included in a new helm config option of
`gameservers.podPreserveUnknownFields` in case a user needs to escape
the field pruning on a Pod, or if PR causes a bug of any kind.

Also worth noting, this is not comprehensive validation, and therefore I
have not closed the the referenced issue.

Work on googleforgames#1298
markmandel added a commit to markmandel/agones that referenced this issue Jan 20, 2021
This PR provides the mechanisms to pull core Kubernetes resource OpenAPI
schemas out of the API, and customise and convert them into a format
that is embeddable in our own CRD specifications.

This has been used to extract OpenAPI schemas for `ObjectMeta`, and
`PodTemplateSpec`, and then embedded in `GameServer` as well as parent
CRDs, creating a layer of validation for Pods definitions that are
implemented via a GameServer.

Also included in a new helm config option of
`gameservers.podPreserveUnknownFields` in case a user needs to escape
the field pruning on a Pod, or if PR causes a bug of any kind.

Also worth noting, this is not comprehensive validation, and therefore I
have not closed the the referenced issue.

Work on googleforgames#1298
markmandel added a commit to markmandel/agones that referenced this issue Jan 20, 2021
This PR provides the mechanisms to pull core Kubernetes resource OpenAPI
schemas out of the API, and customise and convert them into a format
that is embeddable in our own CRD specifications.

This has been used to extract OpenAPI schemas for `ObjectMeta`, and
`PodTemplateSpec`, and then embedded in `GameServer` as well as parent
CRDs, creating a layer of validation for Pods definitions that are
implemented via a GameServer.

Also included in a new helm config option of
`gameservers.podPreserveUnknownFields` in case a user needs to escape
the field pruning on a Pod, or if PR causes a bug of any kind.

Also worth noting, this is not comprehensive validation, and therefore I
have not closed the the referenced issue.

Work on googleforgames#1298
markmandel added a commit to markmandel/agones that referenced this issue Jan 22, 2021
This PR provides the mechanisms to pull core Kubernetes resource OpenAPI
schemas out of the API, and customise and convert them into a format
that is embeddable in our own CRD specifications.

This has been used to extract OpenAPI schemas for `ObjectMeta`, and
`PodTemplateSpec`, and then embedded in `GameServer` as well as parent
CRDs, creating a layer of validation for Pods definitions that are
implemented via a GameServer.

Also included in a new helm config option of
`gameservers.podPreserveUnknownFields` in case a user needs to escape
the field pruning on a Pod, or if PR causes a bug of any kind.

Also worth noting, this is not comprehensive validation, and therefore I
have not closed the the referenced issue.

Work on googleforgames#1298
markmandel added a commit that referenced this issue Jan 22, 2021
* CRD OpenAPI Spec for ObjectMeta & PodTemplateSpec

This PR provides the mechanisms to pull core Kubernetes resource OpenAPI
schemas out of the API, and customise and convert them into a format
that is embeddable in our own CRD specifications.

This has been used to extract OpenAPI schemas for `ObjectMeta`, and
`PodTemplateSpec`, and then embedded in `GameServer` as well as parent
CRDs, creating a layer of validation for Pods definitions that are
implemented via a GameServer.

Also included in a new helm config option of
`gameservers.podPreserveUnknownFields` in case a user needs to escape
the field pruning on a Pod, or if PR causes a bug of any kind.

Also worth noting, this is not comprehensive validation, and therefore I
have not closed the the referenced issue.

Work on #1298

* Generated yaml for Schema improvements

* Upgrade Helm to 3.5.0

Needed upgrade to fix bug wth upgrade.

* Updates based on review

- Fix for typo in license.
- Fix comment in test to be more clear.
- Rebase against master and regen index.yaml
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

@github-actions github-actions bot added the stale Pending closure unless there is a strong objection. label Jun 1, 2023
@github-actions
Copy link

This issue is marked as obsolete due to inactivity for last 60 days. To avoid issue getting closed in next 30 days, please add a comment or add 'awaiting-maintainer' label. Thank you for your contributions

@github-actions github-actions bot added obsolete and removed stale Pending closure unless there is a strong objection. labels Jul 15, 2023
@github-actions github-actions bot added the wontfix Sorry, but we're not going to do that. label Aug 15, 2023
@github-actions
Copy link

We are closing this as there was no activity in this issue for last 90 days. Please reopen if you’d like to discuss anything further.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones obsolete wontfix Sorry, but we're not going to do that.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants