Replacement of Natural by Integer in 6.0.0 release seems incorrect in some cases

[arobertn]

With the 6.0.0 release Natural has been replaced by Integer many places in the generated library, meaning that for things such as ports, number of replicas, etc. we have to write "+80", "+1" etc., which is not only cumbersome but actively incorrect. Negative numbers do not make sense for these values and I would not want type-checking letting them through. I understand that Integer is needed in places for k8s declarations, but was this change perhaps applied more widely than intended?

As a side note I'm wondering if there will at some point be effort to minimize (not necessarily eliminate) compatibility-breaking changes or if this library is strictly experimental / non-production?

[Gabriella439]

My inclination is to leave the logic as is because we already make an effort to use Natural when we can infer that from the OpenAPI spec. See this logic, for example:

https://github.com/dhall-lang/dhall-haskell/blob/f536f8f2de26768c2026c85ef03c25e0ac0b9121/dhall-openapi/src/Dhall/Kubernetes/Convert.hs#L166-L168

As time goes on I'm trying to slowly decouple the openapi-to-dhall tool from Kubernetes specifically, meaning that even though it was created for dhall-kubernetes I'm moving towards less Kubernetes-specific customizations.

For example, if these Kubernetes fields really can never be non-negative then the right thing would be to upstream that into the Kubernetes OpenAPI spec by specifying the minimum value for those fields and then openapi-to-dhall would do the right thing and select Natural as the Dhall type.

[Gabriella439]

Regarding stability guarantees, I do my best to preserve stability, but I will approve a breaking change occasionally if I feel that it is necessary and I will try to spread breaking changes out over time if there are multiple such changes.

[arobertn]

OK, so now I'm not sure where the problem is then. Can you post a link to the exact OpenAPI spec that is being used? At least in the human-readable docs, container port numbers for example are said to have a lower bound of 0, and the code you posted for the converter should be translating this as Natural, but it is Integer.

EDIT: I'm looking at swagger.json in this GitHub repo, and it talks about these limits in comments, but I don't see any mechanism for specifying these in the code itself. So I'm not sure if this is actually the content the dhall library generator is reading.

[Gabriella439]

We use https://raw.githubusercontent.com/kubernetes/kubernetes/release-X.Y/api/openapi-spec/swagger.json as the input to openapi-to-dhall

OpenAPI supports specifying minimum values for numeric types, but it looks like the Kubernetes OpenAPI spec is not using that feature. For example, this particular field:

        "successThreshold": {
          "description": "Minimum consecutive successes for the probe to be considered successful after having failed. Defaults to 1. Must be 1 for liveness and startup. Minimum value is 1.",
          "format": "int32",
          "type": "integer"
        },

… should really be:

        "successThreshold": {
          "description": "Minimum consecutive successes for the probe to be considered successful after having failed. Defaults to 1. Must be 1 for liveness and startup. Minimum value is 1.",
          "format": "int32",
          "type": "integer",
          "minimum": 1,
        },

[arobertn]

Thanks. I opened kubernetes/kubernetes#105533 upstream after not finding anything similar. I agree that's where the fix should be but I don't know how practical it will prove. In the meantime it could be considered to use Natural instead of Integer as the default dhall translation of OpenAPI integer, and have special-condition code or a postprocessing step manually change the couple of instances that actually should be integer. I know this is not pleasing from an engineering perspective, but it would allow the library definitions to be more accurate and also prevent the need for users to adjust in the future when regenerating the lib after the problem is fixed upstream. (Since 6.0.0 is still fairly new likely fewer users have already made changes for it that would need to be undone.)

EDIT: I've gone through 10% of the swagger.json and found only one instance of an integer (out of maybe a hundred) that can take negative values: container exitCode.

[arobertn]

Created dhall-lang/dhall-haskell#2316 switching the default interpretation of a swagger integer from Integer to Natural.

[somesocks]

An unfortunate side-effect of the int -> nat migration is incompatibility between port definitions that use the IntOrString union for ports and port definitions that use Natural. For example, ContainerPort port is now a Natural, but to set up a readiness or liveness probe for a container on the same port uses a HTTPGetAction which specifies a port as an IntOrString.

Is there a plan for how to fix this? Maybe we need an equivalent NatOrString?

[arobertn]

How did this work before 6.0.0? Was the same issue there?

In any case, NatOrString seems like a natural solution ;), but in this case there are no min/max constraints available from the Swagger side to say when to use Nat or Int, so it would need to just follow the Natural/Integer default setting for the whole conversion. In the case of dhall-kubernetes this would be OK (IntOrString is used for two reasons: ports where an IANA service name can be used instead, or numbers where an optional '%' can occur). I'm not familiar with other use cases, but setting the default in the conversion to Integer (which IS the default) would use IntOrString everywhere as now.

