Skip to content

txt records per hostname and owner #461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

txt records per hostname and owner #461

wants to merge 2 commits into from

Conversation

maksymvavilov
Copy link
Contributor

@maksymvavilov maksymvavilov commented May 29, 2025

closes #346

Rewrite of the TXT registry to create TXT records per owner.

Record will be created with one target per key/value pair.
Controller will be able to read records that will have multiple key/value pairs in one target. Also, if there are no extra labels to be stored the target will be `""` (not and empty string!)

TXT records are stored alongside endpoints in the provider. Note that the deletion of the DNSRecord/endpoint not always results in the deletion of the corresponding endpoint in the provider but will always result in the deletion of the corresponding TXT record. The same is true about creation. This is because multiple owners can define the same endpoint, but they will always define unique TXT records.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this doc - I'll do it the last

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Hope this makes sense. I'm lacking in experience writing docs, but hopefully, this time it went ok.

Copy link
Member

@mikenairn mikenairn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did a quick scan, left a couple of comments. I need to set some time aside to look at those txt registry changes more to better understand what is happening there.

Copy link
Member

@mikenairn mikenairn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this down and went through each provider using the multi record test, trying a few different combinations of records. It appears to create records correctly, but there are some issues with updates and deletes, in particular on GCP. I left some comments where i think you need to look at the code again relating to that, and also had a go at explaining how the length of the expected endpoints is being calculated for each provider in the tests.

I have not gone too deep into the code around labelling tbh, will wait and see where it ends up after you fix the issues with failing tests.

--type=kuadrant.io/azure \
--from-file=azure.json=/local/path/to/azure.json
```
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has nothing tto do with this PR but hope it will slide - just a formating of an .md

a.Targets.Same(b.Targets) &&
a.RecordType == b.RecordType &&
a.SetIdentifier == b.SetIdentifier &&
a.RecordTTL == b.RecordTTL &&
SameProviderSpecific(a.ProviderSpecific, b.ProviderSpecific)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing the check on the labels being the same here? What implications does that have? Where is SameEndpoint being used currently?

Copy link
Contributor Author

@maksymvavilov maksymvavilov Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Included labels check into the SameEndpoints()
The reason for the separation was to explicitly check for all labels in a few places. Rolled back to only check owner key

@maksymvavilov maksymvavilov force-pushed the gh-346-2 branch 2 times, most recently from 73c0e32 to b1b4e62 Compare July 2, 2025 09:56
@maksymvavilov maksymvavilov force-pushed the gh-346-2 branch 2 times, most recently from 8fdc67c to 98dac52 Compare July 3, 2025 13:05
Signed-off-by: Maskym Vavilov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

registry: Create per record+owner metadata records (TXT Registry)
3 participants