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

Identical time values shouldn't be treated as different or the FAQ should suggest using knownTypeFields #6008

Closed
3 tasks done
jsoref opened this issue Apr 9, 2021 · 13 comments · Fixed by #20929
Closed
3 tasks done
Assignees
Labels
component:docs enhancement New feature or request

Comments

@jsoref
Copy link
Member

jsoref commented Apr 9, 2021

Workaround
See #6008 (comment)

Not a general suport forum
If you are trying to resolve an environment-specific issue or have a one-off question about the edge case that does not require a feature then please consider asking a question in argocd slack channel.

Checklist

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

The prometheus-community helm chart is using a short time format, and cert-manager is using golang's Duration.string() function which writes out long time format (including hours and minutes).

duration: 8760h0m0s and duration: 8760h are treated as different.

This triggers annoying sync behaviors

prometheus-community/helm-charts#750

To Reproduce

install cert-manager using:

name: cert-manager
apiVersion: v2
appVersion: "1.0"
description: Cert Manager
version: 1.0.4
dependencies:
  - name: cert-manager
    version: 1.0.4
    repository: https://charts.jetstack.io

install prometheus operator using:

name: prometheus-operator
apiVersion: v2
appVersion: "1.0"
description: Prometheus operator
version: 14.4.0
dependencies:
  - name: kube-prometheus-stack
    version: 14.4.0
    repository: https://prometheus-community.github.io/helm-charts

Possibly add some values to ensure the certificate is created:

values.yaml:

kube-prometheus-stack:
  prometheusOperator:
    tlsProxy:
      enabled: false
    admissionWebhooks:
      certManager:
        enabled: true
      enabled: false

prometheus-community/helm-charts#750

Expected behavior

ideally duration fields would be understood as times and compared as times.

Screenshots

image

# Live # Desired
56 prometheus-kube-prometheus-operator.metrics 56 prometheus-kube-prometheus-operator.metrics
57 prometheus-kube-prometheus-operator.metrics.svc 57 prometheus-kube-prometheus-operator.metrics.svc
58 duration: 8760h0m0s 58 duration: 8760h
59 issuerRef: 59 issuerRef:
60 name: prometheus-kube-prometheus-root-issuer 60 name: prometheus-kube-prometheus-root-issuer

Version

{
    "Version": "v2.0.0+d085636",
    "BuildDate": "2021-03-30T19:10:08Z",
    "GitCommit": "d085636fd72c170d0553a2db1f0de8e6ea0df398",
    "GitTreeState": "clean",
    "GoVersion": "go1.16",
    "Compiler": "gc",
    "Platform": "linux/amd64",
    "KsonnetVersion": "v0.13.1",
    "KustomizeVersion": "v3.9.4 2021-02-09T19:22:10Z",
    "HelmVersion": "v3.5.1+g32c2223",
    "KubectlVersion": "v0.20.4",
    "JsonnetVersion": "v0.17.0"
}

Logs

Paste any relevant application logs here.
@jsoref jsoref added the bug Something isn't working label Apr 9, 2021
@jannfis jannfis added the bug/severity:minor Bug has limited impact, maybe affects only an edge-case or is of cosmetic nature label Apr 12, 2021
@andrein
Copy link

andrein commented Oct 4, 2021

I'm hitting the same issue with https://github.com/ns1/cert-manager-webhook-ns1

image

@leotomas837
Copy link

leotomas837 commented Dec 12, 2022

Any news on this ? I am hitting the same issue too by installing Istio-csr (from cert-manager). The certificate is included in the chart so I have no control over the duration (on my own certificates I set the full duration for ex 1h0m0s to prevent the OutOfSync).

@micahmo
Copy link

micahmo commented Dec 12, 2022

We have added an ignoreDifferences to our Application to handle this case. The downside is that if the duration really does change, Argo won't detect that it's out of sync. But since that's rare, this is preferable to having an app that's forever out of sync.

ignoreDifferences:
- group: cert-manager.io
  kind: Certificate
  jsonPointers:
  - /spec/duration
  - /spec/renewBefore

This is definitely still a bug, though, when the values are functionally identical.

@leotomas837
Copy link

@micahmo Thanks for the tip, well found 👍🏻 but indeed that is not entirely ideal (I guess you need to force sync to apply changes ? Or change manually)

@mstarostik
Copy link

Same for the timeout fields in ClusterAPI's MachineHealthCheck resources:

[...]
  unhealthyConditions:
  - type: Ready
    status: Unknown
    timeout: 300s
  - type: Ready
    status: "False"
    timeout: 300s

Becomes:

[...]
  unhealthyConditions:
  - type: Ready
    status: Unknown
    timeout: 5m0s
  - type: Ready
    status: "False"
    timeout: 5m0s

and is treated as out of sync by Argo CD, correctly so given it's a string field. However, some heuristics would be nice to compare the usual time values. As doing that in general might cause underdetection of real differences, one suggestion would be to expand on ignoreDifferences to declare fields as elligible for semantical time value comparison instead of treating them as an opaque string. Or automatically apply such a logic when field names include substrings like time, interval etc.

@theelderbeever
Copy link

Having this problem with a Redpanda helm chart as well with the additional issue that Argo wants to add isCA: false.

image

@Kavinraja-G
Copy link

Haha, I came here after facing the same issue with Redpanda and few others. Is there any progress on this issue ?

@dntosas
Copy link

dntosas commented Nov 23, 2023

are you using ServerSideApply=true people ?

matthewhughes934 added a commit to matthewhughes934/argo-cd that referenced this issue Dec 10, 2023
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).
matthewhughes934 added a commit to matthewhughes934/argo-cd that referenced this issue Dec 10, 2023
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
matthewhughes934 added a commit to matthewhughes934/argo-cd that referenced this issue Dec 10, 2023
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
matthewhughes934 added a commit to matthewhughes934/argo-cd that referenced this issue Dec 10, 2023
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

