-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
CRD source based on getting endpoints from CRD #657
CRD source based on getting endpoints from CRD #657
Conversation
@shashidharatd: GitHub didn't allow me to request PR reviews from the following users: mtsr, irfanurrehman. Note that only kubernetes-incubator members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
fdf8414
to
6fb9afc
Compare
/cc @njuettner, PTAL. Thanks ! |
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.
Very interesting change, left a few comments.
pkg/apis/externaldns/types.go
Outdated
@@ -185,7 +189,7 @@ func (cfg *Config) ParseFlags(args []string) error { | |||
app.Flag("kubeconfig", "Retrieve target cluster configuration from a Kubernetes configuration file (default: auto-detect)").Default(defaultConfig.KubeConfig).StringVar(&cfg.KubeConfig) | |||
|
|||
// Flags related to processing sources | |||
app.Flag("source", "The resource types that are queried for endpoints; specify multiple times for multiple sources (required, options: service, ingress, fake, connector)").Required().PlaceHolder("source").EnumsVar(&cfg.Sources, "service", "ingress", "fake", "connector") | |||
app.Flag("source", "The resource types that are queried for endpoints; specify multiple times for multiple sources (required, options: service, ingress, fake, connector)").Required().PlaceHolder("source").EnumsVar(&cfg.Sources, "service", "ingress", "fake", "connector", "generic") |
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 wonder if crd
is a better name here than generic
, but I'll leave that question to folks who have more review history than my basically zero in this repo :)
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.
agree, crd
source seems better.
pkg/apis/externaldns/types.go
Outdated
@@ -194,6 +198,8 @@ func (cfg *Config) ParseFlags(args []string) error { | |||
app.Flag("publish-internal-services", "Allow external-dns to publish DNS records for ClusterIP services (optional)").BoolVar(&cfg.PublishInternal) | |||
app.Flag("publish-host-ip", "Allow external-dns to publish host-ip for headless services (optional)").BoolVar(&cfg.PublishHostIP) | |||
app.Flag("connector-source-server", "The server to connect for connector source, valid only when using connector source").Default(defaultConfig.ConnectorSourceServer).StringVar(&cfg.ConnectorSourceServer) | |||
app.Flag("generic-source-crd-apiversion", "Api version of the crd for generic source, format (group/version) valid only when using generic source").Default(defaultConfig.GenericSourceCRDApiVer).StringVar(&cfg.GenericSourceCRDApiVer) |
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.
It might be helpful to have an example in the flag description like test.k8s.io/v1alpha1
as you have used in the test.
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.
Suggest also capitalizing API
and CRD
.
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.
ok, would update.
pkg/apis/externaldns/types.go
Outdated
@@ -194,6 +198,8 @@ func (cfg *Config) ParseFlags(args []string) error { | |||
app.Flag("publish-internal-services", "Allow external-dns to publish DNS records for ClusterIP services (optional)").BoolVar(&cfg.PublishInternal) | |||
app.Flag("publish-host-ip", "Allow external-dns to publish host-ip for headless services (optional)").BoolVar(&cfg.PublishHostIP) | |||
app.Flag("connector-source-server", "The server to connect for connector source, valid only when using connector source").Default(defaultConfig.ConnectorSourceServer).StringVar(&cfg.ConnectorSourceServer) | |||
app.Flag("generic-source-crd-apiversion", "Api version of the crd for generic source, format (group/version) valid only when using generic source").Default(defaultConfig.GenericSourceCRDApiVer).StringVar(&cfg.GenericSourceCRDApiVer) | |||
app.Flag("generic-source-crd-kind", "Kind of the crd for generic source, valid only when using generic source").Default(defaultConfig.GenericSourceCRDKind).StringVar(&cfg.GenericSourceCRDKind) |
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.
Suggestion, make the description here: Kind of the CRD for the generic source in API group and version specified by generic-source-crd-apiversion
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.
will update.
source/generic.go
Outdated
|
||
// NewCRDClientForAPIVersionKind return rest client for the given apiVersion and kind of the CRD | ||
func NewCRDClientForAPIVersionKind(client kubernetes.Interface, kubeConfig, kubeMaster, apiVersion, kind string) (*rest.RESTClient, *runtime.Scheme, error) { | ||
if kubeConfig == "" { |
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.
Is this the same as using the in-cluster 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.
yes, it is almost the same. if user has not set the kubeconfig path explicitly, it would look into standard path $HOME/.kube/config
and if that is also not present then uses in-cluster config. I just reused the logic already done by external-dns in here https://github.com/kubernetes-incubator/external-dns/blob/master/source/store.go#L111
endpoint/endpoint.go
Outdated
} | ||
|
||
// DNSEndpointStatus defines the observed state | ||
type DNSEndpointStatus struct { |
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.
Question: should we have part of the contract between such a CRD and external-dns that external-dns maintains status? We should have observedGeneration
here.
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.
This is a good question, and mostly raises another off topic question about optimisation for all the sources in general. currently, all the sources does not seem to be using informer pattern. they just do List
the object types from the kube client. It is so because the design of external-dns needs all the objects (providing Endpoint) to feed the planner, which then calculates what changes need to be done in DNS provider.
Coming to this source in particular, it also uses the List
operation to get all the CRD objects for now. we could in future write an informer and do a watch, which would give us a local cache of these objects. In that scenario, having such a contract would may be meaningful. Also observedGeneration
in status field makes sense for a single CRD object, but here we are dealing with list of such objects, and i am unsure how meaningful that is to maintain 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.
Well - the a single instance of this resource is itself a list. I think observedGeneration
makes sense as a status field. Otherwise, how do you know - as the controller or the user - that the controller has 'seen' a particular generation of the resource?
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.
@pmorie, currently there is nothing in status, so observedGeneration
does not make sense for such an object. It would be unnecessary complexity. wdyt?
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.
@pmorie, now i have added the ObservedGeneration
field in the Status
according to the discussion. PTAL.
6fb9afc
to
a50a306
Compare
a50a306
to
112ba38
Compare
source/crd.go
Outdated
|
||
config.ContentConfig.GroupVersion = &groupVersion | ||
config.APIPath = "/apis" | ||
config.ContentType = runtime.ContentTypeJSON |
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.
No need to specify the content-type by default it's "application/json"
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.
Fixed.
source/crd.go
Outdated
config.ContentConfig.GroupVersion = &groupVersion | ||
config.APIPath = "/apis" | ||
config.ContentType = runtime.ContentTypeJSON | ||
config.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: serializer.NewCodecFactory(scheme)} |
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.
why not using ...{CodeFactory: scheme.Codecs} directly?
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 did not find scheme.Codecs
in the latest vendored code. Am i missing something.
source/crd.go
Outdated
result := endpoint.DNSEndpointList{} | ||
err := cs.crdClient.Get(). | ||
Namespace(cs.namespace). | ||
Resource(strings.ToLower(cs.crdKind)+"s"). |
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.
Can we somehow make this nicer?
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.
Updated. however not sure what exactly you meant :)
pkg/apis/externaldns/types.go
Outdated
@@ -142,6 +144,8 @@ var defaultConfig = &Config{ | |||
ExoscaleEndpoint: "https://api.exoscale.ch/dns", | |||
ExoscaleAPIKey: "", | |||
ExoscaleAPISecret: "", | |||
CRDSourceAPIVersion: "", |
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.
what do you think of externaldns.k8s.io/v1alpha as APIVersion and DNSEndpoint as Kind for default settings?
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.
Agree. Done
pkg/apis/externaldns/types.go
Outdated
@@ -194,6 +198,8 @@ func (cfg *Config) ParseFlags(args []string) error { | |||
app.Flag("publish-internal-services", "Allow external-dns to publish DNS records for ClusterIP services (optional)").BoolVar(&cfg.PublishInternal) | |||
app.Flag("publish-host-ip", "Allow external-dns to publish host-ip for headless services (optional)").BoolVar(&cfg.PublishHostIP) | |||
app.Flag("connector-source-server", "The server to connect for connector source, valid only when using connector source").Default(defaultConfig.ConnectorSourceServer).StringVar(&cfg.ConnectorSourceServer) | |||
app.Flag("crd-source-apiversion", "API version of the CRD for crd source, e.g. `dns.k8s.io/v1alpha1`, valid only when using crd source").Default(defaultConfig.CRDSourceAPIVersion).StringVar(&cfg.CRDSourceAPIVersion) |
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.
dns.k8s.io/v1alpha1 -> externaldns.k8s.io/v1alpha1
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.
Done
endpoint/endpoint.go
Outdated
// Labels stores labels defined for the Endpoint | ||
Labels Labels | ||
Labels Labels `json:"labels,omitempty"` | ||
} |
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.
There's currently a PR #650 which includes a ProviderSpecific field to set provider specific settings for endpoints.
Could you add it additionally?
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.
Included.
Please add some documentation how to use it. This would be really helpful for customers, which like to use CRD. |
112ba38
to
c4eccb5
Compare
@njuettner, Thanks for your comments. have tried addressing them. Also have added documentation on how to use CRD source. PTAL once again. |
@shashidharatd thank you for adding the documentation. some notes from my side:
|
c4eccb5
to
bb5401e
Compare
@njuettner, have added the crd manifest and an example. ptal. Thanks ! |
👍 for the hard work, thank you! From my point of view it looks good. I would appreciate if @linki or @ideahitme could have a look again 🙂. |
Thanks @njuettner, @linki / @ideahitme ptal |
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.
A couple notes and one question - am I missing something obvious?
docs/contributing/crd-source.md
Outdated
|
||
### Details | ||
|
||
CRD source watches for the user specified CRD to extract [Endpoints](https://github.com/kubernetes-incubator/external-dns/blob/master/endpoint/endpoint.go) from its `Spec`. |
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.
s/the user/a user/
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.
Fixed.
docs/contributing/crd-source.md
Outdated
|
||
### Creating DNS Records | ||
|
||
Create the objects of CRD type by filing in the fields of CRD and DNS record would be created accordingly. |
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.
filling
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.
Fixed.
docs/contributing/crd-source.md
Outdated
|
||
### Registering CRD | ||
|
||
Here is typical example of CRD which provides Endpoints to `CRD source`: |
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.
You should link to the example here so people know that there's an example the rest of this file is in terms of.
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.
Added.
@@ -107,18 +109,25 @@ func (t Targets) IsLess(o Targets) bool { | |||
return false | |||
} | |||
|
|||
// ProviderSpecific holds configuration which is specific to individual DNS providers | |||
type ProviderSpecific 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.
It's worth considering whether this should be a list of key-value pairs. Maps are supposed to be avoided per the API conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#lists-of-named-subobjects-preferred-over-maps
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 think list of k-v pairs might be better because this is a contract many different types will have to implement.
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.
This field is added by the ongoing pr #650. Its optional for the feature in this pr to work.
I would post the same comments in that pr. In this pr i have just added json tag for it.
type DNSEndpointStatus struct { | ||
// The generation observed by the external-dns controller. | ||
// +optional | ||
ObservedGeneration int64 `json:"observedGeneration,omitempty"` |
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.
👍
endpoint/endpoint.go
Outdated
// +genclient | ||
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
||
// DNSEndpoint is the CRD wrapper for Endpoint |
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 would suggest something like:
// DNSEndpoint is a contract that a user-specified CRD must implement to be used as a source for external-dns.
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 would also call out that the user-specified CRD must have the status subresource.
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.
Updated.
endpoints = append(endpoints, dnsEndpoint.Spec.Endpoints...) | ||
dnsEndpoint.Status.ObservedGeneration = dnsEndpoint.Generation | ||
// Update the ObservedGeneration | ||
_, err = cs.UpdateStatus(&dnsEndpoint) |
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 should update the observed generation as we do work - it's not clear to me where this method is used. Am I missing something?
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.
Endpoints()
is the interface method for Source. The controller calls the Source's Endpoint method and proceeds onto making necessary changes to DNS records in Provider. Source is a interface with just this Endpoints()
method.
bb5401e
to
1529c02
Compare
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.
@pmorie, have tried addressing your comments. ptal, Thanks !
docs/contributing/crd-source.md
Outdated
|
||
### Details | ||
|
||
CRD source watches for the user specified CRD to extract [Endpoints](https://github.com/kubernetes-incubator/external-dns/blob/master/endpoint/endpoint.go) from its `Spec`. |
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.
Fixed.
docs/contributing/crd-source.md
Outdated
|
||
### Registering CRD | ||
|
||
Here is typical example of CRD which provides Endpoints to `CRD source`: |
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.
Added.
docs/contributing/crd-source.md
Outdated
|
||
### Creating DNS Records | ||
|
||
Create the objects of CRD type by filing in the fields of CRD and DNS record would be created accordingly. |
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.
Fixed.
@@ -107,18 +109,25 @@ func (t Targets) IsLess(o Targets) bool { | |||
return false | |||
} | |||
|
|||
// ProviderSpecific holds configuration which is specific to individual DNS providers | |||
type ProviderSpecific 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.
This field is added by the ongoing pr #650. Its optional for the feature in this pr to work.
I would post the same comments in that pr. In this pr i have just added json tag for it.
endpoint/endpoint.go
Outdated
// +genclient | ||
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
||
// DNSEndpoint is the CRD wrapper for Endpoint |
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.
Updated.
endpoints = append(endpoints, dnsEndpoint.Spec.Endpoints...) | ||
dnsEndpoint.Status.ObservedGeneration = dnsEndpoint.Generation | ||
// Update the ObservedGeneration | ||
_, err = cs.UpdateStatus(&dnsEndpoint) |
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.
Endpoints()
is the interface method for Source. The controller calls the Source's Endpoint method and proceeds onto making necessary changes to DNS records in Provider. Source is a interface with just this Endpoints()
method.
My feedbacks have been addressed, thanks @shashidharatd |
@linki @ideahitme We're very interested in getting this merged to use external-dns with federation-v2 - does either of you (or another maintainer) have bandwidth to review this PR? |
I think let's merge it now. This seems to be fine and was already reviewed a lot. Thanks again @shashidharatd and @pmorie 👍 |
Thanks @njuettner |
Is it possible to make latency-based route53 record sets with this feature? |
@shashidharatd do you know? |
Honestly, i have no idea. But the Endpoint definition is quite generic, if you can fit in the definition of |
* Update install.md Add small comment about Windows elevated command prompt * Update install.md More precise description about the symbolic link requirement
This pr will allow one to define a CRD with embedded DNS records (in the form of Endpoints) and then make
external-dns
to monitor that CRD for Endpoints and rest of the usual magic of programming DNS server continues.Ref: #555
Here is how to use it.
/cc @linki @mtsr @pmorie @irfanurrehman
p.s: This pr includes 2 commits from relatively independent pr #655