Skip to content
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

feat(txt-registry): add option to use only new format #4946

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/registry/txt.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,24 @@
The TXT registry is the default registry.
It stores DNS record metadata in TXT records, using the same provider.

## Record Format Options
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you include a subsection on how to handle the migration from two records to a single record? Specifically, what are the steps involved in this consolidation, and will the external DNS automatically delete the old records or will manual cleanup be necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Right now i haven't thought of any automatic cleanup, so for now it would be a manual cleanup for the old format TXT records.

Is that an okay approach, or would you like it to happen automatically?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioning this in the documentation likely is enough.

Copy link
Author

Choose a reason for hiding this comment

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

Added an extra section describing a manual migration flow.

The TXT registry supports two formats for storing DNS record metadata:
- Legacy format: Creates a TXT record without record type information
- New format: Creates a TXT record with record type information (e.g., 'a-' prefix for A records)

By default, the TXT registry creates records in both formats for backwards compatibility. You can configure it to use only the new format by using the `--txt-new-format-only` flag. This reduces the number of TXT records created, which can be helpful when working with provider-specific record limits.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a bit more information about this behaviour as well? e.g. why external-dns creates two records

Copy link
Author

Choose a reason for hiding this comment

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

I don't have the history of why there are 2 formats, but would be glad to add some more text about it.

We just reached a point in my team at work where we would like to get rid of our duplicate TXT records in all of our DNS zones.


Note: AAAA records always use only the new format regardless of this setting.

Example:
```sh
# Default behavior - creates both formats
external-dns

# Only create new format records
external-dns --txt-new-format-only
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required - --managed-record-types=TXT or with that flags we imply implicitly that TXT records should be handled?

Copy link
Author

Choose a reason for hiding this comment

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

The flag, is the initial implementation thought as an addition to the flags you would already start external-dns with.

So no it doesn't implicitly tell that we are using TXT records.

Copy link
Author

Choose a reason for hiding this comment

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

The usage of the flag should be more clear now.

```

## Prefixes and Suffixes

