Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust equivalent for apimachinery's meta/v1.Duration #1222

Closed
hawkw opened this issue May 31, 2023 · 2 comments · Fixed by #1224
Closed

Rust equivalent for apimachinery's meta/v1.Duration #1222

hawkw opened this issue May 31, 2023 · 2 comments · Fixed by #1224
Assignees
Labels
core generic apimachinery style work

Comments

@hawkw
Copy link
Contributor

hawkw commented May 31, 2023

Would you like to work on this feature?

yes

What problem are you trying to solve?

Some objects contain strings which represent durations. These strings are often parsed in the format implemented by the Go standard library's time.Duration type. For example, kubernetes-sigs/gateway-api#1997 adds duration fields to the Gateway API's HTTPRoute type, and explicitly specifies that they are formatted according to the Go time.Duration format:

// Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration
// and MUST BE >= 1ms or 0 to disable (no timeout).

(ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L336-L337)

Describe the solution you'd like

In the Go apimachinery's meta package, there is a v1.Duration struct for representing durations, whose JSON representation is a string in the Go time.ParseDuration/time.Duration format: https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration.
That type is used in the Go bindings for Kubernetes objects that include durations in this format, such as in the Gateway API proposal I mentioned above:

       // +kubebuilder:validation:Format=duration
       Request *metav1.Duration `json:"request,omitempty"`

(ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L353-L354)

It would be nice if there was a similar type in the kube ecosystem.

Such a type should implement serde::Serialize/serde::Deserialize to and from JSON strings in the same format as Go's time.Duration. Since Go time.Durations are signed, and Rust's std::time::Durations are unsigned, the new type should also track whether the duration is positive or negative. Conversions to and from std::time::Duration should also be provided. Finally, we should be sure to check our implementation against the cases that the Go library tests to ensure that the same strings parse to equivalent duration values.

Describe alternatives you've considered

This doesn't technically need to be implemented in kube-core. We could say that users should represent duration fields as Strings, and user code should be responsible for parsing them. However, in my opinion, it's preferable to provide it in kube-core, because there is an equivalent in the Go apimachinery.

Also, if we don't provide a nice way to parse these duration types, folks may take the easy way out and use other Rust libraries for parsing durations, such as humantime. These crates implement a format that is similar to, but not identical to the Go standard library's format, leading to potential issues when the format is accepted by Go but rejected by Rust.

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

kube-core

@hawkw
Copy link
Contributor Author

hawkw commented May 31, 2023

I've already written an implementation of this for my own code, and I'm happy to open a PR upstreaming it to kube-core --- I wanted to make sure there was an open issue describing why this is desirable first, though.

@clux
Copy link
Member

clux commented Jun 1, 2023

Thanks for writing this up. Agree with the reasoning and that we need to stay upstream with the Go format.
Would definitely be a good thing to have in kube-core, and am happy to look at a PR!

The format looks like a dict in their tests and while it could potentially be hackable through some complicated chrono+serde_with usage, I would be happier hiding this inside the de/serializer for the type so users always get the right thing.

Interestingly, the struct is not in present at all in k8s-openapi: Arnavion/k8s-openapi#141, but the type does generate from protobufs in k8s-pb (old version, but something has to handle casttype=time.Duration ultimately).

@clux clux added the core generic apimachinery style work label Jun 1, 2023
hawkw added a commit to hawkw/kube that referenced this issue Jun 1, 2023
Some objects contain strings which represent durations. These strings
are often parsed in the format implemented by the Go standard library's
`time.Duration` type. For example,
kubernetes-sigs/gateway-api#1997 adds duration
fields to the Gateway API's HTTPRoute type, and explicitly specifies
that they are formatted according to the Go `time.Duration` format:
> ```go
> // Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration
> // and MUST BE >= 1ms or 0 to disable (no timeout).
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L336-L337)

In the Go apimachinery's `meta` package, there is a `v1.Duration` struct
for representing durations, whose JSON representation is a string in the
Go `time.ParseDuration`/`time.Duration` format:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration.

That type is used in the Go bindings for Kubernetes objects that include
durations in this format, such as in the Gateway API proposal I
mentioned above:
> ```go
>        // +kubebuilder:validation:Format=duration
>        Request *metav1.Duration `json:"request,omitempty"`
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L353-L354)

