external-dns: Not filtering to provided HostedZoneID's

[DanielAtanasovski]

Describe the bug

Since this change from the Bitnami to the kubernetes-sigs helm chart, provided HostedZones are not being filtered to and passed as args to the external-dns container like previously.

Expected Behavior

Previous generated container args:

      containers:
        - name: external-dns
          image: docker.io/bitnami/external-dns:0.13.4-debian-11-r14
          imagePullPolicy: "IfNotPresent"
          args:
            # Generic arguments
            - --metrics-address=:7979
            - --log-level=info
            - --log-format=text
            - --zone-id-filter=XXXXXXXXXX
            - --policy=upsert-only
            - --provider=aws
            - --registry=txt
            - --interval=1m
            - --source=service
            - --source=ingress
            # AWS arguments
            - --aws-api-retries=3
            - --aws-zone-type=
            - --aws-batch-change-size=1000

Current Behavior

Current container args:

          image: registry.k8s.io/external-dns/external-dns:v0.13.5
          imagePullPolicy: IfNotPresent
          args:
            - --log-level=info
            - --log-format=text
            - --interval=1m
            - --source=service
            - --source=ingress
            - --policy=upsert-only
            - --registry=txt
            - --provider=aws
          ports:

Resulting in errors where it fails to list records in an unrelated zone in the same account:

level=fatal msg="failed to list resource records sets for zone /hostedzone/SOME_OTHER_ZONE: AccessDenied: User: arn:aws:sts::ACCOUNTID:assumed-role/CLUSTER_ROLE/ is not authorized to perform: route53:ListResourceRecordSets on resource: arn:aws:route53:::hostedzone/SOME_OTHER_ZONE because no identity-based policy allows the route53:ListResourceRecordSets action\n\tstatus code: 403,"

Reproduction Steps

Deploy a cluster with the external-dns addon and pass in the hostedzone from the resource provider.

const hostedZoneName = some-zone

const addOn = new blueprints.addons.ExternalDnsAddOn({
    hostedZoneResources: [hostedZoneName]; // can be multiple
});

const blueprint = blueprints.EksBlueprint.builder()
  .addOns(addOn)
  .resourceProvider(hostedZoneName, new blueprints.LookupHostedZoneProvider(hostedZoneName))
  .addOns(addOn)
  .build(app, 'my-stack-name');

Possible Solution

extraArgs is a property where the expected outcome can probably be recreated by manually inserting the zone-id-filter arg.

Additional Information/Context

There doesn't exist a zoneIdFilters property now like there was previously with the Bitnami helm chart, but that's still passed in.

CDK CLI Version

2.86.0

EKS Blueprints Version

1.10.1

Node.js Version

18.16.1

Environment details (OS name and version, etc.)

AWS EKS v1.26

Other information

No response

[klaudworks]

We got the same issue here.

EKS Blueprints Version
1.10.1

Kubernetes Version:
AWS EKS v1.25

      new blueprints.addons.ExternalDnsAddOn({
        release: 'external-dns',
        version: '1.13.0',
        hostedZoneResources: hostedZones,
      }),

[aceisScope]

we encountered the same issue here with cdk, which is a bit similar to this one in terraform aws-ia/terraform-aws-eks-blueprints-addons#17

[klaudworks]

First of all, here is one way to do a hotfix. It requires you to specify a domain filter such as example.com. Then only hosted zones for domains like example.com and it's subdomains are considered by external dns.

hostedZoneDomains = ['example.com', 'example2.com']
  new blueprints.addons.ExternalDnsAddOn({
    release: 'external-dns',
    version: '1.13.0',
    hostedZoneResources: hostedZoneDomains,
    values: {
      domainFilters: hostedZoneDomains,
    },
}),

Don't be confused that I set the hostedZoneResources to an array of the relevant domains. This works because I also use the domain name as the name for the resource provider.

builder.resourceProvider(
  'example1.com',
   new ImportHostedZoneProvider('myZoneId'),
);

I'd be happy to write the fix if you can assign the issue to me. Should be no big deal to provide the zoneIds to the extraArgs in the helm chart.
However, I think the more elegant solution would be to let the user specify domains instead of zone ids. We could just let user pass in the relevant domain names and not require him to specify zoneIds as resources at all. What do you think?

[shapirov103]

I observed a somewhat similar issue before, we should treat it as a defect/enhancement/regression issue.
The reason why domain names are not just passed over to the addon is because we need to abstract how exactly the hosted zone is looked up. In a multi-account pattern, the hosted zone maybe in a separate account, so attempting to look it up will fail.
The first fix for this is to apply the values.domainFilters automatically based on the hosted zones. Currently it maps to the zoneIdFilters while it should map to domainFilters.

[klaudworks]

Okay thanks for the explanation about why the abstraction layer is needed. I only considered my simpler single cluster setup. Do you mind if I implement the fix anyways? I could get it done next week. I can apply the domainFilters based on the hosted zone ID. If there is any uncertainty about the implementation, I will just make a suggestion about my preferred solution here.

[shapirov103]

@2start I was thinking to just apply a simple fix of mapping the domain filters to the right parameter for the helm chart.
Provided it is a defect, I would rather address sooner and include in the next patch release so if you can submit this week, please go ahead.
If you have additional suggestions, please share as well. Thank you!
@parkand1 ^^

[klaudworks]

@shapirov103 Okay cool, looking forward to that. I'll open a PR this week!

[shapirov103]

Fixed with #818