Unexpected results from the duration format

[cbandy]

trafficstars

The OpenAPI definition of the duration format follows RFC 3339. The Kubernetes definition is more relaxed, which is nice, but IsDuration allows text that produces unexpected values from ParseDuration and unmarshaling.

https://go.dev/play/p/kZhy7fepBAM

I expected 1.5 hours to be 90 minutes, but this valid text is interpreted as 5 hours:

fmt.Println(time.ParseDuration("1.5h"))        // 1h30m0s <nil>
fmt.Println(strfmt.ParseDuration("1.5 hours")) // 5h0m0s <nil>

I expected 1.5 days to be 36 hours, but this valid text is interpreted as 120 hours:

fmt.Println(strfmt.ParseDuration("1.5 days")) // 120h0m0s <nil>
fmt.Println(1.5 * 24 * time.Hour)             // 36h0m0s

[benjaminjb]

I played around with this and it looks like the durationMatcher regexp is giving this unexpected result:

https://github.com/kubernetes/kube-openapi/blob/2c72e554b1e7755b5fbee01cc910063070d5b4ec/pkg/validation/strfmt/duration.go#L55

If I use "1.5" or "1/2" or anything else -- it's just grabbing the last digits before the units. Not sure what the way forward is here, though I sort of lean towards ints only. (If someone wants 1.5 hours, they can put in 90 minutes, right?)

[Jefftree]

Right as @benjaminjb mentioned, per the durationMatcher code, it looks like only integers are accepted and decimals are not. This is only if the string is not parseable by time.ParseDuration() so things formatted like 1.5h, etc are still correctly parsed.

Some history: strfmt was forked from go-openapi. It looks like this is buggy on not just decimal points but also negative numbers like -1 hour being parsed as a positive duration.

Given that there are other ways to express the same duration, I'd imagine it's not the highest priority, @liggitt @sttts do you have any thoughts on how we should tackle this?

• Fix the bug in our library to properly handle decimals/negative numbers. Will most likely involve copying some of the fraction operations in the golang time library. (Maybe also contribute back to go-openapi?)
• Recommend users to not to use the expanded form of timeUnits and stick to the single letters s, m, h, etc.

[Jefftree]

Looks like go-openapi already has a PR to fix parsing negative durations: https://github.com/go-openapi/strfmt/pull/144

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[benjaminjb]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[benjaminjb]

@Jefftree that PR you pointed to got merged -- you haven't had a chance to check out if it solves this issue, have you?

[liggitt]

I commented on @Jefftree's recreation of that change in https://github.com/kubernetes/kube-openapi/pull/552, but I don't think the approach from https://github.com/go-openapi/strfmt/pull/144 makes sense to mimic

[cbandy]

A commit was recently merged upstream (not released) to address fractional values:

• https://github.com/go-openapi/strfmt/pull/170

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[cbandy]

This might be fixed upstream: https://github.com/go-openapi/strfmt/releases/tag/v0.24.0

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.