Using the `txtSuffix` field in the helm chart does nothing

kubernetes-sigs / external-dns

Configure external DNS servers (AWS Route53, Google CloudDNS and others) for Kubernetes Ingresses and Services

Apache License 2.0

7.76k stars 2.58k forks source link

Using the `txtSuffix` field in the helm chart does nothing #4656

Open jmtx1020 opened 3 months ago

jmtx1020 commented 3 months ago

What happened:

I set the txtSuffix and txtPrefix but only the txtSuffix shows up when I render the YAML.

What you expected to happen: I would expect both to show.

How to reproduce it (as minimally and precisely as possible):

values.yaml:

  provider:
    name: cloudflare
  env:
    - name: CF_API_TOKEN
      valueFrom:
        secretKeyRef:
          name: external-cf-secrets
          key: CF_API_TOKEN
  txtPrefix: "test-prefix"
  txtSuffix: "test-suffix"
  txtOwnerId: "testing"

relevant lines from the deployment.yaml:

          args:
            - --log-level=info
            - --log-format=text
            - --interval=1m
            - --source=service
            - --source=ingress
            - --policy=upsert-only
            - --registry=txt
            - --txt-owner-id=testing
            - --txt-prefix=test-prefix
            - --provider=cloudflare

Anything else we need to know?: Using the latest version of the helm chart 1.14.5, Environment:

External-DNS version (use external-dns --version): v0.14.2
DNS provider: cloudflare, ns1

jmtx1020 commented 3 months ago

Update, I reviewed the code for the helm chart here:

            {{- if and (eq .Values.txtPrefix "") (ne .Values.txtSuffix "") }}
            - --txt-suffix={{ .Values.txtSuffix }}
            {{- end }}

It will work if I set txtPrefix: "" but not if its not set. If this is intended we can close this issue but this seems strange to me since both values default to nil

kundan2707 commented 3 months ago

@jmtx1020 it mentioned in its description that txtSuffix and txtSuffix are mutually exclusive. so it seems correct

jmtx1020 commented 3 months ago

Hey @kundan2707 ,

We can close this issue if it's meant to work that way but it seemed weird to have to set something to "" to make the other work.

k8s-triage-robot commented 1 week ago

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

jakeyheath commented 5 days ago

Not stale. If the default value is used for txtPrefix, but you explicitly set the txtSuffix, then txtSuffix doesn't render because txtPrefix by default is not an "".

Here's some examples:

# values.yaml
txtSuffix: test

helm template external-dns/external-dns --version 1.14.3 --values values.yaml

args:
  - --log-level=info
  - --log-format=text
  - --interval=1m
  - --source=service
  - --source=ingress
  - --policy=upsert-only
  - --registry=txt
  - --provider=aws

helm template external-dns/external-dns --version 1.13.0 --values values.yaml

args:
  - --log-level=info
  - --log-format=text
  - --interval=1m
  - --source=service
  - --source=ingress
  - --policy=upsert-only
  - --registry=txt
  - --txt-suffix=test
  - --provider=aws

In fact, this was a breaking change for us since we didn't provide txtPrefix, but set txtSuffix. Many of our records started to collide after we upgraded the helm chart.

jakeyheath commented 4 days ago

/remove-lifecycle stale