feat: configure arbitrary provider-specific properties via annotations

[Dadeos-Menlo]

Description

Support for provider-specific annotations, that manifest as Endpoint.ProviderSpecific properties, is currently restricted to a subset of pre-defined providers. This functionality should be available to all providers, without requirement for special-case registration within the getProviderSpecificAnnotations(…) function.

The proposed changes include:

• Inversion of logic within the getProviderSpecificAnnotations(…) function so as to treat all annotations prefixed with external-dns.alpha.kubernetes.io/, but that are not otherwise recognised, as provider-specific properties

Notes:

• The existing allocation of annotations (i.e. whether they might be considered provider-specific or not) is somewhat unfortunate; but it is appreciated that any efforts to tidy-up annotations are likely constrained by concerns regarding backwards-compatibility
• It is assumed that the naming of provider-specific properties (i.e. whether they should be of the form provider/property or provider-property) is an internal implementation detail, and therefore the proposed renaming does not represent any backwards-incompatibility
• The documented suggestion for adopting annotations of the form …/webhook-<custom-annotation> is considered unwise - provider-specific properties are considered to be provider-specific, whereas the Webhook provider is considered to be a wrapper of providers, rather than a provider in and of itself (see: Moving providers out of tree #4347)
  • The renaming of Webhook related provider-specific properties (i.e. from webhook-property to webhook/property has not been implemented - implementation would be trivial, but given the conclusion that provider-specific properties associated with a Webhook provider are nonsensical, such an implementation is not being initially proposed
    • It is not clear to me, but seems unlikely, that anyone will have adopted the Webhook framework and are relying upon provider-specific properties

Checklist

• Unit tests updated
• End user documentation updated

[k8s-ci-robot]

Hi @Dadeos-Menlo. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign szuecs for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Dadeos-Menlo]

I've just discovered the existence of CRD Source, which appears to involve the crd source interrogating Kubernetes custom-resource objects of type DNSEndpoint.

Unfortunately, the DNSEndpoint CRD exposes the providerSpecific properties, which is somewhat contrary to my previous assumptions that these represent "an internal implementation detail".

This design is very unfortunate and hopefully, given its "alpha" status, can still be addressed. A far better approach would be to have end-users apply annotations on the DNSEntrypoint objects, in much the same way as end-users are expected to apply annotations to every other type of Kubernetes object that may be used as a source, and have the crdSource call getProviderSpecificAnnotations(…) passing the annotations from the DNSEndpoint object being considered.

I suggest that part of the confusion surrounding these provider-specific annotations/properties is that Endpoint.ProviderSpecific fulfils two distinct roles:

• Providing a mechanism to convey additional, potentially provider-specific, configuration from end-users to DNS provider[s] (i.e. as specified via annotations on sources)
• Providing a mechanism for DNS provider[s] to convey additional configuration from the DNS record[s] obtained from interrogating the DNS Zone, to be used when updating/modifying any existing DNS records

This commingling of responsibilities can easily result in unintended consequences; the most readily reproducible ones being that specifying an unsupported provider-specific property, such as suggested in the CRD source example (i.e. specifying a Cloudflare provider-specific property when using any provider other than Cloudflare) results in the DNS records generated being perpetually out-of-sync with the source object (i.e. the Endpoint generated from the source contains the provider-specific property, the Endpoint generated from the provider observing the pre-existing DNS record does not contain the provider-specific property, the system sees this as an inconsistency to be resolved and therefore deletes and recreates the DNS record ad infinitum).

[mloiseleur]

/ok-to-test

[mloiseleur]

This design is very unfortunate and hopefully, given its "alpha" status, can still be addressed. A far better approach would be to have end-users apply annotations on the DNSEntrypoint objects, in much the same way as end-users are expected to apply annotations to every other type of Kubernetes object that may be used as a source, and have the crdSource call getProviderSpecificAnnotations(…) passing the annotations from the DNSEndpoint object being considered.

I think I agree with you: it would be more consistent to use annotations, maybe in a v1alpha2 version of the CRD.

For the webhook annotation, it was introduced because some webhook providers needs it. See for example mikrotik.

[mloiseleur]

The logic seems better to me with this logic, thanks 👍 .
Would you please re-add documentation on webhook and fix ci ?
/retitle feat: configure arbitrary provider-specific properties via annotations

[Dadeos-Menlo]

For the webhook annotation, it was introduced because some webhook providers needs it. See for example mikrotik.

I've reinstated the documentation of webhook annotations, but still consider this approach misguided.

The provider-properties associated with the mikrotik example cited should be named mikrotik-…, rather than webhook-…; admittedly an approach that is not feasible without the changes proposed here (NB: the changes to the internal representation of provider-specific properties proposed here necessitate the following change to this line:

--- provider.go.orig
+++ provider.go
@@ -94,7 +94,7 @@
 func getProviderSpecific(ep *endpoint.Endpoint, ps string) string {
        value, valueExists := ep.GetProviderSpecificProperty(ps)
        if !valueExists {
-               value, _ = ep.GetProviderSpecificProperty(fmt.Sprintf("webhook/%s", ps))
+               value, _ = ep.GetProviderSpecificProperty(fmt.Sprintf("webhook-%s", ps))
        }
        return value
 }

).

Imagine scenarios if/when the AWS provider was to be repackaged to use the Webhook provider architecture; one wouldn't expect to have to rename all of the existing annotations on one's Kubernetes objects. Not to mention the preclusion of deploying a single instance of external-dns, managing multiple providers deployed via the Webhook architecture, whose webhook-… prefixed annotations would clash/commingle.

[Dadeos-Menlo]

b21b0ff adds annotation support to the DNSEndpoint custom-resources; the DNSEndpoint.Spec.Endpoints configuration appears to be largely redundant given support for the various annotations.

The only potential benefit for supporting the DNSEndpoint.Spec.Endpoints configuration is that it facilitates the representation of multiple Endpoint objects via a single DNSEndpoint Kubernetes resource; but given the inconsistent pluralisation and the challenges of associating annotations with multiple Endpoint, it is likely more intuitive to require a 1-to-1 relationship between DNSEndpoint resource and Endpoint object, or at least having one DNSEndpoint represent a single DNS name, with perhaps multiple record types (e.g. Endpoint objects representing "A" and "AAAA" records).