This branch adds a new `Duration` type to `kube-core`, which is
essentially an equivalent to `metav1.Duration`. This is a wrapper around
a Rust `std::time::Duration` which can be serialized and deserialized
using the same format as the Go `metav1.Duration` type. In addition,
because Go `time.Duration`s are represented by a signed number,
`Duration`s can also be negative if the provided duration string is
negative, and implement the correct ordering properties when compared to
each other and to `std::time::Duration`s. A parsed `Duration` can be
converted into a `std::time::Duration` by discarding its sign.

Closes kube-rs#1222
hawkw added a commit to hawkw/kube that referenced this issue Jun 1, 2023
## Motivation

Some objects contain strings which represent durations. These strings
are often parsed in the format implemented by the Go standard library's
`time.Duration` type. For example,
kubernetes-sigs/gateway-api#1997 adds duration
fields to the Gateway API's HTTPRoute type, and explicitly specifies
that they are formatted according to the Go `time.Duration` format:
> ```go
> // Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration
> // and MUST BE >= 1ms or 0 to disable (no timeout).
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L336-L337)

In the Go apimachinery's `meta` package, there is a `v1.Duration` struct
for representing durations, whose JSON representation is a string in the
Go `time.ParseDuration`/`time.Duration` format:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration.

That type is used in the Go bindings for Kubernetes objects that include
durations in this format, such as in the Gateway API proposal I
mentioned above:
> ```go
>        // +kubebuilder:validation:Format=duration
>        Request *metav1.Duration `json:"request,omitempty"`
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L353-L354)

## Solution

This branch adds a new `Duration` type to `kube-core`, which is
essentially an equivalent to `metav1.Duration`. This is a wrapper around
a Rust `std::time::Duration` which can be serialized and deserialized
using the same format as the Go `metav1.Duration` type. In addition,
because Go `time.Duration`s are represented by a signed number,
`Duration`s can also be negative if the provided duration string is
negative, and implement the correct ordering properties when compared to
each other and to `std::time::Duration`s. A parsed `Duration` can be
converted into a `std::time::Duration` by discarding its sign.

Closes kube-rs#1222
hawkw added a commit to hawkw/kube that referenced this issue Jun 1, 2023
## Motivation

Some objects contain strings which represent durations. These strings
are often parsed in the format implemented by the Go standard library's
`time.Duration` type. For example,
kubernetes-sigs/gateway-api#1997 adds duration
fields to the Gateway API's HTTPRoute type, and explicitly specifies
that they are formatted according to the Go `time.Duration` format:
> ```go
> // Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration
> // and MUST BE >= 1ms or 0 to disable (no timeout).
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L336-L337)

In the Go apimachinery's `meta` package, there is a `v1.Duration` struct
for representing durations, whose JSON representation is a string in the
Go `time.ParseDuration`/`time.Duration` format:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration.

That type is used in the Go bindings for Kubernetes objects that include
durations in this format, such as in the Gateway API proposal I
mentioned above:
> ```go
>        // +kubebuilder:validation:Format=duration
>        Request *metav1.Duration `json:"request,omitempty"`
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L353-L354)

## Solution

This branch adds a new `Duration` type to `kube-core`, which is
essentially an equivalent to `metav1.Duration`. This is a wrapper around
a Rust `std::time::Duration` which can be serialized and deserialized
using the same format as the Go `metav1.Duration` type. In addition,
because Go `time.Duration`s are represented by a signed number,
`Duration`s can also be negative if the provided duration string is
negative, and implement the correct ordering properties when compared to
each other and to `std::time::Duration`s. A parsed `Duration` can be
converted into a `std::time::Duration` by discarding its sign.

Closes kube-rs#1222

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to linkerd/linkerd2 that referenced this issue Jun 2, 2023
PR #10969 adds support for the GEP-1742 `timeouts` field to the
HTTPRoute CRD. This branch implements actual support for these fields in
the policy controller. The timeout fields are now read and used to set
the timeout fields added to the proxy-api in
linkerd/linkerd2-proxy-api#243.