In the tests I've used `require.*` methods since I find this simpler
than `if !assert.Blah(...) { return }` which the other tests in this
file are using.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
ishitasequeira pushed a commit that referenced this issue Dec 14, 2023
So that this can be used for custom resources. The motivation for this
is issue #6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

In the tests I've used `require.*` methods since I find this simpler
than `if !assert.Blah(...) { return }` which the other tests in this
file are using.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

In the tests I've used `require.*` methods since I find this simpler
than `if !assert.Blah(...) { return }` which the other tests in this
file are using.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
JulienFuix pushed a commit to JulienFuix/argo-cd that referenced this issue Feb 6, 2024
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

In the tests I've used `require.*` methods since I find this simpler
than `if !assert.Blah(...) { return }` which the other tests in this
file are using.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
@matthewhughes934
Copy link
Contributor

matthewhughes934 commented Feb 22, 2024

There's a solution for this in v2.10.0 via a7d0da9 which adds support for meta/v1/duration as a known diffing type (docs: https://argo-cd.readthedocs.io/en/stable/user-guide/diffing/#known-kubernetes-types-in-crds-resource-limits-volume-mounts-etc) so it's just a matter of letting Argo know about the type of the field, e.g. for the spec.duration on a cert-manager.io/v1/Certificate

apiVersion: v1
kind: configMap
metadata:
  name: argocd-cm
data:
  resource.customizations.knownTypeFields.cert-manager.io_Certificate |
    - field: spec.duration
      type: meta/v1/Duration

lyda pushed a commit to lyda/argo-cd that referenced this issue Mar 28, 2024
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

In the tests I've used `require.*` methods since I find this simpler
than `if !assert.Blah(...) { return }` which the other tests in this
file are using.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
Signed-off-by: Kevin Lyda <kevin@lyda.ie>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this issue Jun 16, 2024
So that this can be used for custom resources. The motivation for this
is issue argoproj#6008: with this change one should be able to use the known
type to ensure equal durations don't present a diff, e,g. via adding to
`argocd-cm`:

    data:
      resource.customizations.knownTypeFields.cert-manager.io_Certificate: |
        - field: spec.duration
          type: meta/v1/Duration

This change is based on 5b464c9, though
I've included the API version in the type path to be consistent with the
generated type (i.e. `meta/v1/Duration` and not `meta/Duration`).

For documentation I've just manually listed the extra types that aren't
auto-generated, though this requires having to keep this list and the
code in-sync but this is probably not a big issue since the number of
extra types is not frequently changed.

In the tests I've used `require.*` methods since I find this simpler
than `if !assert.Blah(...) { return }` which the other tests in this
file are using.

Signed-off-by: Matthew Hughes <matthewhughes934@gmail.com>
@andrii-korotkov-verkada
Copy link
Contributor

knownTypeFields approach is probably the best way to go.

@jsoref
Copy link
Member Author

jsoref commented Nov 17, 2024

Then knownTypeFields should be added to the FAQ.

@jsoref jsoref reopened this Nov 17, 2024
@jsoref jsoref changed the title Identical time values shouldn't be treated as different Identical time values shouldn't be treated as different or the FAQ should suggest using knownTypeFields Nov 17, 2024
@andrii-korotkov-verkada
Copy link
Contributor

It's already there https://argo-cd.readthedocs.io/en/stable/faq/#why-are-my-resource-limits-out-of-sync. Let me know if that's worded well.

@jsoref
Copy link
Member Author

jsoref commented Nov 17, 2024

Hmm...

It should include an example with a duration:

  • 8760h normalized to 8760h0m0s (assuming I'm properly understanding all the arrows)

The text at the bottom should be changed a bit:

-To fix this use diffing customizations [settings](https://argo-cd.readthedocs.io/en/stable/user-guide/diffing/#known-kubernetes-types-in-crds-resource-limits-volume-mounts-etc).
+To fix this use [diffing customizations](https://argo-cd.readthedocs.io/en/stable/user-guide/diffing/#known-kubernetes-types-in-crds-resource-limits-volume-mounts-etc).

I'm pretty sure that wouldn't have been enough for me to figure out what to look at.

The problem is that the problem from the FAQ's perspective is that My Resource Limits Out Of Sync, but as a user (= human), my problem is that "the value I gave ArgoCD isn't being treated the way I expect".

Roughly, at least this FAQ entry, but likely a number of the others needs some SEO.

A bullet entry for "Why is my app being synchronised over and over again?" that links to the same place might help a bit.

@andrii-korotkov-verkada andrii-korotkov-verkada added enhancement New feature or request component:docs and removed bug Something isn't working bug/severity:minor Bug has limited impact, maybe affects only an edge-case or is of cosmetic nature labels Nov 25, 2024
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 25, 2024
Closes argoproj#6008

Make more emphasis on the issue users may be searching for, update wording slightly and another example.

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 25, 2024
Closes argoproj#6008

Make more emphasis on the issue users may be searching for, update wording slightly and another example.

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
andrii-korotkov-verkada added a commit to andrii-korotkov-verkada/argo-cd that referenced this issue Nov 25, 2024
Closes argoproj#6008

Make more emphasis on the issue users may be searching for, update wording slightly and another example.

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
adriananeci pushed a commit to adriananeci/argo-cd that referenced this issue Dec 4, 2024
Closes argoproj#6008

Make more emphasis on the issue users may be searching for, update wording slightly and another example.

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
Signed-off-by: Adrian Aneci <aneci@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:docs enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.