config/v1: add public and private zones to DNSSpec #202

abhinavdahiya · 2019-02-07T19:45:27Z

stemmed from conversation in openshift/installer#1169 (comment)

Based on doc DNS should be the source of truth for operators that manage DNS for their components.
DNS currently specifies the BaseDomain that should be used to make aure all the records after subdomains to BaseDomain.

A missing piece for operators that need to create DNS records is where should these records be created. For example, the cluster-ingress-operator
creates DNS records in public and private r53 zones on AWS by listing all zones that match BaseDomain base-domain. The ingress operator is currently making an
assumption that the public zone matching the BaseDomain is probably the correct zone.

With changes in installer PR of creating private r53 zone cluster_name.base_domain and using public zone base_domain. The BaseDomain in DNSSpec will
be set to the cluster_domain (cluster_name.base_domain) as all records must be subdomain of the cluster_domain. This breaks the previous assumption for the cluster-ingress-operator
or any other operator.

Clearly there is a gap to be filled regarding where the DNS records should be created. The installer knows which public and private zones should be used and can provide that information for the operators.

DNSSpec is extended to include a required PrivateZone field and an optional PublicZone field to provide operators location of where the corresponding records should be created.

DNSZone struct is also added to allow defining the DNS zone either using an ID or a string to string map Tags. ID allows installer to specify the public zone as it is predetermined, while Tags allow
installer to point to a private DNS zone that will be created for the cluster.

Open questions:

does the DNSZone provide enough flexibility and information to be platform agnostic?

/cc @ironcladlou @smarterclayton @wking @crawford

ironcladlou · 2019-02-07T20:24:36Z

config/v1/types_dns.go