In addition, I've added code to ensure that the timeout fields are
parsed correctly when a JSON manifest is deserialized. The current
implementation represents timeouts in the bindings as a Rust
`std::time::Duration` type. `Duration` does implement
`serde::Deserialize` and `serde::Serialize`, but its serialization
implementation attempts to (de)serialize it as a struct consisting of a
number of seconds and a number of subsecond nanoseconds. The timeout
fields are instead supposed to be represented as strings in the Go
standard library's `time.ParseDuration` format. Therefore, I've added a
newtype which wraps the Rust `std::time::Duration` and implements the
same parsing logic as Go. Eventually, I'd like to upstream the
implementation of this to `kube-rs`; see kube-rs/kube#1222 for details.

Depends on #10969
Depends on linkerd/linkerd2-proxy-api#243

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to hawkw/kube that referenced this issue Jun 4, 2023
## Motivation

Some objects contain strings which represent durations. These strings
are often parsed in the format implemented by the Go standard library's
`time.Duration` type. For example,
kubernetes-sigs/gateway-api#1997 adds duration
fields to the Gateway API's HTTPRoute type, and explicitly specifies
that they are formatted according to the Go `time.Duration` format:
> ```go
> // Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration
> // and MUST BE >= 1ms or 0 to disable (no timeout).
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L336-L337)

In the Go apimachinery's `meta` package, there is a `v1.Duration` struct
for representing durations, whose JSON representation is a string in the
Go `time.ParseDuration`/`time.Duration` format:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration.

That type is used in the Go bindings for Kubernetes objects that include
durations in this format, such as in the Gateway API proposal I
mentioned above:
> ```go
>        // +kubebuilder:validation:Format=duration
>        Request *metav1.Duration `json:"request,omitempty"`
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L353-L354)

## Solution

This branch adds a new `Duration` type to `kube-core`, which is
essentially an equivalent to `metav1.Duration`. This is a wrapper around
a Rust `std::time::Duration` which can be serialized and deserialized
using the same format as the Go `metav1.Duration` type. In addition,
because Go `time.Duration`s are represented by a signed number,
`Duration`s can also be negative if the provided duration string is
negative, and implement the correct ordering properties when compared to
each other and to `std::time::Duration`s. A parsed `Duration` can be
converted into a `std::time::Duration` by discarding its sign.

Closes kube-rs#1222

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@clux clux closed this as completed in #1224 Jun 5, 2023
clux pushed a commit that referenced this issue Jun 5, 2023
* add `Duration` to `kube-core`

## Motivation

Some objects contain strings which represent durations. These strings
are often parsed in the format implemented by the Go standard library's
`time.Duration` type. For example,
kubernetes-sigs/gateway-api#1997 adds duration
fields to the Gateway API's HTTPRoute type, and explicitly specifies
that they are formatted according to the Go `time.Duration` format:
> ```go
> // Timeout values are formatted like 1h/1m/1s/1ms as parsed by Golang time.ParseDuration
> // and MUST BE >= 1ms or 0 to disable (no timeout).
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L336-L337)

In the Go apimachinery's `meta` package, there is a `v1.Duration` struct
for representing durations, whose JSON representation is a string in the
Go `time.ParseDuration`/`time.Duration` format:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Duration.

That type is used in the Go bindings for Kubernetes objects that include
durations in this format, such as in the Gateway API proposal I
mentioned above:
> ```go
>        // +kubebuilder:validation:Format=duration
>        Request *metav1.Duration `json:"request,omitempty"`
> ```
> (ref: https://github.com/kubernetes-sigs/gateway-api/blob/659d478ec039271a95429b133a999d1593808e96/geps/gep-1742.md?plain=1#L353-L354)

## Solution

This branch adds a new `Duration` type to `kube-core`, which is
essentially an equivalent to `metav1.Duration`. This is a wrapper around
a Rust `std::time::Duration` which can be serialized and deserialized
using the same format as the Go `metav1.Duration` type. In addition,
because Go `time.Duration`s are represented by a signed number,
`Duration`s can also be negative if the provided duration string is
negative, and implement the correct ordering properties when compared to
each other and to `std::time::Duration`s. A parsed `Duration` can be
converted into a `std::time::Duration` by discarding its sign.

Closes #1222

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* Apply suggestions from @mateiidavid

Co-authored-by: Matei David <matei.david.35@gmail.com>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* this is what i get for writing all this code without rust-analyzer

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* add re-export

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* move zero check into while condition

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* unindent via early return

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* flatten function that's only called once.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* fix accideentally flipped while condition

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* run rustfmt

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

---------

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Matei David <matei.david.35@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core generic apimachinery style work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants