-
Notifications
You must be signed in to change notification settings - Fork 585
Add base domain to DNS config #146
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
Add base domain to DNS config #146
Conversation
Add base domain to the DNS config type. This value is useful to any component which implements DNS management for the cluster. For example, cluster-ingress-operator manages wildcard DNS records for routing within the base domain.
57b4120 to
3bf7332
Compare
| type DNSSpec struct { | ||
| // BaseDomain is the base domain of the cluster. All managed DNS records will | ||
| // be sub-domains of this base. | ||
| BaseDomain string `json:"baseDomain"` |
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 might be useful in Infra ?
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.
Yeah, this is probably a higher level config setting. Both DNS and the IngressOperator need it. I am not sure if anyone else wants to know, so I don't know if keeping it in DNS is sufficient for now.
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 there supposed to be any correspondence between config object and operator? Logically DNS-related configuration belongs here: "DNS holds cluster-wide information about DNS." That doesn't specifically mean config for the DNS operator.
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.
@Miciah shares my interpretation of this type. If the base domain (the only required DNS input to the installer?) doesn't belong here I'm not sure what does.
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.
Yeah, this is probably a higher level config setting. Both DNS and the IngressOperator need it. I am not sure if anyone else wants to know, so I don't know if keeping it in DNS is sufficient for now.
As a cluster-admin, if I want to change my DNS suffix, where would I expect to do it? The types here are not mapped to operators, but instead to features. The feature here being DNS.
The ingress operator is perfectably able to consume the information from the DNS config type if it needs it as an input.
config/v1/types_dns.go
Outdated
| } | ||
|
|
||
| type DNSSpec struct { | ||
| // BaseDomain is the base domain of the cluster. All managed DNS records will |
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.
baseDomain and providing an example would help.
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, PTAL
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, ironcladlou The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add base domain to the DNS config type. This value is useful to any component
which implements DNS management for the cluster. For example,
cluster-ingress-operator manages wildcard DNS records for routing within the
base domain.
/cc @openshift/sig-network-edge @derekwaynecarr @deads2k @rajatchopra @crawford @smarterclayton