feat: Add record truncation and improve logging #29
Conversation
- Add new flag `--truncate-long-records` to handle long record names - Implement record validation and truncation using SHA-256 hash - Enhance logging with emoji-based status indicators - Update Dockerfile to use Go 1.23 - Update .dockerignore to exclude manifests directory - Improve error and configuration logging messages
|
Added requested changes |
|
@blake Cannot reassign back to you. |
| @@ -216,6 +220,7 @@ func main() { | |||
| flag.BoolVar(&exposeIPv4, "expose-ipv4", lookupEnvOrBool("EXTERNAL_MDNS_EXPOSE_IPV4", exposeIPv4), "Publish A DNS entry (default: true)") | |||
| flag.BoolVar(&exposeIPv6, "expose-ipv6", lookupEnvOrBool("EXTERNAL_MDNS_EXPOSE_IPV6", exposeIPv6), "Publish AAAA DNS entry (default: false)") | |||
| flag.IntVar(&recordTTL, "record-ttl", lookupEnvOrInt("EXTERNAL_MDNS_RECORD_TTL", recordTTL), "DNS record time-to-live") | |||
| flag.BoolVar(&truncateLongRecords, "truncate-long-records", lookupEnvOrBool("EXTERNAL_MDNS_TRUNCATE_LONG_RECORDS", false), "Truncate long record names using SHA-256 hash (default: false)") | |||
There was a problem hiding this comment.
@thekoma What do you think about enabling this by default given that the current behavior violates section 2.3.1 of RFC 1035, DOMAIN NAMES - IMPLEMENTATION AND SPECIFICATION.
|
As the name while predictable is not straightforward I opted to skip it
instead of shortening it.
So we are still in RFC. Before this patch the program would just crash when
creating an out of RFC record.
Andrea Cervesato
Phone: +39.392.23.80.611
Mail: ***@***.***
Il lun 24 feb 2025, 06:39 Blake Covarrubias ***@***.***> ha
scritto:
… ***@***.**** commented on this pull request.
------------------------------
In main.go
<#29 (comment)>:
> @@ -216,6 +220,7 @@ func main() {
flag.BoolVar(&exposeIPv4, "expose-ipv4", lookupEnvOrBool("EXTERNAL_MDNS_EXPOSE_IPV4", exposeIPv4), "Publish A DNS entry (default: true)")
flag.BoolVar(&exposeIPv6, "expose-ipv6", lookupEnvOrBool("EXTERNAL_MDNS_EXPOSE_IPV6", exposeIPv6), "Publish AAAA DNS entry (default: false)")
flag.IntVar(&recordTTL, "record-ttl", lookupEnvOrInt("EXTERNAL_MDNS_RECORD_TTL", recordTTL), "DNS record time-to-live")
+ flag.BoolVar(&truncateLongRecords, "truncate-long-records", lookupEnvOrBool("EXTERNAL_MDNS_TRUNCATE_LONG_RECORDS", false), "Truncate long record names using SHA-256 hash (default: false)")
@thekoma <https://github.com/thekoma> What do you think about enabling
this by default given that the current behavior violates section 2.3.1
<https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1> of RFC
1035, DOMAIN NAMES - IMPLEMENTATION AND SPECIFICATION.
—
Reply to this email directly, view it on GitHub
<#29 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4LAV4PPBR3ATEEEKO26L2RKV7ZAVCNFSM6AAAAABWA3UPO6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMZWGA2DGNRQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@thekoma are you okay with me rebasing these changes onto master, resolving the merge conflicts, and force pushing the changes to your branch? |
|
Sure Blake! Rebase it as you like! |
| // Verify each component | ||
| for _, component := range components { | ||
| if len(component) > 63 { | ||
| if truncate { |
There was a problem hiding this comment.
@thekoma I was able to rebase these changes onto the master branch. I came to a few realizations in the process of testing things out.
First, it seems that truncating the namespace is unnecessary. In my cluster, Kubernetes version 1.28.15, the create namespace command errors if the namespace is longer than 63 characters.
kubectl create namespace TheQuickBrownFoxJumpsOverTheLazyDoggoDyzaLehTrevOspmuJxoFnworBkciuQehT
The Namespace "TheQuickBrownFoxJumpsOverTheLazyDoggoDyzaLehTrevOspmuJxoFnworBkciuQehT" is invalid:
* metadata.name: Invalid value: "TheQuickBrownFoxJumpsOverTheLazyDoggoDyzaLehTrevOspmuJxoFnworBkciuQehT": must be no more than 63 characters
* metadata.name: Invalid value: "TheQuickBrownFoxJumpsOverTheLazyDoggoDyzaLehTrevOspmuJxoFnworBkciuQehT": a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')
* metadata.labels: Invalid value: "TheQuickBrownFoxJumpsOverTheLazyDoggoDyzaLehTrevOspmuJxoFnworBkciuQehT": must be no more than 63 charactersSecond, if namespace truncation were needed, it might actually be a worse UX than just refusing to publish the record if the namespace is longer than 63 characters.
The reason is that truncating the namespace will result in the record being published under a completely different subdomain/namespace than where the service actually resides in the cluster. Unless the end-user has access to read the logs from external-mdns, it won't be obvious under what subdomain the record has been published.
The same UX issue applies to the service name if it too is longer than 63 chars.
Given that, I feel a better UX would be to remove the truncation feature and just leave the portion that logs the record is not being advertised since the name or namespace are longer than 63 characters. Users can instead use the new external-mdns.blakecovarrubias.com/hostnames annotation added in #22 to create a shorter service name in lieu of an automatically generated truncated service name.
What do you think about that?
There was a problem hiding this comment.
The truncation is needed when you use the service-namespace instead the service.namespace format.
By design kubernetes uses dns discovery and pointers in CNI so it only makes sense that no truncation is ever necessary.
In my code I address exactly this situation.
--truncate-long-recordsto handle long record names