Skip to content

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

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 2 commits into from
Feb 12, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b898e0
config/v1: add public and private zones to DNSSpec
abhinavdahiya Feb 7, 2019
2d43621
config/v1: update generated content
abhinavdahiya Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions config/v1/types_dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,40 @@ type DNSSpec struct {
// 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"`
Copy link
Contributor

@ironcladlou ironcladlou Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@wking wking Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@wking wking Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

// publicZone is the location where all the DNS records that are publicly accessible to
// the internet exist.
// If this field is nil, no public records should be created.
// +optional
PublicZone *DNSZone `json:"publicZone,omitempty"`
Copy link
Contributor

@smarterclayton smarterclayton Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed to be changed?

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically no.

Copy link
Member

@wking wking Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically no.

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

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// privateZone is the location where all the DNS records that are only available internally
// to the cluster exist.
// If this field is nil, no private records should be created.
// +optional
PrivateZone *DNSZone `json:"privateZone,omitempty"`
}

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

@ironcladlou ironcladlou Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will ID+Tags

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

Copy link
Contributor

@ironcladlou ironcladlou Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Miciah Miciah Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@wking wking Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@wking wking Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@wking wking Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.

Copy link
Contributor

@ironcladlou ironcladlou Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// id is the identifier that can be used to find the DNS hosted zone.
//
// on AWS zone can be fetched using `ID` as id in [1]
// on Azure zone can be fetched using `ID` as a pre-determined name in [2],
// on GCP zone can be fetched using `ID` as a pre-determined name in [3].
//
// [1]: https://docs.aws.amazon.com/cli/latest/reference/route53/get-hosted-zone.html#options
// [2]: https://docs.microsoft.com/en-us/cli/azure/network/dns/zone?view=azure-cli-latest#az-network-dns-zone-show
// [3]: https://cloud.google.com/dns/docs/reference/v1/managedZones/get
// +optional
ID string `json:"id,omitempty"`

// tags can be used to query the DNS hosted zone.
Copy link
Contributor

@smarterclayton smarterclayton Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@smarterclayton smarterclayton Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@smarterclayton smarterclayton Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@wking wking Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.

Copy link
Contributor

@Miciah Miciah Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ironcladlou ironcladlou Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@abhinavdahiya abhinavdahiya Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@smarterclayton smarterclayton Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine.

Copy link
Contributor

@smarterclayton smarterclayton Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// on AWS, resourcegroupstaggingapi [1] can be used to fetch a zone using `Tags` as tag-filters,
//
// [1]: https://docs.aws.amazon.com/cli/latest/reference/resourcegroupstaggingapi/get-resources.html#options
// +optional
Tags map[string]string `json:"tags,omitempty"`
}

type DNSStatus struct {
Expand Down
43 changes: 42 additions & 1 deletion config/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 13 additions & 1 deletion config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.