Add Kubernetes CRD Annotations to Duration Type

[ibakshay]

Description

This PR adds Kubernetes CRD kube-builder annotations to explicitly declare that the Duration type serializes as a string. While the Duration type has implicit JSON serialization that converts to a string format (through its MarshalJSON method), Kubernetes CRD validation isn't aware of this implicit conversion and by default treats Duration as an integer type. By adding kubebuilder annotations, we explicitly inform Kubernetes that this type should be treated and validated as a string instead.

Background

The Duration type currently has a custom JSON serialisation that implicitly converts to a string:

// MarshalJSON implements the json.Marshaler interface.
func (d Duration) MarshalJSON() ([]byte, error) {
    return json.Marshal(d.String())
}

Changes

Added kube-builder annotations to type Duration time.Duration in model/time.go:

// +kubebuilder:validation:Type=string
// +kubebuilder:validation:Pattern:="^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$"
type Duration time.Duration

These annotations resolve serialization conflicts when Duration is used in Kubernetes CRDs, allowing for correct validation and serialisation in Kubernetes environments while maintaining the existing string serialisation behaviour.

Impact

• Improves compatibility with Kubernetes CRDs
• No change to existing behaviour in non-Kubernetes contexts
• Backwards compatible

Testing

• Verified annotations don't affect existing Duration behavior
• Tested with a Kubernetes operator to ensure proper CRD validation

fixes perses/perses-operator#20

[Nexucis]

I would be fine to merge it. @SuperQ WDYT ?

[ArthurSens]

Could we look it up how Prometheus-Operator deals with this? I'm pretty sure they also use duration in their APIs 🤔

[ibakshay]

Could we look it up how Prometheus-Operator deals with this? I'm pretty sure they also use duration in their APIs 🤔

Thank you for the review, @ArthurSens. I checked the prometheus-operator implementation - interestingly, while they do use type Duration time.Duration elsewhere in their codebase, they don't use it in any CRD spec or status fields. That's why they haven't faced this serialization issue.

[ArthurSens]

What I'd like to avoid is keeping comments here that make little sense to this codebase, I don't think they will age well 😬.

I've just checked the prometheus-operator codebase and they do have a few "Duration" fields in their CRDs (scrape interval, prometheus retention period, etc). They've declared their own Duration field with the kubebuilder tags

// Duration is a valid time duration that can be parsed by Prometheus model.ParseDuration() function.
// Supported units: y, w, d, h, m, s, ms
// Examples: `30s`, `1m`, `1h20m15s`, `15d`
// +kubebuilder:validation:Pattern:="^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$"
type Duration string

https://github.com/prometheus-operator/prometheus-operator/blob/d2599cfe67beb97b9208e79422457b0f7cde3c4a/pkg/apis/monitoring/v1/types.go#L44

Would that be a problem for perses?

[ibakshay]

What I'd like to avoid is keeping comments here that make little sense to this codebase, I don't think they will age well 😬.

I've just checked the prometheus-operator codebase and they do have a few "Duration" fields in their CRDs (scrape interval, prometheus retention period, etc). They've declared their own Duration field with the kubebuilder tags

// Duration is a valid time duration that can be parsed by Prometheus model.ParseDuration() function.
// Supported units: y, w, d, h, m, s, ms
// Examples: `30s`, `1m`, `1h20m15s`, `15d`
// +kubebuilder:validation:Pattern:="^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$"
type Duration string

https://github.com/prometheus-operator/prometheus-operator/blob/d2599cfe67beb97b9208e79422457b0f7cde3c4a/pkg/apis/monitoring/v1/types.go#L44

Would that be a problem for perses?

@ArthurSens, Thank you for doing this research and sharing the prometheus-operator implementation! This is really helpful context.
While this is surely a workable solution, I believe we could create a lot of value for the ecosystem by adding the kubebuilder annotations directly in the prometheus/common repository. This way, we could prevent code duplication across dependent repositories like prometheus-operator and perses-operator, making maintenance easier for everyone.

[ArthurSens]

@ArthurSens, Thank you for doing this research and sharing the prometheus-operator implementation! This is really helpful context. While this is surely a workable solution, I believe we could create a lot of value for the ecosystem by adding the kubebuilder annotations directly in the prometheus/common repository. This way, we could prevent code duplication across dependent repositories like prometheus-operator and perses-operator, making maintenance easier for everyone.

Well, if in the future you need to make changes to the annotation you'll first need to make changes here, then wait for a release and only then be able to pick up the changes in your operator.

As we work towards v1.0.0 of prometheus/common we'll need to make stability guarantees and those kinds of changes will become harder. This will be painful for both prometheus/common and for perses-operator 😬

