-
Notifications
You must be signed in to change notification settings - Fork 327
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(kuma-cp) DNS resolver persistence #850
Conversation
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
9f2c5ee
to
a87061a
Compare
resource := &config_model.ConfigResource{} | ||
err := p.manager.Get(context.Background(), resource, store.GetByKey("kuma-internal-config", "")) | ||
if err != nil { | ||
err = p.manager.Create(context.Background(), resource, store.CreateByKey("kuma-internal-config", "")) |
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.
and isn't resource
is empty at that line, why we try to create it?
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.
I only have doubts about the VIPs synchronizer running also on the leader. That sounds redundant to me.
resource := &config_model.ConfigResource{} | ||
err := p.manager.Get(context.Background(), resource, store.GetByKey(dnsConfigKey, "")) | ||
if err != nil { | ||
if store.IsResourceNotFound(err) { |
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.
👍
func (s *dnsResolver) SetVIPs(list VIPList) { | ||
s.Lock() | ||
defer s.Unlock() | ||
s.viplist = list |
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.
I hope this does not get in the range of hundreds of thousands in practice.
pkg/dns/resolver.go
Outdated
return service, nil | ||
} | ||
|
||
var _ DNSResolver = &dnsResolver{} |
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.
nit: Should this be on top?
pkg/dns/sync_test.go
Outdated
close(stop) | ||
}) | ||
|
||
It("", func() { |
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.
Maybe a name here?
} | ||
} | ||
|
||
if change { |
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.
👍
} | ||
|
||
func (d *vipsSynchronizer) synchronize() error { | ||
vipList, err := d.persistence.Get() |
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.
But this will run also on the leader? We set the VIPs and then get them back?
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.
LGTM. Approved
@@ -64,6 +67,9 @@ func (d *vipsSynchronizer) Start(stop <-chan struct{}) error { | |||
} | |||
|
|||
func (d *vipsSynchronizer) synchronize() error { | |||
if d.leadInfo.IsLeader() { |
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.
👍
Summary
Introduce a new ConfigStore inspired by the SecretsStore. Implement DNS persistence on top of it.
Example format of the resulting storage in ConfigMap on K8s: