-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: add new flags to allow migration of OwnerID #4823
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
base: master
Are you sure you want to change the base?
Changes from all commits
08dd8b4
ac01c41
5ed390d
502e17c
b1e4029
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -199,6 +199,10 @@ type Controller struct { | |||||
ExcludeRecordTypes []string | ||||||
// MinEventSyncInterval is used as window for batching events | ||||||
MinEventSyncInterval time.Duration | ||||||
// migrate txt-owner flag | ||||||
TXTOwnerMigrate bool | ||||||
// old txt-owner whitch needed to modify | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
TXTOwnerOld string | ||||||
} | ||||||
|
||||||
// RunOnce runs a single iteration of a reconciliation loop. | ||||||
|
@@ -242,13 +246,15 @@ func (c *Controller) RunOnce(ctx context.Context) error { | |||||
registryFilter := c.Registry.GetDomainFilter() | ||||||
|
||||||
plan := &plan.Plan{ | ||||||
Policies: []plan.Policy{c.Policy}, | ||||||
Current: records, | ||||||
Desired: endpoints, | ||||||
DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, registryFilter}, | ||||||
ManagedRecords: c.ManagedRecordTypes, | ||||||
ExcludeRecords: c.ExcludeRecordTypes, | ||||||
OwnerID: c.Registry.OwnerID(), | ||||||
Policies: []plan.Policy{c.Policy}, | ||||||
Current: records, | ||||||
Desired: endpoints, | ||||||
DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, registryFilter}, | ||||||
ManagedRecords: c.ManagedRecordTypes, | ||||||
ExcludeRecords: c.ExcludeRecordTypes, | ||||||
OwnerID: c.Registry.OwnerID(), | ||||||
TXTOwnerMigrate: c.TXTOwnerMigrate, | ||||||
TXTOwnerOld: c.TXTOwnerOld, | ||||||
} | ||||||
|
||||||
plan = plan.Calculate() | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes appear to be unrelated to the "TXT registry owner migration" functionality. Can they be separated into another pull-request? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,10 +56,14 @@ type TXTRegistry struct { | |
// encrypt text records | ||
txtEncryptEnabled bool | ||
txtEncryptAESKey []byte | ||
|
||
//Handle Owner ID migration | ||
isMigrationEnabled bool | ||
oldOwnerID string | ||
} | ||
|
||
// 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) { | ||
func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID string, cacheInterval time.Duration, txtWildcardReplacement string, managedRecordTypes, excludeRecordTypes []string, txtEncryptEnabled bool, txtEncryptAESKey []byte, isMigrationEnabled bool, oldOwnerID string) (*TXTRegistry, error) { | ||
if ownerID == "" { | ||
return nil, errors.New("owner id cannot be empty") | ||
} | ||
|
@@ -88,6 +92,8 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st | |
excludeRecordTypes: excludeRecordTypes, | ||
txtEncryptEnabled: txtEncryptEnabled, | ||
txtEncryptAESKey: txtEncryptAESKey, | ||
isMigrationEnabled: isMigrationEnabled, | ||
oldOwnerID: oldOwnerID, | ||
}, nil | ||
} | ||
|
||
|
@@ -250,6 +256,10 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) | |
UpdateOld: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateOld), | ||
Delete: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.Delete), | ||
} | ||
if im.isMigrationEnabled { | ||
filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, endpoint.FilterEndpointsByOwnerID(im.oldOwnerID, changes.UpdateOld)...) | ||
filteredChanges.Delete = append(filteredChanges.Delete, endpoint.FilterEndpointsByOwnerID(im.oldOwnerID, changes.Delete)...) | ||
} | ||
Comment on lines
+259
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain the purpose of those lines? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to apply the changes which should include the deletion of the old entries in the registry, otherwise we're left we unmanaged and unwanted records |
||
for _, r := range filteredChanges.Create { | ||
if r.Labels == nil { | ||
r.Labels = make(map[string]string) | ||
|
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.
The state represented by
TXTOwnerMigrate
appears to be equivalent tolen(TXTOwnerOld) != 0
; what's the benefit of requiring an additional flag?