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

model.Duration Serialization Incompatibility with Kubernetes API #20

Open
Tracked by #419
ibakshay opened this issue Oct 18, 2024 · 1 comment · May be fixed by prometheus/common#708
Open
Tracked by #419

model.Duration Serialization Incompatibility with Kubernetes API #20

ibakshay opened this issue Oct 18, 2024 · 1 comment · May be fixed by prometheus/common#708

Comments

@ibakshay
Copy link
Contributor

ibakshay commented Oct 18, 2024

While working on updating the dependencies and go version on my fork and encountered an issue with the serialization of model.Duration in the Perses CRD.

Some of the Perses CRD spec fields such as spec.config.ephemeral_dashboard.cleanup_interval, spec.config.provisioning.interval uses model.Duration from prometheus/common/time.go.

prometheus/common/time.go has a custom JSON serialisation that converts implicitly to type:string. This conflicts with the Kubernetes API's expectation of a type: integer instead. This mismatch prevents the perses_controller from updating Perses resources, resulting in validation errors like:

time="2024-10-18T14:39:38+02:00" level=info msg="Adding Finalizer for Perses" module=perses_controller
time="2024-10-18T14:39:43+02:00" level=error msg="Failed to update custom resource to add finalizer" error="Perses.perses.dev \"perses-sample\" is invalid: spec.config.ephemeral_dashboard.cleanup_interval: Invalid value: \"string\": spec.config.ephemeral_dashboard.cleanup_interval in body must be of type integer: \"string\"" module=perses_controller

This occurs because the prometheus/commonpackage marshals model.Duration` to a string:

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

Perses CRD definition (config/crd/bases/perses.dev_perses.yaml)

---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: v0.14.0
  name: perses.perses.dev
spec:
  # ... (other parts of the CRD)

  versions:
  - name: v1alpha1
    schema:
      openAPIV3Schema:
        description: Perses is the Schema for the perses API
        properties:
          # ... (other properties)
          spec:
            description: PersesSpec defines the desired state of Perses
            properties:
              config:
                properties:
                  # ... (other properties)
                  ephemeral_dashboard:
                    description: EphemeralDashboard contains the config about the ephemeral dashboard feature
                    properties:
                      cleanup_interval:
                        description: The interval at which to trigger the cleanup of ephemeral dashboards, based on their TTLs.
                        format: int64  # This indicates that Kubernetes expects an integer
                        type: integer
                      # ... (other properties)
                  # ... (other properties that use model.Duration)
            # ... (rest of the schema)

As seen in the CRD definition, spec.config.ephemeral_dashboard.cleanup_interval is explicitly defined as an integer (format: int64, type: integer) showing the incompatibility. This issue needs to be addressed to ensure compatibility of the Perses controller for managing the time duration fields of the Perses Config .

@frezes
Copy link

frezes commented Oct 21, 2024

There is a workaround method, use patch to modify the crd field: main...frezes:perses-operator:develop#diff-2c2b2008b7840f3d21a3262975c4aff6ba1e5f89f394ed344a512beb53525c84R1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants