-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Don't create endpoint if attempting to create one with invalid dns name #3017
Don't create endpoint if attempting to create one with invalid dns name #3017
Conversation
49d6239
to
7dbf503
Compare
@olemarkus is this AWS specific? |
The 63 char limit is per https://www.ietf.org/rfc/rfc1034.txt. If we had new installations, we'd probably use a different prefix as that workaround, but setting it on our existing services will make external-dns lose ownership of all existing records. It would be a ton of work getting that cleaned up. |
txtNew.WithSetIdentifier(r.SetIdentifier) | ||
txtNew.ProviderSpecific = r.ProviderSpecific | ||
} else { | ||
return []*endpoint.Endpoint{txt} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we warn about this fallback is used?
I am not completely sure anymore about what behavior this will introduce in the long run.
I guess we loose only the ability to create other records of the same name, which seems ok for these kind of long records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using a subdomain for the prefix was a mistake, I think. And I guess that is the migration path for those that need e.g AAAA-records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already warn in NewEndpoingWithTTL
. Warning again is perhaps redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you are right with the return nil it's the same case :)
Subdomain can be used as far as I see by using --txt-prefix or --txt-suffix or example similar to what was written in #2839 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, szuecs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
We should expect that 63 char labels works also with the txt registry. But as can be seen from #2839 and others, this is not the case after introducing the new format.
This PR doesn't fix the underlying problem in any way, but it maintains backwards compatibility by ignoring the new format if it results in endpoint with too long name. This way, e.g route53 batch changes will keep working with the consequence of not migrating to the new format.
Note: This will also prevent subtle breakage in case e.g the service annotation contains invalid DNS labels.
Checklist