-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Description
What would you like to be added:
Finer grained control over the custom resource endpoint.DNSEndpoint
to allow all types of record to be created by that resource. One way of creating an x509 Certificate with cert-manager is to use the DNS01 Challenge which requires the creation of a TXT record. It would be a significant improvement to the external-dns<->cert-manager interoperability if we could create TXT record with this custom resource.
Essentially, I'm proposing three changes to endpoint.DNSEndpoint
:
- An optional field in the spec to specify whether the record should store the status in the status sub-resource (for backward compatibility)
- Adding a Finalizer to the Resource to allow the control loop to delete records that are marked to be deleted
- Add a fields to the sub resource status to keep track of the records as they should be in the provider
type DNSEndpointSpec struct {
Endpoints []*Endpoint `json:"endpoints,omitempty"`
// +optional
StoreStateInResource *bool `json:"storeStateInResource,omitempty"`
}
type DNSEndpointStatus struct {
// The generation observed by the external-dns controller.
// +optional
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
Records []DNSEndpointRecord `json:"records,omitempty"`
}
This way, the control loop could work essentially the same way as it does now, but instead of fetching records from TXT records or DynamoDB, it could look at its status' subresource to validate the state of the record on the provider's side. This would allow the DNSEndpoint custom resource to support TXT records the same way it does everything else.
The deletion of a DNSEndpoint custom resource would be guarded by a finalizer. The control loop would check for the customRecord.ObjectMeta.DeletionTimestamp.IsZero()
and if the value is not zero, it would attempt the deletion on the provider's side, then remove the finalizer to let Kubernetes gracefully remove the custom resource.
One thing to note is that the StoreStateInResource
is an optional field, which means that if a custom resource doesn't have this value set to true
, the custom resource would not store the state in the sub resource and would still use the normal route (TXT,dynamodb,noop). As a result, TXT records would only be supported for endpoint.Endpoint
that have that value set to true
. This was only added by me to add some level of backward compatibility for the current behavior.
Why is this needed:
All types of Records could be supported, which means it would allow folks like me to create TXT record to use external-dns with cert-manager to create x509 certificate through DNS01 Challenge. Those challenge requires to create a TXT record with a special field that proves the zone is owned by the one requesting the certificate.
I believe this change alone would allow me to use external-dns for creating DNS01 challenge. I am also interested in creating the PR to get this implemented, but I believe starting with a conversation might be the best option, right now.
I'm sure there are things I'm overseeing, but I believe this has enough information to at least get a conversation started.
Activity
mloiseleur commentedon Jun 27, 2024
Many thanks for this very interesting proposal 👍 !
Registry is a global parameter on
external-dns
level. Wdyt of keeping it global for this use case ? I mean enabling this behavior by opt-in with something like--registry=crd
.pier-oliviert commentedon Jun 27, 2024
@mloiseleur I think a global opt-in parameter could definitively work. I think I would want to have that global parameter be clear about the intent as much as possible. I think
--registry=crd
could possibly lead to some confusion for a user where they could potentially expect the registry to work for all sources.Like, if someone sets this up, it would only affect DNS entries created by
DNSEnpdoint
. Maybe--crd-self-registry
as a boolean flag? I usually don't like boolean flags that much, but I have a hard time coming up with a better solution.mloiseleur commentedon Jun 27, 2024
Mmmmh 🤔 , if it's not working like a registry, it means
DNSEndPoint
would have their status in the CRD and other Ingress/IngressRoute/HTTPRoute resource would need to have a separate registry ? andDNSEndpoint
would have then status in both CR status field and in the registry.I've mixed feelings about this approach. It would add yet another issue on registry and there are already many known limitations on current dns-based registry.
Wdyt of using another (dedicated) CRD, let's say
DNSStatus
, that would be created and used by bothDNSEndpoint
and other sources as a registry ? It would follow a common pattern between many resources in Kubernetes API.mcharriere commentedon Jun 29, 2024
I also think this is a better approach, in one hand because using the
status
field to store some data could be consider semantically incorrect. e.g the ownerId fits better in the spec of a registry CR than in the status.Also, having a dedicated CRD would make the implementation less problematic, as we wouldn't need to deal with the existing CRD logic nor the sources.
I'd really like to see a CRD based registry.
pier-oliviert commentedon Jun 29, 2024
The Status's CRD didn't click for me until I read your comment @mcharriere and I have to say that I now agree with both of you. A new CRD based registry really makes sense specially when considering the implementation, like you said.
pier-oliviert commentedon Jul 1, 2024
Took some time over the weekend to think about a new CRD for the registry. I used
DNSRegistry
here instead ofDNSStatus
just because aDNSStatus
has a subresource namedstatus
which felt unwieldy to me. The name isn't that important right now and we can decide to name this new CRD any way we want.This new registry would act similarly to the TXTRegistry that already exists with external-dns. ApplyChange's, which I believe is the core of that interface would manage this new custom resource the same way the TXTRegistry would it's TXT record.
The
OwnerID
value would be stored as a spec field as I think that's something we'd want to track. I could also see us using custom resource labels on this new custom resource to allow filtering capabilities.Here's an example of what I mean:
I'm not sure if it would be useful to us to do that, but it's definitively an option if there's a need to fetch registry entries per owner-id, for example.
I also think that a lot of the encryption/special logic in TXTRegistry wouldn't need to be ported to this new CRD registry as the data would live inside the cluster so there wouldn't be any need to encrypt the data here, but I'll defer it to you.
Last note, my original issue here talks about changing the behavior of
endpoint.DNSEndpoint
but this discussion is moving in a different direction (creating a new registry). I don't know if I should leave this issue as is and we continue here, or if I should create a new issue to be more clear that the conversation is about a new Registry CRD. Let me know what you all think.Registry: New CRD registry (kubernetes-sigs#4575)
Registry: New CRD registry (kubernetes-sigs#4575)
mcharriere commentedon Jul 4, 2024
I like the
DNSRegistry
name better, although still a bit confusing. Does one resource of theDNSRegistry
kind represent the entire registry or each resource only represents 1 entry in the registry? But I agree it's not super important right now.Yes, i think that's correct, and following that logic I'd expect external-dns to create one resource of the type
DNSRegistry
for each DNS record it manages.I'm inclined for the spec field, but I see the benefits of the label too.
Agree.
pier-oliviert commentedon Jul 4, 2024
I had the same feeling too. Maybe
DNSRegistryEntry
would work better?mloiseleur commentedon Jul 4, 2024
@pier-oliviert @mcharriere What about a shorter name like
DNSRecord
?Raffo commentedon Jul 6, 2024
Generally I am in favor of the idea. This seems to be a good extension of the CRD that never received the right amount of design or love that it probably deserved. To give an historical perspective, when ExternalDNS was originally designed and implemented, CRDs were not a thing, TPRs were around, but their use was not as widespread and as stable as CRDs are today. The goal of the project was also mostly to solve the use case of giving quick DNS records for applications, which only included A/CNAME records and TXT for the registry. The extensions of the project have been organic through the years, some to address limitations, some to extend it, some to serve the community better.
I would be happy to take a look at a PR (is there already one?) that implements what discussed here, it may give us a better idea of what the tradeoffs are, if it's not too complicated to implement.
I'd spend enough time to design the name of the CRD. I thing
DNSRegistryEntry
is a decent name, which make it clear enough what the cardinality of the CRD is, while not making it super generic. I'd love to hear other proposal as well though. Pay attention to the status sections as well as they are normally a relatively complicated thing in Kubernetes to get right as not all tools consider waiting on resources in the same way.Registry: CRD for a new type of registry (kubernetes-sigs#4575)
16 remaining items