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

[WIP] Dynamic updates API with listen server #1822

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@syndbg
Contributor

syndbg commented May 20, 2018

1. What does this pull request do?

Adds an "experimental" API for dynamic updates of resource records.

Currently I plan to just test the waters and validate the direction I'm going in.

Tests are lacking, it's just a basic proof-of-concept. I'll add relevant ones in the next 24 hours.

Feedback is greatly appreciated by maintainers and anyone interested.

Currently what works is creating a POST request with A record that gets added to a zone managed by the file plugin.

2. Which issues (if any) are related?

Somewhat related in the concept of managing records

#522

3. Which documentation changes (if any) need to be made?

This is still WIP, I'll update when I have a clear change/feature-set.

@corbot

This comment has been minimized.

corbot bot commented May 20, 2018

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked bradbeam (via /OWNERS) for a review.

@corbot corbot bot requested a review from bradbeam May 20, 2018

@miekg miekg removed the request for review from bradbeam May 21, 2018

@miekg

This comment has been minimized.

Member

miekg commented May 21, 2018

[unrelated to this PR]:
@bradbeam reseeding the random now before choosing a reviewer :)
(testing corbot is a bit tricky, so some of it happens in prod)

@fturib

This comment has been minimized.

Member

fturib commented May 21, 2018

/cc @johnbelamaric @rajansandeep @chrisohaver : that is a beginning of implementation of the "registration" part of the "Service Discovery and Registration" feature.

@miekg

This comment has been minimized.

Member

miekg commented May 21, 2018

@syndbg

This comment has been minimized.

Contributor

syndbg commented May 21, 2018

For me the use cases revolve around:

  • using coredns as main DNS server,
  • having the ability to manage (add mostly) DNS records via API,
  • having no other dependencies and other sources of truth, such as databases.

My personal problem with the other source-of-truth approach such as databases is asking the questions:

  • Do I need to persist the records? Is in-memory good enough for me?

My use-case answer: In-memory is good enough for me and it's better not to persist. It may create awkward sync scenarios if it was persisted.

  • What's the ease of communication with the other source of truth? It varies by database/kv store and etc.

Curl/whatever API is good enough and easily accessible in any language. Other stores require specific drivers/binary protocol. Rest API support is rare except for CouchDB, from what I can remember right now.

  • What is CoreDNS doing to sync with the other source of truth? Do I do it via event listening and/or polling?

I would prefer to do it via event listening, but this is not supported by any store.

@miekg

This comment has been minimized.

Member

miekg commented May 21, 2018

kinda neat that this is contained in a plugin.

Another problem with providing these kind of interfaces is the feature creep the elect: user, permissions all that kind of stuff. Even with the DNS dynamic updates (which we probably should support at some point sigh) you get into this territory.

Potentially we could create a coredns/dynapi to flesh out the idea and have a official out-of-tree home for it.

@johnbelamaric

This comment has been minimized.

Member

johnbelamaric commented May 21, 2018

Yeah. We have separate things:

  1. The Go API (interface or interfaces) for plugins to implement to say they are "writable".
  2. The API used by clients to push actual writes.

Each plugin can decide what they want to do for the first - or if they want to support it at all. Then, for the second, we have a separate set of plugins to expose different ways to do it:

  • DDNS
  • HTTPS
  • gRPC

The authentication and authorization are whole separate topics that would need to be addressed, hopefully in ways that are usable across client APIs as much as possible.

@johnbelamaric

This comment has been minimized.

Member

johnbelamaric commented May 21, 2018

Each plugin can decide what they want to do for the first - or if they want to support it at all. Then, for the second, we have a separate set of plugins to expose different ways to do it:

I don't mean each one defines its own API - I mean they can implement "Writable" or not. When we get a write request in, we figure out which plugin manages that zone and if it doesn't implement the interface we fail.

@syndbg

This comment has been minimized.

Contributor

syndbg commented May 22, 2018

@johnbelamaric I would say implementing Writable is the way to go. Doing a strategy pattern is much more manageable.

@syndbg

This comment has been minimized.

Contributor

syndbg commented May 26, 2018

Just a heads up. I'll be with very limited available time during the next 1-2 weeks.

Slow progress is expected.

@miekg

This comment has been minimized.

Member

miekg commented May 29, 2018

@syndbg syndbg force-pushed the syndbg:dynapi branch 8 times, most recently from 07bedb9 to d452c26 Jun 17, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jun 17, 2018

Codecov Report

