dhall-kubernetes icon indicating copy to clipboard operation
dhall-kubernetes copied to clipboard

Implicit ServiceSpec union?

Open ari-becker opened this issue 5 years ago • 5 comments

Trying to run kubectl apply on:

apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  type: ClusterIP
  selector:
    app: foo

results in the following error: The Service "foo" is invalid: spec.ports: Required value. Putting ports: [] results in the same error.

The ports field in ServiceSpec is an Optional (List ServicePort.Type) only because of ExternalName Services (example taken from linked documentation):

apiVersion: v1
kind: Service
metadata:
  name: my-service
  namespace: prod
spec:
  type: ExternalName
  externalName: my.database.example.com

Similarly, externalName is a required field for an ExternalName ServiceSpec but should not be specified at all for other kinds of ServiceSpec.

Ran into this issue when the super-configuration that was generating a Kubernetes.Service.Type had an optional field that was being mapped to the Kubernetes Service's .spec.ports, where the field wasn't specified and the default was an empty optional.

Related: https://github.com/dhall-lang/dhall-lang/issues/691

ari-becker avatar Aug 24 '20 12:08 ari-becker

@ari-becker: Just to make sure I understand: do you mean that there is an API invariant that the Kubernetes server is enforcing that is not codified in the OpenAPI specification? Specifically:

  • is there an invariant that .spec.ports must be present if spec.type is not ExternalName?
  • is there an invariant that .spec.ports must be absent if spec.type is ExternalName?

Gabriella439 avatar Aug 26 '20 03:08 Gabriella439

@Gabriel439

is there an invariant that .spec.ports must be present if spec.type is not ExternalName?

Yes. If spec.type is undefined, the API server presumes ClusterIP by default. There are four types of Services - ClusterIP, NodePort, LoadBalancer, and ExternalName. Each of ClusterIP, NodePort, and LoadBalancer require ports to be defined, and the API server will return an error if the field is undefined or is an empty list. Only if type is defined as ExternalName does the API server accept not defining any ports.

is there an invariant that .spec.ports must be absent if spec.type is ExternalName?

I just tested this, and the API server does accept a manifest for a Service of type ExternalName which defines ports. In practice, these ports are ignored, as ExternalName is used to manually insert CNAME records into Kubernetes's cluster DNS. With that said, I can understand a user defining a port for a Service of type ExternalName as a means of documenting usage for downstream users.

ari-becker avatar Aug 26 '20 07:08 ari-becker

@ari-becker: I think my inclination here is to either:

  • … ask the Kubernetes upstream to fix the OpenAPI spec to codify these invariants, or:
  • … created a more strongly-typed representation downstream of dhall-kubernetes (e.g. in dhall-packages or whatever evolves into the "Helm for Dhall").

The main reason why is that we're slowly trying to generalize the dhall-kubernetes logic to dhall-openapi, so the less we deviate from the OpenAPI specification the fewer Kubernetes-specific adjustments the code will have to make.

I do agree, though, that this sounds like the obvious more strongly-typed representation would be a Dhall union with at least two alternatives (one alternative for ExternalName and at least one alternative for other types)

Gabriella439 avatar Aug 26 '20 15:08 Gabriella439

@Gabriel439 Asking Kubernetes upstream to fix the OpenAPI spec is non-trivial.

The spec itself is a computer-generated ~ 5 MB document, which seems to be generated from the JSON tags on the Golang structs of the API objects, see e.g. ServiceSpec (at least that's my best guess without spending much more time in the codebase). For better or worse, these are long-stabilized APIs whose validations are unlikely to be changed at this point.

I'm wondering about a different approach - keep moving towards a generalized dhall-openapi, but trying to find a way to "append" additional type-level assertions after the initial generation to form the final types. I'm not sure how that would really look though.

ari-becker avatar Aug 27 '20 11:08 ari-becker

not providing ports is also supported for headless services by the way, not just ExternalNames

arianvp avatar Oct 03 '20 12:10 arianvp