+
+// DNSZone is used to define a DNS hosted zone.
+// A zone can be identified by an ID or tags.
+type DNSZone struct {


So is this meant to be a platform specific, opaque struct that will be able to address the zone on any supported platform? Will ID+Tags cover all those or do we need a union type that allows for a more strongly typed config object per supported platform?

Will ID+Tags

ID or Tags, I don't think you need both...

I think ID should cover it. @openshift/sig-network-edge WDYT?

Either should work for us. What's more convenient for the user? Are we going to respond to updates to this config?

Is an ID string AWS-specific or cross-platform? Will other providers need additional information to discover/update the zones?

I think an unique identifier or set of tags should be enough to find the zone across platforms.

... it seems like this value becomes an opaque platform-specific value that would require the consumer to use the Infrastructure.Status.Platform to infer how to decode...

Are we supporting cross-platform DNS providers (e.g. an AWS cluster with Route 53 for internal DNS and... some other DNS provider for the public zone)? I think AWS load balancers may require Route 53? If we aren't worried about per-platform zone entries, then we can get the platform information from the infrastructure config.

While a string might be sufficient for all platforms, using:

type DNSZone struct { ID *string `json:"id"` }

gives us room to add more properties if we end up needing them later.

While a string might be sufficient for all platforms, using:

type DNSZone struct { ID *string `json:"id"` }

gives us room to add more properties if we end up needing them later.

The installer currently cannot use ID for the pricate r53 zone as it is created by the terraform.. so id is not known when DNS object is created. We certainly know the tags that will exist on that private zone.

... so id is not known when DNS object is created...

Ah, that makes sense. I guess the ingress operator could perform the tag -> ID lookup in its first round and then push its own update to the config to get more efficient lookups later, if we're concerned about API quotas.

So we need an ID for the public zone and tags for the private zone? If so it seems like AWS implies details leaking into this API, but maybe I'm misunderstanding?

If tags are sufficient to address the private zone, should the be sufficient to address the public zone?

ironcladlou · 2019-02-07T21:43:05Z

config/v1/types_dns.go

 	//
 	// For example, given the base domain `openshift.example.com`, an API server
 	// DNS record may be created for `cluster-api.openshift.example.com`.
 	BaseDomain string `json:"baseDomain"`


With the zone info, ingress operator doesn't even need this field... and if it's immutable, it could be on status? I can't remember who else might consume this field.

With the zone info, ingress operator doesn't even need this field...

👍 Let's drop it in favor of a Domain property, which everyone adds their own special subdomains to.

+1 Let's drop it in favor of a Domain property...

Oh, @ironcladlou was saying you could pull this from the cloud too. I'm a bit more nervous about it DNSSpec being portable between platform without it, but we can always add it back if we hit a platform that needs it.

wking · 2019-02-07T22:27:47Z

config/v1/types_dns.go

+	PublicZone *DNSZone `json:"publicZone,omitempty"`
+	// privateZone is the location where all the DNS records that are only available internally
+	// to the cluster exist.
+	PrivateZone DNSZone `json:"privateZone"`


Public vs. private can come out of the AWS API (e.g. here). Can we just have a slice of zone IDs?

I think the configuration should be specifying, go create public records here and private records here. not the other way around...

I think the configuration should be specifying, go create public records here and private records here. not the other way around...

Are you concerned that sometimes this information would not be available from the AWS API? Or are you concerned about the ingress operator getting the check wrong and accidentally pushing private records to a public zone? I don't see why bugs on this front would be more likely in ingress code than in installer code.

On the other hand, I don't see a need for multiple public or private zones either, so having separate properties isn't a big change. If the ingress operator wants to structure it's handler in a loop and we get separate entries here, it can use []*DNSZone{&spec.PrivateZone, spec.PublicZone} with a guard for nil inside the loop.

This has to be infrastructure independent for sure.

Does this need to be a pointer too (or use a zone slice), so we have a way to tell ingress not to worry about DNS in the libvirt/BYOI cases?

abhinavdahiya · 2019-02-07T23:03:44Z

cc'ing OWNERS for check on spec vs status semantics.

/cc @deads2k @eparis

smarterclayton · 2019-02-08T17:04:25Z

config/v1/types_dns.go

+	// id is the identifier that can be used to find the DNS hosted zone.
+	// +optional
+	ID *string `json:"id"`
+	// tags can be used to query the DNS hosted zone.


Tags is dangerous, that's not going to be identical across platforms. What is this actually capturing?

If this isn't truly platform independent you need to make this specific to the platform, or make the details opaque (i.e. have a string defined as opaque that no one except the person who wrote it is allowed to read).

The installer currently cannot use ID to define the private r53 zone as it is created by the private zone is created by terraform.. so ID is not known when DNS object is created by installer. We certainly know the tags that will exist on that private zone.

Therefore DNSZone has ID and Tags, where one of them is going to be set to find the zone.

And I thought finding a resource using ID or Tags is something all platforms support?
AWS ID: id Tags: tags
Azure ID: name
GCP ID: name/id

If the docs for a human setting this would have to explain it, then it should probably be multiple fields which cannot all be set together (union type) like AWSID, AWSTags, AzureName, GCPName etc.

We intend for end users to set this? What happens when a human changes this? Which operator(s) react and make it valid? If we think that this is set once at install time, and we don't envision a human changing (now, a human could override it maybe later) then status may be more appropriate.

... like AWSID, AWSTags, AzureName, GCPName etc.

If "ID strings" are a cross-platform concept (and they are, right?), then I don't see a need for platform-specific versions of ID property. Same for tags, although from @abhinavdahiya's comment above it sounds like tags may not be cross-platform, with some platforms supporting tags and some supporting creator supplied names.

We intend for end users to set this?

I don't see a problem with users setting tags, but I don't think users will need that functionality either. And while it's theoretically possible to suck Route 53 zone creation up into the asset graph so we could fill in the ID here, that would break the current, useful separation between "create all the assets locally" and "push them up to the target cloud" that BYOH and similar rely on (I think?).

What happens when a human changes this? Which operator(s) react and make it valid?

I don't think this is different from the ID. In both cases, the ingress operator is pulling fresh values from the config and using them to look up the zones under management.

What happens when a human changes this? Which operator(s) react and make it valid?

I don't think this is different from the ID. In both cases, the ingress operator is pulling fresh values from the config and using them to look up the zones under management.

The operator can use a watch on the config object to react when ID or Tags changes in the config object, but it would be trickier to detect changes when an administrator modifies tags on the hosted zones—AFAICT, that would involve polling. If the operator is supposed to react to this sort of configuration change, then using ID will make the implementation more reactive and less burdensome in terms of AWS API requests.

So treating ID and Tags as the basis of a platform-agnostic union type seems to cover all of the providers mentioned so far. Is the fear that we'll discover another case later that doesn't fit the model, forcing us to walk it back into platform-specific types anyway? What other concerns are there with ID+Tags?

I think we should move with ID/Tags for now. since it can cover most cloud platforms with programmable DNS.

That's fine.

Please add slightly more godoc here capturing this discussion and how it should be used.

smarterclayton · 2019-02-08T17:04:38Z

config/v1/types_dns.go

+type DNSZone struct {
+	// id is the identifier that can be used to find the DNS hosted zone.
+	// +optional
+	ID *string `json:"id"`


Should be ID string if default value empty means "not set". Pointer strings are only for special cases where empty string means something (and is usually a bad api design anyway because it's hard to reason about).

smarterclayton · 2019-02-08T17:06:40Z

config/v1/types_dns.go

+	// the internet exist.
+	// If this field is nil, no public records should be created.
+	// +optional
+	PublicZone *DNSZone `json:"publicZone,omitempty"`


Is this allowed to be changed?

Technically no.

Technically no.

Why not? "I don't want public DNS anymore" and "actually, I want it back" seem like reasonable admin decisions, right?

Technically no.

Why not? "I don't want public DNS anymore" and "actually, I want it back" seem like reasonable admin decisions, right?

Oh Yeah, true

abhinavdahiya · 2019-02-08T22:24:21Z

Provided comments on DNSZone struct to show how it can be used to fetch zone platform-agnostic.

wking · 2019-02-08T22:28:12Z

config/v1/types_dns.go

+}
+
+// DNSZone is used to define a DNS hosted zone.
+// A zone is be identified by an ID or tags.


nit: "is" -> "can"

wking · 2019-02-08T22:33:11Z

config/v1/types_dns.go

+// A zone can be identified by an ID or tags.
+//
+// For example,
+// on AWS zone can be fetched using `ID` as [id][1] or using [resourcegroupstaggingapi][2] to fetch zone with filertags from `Tags`,


nit: gofmt has fairly minimal markup, so probably [id][1] -> id [1] and similar. Run godoc -html ./your/package to check.

wking · 2019-02-08T22:40:41Z

config/v1/types_dns.go

+	PublicZone *DNSZone `json:"publicZone,omitempty"`
+	// privateZone is the location where all the DNS records that are only available internally
+	// to the cluster exist.
+	PrivateZone DNSZone `json:"privateZone"`


Does this need to be a pointer too (or use a zone slice), so we have a way to tell ingress not to worry about DNS in the libvirt/BYOI cases?

ironcladlou · 2019-02-11T14:50:50Z

config/v1/types_dns.go

+// A zone can be identified by an ID or tags.
+//
+// For example,
+// on AWS zone can be fetched using `ID` as id in [1] or using resourcegroupstaggingapi [2] to fetch zone with filertags from `Tags`,


s/filertags/tags

smarterclayton · 2019-02-11T19:08:57Z

config/v1/types_dns.go

+type DNSZone struct {
+	// id is the identifier that can be used to find the DNS hosted zone.
+	// +optional
+	ID string `json:"id"`


Please add slightly more godoc

smarterclayton · 2019-02-11T19:09:44Z

Looks fine, add slightly more godoc to the fields in DNSZone describing use in cloud environments and then I'll approve and merge

abhinavdahiya · 2019-02-11T19:22:40Z

Looks fine, add slightly more godoc to the fields in DNSZone describing use in cloud environments and then I'll approve and merge

moved the cloud specefic docs from DNSZone struct to corresponding fields in DNSZone. PTAL

Based on [1] `DNS` should be the source of truth for operators that manage DNS for their components. `DNS` currently specifies the `BaseDomain` that should be used to make aure all the records after subdomains to `BaseDomain`. A missing piece for operators that need to create DNS records is where should these records be created. For example, the `cluster-ingress-operator` creates DNS records in public and private r53 zones on AWS by listing all zones that match `BaseDomain` [2]. The ingress operator is currently making an assumption that the public zone matching the `BaseDomain` is *probably* the correct zone. With changes in installer [3] of creating private r53 zone `cluster_name.base_domain` and using public zone `base_domain`. The `BaseDomain` in `DNSSpec` will be set to the `cluster_domain` (`cluster_name.base_domain`) as all records must be subdomain of the `cluster_domain`. This breaks the previous assumption for the `cluster-ingress-operator` or any other operator. Clearly there is a gap to be filled regarding where the DNS records should be created. The installer knows which public and private zones should be used and can provide that information for the operators. `DNSSpec` is extended to include a required `PrivateZone` field and an optional `PublicZone` field to provide operators location of where the corresponding records should be created. `DNSZone` struct is also added to allow defining the DNS zone either using an `ID` or a string to string map `Tags`. `ID` allows installer to specify the public zone as it is predetermined, while `Tags` allow installer to point to a private DNS zone that will be created for the cluster. [1]: https://github.com/openshift/api/blob/d67473e7f1907b74d1f27706260eecf0bc9f2a52/config/v1/types_dns.go#L9 [2]: https://github.com/openshift/cluster-ingress-operator/blob/e7517023201c485428b3cdb3a86612230cf49e0a/pkg/dns/aws/dns.go#L111 [3]: openshift/installer#1169

generated using `make generate` ```console $ protoc --version libprotoc 3.0.0 $ go version go version go1.10.3 linux/amd64 ```

ironcladlou · 2019-02-12T19:46:00Z

ping @smarterclayton

smarterclayton · 2019-02-12T22:00:54Z

/lgtm

openshift-ci-robot · 2019-02-12T22:01:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

To get openshift/api#202 ```console $ dep ensure -update github.com/openshift/api github.com/openshift/client-go ``` ```console $ dep version dep: version : v0.5.0 build date : 2018-07-26 git hash : 224a564 go version : go1.10.3 go compiler : gc platform : linux/amd64 features : ImportDuringSolve=false ```

openshift-ci-robot requested review from crawford, ironcladlou, smarterclayton and wking February 7, 2019 19:45

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 7, 2019

abhinavdahiya mentioned this pull request Feb 7, 2019

Move DNS records from base_domain to cluster_domain openshift/installer#1169

Merged

6 tasks

ironcladlou reviewed Feb 7, 2019

View reviewed changes

ironcladlou mentioned this pull request Feb 7, 2019

WIP: dns: clean DNS records on clusteringress deletion openshift/cluster-ingress-operator#114

Closed

wking reviewed Feb 7, 2019

View reviewed changes

openshift-ci-robot requested review from deads2k and eparis February 7, 2019 23:03

smarterclayton reviewed Feb 8, 2019

View reviewed changes

abhinavdahiya force-pushed the config_dns branch from 647c0b2 to 7bce3ad Compare February 8, 2019 22:23

wking reviewed Feb 8, 2019

View reviewed changes

abhinavdahiya force-pushed the config_dns branch 2 times, most recently from a535f47 to 0e38866 Compare February 8, 2019 22:34

wking reviewed Feb 8, 2019

View reviewed changes

abhinavdahiya force-pushed the config_dns branch from 0e38866 to 4b860b6 Compare February 9, 2019 00:01

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 9, 2019

ironcladlou suggested changes Feb 11, 2019

View reviewed changes

abhinavdahiya force-pushed the config_dns branch from 4b860b6 to baae572 Compare February 11, 2019 16:58

smarterclayton reviewed Feb 11, 2019

View reviewed changes

abhinavdahiya force-pushed the config_dns branch from baae572 to 2236be3 Compare February 11, 2019 19:21

abhinavdahiya added 2 commits February 11, 2019 16:46

config/v1: update generated content

2d43621

generated using `make generate` ```console $ protoc --version libprotoc 3.0.0 $ go version go version go1.10.3 linux/amd64 ```

abhinavdahiya force-pushed the config_dns branch from 2236be3 to 2d43621 Compare February 12, 2019 00:46

abhinavdahiya mentioned this pull request Feb 12, 2019

manifest: set the public and private zones for AWS openshift/installer#1233

Merged

openshift-ci-robot assigned smarterclayton Feb 12, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2019

openshift-merge-robot merged commit abd1941 into openshift:master Feb 12, 2019

Miciah mentioned this pull request Nov 25, 2021

add service LoadBalancer support for AlibabaCloud platform openshift/cluster-ingress-operator#636

Merged

config/v1: add public and private zones to DNSSpec #202

config/v1: add public and private zones to DNSSpec #202

Uh oh!

Conversation

abhinavdahiya commented Feb 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Feb 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Feb 7, 2019 •

edited

Loading

abhinavdahiya Feb 7, 2019 •

edited

Loading

smarterclayton Feb 8, 2019 •

edited

Loading