[somesocks]

A quick grep through 6.0.0 shows no usage of Natural at all, so I assume all port types were Integer or IntOrString back then.

In any case, NatOrString seems like a natural solution ;), but in this case there are no min/max constraints available from the Swagger side to say when to use Nat or Int, so it would need to just follow the Natural/Integer default setting for the whole conversion.

So this would mean that all instances of IntOrString would be replaced by NatOrString? I'm not sure that's a safe assumption.

To be honest, I'm concerned about #178 and using --preferNaturalInt in general. Its extremely common to use negative integers to mean 'no limit' or other special statuses, and I could easily see -1 becoming a valid value for properties like revisionHistoryLimit or minReadySeconds or unavailableReplicas. The kubernetes team could safely push a change like that as a patch update for any version of kubernetes, because for them its a non-breaking change that's backwards-compatible with their spec. But the consequence to dhall-kubernetes would be having to revert --preferNaturalInt and make another large update, one that's likely a breaking change for every project that depends on dhall-kubernetes.

[arobertn]

@somesocks Ports and other quantities you mention were Natural in dhall-kubernetes before 6.0.0 (the most recent release, which introduced the switch to Integer). That is why I asked how this was handled BEFORE 6.0.0. Or was IntOrString also introduced at that time, and before it was just String, so "80" not +80?

[Gabriella439]

How about we revert #178 for now since if a user wants to use the Natural-based API they can still use openapi-to-dhall on the Kubernetes API to locally generate a Natural-based API from a Kubernetes specification

Also, I have similar concerns to @somesocks that it might be dangerous for us to deviate from the OpenAPI specification without upstreaming fixes to the spec

[arobertn]

For what it's worth, while working on #178 I looked at the docs of all integer fields in the K8s configuration API docs specifically for evidence that out-of-range values like -1 should have some meaning. I did not find a single case. It does not seem this is an idiom the K8s API uses. Instead it relies on optionality to indicate unspecified values where they would impact behavior.

This is currently the case (confirmed also with testing) in the three examples @somesocks provides.

I suppose it is possible in the future Kubernetes will decide for some or all numeric fields to interpret out-of-range values as absent, but I seriously doubt they would make a change like this within a release, and wouldn't consider that possibility worth reverting #178 for.

Even if #178 were reverted, at some point Kubernetes will start providing numeric constraints on the integer fields in the Swagger, and Natural will be automatically used on the Dhall side for some items like port – potentially again leading to the inconvenience that @somesocks mentioned. Because in that case numeric constraints in the Swagger will not be there to save us. How about introducing NatOrString and defauting it or IntOrString driven by --preferNaturalInt but having a separate exceptions list parameter for it?

It's not ideal but as already done with optionality, limited manual specification can add useful precision to automatic translation of an imprecise specification.

[somesocks]

@arobertn it looks like before 6.0.0 this IntOrString was incorrectly converted to a <Natural | String> union. In fact, it looks like almost all instances of Natural before 6.0.0 were because of a bug in dhall-openapi incorrectly interpreting the integer type as a Natural, hence the big shift to Integer types. See #162 and dhall-lang/dhall-haskell#2126

To be honest, I'm a little conflicted on this one. After reading through the kubernetes openapi spec and some of their code, I'm inclined to agree with you that using -1 as a special case for existing parameters seems very unlikely. But, it still seems risky to assume all integers in the kubernetes spec will be naturals by default. It seems safer to assume integers by default, with an exceptions list for naturals.

Also, I was just about to ask if we could fix this upstream by updating the kubernetes spec, and I found your question about it kubernetes/kubernetes#105533 =P. Do you know if anyone has taken this up? I don't mind putting in a little work and submitting a PR if they still haven't pushed forward on it.

[arobertn]

@somesocks I don't know anything more than the comments in ticket – they are interested in doing it but priority is not such that anyone has started working on it. It was resurrected from "stale" status recently.

[arobertn]

Support for NatOrString has now been added to dhall-haskell/openapi-to-dhall: dhall-lang/dhall-haskell#2388 . The --prefNaturalInt flag determines whether openapi IntOrString is interpreted as Dhall IntOrString or NatOrString by default, and exceptions can be specified the same way as for purely numeric fields, using --natIntExceptions. (Out of the current list of IntOrString fields in Kubernetes to 1.22, no exceptions need to be made to defaulting to NatOrString.)

[Gabriella439]

I believe this is fixed now that #182 is merged