I strongly advise that these 2 lines of code live closer to the operator that uses it for easier evolvability :)

[Nexucis]

Well, if in the future you need to make changes to the annotation you'll first need to make changes here, then wait for a release and only then be able to pick up the changes in your operator.

As we are in golang we don't really have to wait for a release if we want to speed up. We just need to get the commit directly.

@ArthurSens I doubt a bit this annotation will have to change in the future. I mean I am pretty sure in prometheus-operator this thing has not been touched since a decade. I don't have in mind any example why it should change.

We could try, merge it, like that operator that depends on it will all benefit and won't have to do it on their own side. And if it becomes an issue like you are describing (which I doubt), then it will always be time to change the approach.

On my side I would like to try it.

[ArthurSens]

Copying a portion of what I've shared in slack :)

Sorry if I sounded like a pessimist blocker, that was not my intention there 😅 . I'm trying to share how was our experience in Prometheus-Operator with CRD fields that were supposed to be durations.

We actually had to make several iterations of this field because it rarely worked as we wanted. In Prometheus-Operator we ended up with 3 duration implementations:

• Duration - prometheus/common/model.ParseDuration() compatible
• NonEmptyDuration - Similar to duration above, but does not allow 0. (most used in prom-operator)
• GoDuration - Go's time.ParseDuration() compatible (what's being added in this PR)

I'm not 100% sure what are Perses needs in terms of duration validation. Do you think Go's time validation is indeed what you need? In Prometheus-Operator it's the least common option 🤔

[ArthurSens]

Briefly discussed in slack, format=duration is based on go's time.duration so we need to use our own regex.

I'm still worried that we're deviating from the package's purpose here, so would love to see what other maintainers think! @gotjosh @roidelapluie @SuperQ

[ibakshay]

Hi @ArthurSens,

Thank you for sharing your experience with duration implementations in Prometheus-Operator. It was very much helpful indeed. Let me clarify our use case and design approach:

The Perses intentionally designed the operator to maintain strict consistency with the Perses backend specifications. This means:

• All spec fields in the operator are directly imported from the Perses backend
  For example: Please see here
• So, Only the Perses backend uses type Duration time.Duration from prometheus/common. This is implemented by @Nexucis in this PR.

CC: @jgbernalp
For better understanding, I've created a rough diagram showing the dependency flow:

flowchart TD
    A[prometheus/common] -->|model.Duration| B[Perses Backend]

    subgraph "Perses Backend"
        B --> C[Config Package]
        B --> D[API Package]
        
        subgraph "API Entities"
            D1[Dashboard Entity]
            D2[Datasource Entity]
            D -->|contains| D1
            D -->|contains| D2
        end
        
        C -->|Duration fields| C1["
            • Authentication TTLs
            • Authorization Intervals
            • Cleanup Intervals
            • Refresh Intervals
            • Time Ranges
        "]
        
        D1 -->|Duration fields| D3["
            • Dashboard TTLs
            • Time Range Settings
        "]

        D2 -->|Duration fields| D4["
            • Scrape Intervals
            • Refresh Intervals
        "]
    end

    subgraph "Perses Operator CRDs"
        E[Perses CRD]
        F[PersesDatasource CRD]
        G[PersesDashboard CRD]
    end

    C1 -.->|used in| E
    D4 -->|imports spec fields| F
    D3 -->|imports spec fields| G

    classDef common fill:#f9f,stroke:#333
    classDef backend fill:#bbf,stroke:#333
    classDef operator fill:#bfb,stroke:#333
    classDef fields fill:#f0f0f0,stroke:#ddd
    classDef entity fill:#e6ffe6,stroke:#333
    classDef usage stroke-dasharray: 5 5

    class A common
    class B,C,D backend
    class E,F,G operator
    class C1,D3,D4 fields
    class D1,D2 entity

style C1 text-align:left
style D3 text-align:left
style D4 text-align:left

linkStyle 8 stroke:#666,stroke-dasharray: 5 5

Loading

[ibakshay]

@ArthurSens, @Nexucis, @jgbernalp, I would like to kindly follow up on this PR that has been open for a while. :)

[simonpasquier]

As a prometheus operator maintainer, I don't have a strong opinion on this. My gut feeling is that keeping the kubebuilder annotation in this repository is a bit artificial since nothing here makes use of it and it will show up in the Godoc (https://pkg.go.dev/github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1#Duration). But after all, it's a decision for prometheus/common maintainers.
If we want to reuse API structures between operators, maybe we can create/maintain a dedicated repo?