In order to avoid having the registry TXT records collide with
Expand Down
2 changes: 1 addition & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ func main() {
case "noop":
r, err = registry.NewNoopRegistry(p)
case "txt":
r, err = registry.NewTXTRegistry(p, cfg.TXTPrefix, cfg.TXTSuffix, cfg.TXTOwnerID, cfg.TXTCacheInterval, cfg.TXTWildcardReplacement, cfg.ManagedDNSRecordTypes, cfg.ExcludeDNSRecordTypes, cfg.TXTEncryptEnabled, []byte(cfg.TXTEncryptAESKey))
r, err = registry.NewTXTRegistry(p, cfg.TXTPrefix, cfg.TXTSuffix, cfg.TXTOwnerID, cfg.TXTCacheInterval, cfg.TXTWildcardReplacement, cfg.ManagedDNSRecordTypes, cfg.ExcludeDNSRecordTypes, cfg.TXTEncryptEnabled, []byte(cfg.TXTEncryptAESKey), cfg.TXTNewFormatOnly)
case "aws-sd":
r, err = registry.NewAWSSDRegistry(p, cfg.TXTOwnerID)
default:
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/externaldns/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ type Config struct {
TXTSuffix string
TXTEncryptEnabled bool
TXTEncryptAESKey string `secure:"yes"`
TXTNewFormatOnly bool
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to update types_test.go as well?

Copy link
Author

Choose a reason for hiding this comment

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

Added tests for the new flag now

Interval time.Duration
MinEventSyncInterval time.Duration
Once bool
Expand Down Expand Up @@ -299,6 +300,7 @@ var defaultConfig = &Config{
MinEventSyncInterval: 5 * time.Second,
TXTEncryptEnabled: false,
TXTEncryptAESKey: "",
TXTNewFormatOnly: false,
Interval: time.Minute,
Once: false,
DryRun: false,
Expand Down Expand Up @@ -580,6 +582,7 @@ func (cfg *Config) ParseFlags(args []string) error {
app.Flag("txt-wildcard-replacement", "When using the TXT registry, a custom string that's used instead of an asterisk for TXT records corresponding to wildcard DNS records (optional)").Default(defaultConfig.TXTWildcardReplacement).StringVar(&cfg.TXTWildcardReplacement)
app.Flag("txt-encrypt-enabled", "When using the TXT registry, set if TXT records should be encrypted before stored (default: disabled)").BoolVar(&cfg.TXTEncryptEnabled)
app.Flag("txt-encrypt-aes-key", "When using the TXT registry, set TXT record decryption and encryption 32 byte aes key (required when --txt-encrypt=true)").Default(defaultConfig.TXTEncryptAESKey).StringVar(&cfg.TXTEncryptAESKey)
app.Flag("txt-new-format-only", "When using the TXT registry, only use new format records which include record type information (e.g., prefix: 'a-'). Reduces number of TXT records (default: disabled)").Default(defaultConfig.TXTNewFormatOnly).BoolVar(&cfg.TXTNewFormatOnly)
app.Flag("dynamodb-region", "When using the DynamoDB registry, the AWS region of the DynamoDB table (optional)").Default(cfg.AWSDynamoDBRegion).StringVar(&cfg.AWSDynamoDBRegion)
app.Flag("dynamodb-table", "When using the DynamoDB registry, the name of the DynamoDB table (default: \"external-dns\")").Default(defaultConfig.AWSDynamoDBTable).StringVar(&cfg.AWSDynamoDBTable)

Expand Down
25 changes: 19 additions & 6 deletions registry/txt.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,19 @@ type TXTRegistry struct {
// encrypt text records
txtEncryptEnabled bool
txtEncryptAESKey []byte

newFormatOnly bool
}

// NewTXTRegistry returns new TXTRegistry object
func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID string, cacheInterval time.Duration, txtWildcardReplacement string, managedRecordTypes, excludeRecordTypes []string, txtEncryptEnabled bool, txtEncryptAESKey []byte) (*TXTRegistry, error) {
// NewTXTRegistry returns a new TXTRegistry object. When newFormatOnly is true, it will only
// generate new format TXT records, otherwise it generates both old and new formats for
// backwards compatibility.
func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID string,
cacheInterval time.Duration, txtWildcardReplacement string,
managedRecordTypes, excludeRecordTypes []string,
txtEncryptEnabled bool, txtEncryptAESKey []byte,
newFormatOnly bool) (*TXTRegistry, error) {

if ownerID == "" {
return nil, errors.New("owner id cannot be empty")
}
Expand Down Expand Up @@ -88,6 +97,7 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st
excludeRecordTypes: excludeRecordTypes,
txtEncryptEnabled: txtEncryptEnabled,
txtEncryptAESKey: txtEncryptAESKey,
newFormatOnly: newFormatOnly,
}, nil
}

Expand Down Expand Up @@ -209,12 +219,14 @@ func (im *TXTRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error
return endpoints, nil
}

// generateTXTRecord generates both "old" and "new" TXT records.
// Once we decide to drop old format we need to drop toTXTName() and rename toNewTXTName
// generateTXTRecord generates TXT records in either both formats (old and new) or new format only,
// depending on the newFormatOnly configuration. The old format is maintained for backwards
// compatibility but can be disabled to reduce the number of DNS records.
func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpoint {
endpoints := make([]*endpoint.Endpoint, 0)

if !im.txtEncryptEnabled && !im.mapper.recordTypeInAffix() && r.RecordType != endpoint.RecordTypeAAAA {
// Create legacy format record by default unless newFormatOnly is true
if !im.newFormatOnly && !im.txtEncryptEnabled && !im.mapper.recordTypeInAffix() && r.RecordType != endpoint.RecordTypeAAAA {
// old TXT record format
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey))
if txt != nil {
Expand All @@ -224,7 +236,8 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo
endpoints = append(endpoints, txt)
}
}
// new TXT record format (containing record type)

// Always create new format record
recordType := r.RecordType
// AWS Alias records are encoded as type "cname"
if isAlias, found := r.GetProviderSpecificProperty("alias"); found && isAlias == "true" && recordType == endpoint.RecordTypeA {
Expand Down
Loading