-
Notifications
You must be signed in to change notification settings - Fork 232
USHIFT-6796: C2CC: DNS forwarding between clusters #6638
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
Changes from all commits
b15a453
94b5de7
1a5258b
e4d674b
541281b
efdf50b
c7702e3
7fd9db8
850b285
1dca8ac
19ca597
2d956d9
2ff171a
2057172
f6bb333
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| apiVersion: v1 | ||
| data: | ||
| Corefile: | | ||
| {{- .C2CCDNSBlocks }} | ||
| .:5353 { | ||
| bufsize 1232 | ||
| errors | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,14 @@ apiServer: | |
| # Defaults to VersionTLS12. | ||
| minVersion: VersionTLS12 | ||
| clusterToCluster: | ||
| # DNS cache settings for CoreDNS server blocks generated for remote clusters. | ||
| dns: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add these 2 TTL values are set to 10 seconds by default (when not defined). Should it be also add it in cmd/generate-config/config/config-openapi-spec.json ? |
||
| # Maximum TTL (seconds) for denial (NXDOMAIN/NODATA) DNS cache entries in CoreDNS | ||
| # server blocks generated for remote clusters. Must be >= 0. Setting to 0 disables denial caching. | ||
| cacheNegativeTTL: 10 | ||
| # Maximum TTL (seconds) for positive DNS cache entries in CoreDNS server blocks | ||
| # generated for remote clusters. Must be >= 0. Setting to 0 disables positive caching. | ||
| cacheTTL: 10 | ||
| # List of remote clusters to establish connectivity with. | ||
| # C2CC is disabled when this list is empty. | ||
| remoteClusters: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,22 @@ import ( | |
| netutils "k8s.io/utils/net" | ||
| ) | ||
|
|
||
| type C2CCDNS struct { | ||
| // Maximum TTL (seconds) for positive DNS cache entries in CoreDNS server blocks | ||
| // generated for remote clusters. Must be >= 0. Setting to 0 disables positive caching. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:default=10 | ||
| CacheTTL *int `json:"cacheTTL,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think it's necessary to add a check and an error log message if the user set a non integer as a TTL value. Or is it enough with the type enforced in this struct?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that might be too defensive: I think the parser will throw an error |
||
| // Maximum TTL (seconds) for denial (NXDOMAIN/NODATA) DNS cache entries in CoreDNS | ||
| // server blocks generated for remote clusters. Must be >= 0. Setting to 0 disables denial caching. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:default=10 | ||
| CacheNegativeTTL *int `json:"cacheNegativeTTL,omitempty"` | ||
| } | ||
|
|
||
| type C2CC struct { | ||
| // DNS cache settings for CoreDNS server blocks generated for remote clusters. | ||
| DNS C2CCDNS `json:"dns"` | ||
| // List of remote clusters to establish connectivity with. | ||
| // C2CC is disabled when this list is empty. | ||
| RemoteClusters []RemoteCluster `json:"remoteClusters,omitempty"` | ||
|
|
@@ -39,6 +54,7 @@ type ResolvedRemoteCluster struct { | |
| ClusterNetwork []*net.IPNet | ||
| ServiceNetwork []*net.IPNet | ||
| Domain string | ||
| DNSIP string // 10th IP of ServiceNetwork[0], computed during validation when Domain is set | ||
| } | ||
|
|
||
| func (rc *ResolvedRemoteCluster) AllCIDRs() []*net.IPNet { | ||
|
|
@@ -164,6 +180,13 @@ func (c *C2CC) validate(cfg *Config) error { | |
| return fmt.Errorf("cluster to cluster requires OVN-Kubernetes CNI (network.cniPlugin must be \"\" or \"ovnk\", got %q)", cfg.Network.CNIPlugin) | ||
| } | ||
|
|
||
| if c.DNS.CacheTTL != nil && *c.DNS.CacheTTL < 0 { | ||
| return fmt.Errorf("dns.cacheTTL must be >= 0, got %d", *c.DNS.CacheTTL) | ||
| } | ||
| if c.DNS.CacheNegativeTTL != nil && *c.DNS.CacheNegativeTTL < 0 { | ||
| return fmt.Errorf("dns.cacheNegativeTTL must be >= 0, got %d", *c.DNS.CacheNegativeTTL) | ||
| } | ||
|
|
||
| resolved, parseErrs := c.parseRemoteClusters() | ||
| if len(parseErrs) > 0 { | ||
| return errors.Join(parseErrs...) | ||
|
|
@@ -259,13 +282,25 @@ func validateRemoteCluster( | |
| if domainErrs := validation.IsDNS1123Subdomain(rc.Domain); len(domainErrs) > 0 { | ||
| errs = append(errs, fmt.Errorf("%s.domain %q is not a valid DNS name: %s", label, rc.Domain, strings.Join(domainErrs, ", "))) | ||
| } | ||
| if rc.Domain == "cluster.local" { | ||
| errs = append(errs, fmt.Errorf("%s.domain cannot be cluster.local", label)) | ||
| } | ||
| if prev, ok := seenRemoteDomains[rc.Domain]; ok { | ||
| errs = append(errs, fmt.Errorf("%s.domain %q duplicates remoteClusters[%d]", label, rc.Domain, prev)) | ||
| } else { | ||
| seenRemoteDomains[rc.Domain] = i | ||
| } | ||
| } | ||
|
|
||
| if rc.Domain != "" && len(rc.ServiceNetwork) > 0 { | ||
| dnsIP, err := getClusterDNS(rc.ServiceNetwork[0]) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("%s: failed to compute DNS IP from serviceNetwork[0] %q: %w", label, rc.ServiceNetwork[0], err)) | ||
| } else { | ||
| res.DNSIP = dnsIP | ||
| } | ||
| } | ||
|
|
||
| errs = append(errs, validateIPFamilyConsistencyNets(res.ClusterNetwork, label+".clusterNetwork")...) | ||
| errs = append(errs, validateIPFamilyConsistencyNets(res.ServiceNetwork, label+".serviceNetwork")...) | ||
| errs = append(errs, validateNetworkShapeNets(res.ClusterNetwork, res.ServiceNetwork, label)...) | ||
|
|
@@ -345,3 +380,33 @@ func checkCIDRConflicts(cidr *net.IPNet, cidrStr, label string, seenCIDRs []labe | |
| func cidrsOverlap(a, b *net.IPNet) bool { | ||
| return a.Contains(b.IP) || b.Contains(a.IP) | ||
| } | ||
|
|
||
| // RenderC2CCDNSBlocks generates CoreDNS server blocks for cross-cluster DNS. | ||
| func RenderC2CCDNSBlocks(resolved []ResolvedRemoteCluster, cacheTTL, cacheNegativeTTL int) string { | ||
| var blocks []string | ||
| for _, rc := range resolved { | ||
| if rc.Domain == "" { | ||
| continue | ||
| } | ||
| blocks = append(blocks, formatDNSBlock(rc.Domain, rc.DNSIP, cacheTTL, cacheNegativeTTL)) | ||
| } | ||
| if len(blocks) == 0 { | ||
| return "" | ||
| } | ||
| return "\n" + strings.Join(blocks, "\n") | ||
| } | ||
|
|
||
| func formatDNSBlock(domain, dnsIP string, cacheTTL, cacheNegativeTTL int) string { | ||
| return fmt.Sprintf(` %s:5353 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make this a constant at the top of the file?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's only used in single place so I'd rather not move it to a place almost 400 lines away. |
||
| bufsize 1232 | ||
| errors | ||
| log . { | ||
| class error | ||
| } | ||
| rewrite stop name suffix .%s .cluster.local answer auto | ||
| forward . %s | ||
| cache %d { | ||
| denial 9984 %d | ||
| } | ||
| }`, domain, domain, dnsIP, cacheTTL, cacheNegativeTTL) | ||
| } | ||
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 it possible to mask out the next server block by setting up a special domain?
Say you configure
cluster.localas domain. That would renderWhich is more specific than the catch all, and would route all the
cluster.localrequests to remote cluster.This is a bit of a stretch because no real use case configures a remote cluster with the same domain than the local one, but should we validate 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.
True, good catch