Chart.spec.extensions.helm.charts.timeout only accepts numerical value

[ggolin]

k0s version: v1.29.2+k0s.0 k0sctl version: version: v0.17.4 commit: 372a589

Given the following config:

# generated-by-k0sctl 2024-03-11T14:06:29-07:00
apiVersion: k0s.k0sproject.io/v1beta1
kind: ClusterConfig
spec:
  api:
    address: 10.81.34.223
    sans:
    - 10.81.34.223
    - 10.81.34.225
    - 10.81.34.224
    - 127.0.0.1
  extensions:
    helm:
      charts:
      - chartname: appscode/gateway-api
        name: gateway-api
        namespace: kube-system
        order: 2
        version: v1.0.0
      - chartname: metallb/metallb
        name: metallb
        namespace: metallb
        order: 1
      - chartname: oci://[redacted private registry].com/helm-charts/nginx-ingress-controller
        name: nginx-ingress-controller
        namespace: nginx-ingress
        order: 3
        timeout: 100s

Config validation reports the following error:

Error: failed to read config: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go struct field Chart.spec.extensions.helm.charts.timeout of type time.Duration

Changing that setting to 10000000 results in a valid config.

According to the documentation this field should accept a user friendly setting.

[twz123]

Right. I think this was an oversight when creating the Chart struct. The Timeout field uses time.Duration instead of k8s.io/apimachinery/pkg/apis/meta/v1.Duration.

Changing that setting to 10000000 results in a valid config.

I think this might not be what you want. IIUC, time.Duration gets parsed as nanoseconds, so 10000000 will be 10 ms.

Normally, I would have said that this should probably be fixed in a backwards-compatible way, but I doubt that anyone will get this right (having to specify it as nanoseconds). So I'm a bit on the fence about whether k0s should support both strings treated as durations and numbers treated as nanoseconds, or whether it should just drop support for numbers. This would be a breaking change, though.

[ggolin]

Right. I think this was an oversight when creating the Chart struct. The Timeout field uses time.Duration instead of k8s.io/apimachinery/pkg/apis/meta/v1.Duration.

Changing that setting to 10000000 results in a valid config.

I think this might not be what you want. IIUC, time.Duration gets parsed as nanoseconds, so 10000000 will be 10 ms.

Normally, I would have said that this should probably be fixed in a backwards-compatible way, but I doubt that anyone will get this right (having to specify it as nanoseconds). So I'm a bit on the fence about whether k0s should support both strings treated as durations and numbers treated as nanoseconds, or whether it should just drop support for numbers. This would be a breaking change, though.

I'd say that using nanoseconds in this context is less than optimal :) Using minutes should cover all reasonable use-cases.

[twz123]

I'd say that using nanoseconds in this context is less than optimal :) Using minutes should cover all reasonable use-cases.

Agree. As said, this was definitely not on purpose. Yet we need to closely consider removing support for it, as this will render k0s configurations invalid that specify timeouts as numbers.

[github-actions[bot]]

The issue is marked as stale since no activity has been recorded in 30 days

[twz123]

Concerning

Normally, I would have said that this should probably be fixed in a backwards-compatible way, but I doubt that anyone will get this right (having to specify it as nanoseconds). So I'm a bit on the fence about whether k0s should support both strings treated as durations and numbers treated as nanoseconds, or whether it should just drop support for numbers. This would be a breaking change, though.

and

Yet we need to closely consider removing support for it, as this will render k0s configurations invalid that specify timeouts as numbers.

It's probably best to intentionally break the API here. The only people who would be negatively affected by this are those who have actually done this correctly and used nanoseconds. My guess is that there are not many. Everyone else who set the timeout will actually benefit from this, because they'll realize they had a wrong config setting.