Merging #1822 into master will decrease coverage by 0.38%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1822      +/-   ##
==========================================
- Coverage   54.91%   54.52%   -0.39%     
==========================================
  Files         200      200              
  Lines        9725     9803      +78     
==========================================
+ Hits         5340     5345       +5     
- Misses       3974     4048      +74     
+ Partials      411      410       -1
Impacted Files Coverage Δ
plugin/file/zone.go 68.57% <ø> (ø) ⬆️
plugin/file/file.go 25.5% <0%> (-28.02%) ⬇️
plugin/forward/persistent.go 98.59% <0%> (+2.81%) ⬆️
plugin/forward/connect.go 86.48% <0%> (+4.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a77cd...8e3e6da. Read the comment docs.

@syndbg syndbg force-pushed the syndbg:dynapi branch from d452c26 to 32ab37a Jun 18, 2018

@syndbg

This comment has been minimized.

Contributor

syndbg commented Jul 1, 2018

@miekg Input is appreciated for the direction I went in.

@miekg

This comment has been minimized.

Member

miekg commented Jul 1, 2018

@syndbg

This comment has been minimized.

Contributor

syndbg commented Jul 4, 2018

@miekg I've sent you a direct message in CNCF's slack on how to proceed.

My handle is syndbg. If you can take a look that would be great.

It's regarding how to proceed.

syndbg added some commits May 20, 2018

Refactor
Extract to `dynapi` package.
Rename `dynapi` plugin to `dynapirest`.

@syndbg syndbg force-pushed the syndbg:dynapi branch from 32ab37a to 8e3e6da Aug 8, 2018

@stickler-ci

This comment has been minimized.

stickler-ci commented Aug 8, 2018

Your configuration file uses an unknown linter. The golint linter is not a supported linter.

@piaoyu

This comment has been minimized.

piaoyu commented Aug 13, 2018

func (f File) Delete(request *dynapi.DynapiRequest) error {
return errors.New("not supported")
}
func (f File) Update(request *dynapi.DynapiRequest) error {
return errors.New("not supported")
}
@syndbg why not support Delete and Update?
When a traditional monolithic app move to the microservices on kubernetes. We want to open the delete and update to the user?

@syndbg

This comment has been minimized.

Contributor

syndbg commented Aug 13, 2018

@piaoyu Hi, it looks like you're looking at an old commit. From the most recent diff, delete and update is supported.

@piaoyu

This comment has been minimized.

piaoyu commented Aug 14, 2018

@syndbg Oh,I put my foot in my mouth. delete and update is supported. But why not use grpc ?

@syndbg

This comment has been minimized.

Contributor

syndbg commented Aug 16, 2018

@miekg Ping. Can we proceed to move dynapirest to a separate git repository inside coredns org?

dyndns will remain as part of coredns/coredns, right?

@miekg

This comment has been minimized.

Member

miekg commented Aug 22, 2018

@syndbg

This comment has been minimized.

Contributor

syndbg commented Aug 23, 2018

@miekg coredns/dynapirest

@vtolstov

This comment has been minimized.

vtolstov commented Aug 31, 2018

any progress? I'm realy want to try create/list/delete domains and records via api

@syndbg

This comment has been minimized.

Contributor

syndbg commented Sep 1, 2018

@vtolstov Waiting for the repository.

Plugin-wise we've been using it since little before May 20th in a production environment where a small daemon reads DHCP-leases and sends them to coredns to dynamically register DNS A records.

Worked great, however one issue was found a while ago for our use case: that DNS records created with no TTL specified would use a default one of 3600. With that fixed with https://github.com/coredns/coredns/pull/1822/files#diff-9233dbaadd466f84dd978f0b7304e340R46 and specifiable TTL as part of the create/update request - it fits our need perfectly with short TTL of DNS records.

@miekg

This comment has been minimized.

Member

miekg commented Sep 1, 2018

@syndbg syndbg referenced this pull request Sep 1, 2018

Open

[WIP] Add `dynapi` plugin #1

0 of 11 tasks complete
@vtolstov

This comment has been minimized.

vtolstov commented Sep 1, 2018

@vtolstov

This comment has been minimized.

vtolstov commented Sep 13, 2018

gentle ping..

@syndbg

This comment has been minimized.

Contributor

syndbg commented Sep 14, 2018

@vtolstov Hi, hopefully this weekend I'll have the time to handle it.

I've seen that you saw the other PR.

@miekg

This comment has been minimized.

Member

miekg commented Sep 29, 2018

https://github.com/coredns/dynapi exists, closing here.

@miekg miekg closed this Sep 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment