search

DataDog / extendeddaemonset

Kubernetes Extended Daemonset controller
Apache License 2.0
98 stars 13 forks source link

Validate CRs .spec.template.metadata #21

Closed juliogreff closed 4 years ago

juliogreff commented 4 years ago

It would be nice for a user to know that an EDS resource they're trying to create is invalid. There are already several validations in the CRDs' definition, but it's still missing some, like in .spec.template.metadata. When a resource with invalid metadata (like annotations or labels where the value is an int instead of a string, which is common) is created, it will break the informer since it's no longer able to de-serialize it into the Go structs, throwing the following error:

E0520 14:25:07.400582       1 reflector.go:123]
sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map.go:204:
Failed to list *v1alpha1.ExtendedDaemonSet:
v1alpha1.ExtendedDaemonSetList.Items: []v1alpha1.ExtendedDaemonSet:
v1alpha1.ExtendedDaemonSet.Spec:
v1alpha1.ExtendedDaemonSetSpec.Template: v1.PodTemplateSpec.ObjectMeta:
v1.ObjectMeta.CreationTimestamp: Annotations: ReadString: expects " or
n, but found 1, error found in #10 byte of ...|rollout":1},"creatio|...,
bigger context
...|e-eu1.staging.dog","wheelofmisery/force-rollout":1},"creationTimestamp":null,"labels":{"app":"datado|...

To avoid that, this commit expands the validation to also check the metadata more thoroughly. The actual validation inlined here comes from K8s OpenAPI specs for a PodSpec. Currently there is no way to reference that, so unfortunately we have to inline it (see https://github.com/kubernetes/kubernetes/issues/54579).

Now, when a user tries to create an invalid resource, they'll be stopped with an error message:

$ kubectl apply -f invalid-foo-eds.yml
The ExtendedDaemonSet "foo" is invalid:
spec.template.metadata.annotations.rollout: Invalid value: "integer":
spec.template.metadata.annotations.rollout in body must be of type
string: "integer"
codecov-commenter commented 4 years ago

Codecov Report

Merging #21 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #21   +/-   ##
=======================================
  Coverage   34.85%   34.85%           
=======================================
  Files          22       22           
  Lines        1406     1406           
=======================================
  Hits          490      490           
  Misses        853      853           
  Partials       63       63
Flag Coverage Δ
#unittests 34.85% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b011cbd...d05a352. Read the comment docs.