From a3ba760a2794be66f58c7ce1951dc100370bd348 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 22 Oct 2016 11:43:18 -0400 Subject: [PATCH] Federation: separate notion of zone-name & dns-suffix We can put subdomains into hosted zones (for example, foo.federation.example.com can be hosted in example.com) By allowing sharing a common hosted zone, this means the user doesn't have to do as much setup. --- .../app/controllermanager.go | 2 +- .../app/options/options.go | 3 + .../pkg/federation-controller/service/BUILD | 1 + .../pkg/federation-controller/service/dns.go | 42 +++-- .../federation-controller/service/dns_test.go | 170 ++++++++++++++++++ .../service/endpoint_helper_test.go | 11 +- .../service/servicecontroller.go | 16 +- hack/verify-flags/known-flags.txt | 1 + 8 files changed, 221 insertions(+), 25 deletions(-) create mode 100644 federation/pkg/federation-controller/service/dns_test.go diff --git a/federation/cmd/federation-controller-manager/app/controllermanager.go b/federation/cmd/federation-controller-manager/app/controllermanager.go index 4b13ad766d79..63f5923d0581 100644 --- a/federation/cmd/federation-controller-manager/app/controllermanager.go +++ b/federation/cmd/federation-controller-manager/app/controllermanager.go @@ -179,7 +179,7 @@ func StartControllers(s *options.CMServer, restClientCfg *restclient.Config) err glog.Infof("Loading client config for service controller %q", servicecontroller.UserAgentName) scClientset := federationclientset.NewForConfigOrDie(restclient.AddUserAgent(restClientCfg, servicecontroller.UserAgentName)) - servicecontroller := servicecontroller.New(scClientset, dns, s.FederationName, s.ZoneName) + servicecontroller := servicecontroller.New(scClientset, dns, s.FederationName, s.ServiceDnsSuffix, s.ZoneName) glog.Infof("Running service controller") if err := servicecontroller.Run(s.ConcurrentServiceSyncs, wait.NeverStop); err != nil { glog.Errorf("Failed to start service controller: %v", err) diff --git a/federation/cmd/federation-controller-manager/app/options/options.go b/federation/cmd/federation-controller-manager/app/options/options.go index 091497110b16..fb8f52bda5e6 100644 --- a/federation/cmd/federation-controller-manager/app/options/options.go +++ b/federation/cmd/federation-controller-manager/app/options/options.go @@ -39,6 +39,8 @@ type ControllerManagerConfiguration struct { FederationName string `json:"federationName"` // zone name, like example.com. ZoneName string `json:"zoneName"` + // ServiceDnsSuffix is the dns suffix to use when publishing federated services. + ServiceDnsSuffix string `json:"serviceDnsSuffix"` // dnsProvider is the provider for dns services. DnsProvider string `json:"dnsProvider"` // dnsConfigFile is the path to the dns provider configuration file. @@ -101,6 +103,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet) { fs.Var(componentconfig.IPVar{Val: &s.Address}, "address", "The IP address to serve on (set to 0.0.0.0 for all interfaces)") fs.StringVar(&s.FederationName, "federation-name", s.FederationName, "Federation name.") fs.StringVar(&s.ZoneName, "zone-name", s.ZoneName, "Zone name, like example.com.") + fs.StringVar(&s.ServiceDnsSuffix, "service-dns-suffix", s.ServiceDnsSuffix, "DNS Suffix to use when publishing federated service names. Defaults to zone-name") fs.IntVar(&s.ConcurrentServiceSyncs, "concurrent-service-syncs", s.ConcurrentServiceSyncs, "The number of service syncing operations that will be done concurrently. Larger number = faster endpoint updating, but more CPU (and network) load") fs.IntVar(&s.ConcurrentReplicaSetSyncs, "concurrent-replicaset-syncs", s.ConcurrentReplicaSetSyncs, "The number of ReplicaSets syncing operations that will be done concurrently. Larger number = faster endpoint updating, but more CPU (and network) load") fs.DurationVar(&s.ClusterMonitorPeriod.Duration, "cluster-monitor-period", s.ClusterMonitorPeriod.Duration, "The period for syncing ClusterStatus in ClusterController.") diff --git a/federation/pkg/federation-controller/service/BUILD b/federation/pkg/federation-controller/service/BUILD index 34da07e1cab3..2abff8ce8572 100644 --- a/federation/pkg/federation-controller/service/BUILD +++ b/federation/pkg/federation-controller/service/BUILD @@ -50,6 +50,7 @@ go_library( go_test( name = "go_default_test", srcs = [ + "dns_test.go", "endpoint_helper_test.go", "service_helper_test.go", "servicecontroller_test.go", diff --git a/federation/pkg/federation-controller/service/dns.go b/federation/pkg/federation-controller/service/dns.go index ba91ccc8d237..20366274dc23 100644 --- a/federation/pkg/federation-controller/service/dns.go +++ b/federation/pkg/federation-controller/service/dns.go @@ -24,6 +24,7 @@ import ( "k8s.io/kubernetes/federation/pkg/dnsprovider" "k8s.io/kubernetes/federation/pkg/dnsprovider/rrstype" + "strings" ) const ( @@ -89,22 +90,28 @@ func (s *ServiceController) getClusterZoneNames(clusterName string) (zones []str return client.cluster.Status.Zones, client.cluster.Status.Region, nil } -// getFederationDNSZoneName returns the name of the managed DNS Zone configured for this federation -func (s *ServiceController) getFederationDNSZoneName() (string, error) { - return s.zoneName, nil +// getServiceDnsSuffix returns the DNS suffix to use when creating federated-service DNS records +func (s *ServiceController) getServiceDnsSuffix() (string, error) { + return s.serviceDnsSuffix, nil } -// getDnsZone is a hack around the fact that dnsprovider does not yet support a Get() method, only a List() method. TODO: Fix that. +// getDnsZone returns the zone, as identified by zoneName func getDnsZone(dnsZoneName string, dnsZonesInterface dnsprovider.Zones) (dnsprovider.Zone, error) { + // TODO: We need query-by-name and query-by-id functions dnsZones, err := dnsZonesInterface.List() if err != nil { return nil, err } + + findName := strings.TrimSuffix(dnsZoneName, ".") for _, dnsZone := range dnsZones { - if dnsZone.Name() == dnsZoneName { - return dnsZone, nil + if findName != "" { + if strings.TrimSuffix(dnsZone.Name(), ".") == findName { + return dnsZone, nil + } } } + return nil, fmt.Errorf("DNS zone %s not found.", dnsZoneName) } @@ -152,11 +159,7 @@ func getResolvedEndpoints(endpoints []string) ([]string, error) { /* ensureDnsRrsets ensures (idempotently, and with minimum mutations) that all of the DNS resource record sets for dnsName are consistent with endpoints. if endpoints is nil or empty, a CNAME record to uplevelCname is ensured. */ -func (s *ServiceController) ensureDnsRrsets(dnsZoneName, dnsName string, endpoints []string, uplevelCname string) error { - dnsZone, err := getDnsZone(dnsZoneName, s.dnsZones) - if err != nil { - return err - } +func (s *ServiceController) ensureDnsRrsets(dnsZone dnsprovider.Zone, dnsName string, endpoints []string, uplevelCname string) error { rrsets, supported := dnsZone.ResourceRecordSets() if !supported { return fmt.Errorf("Failed to ensure DNS records for %s. DNS provider does not support the ResourceRecordSets interface.", dnsName) @@ -258,7 +261,7 @@ func (s *ServiceController) ensureDnsRrsets(dnsZoneName, dnsName string, endpoin /* ensureDnsRecords ensures (idempotently, and with minimum mutations) that all of the DNS records for a service in a given cluster are correct, given the current state of that service in that cluster. This should be called every time the state of a service might have changed -(either w.r.t. it's loadblancer address, or if the number of healthy backend endpoints for that service transitioned from zero to non-zero +(either w.r.t. it's loadbalancer address, or if the number of healthy backend endpoints for that service transitioned from zero to non-zero (or vice verse). Only shards of the service which have both a loadbalancer ingress IP address or hostname AND at least one healthy backend endpoint are included in DNS records for that service (at all of zone, region and global levels). All other addresses are removed. Also, if no shards exist in the zone or region of the cluster, a CNAME reference to the next higher level is ensured to exist. */ @@ -298,7 +301,7 @@ func (s *ServiceController) ensureDnsRecords(clusterName string, cachedService * if zoneNames == nil { return fmt.Errorf("failed to get cluster zone names") } - dnsZoneName, err := s.getFederationDNSZoneName() + serviceDnsSuffix, err := s.getServiceDnsSuffix() if err != nil { return err } @@ -309,16 +312,21 @@ func (s *ServiceController) ensureDnsRecords(clusterName string, cachedService * commonPrefix := serviceName + "." + namespaceName + "." + s.federationName + ".svc" // dnsNames is the path up the DNS search tree, starting at the leaf dnsNames := []string{ - commonPrefix + "." + zoneNames[0] + "." + regionName + "." + dnsZoneName, // zone level - TODO might need other zone names for multi-zone clusters - commonPrefix + "." + regionName + "." + dnsZoneName, // region level, one up from zone level - commonPrefix + "." + dnsZoneName, // global level, one up from region level + commonPrefix + "." + zoneNames[0] + "." + regionName + "." + serviceDnsSuffix, // zone level - TODO might need other zone names for multi-zone clusters + commonPrefix + "." + regionName + "." + serviceDnsSuffix, // region level, one up from zone level + commonPrefix + "." + serviceDnsSuffix, // global level, one up from region level "", // nowhere to go up from global level } endpoints := [][]string{zoneEndpoints, regionEndpoints, globalEndpoints} + dnsZone, err := getDnsZone(s.zoneName, s.dnsZones) + if err != nil { + return err + } + for i, endpoint := range endpoints { - if err = s.ensureDnsRrsets(dnsZoneName, dnsNames[i], endpoint, dnsNames[i+1]); err != nil { + if err = s.ensureDnsRrsets(dnsZone, dnsNames[i], endpoint, dnsNames[i+1]); err != nil { return err } } diff --git a/federation/pkg/federation-controller/service/dns_test.go b/federation/pkg/federation-controller/service/dns_test.go new file mode 100644 index 000000000000..72a3a1475d42 --- /dev/null +++ b/federation/pkg/federation-controller/service/dns_test.go @@ -0,0 +1,170 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + "sync" + "testing" + + "fmt" + "k8s.io/kubernetes/federation/apis/federation/v1beta1" + "k8s.io/kubernetes/federation/pkg/dnsprovider/providers/google/clouddns" // Only for unit testing purposes. + "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/util/sets" + "reflect" + "sort" +) + +func TestServiceController_ensureDnsRecords(t *testing.T) { + tests := []struct { + name string + service v1.Service + expected []string + serviceStatus v1.LoadBalancerStatus + }{ + { + name: "withip", + service: v1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: "servicename", + Namespace: "servicenamespace", + }, + }, + serviceStatus: buildServiceStatus([][]string{{"198.51.100.1", ""}}), + expected: []string{ + "example.com:servicename.servicenamespace.myfederation.svc.federation.example.com:A:180:[198.51.100.1]", + "example.com:servicename.servicenamespace.myfederation.svc.fooregion.federation.example.com:A:180:[198.51.100.1]", + "example.com:servicename.servicenamespace.myfederation.svc.foozone.fooregion.federation.example.com:A:180:[198.51.100.1]", + }, + }, + /* + TODO: getResolvedEndpoints preforms DNS lookup. + Mock and maybe look at error handling when some endpoints resolve, but also caching? + { + name: "withname", + service: v1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: "servicename", + Namespace: "servicenamespace", + }, + }, + serviceStatus: buildServiceStatus([][]string{{"", "randomstring.amazonelb.example.com"}}), + expected: []string{ + "example.com:servicename.servicenamespace.myfederation.svc.federation.example.com:A:180:[198.51.100.1]", + "example.com:servicename.servicenamespace.myfederation.svc.fooregion.federation.example.com:A:180:[198.51.100.1]", + "example.com:servicename.servicenamespace.myfederation.svc.foozone.fooregion.federation.example.com:A:180:[198.51.100.1]", + }, + }, + */ + { + name: "noendpoints", + service: v1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: "servicename", + Namespace: "servicenamespace", + }, + }, + expected: []string{ + "example.com:servicename.servicenamespace.myfederation.svc.fooregion.federation.example.com:CNAME:180:[servicename.servicenamespace.myfederation.svc.federation.example.com]", + "example.com:servicename.servicenamespace.myfederation.svc.foozone.fooregion.federation.example.com:CNAME:180:[servicename.servicenamespace.myfederation.svc.fooregion.federation.example.com]", + }, + }, + } + for _, test := range tests { + fakedns, _ := clouddns.NewFakeInterface() + fakednsZones, ok := fakedns.Zones() + if !ok { + t.Error("Unable to fetch zones") + } + serviceController := ServiceController{ + dns: fakedns, + dnsZones: fakednsZones, + serviceDnsSuffix: "federation.example.com", + zoneName: "example.com", + federationName: "myfederation", + serviceCache: &serviceCache{fedServiceMap: make(map[string]*cachedService)}, + clusterCache: &clusterClientCache{ + rwlock: sync.Mutex{}, + clientMap: make(map[string]*clusterCache), + }, + knownClusterSet: make(sets.String), + } + + clusterName := "testcluster" + + serviceController.clusterCache.clientMap[clusterName] = &clusterCache{ + cluster: &v1beta1.Cluster{ + Status: v1beta1.ClusterStatus{ + Zones: []string{"foozone"}, + Region: "fooregion", + }, + }, + } + + cachedService := &cachedService{ + lastState: &test.service, + endpointMap: make(map[string]int), + serviceStatusMap: make(map[string]v1.LoadBalancerStatus), + } + cachedService.endpointMap[clusterName] = 1 + if !reflect.DeepEqual(&test.serviceStatus, &v1.LoadBalancerStatus{}) { + cachedService.serviceStatusMap[clusterName] = test.serviceStatus + } + + err := serviceController.ensureDnsRecords(clusterName, cachedService) + if err != nil { + t.Errorf("Test failed for %s, unexpected error %v", test.name, err) + } + + zones, err := fakednsZones.List() + if err != nil { + t.Errorf("error querying zones: %v", err) + } + + // Dump every record to a testable-by-string-comparison form + var records []string + for _, z := range zones { + zoneName := z.Name() + + rrs, ok := z.ResourceRecordSets() + if !ok { + t.Errorf("cannot get rrs for zone %q", zoneName) + } + + rrList, err := rrs.List() + if err != nil { + t.Errorf("error querying rr for zone %q: %v", zoneName, err) + } + for _, rr := range rrList { + rrdatas := rr.Rrdatas() + + // Put in consistent (testable-by-string-comparison) order + sort.Strings(rrdatas) + records = append(records, fmt.Sprintf("%s:%s:%s:%d:%s", zoneName, rr.Name(), rr.Type(), rr.Ttl(), rrdatas)) + } + } + + // Ignore order of records + sort.Strings(records) + sort.Strings(test.expected) + + if !reflect.DeepEqual(records, test.expected) { + t.Errorf("Test %q failed. Actual=%v, Expected=%v", test.name, records, test.expected) + } + + } +} diff --git a/federation/pkg/federation-controller/service/endpoint_helper_test.go b/federation/pkg/federation-controller/service/endpoint_helper_test.go index be8c656231a0..e4e7b390af12 100644 --- a/federation/pkg/federation-controller/service/endpoint_helper_test.go +++ b/federation/pkg/federation-controller/service/endpoint_helper_test.go @@ -29,11 +29,12 @@ var fakeDns, _ = clouddns.NewFakeInterface() // No need to check for unsupported var fakeDnsZones, _ = fakeDns.Zones() var fakeServiceController = ServiceController{ - dns: fakeDns, - dnsZones: fakeDnsZones, - federationName: "fed1", - zoneName: "example.com", - serviceCache: &serviceCache{fedServiceMap: make(map[string]*cachedService)}, + dns: fakeDns, + dnsZones: fakeDnsZones, + federationName: "fed1", + zoneName: "example.com", + serviceDnsSuffix: "federation.example.com", + serviceCache: &serviceCache{fedServiceMap: make(map[string]*cachedService)}, clusterCache: &clusterClientCache{ clientMap: make(map[string]*clusterCache), }, diff --git a/federation/pkg/federation-controller/service/servicecontroller.go b/federation/pkg/federation-controller/service/servicecontroller.go index 4858f370003e..69a33255cb3e 100644 --- a/federation/pkg/federation-controller/service/servicecontroller.go +++ b/federation/pkg/federation-controller/service/servicecontroller.go @@ -103,7 +103,10 @@ type ServiceController struct { dns dnsprovider.Interface federationClient fedclientset.Interface federationName string - zoneName string + // serviceDnsSuffix is the DNS suffix we use when publishing service DNS names + serviceDnsSuffix string + // zoneName is used to identify the zone in which to put records + zoneName string // each federation should be configured with a single zone (e.g. "mycompany.com") dnsZones dnsprovider.Zones serviceCache *serviceCache @@ -136,7 +139,8 @@ type ServiceController struct { // New returns a new service controller to keep DNS provider service resources // (like Kubernetes Services and DNS server records for service discovery) in sync with the registry. -func New(federationClient fedclientset.Interface, dns dnsprovider.Interface, federationName, zoneName string) *ServiceController { +func New(federationClient fedclientset.Interface, dns dnsprovider.Interface, + federationName, serviceDnsSuffix, zoneName string) *ServiceController { broadcaster := record.NewBroadcaster() // federationClient event is not supported yet // broadcaster.StartRecordingToSink(&unversioned_core.EventSinkImpl{Interface: kubeClient.Core().Events("")}) @@ -146,6 +150,7 @@ func New(federationClient fedclientset.Interface, dns dnsprovider.Interface, fed dns: dns, federationClient: federationClient, federationName: federationName, + serviceDnsSuffix: serviceDnsSuffix, zoneName: zoneName, serviceCache: &serviceCache{fedServiceMap: make(map[string]*cachedService)}, clusterCache: &clusterClientCache{ @@ -277,6 +282,13 @@ func (s *ServiceController) init() error { if s.zoneName == "" { return fmt.Errorf("ServiceController should not be run without zoneName.") } + if s.serviceDnsSuffix == "" { + // TODO: Is this the right place to do defaulting? + if s.zoneName == "" { + return fmt.Errorf("ServiceController must be run with zoneName, if serviceDnsSuffix is not set.") + } + s.serviceDnsSuffix = s.zoneName + } if s.dns == nil { return fmt.Errorf("ServiceController should not be run without a dnsprovider.") } diff --git a/hack/verify-flags/known-flags.txt b/hack/verify-flags/known-flags.txt index e73dec1d75b7..0cbbf1da7a81 100644 --- a/hack/verify-flags/known-flags.txt +++ b/hack/verify-flags/known-flags.txt @@ -505,6 +505,7 @@ service-address service-cidr service-cluster-ip-range service-dns-domain +service-dns-suffix service-generator service-node-port-range service-node-ports