From 9e6c45c395b4cda611c6b325aa70c8475d446047 Mon Sep 17 00:00:00 2001 From: Kenneth Shelton Date: Sun, 6 Dec 2015 21:23:56 +0000 Subject: [PATCH] Updated comments Updated documentation Fixed e2e test --- docs/user-guide/services.md | 5 - pkg/api/validation/validation.go | 2 +- .../providers/openstack/openstack.go | 1 + pkg/controller/service/servicecontroller.go | 6 +- test/e2e/cluster_upgrade.go | 32 ----- test/e2e/service.go | 124 +++++++++++------- 6 files changed, 79 insertions(+), 91 deletions(-) diff --git a/docs/user-guide/services.md b/docs/user-guide/services.md index 4a6f86d2531..4596ce382c7 100644 --- a/docs/user-guide/services.md +++ b/docs/user-guide/services.md @@ -418,9 +418,6 @@ Valid values for the `ServiceType` field are: which forwards to the `Service` exposed as a `:NodePort` for each Node. -Note that while `NodePort`s can be TCP or UDP, `LoadBalancer`s only support TCP -as of Kubernetes 1.0. - ### Type NodePort If you set the `type` field to `"NodePort"`, the Kubernetes master will @@ -537,8 +534,6 @@ This makes some kinds of firewalling impossible. The iptables proxier does not obscure in-cluster source IPs, but it does still impact clients coming through a load-balancer or node-port. -LoadBalancers only support TCP, not UDP. - The `Type` field is designed as nested functionality - each level adds to the previous. This is not strictly required on all cloud providers (e.g. Google Compute Engine does not need to allocate a `NodePort` to make `LoadBalancer` work, but AWS does) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index e46ae862bcf..f99733eeacb 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -1458,7 +1458,7 @@ func ValidateService(service *api.Service) field.ErrorList { for i := range service.Spec.Ports { portPath := portsPath.Index(i) if !supportedPortProtocols.Has(string(service.Spec.Ports[i].Protocol)) { - allErrs = append(allErrs, validation.NewInvalidError(portPath.Child("protocol"), service.Spec.Ports[i].Protocol, "cannot create an external load balancer with non-TCP/UDP ports")) + allErrs = append(allErrs, field.Invalid(portPath.Child("protocol"), service.Spec.Ports[i].Protocol, "cannot create an external load balancer with non-TCP/UDP ports")) } } } diff --git a/pkg/cloudprovider/providers/openstack/openstack.go b/pkg/cloudprovider/providers/openstack/openstack.go index 9cc3b96bf64..c94d22376df 100644 --- a/pkg/cloudprovider/providers/openstack/openstack.go +++ b/pkg/cloudprovider/providers/openstack/openstack.go @@ -660,6 +660,7 @@ func (lb *LoadBalancer) EnsureLoadBalancer(name, region string, loadBalancerIP n } // The service controller verified all the protocols match on the ports, just check and use the first one + // TODO: Convert all error messages to use an event recorder if ports[0].Protocol != api.ProtocolTCP { return nil, fmt.Errorf("Only TCP LoadBalancer is supported for openstack load balancers") } diff --git a/pkg/controller/service/servicecontroller.go b/pkg/controller/service/servicecontroller.go index d6f8ceb5490..221645b7d31 100644 --- a/pkg/controller/service/servicecontroller.go +++ b/pkg/controller/service/servicecontroller.go @@ -378,8 +378,9 @@ func (s *ServiceController) createLoadBalancer(service *api.Service) error { return err } name := s.loadBalancerName(service) - // getPortsForLB already verified that the protocol matches for all ports. - // The cloud provider will verify the protocol is supported + // - Only one protocol supported per service + // - Not all cloud providers support all protocols and the next step is expected to return + // an error for unsupported protocols status, err := s.balancer.EnsureLoadBalancer(name, s.zone.Region, net.ParseIP(service.Spec.LoadBalancerIP), ports, hostsFromNodeList(&nodes), service.Spec.SessionAffinity) if err != nil { @@ -494,6 +495,7 @@ func getPortsForLB(service *api.Service) ([]*api.ServicePort, error) { if protocol == "" { protocol = sp.Protocol } else if protocol != sp.Protocol && wantsLoadBalancer(service) { + // TODO: Convert error messages to use event recorder return nil, fmt.Errorf("mixed protocol external load balancers are not supported.") } } diff --git a/test/e2e/cluster_upgrade.go b/test/e2e/cluster_upgrade.go index 870eb29b0ef..af4d0c1c60b 100644 --- a/test/e2e/cluster_upgrade.go +++ b/test/e2e/cluster_upgrade.go @@ -200,38 +200,6 @@ var _ = Describe("Cluster Upgrade [Skipped]", func() { BeforeEach(func() { SkipUnlessProviderIs("gce") }) - // The version is determined once at the beginning of the test so that - // the master and nodes won't be skewed if the value changes during the - // test. - By(fmt.Sprintf("Getting real version for %q", testContext.UpgradeTarget)) - var err error - v, err = realVersion(testContext.UpgradeTarget) - expectNoError(err) - Logf("Version for %q is %q", testContext.UpgradeTarget, v) - - By("Setting up the service, RC, and pods") - f.beforeEach() - w = NewServerTest(f.Client, f.Namespace.Name, svcName) - rc := w.CreateWebserverRC(replicas) - rcName = rc.ObjectMeta.Name - svc := w.BuildServiceSpec() - svc.Spec.Type = api.ServiceTypeLoadBalancer - w.CreateService(svc) - - By("Waiting for the service to become reachable") - result, err := waitForLoadBalancerIngress(f.Client, svcName, f.Namespace.Name) - Expect(err).NotTo(HaveOccurred()) - ingresses := result.Status.LoadBalancer.Ingress - if len(ingresses) != 1 { - Failf("Was expecting only 1 ingress IP but got %d (%v): %v", len(ingresses), ingresses, result) - } - ingress = ingresses[0] - Logf("Got load balancer ingress point %v", ingress) - ip = ingress.IP - if ip == "" { - ip = ingress.Hostname - } - testLoadBalancerReachable(ingress, 80) It("of master should maintain responsive services", func() { By("Validating cluster before master upgrade") diff --git a/test/e2e/service.go b/test/e2e/service.go index ab330eba3ec..cead6421543 100644 --- a/test/e2e/service.go +++ b/test/e2e/service.go @@ -825,7 +825,7 @@ var _ = Describe("Services", func() { svc2 := t2.BuildServiceSpec() svc2.Spec.Type = api.ServiceTypeLoadBalancer svc2.Spec.Ports[0].Port = servicePort - // Let this one be UDP so that we can test that as well without an additional test + // UDP loadbalancing is tested via test NetcatTest svc2.Spec.Ports[0].Protocol = api.ProtocolUDP svc2.Spec.Ports[0].TargetPort = intstr.FromInt(80) svc2.Spec.LoadBalancerIP = loadBalancerIP @@ -1171,13 +1171,14 @@ func testLoadBalancerReachable(ingress api.LoadBalancerIngress, port int) bool { return testLoadBalancerReachableInTime(ingress, port, podStartTimeout) } -func testNetcatLoadBalancerReachable(ingress api.LoadBalancerIngress, port int) { - ip := ingress.IP - if ip == "" { - ip = ingress.Hostname - } +func testNetcatLoadBalancerReachable(ingress api.LoadBalancerIngress, port int) bool { + return testNetcatLoadBalancerReachableInTime(ingress, port, podStartTimeout) +} - testNetcatReachable(ip, port) +func conditionFuncDecorator(ip string, port int, fn func(string, int) (bool, error)) wait.ConditionFunc { + return func() (bool, error) { + return fn(ip, port) + } } func testLoadBalancerReachableInTime(ingress api.LoadBalancerIngress, port int, timeout time.Duration) bool { @@ -1186,7 +1187,17 @@ func testLoadBalancerReachableInTime(ingress api.LoadBalancerIngress, port int, ip = ingress.Hostname } - return testReachableInTime(ip, port, timeout) + return testReachableInTime(conditionFuncDecorator(ip, port, testReachable), timeout) + +} + +func testNetcatLoadBalancerReachableInTime(ingress api.LoadBalancerIngress, port int, timeout time.Duration) bool { + ip := ingress.IP + if ip == "" { + ip = ingress.Hostname + } + + return testReachableInTime(conditionFuncDecorator(ip, port, testNetcatReachable), timeout) } func testLoadBalancerNotReachable(ingress api.LoadBalancerIngress, port int) { @@ -1198,72 +1209,83 @@ func testLoadBalancerNotReachable(ingress api.LoadBalancerIngress, port int) { testNotReachable(ip, port) } -func testReachable(ip string, port int) bool { - return testReachableInTime(ip, port, podStartTimeout) +func testReachable(ip string, port int) (bool, error) { + url := fmt.Sprintf("http://%s:%d", ip, port) + if ip == "" { + Failf("Got empty IP for reachability check (%s)", url) + return false, nil + } + if port == 0 { + Failf("Got port==0 for reachability check (%s)", url) + return false, nil + } + + Logf("Testing reachability of %v", url) + + resp, err := httpGetNoConnectionPool(url) + if err != nil { + Logf("Got error waiting for reachability of %s: %v", url, err) + return false, nil + } + defer resp.Body.Close() + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + Logf("Got error reading response from %s: %v", url, err) + return false, nil + } + if resp.StatusCode != 200 { + return false, fmt.Errorf("received non-success return status %q trying to access %s; got body: %s", resp.Status, url, string(body)) + } + if !strings.Contains(string(body), "test-webserver") { + return false, fmt.Errorf("received response body without expected substring 'test-webserver': %s", string(body)) + } + Logf("Successfully reached %v", url) + return true, nil } -func testNetcatReachable(ip string, port int) { +func testNetcatReachable(ip string, port int) (bool, error) { + uri := fmt.Sprintf("udp://%s:%d", ip, port) + if ip == "" { + Failf("Got empty IP for reachability check (%s)", uri) + return false, nil + } + if port == 0 { + Failf("Got port==0 for reachability check (%s)", uri) + return false, nil + } + + Logf("Testing reachability of %v", uri) + con, err := net.Dial("udp", ip+":"+string(port)) if err != nil { - Failf("Failed to connect to: %s:%d (%s)", ip, port, err.Error()) + return false, fmt.Errorf("Failed to connect to: %s:%d (%s)", ip, port, err.Error()) } _, err = con.Write([]byte("\n")) if err != nil { - Failf("Failed to send newline: %s", err.Error()) + return false, fmt.Errorf("Failed to send newline: %s", err.Error()) } var buf []byte = make([]byte, len("SUCCESS")+1) _, err = con.Read(buf) if err != nil { - Failf("Failed to read result: %s", err.Error()) + return false, fmt.Errorf("Failed to read result: %s", err.Error()) } if !strings.HasPrefix(string(buf), "SUCCESS") { - Failf("Failed to retrieve: \"SUCCESS\"") + return false, fmt.Errorf("Failed to retrieve: \"SUCCESS\"") } Logf("Successfully retrieved \"SUCCESS\"") + return true, nil } -func testReachableInTime(ip string, port int, timeout time.Duration) bool { - url := fmt.Sprintf("http://%s:%d", ip, port) - if ip == "" { - Failf("Got empty IP for reachability check (%s)", url) - return false - } - if port == 0 { - Failf("Got port==0 for reachability check (%s)", url) - return false - } - - desc := fmt.Sprintf("the url %s to be reachable", url) - By(fmt.Sprintf("Waiting up to %v for %s", timeout, desc)) - start := time.Now() - err := wait.PollImmediate(poll, timeout, func() (bool, error) { - resp, err := httpGetNoConnectionPool(url) - if err != nil { - Logf("Got error waiting for reachability of %s: %v (%v)", url, err, time.Since(start)) - return false, nil - } - defer resp.Body.Close() - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - Logf("Got error reading response from %s: %v", url, err) - return false, nil - } - if resp.StatusCode != 200 { - return false, fmt.Errorf("received non-success return status %q trying to access %s; got body: %s", resp.Status, url, string(body)) - } - if !strings.Contains(string(body), "test-webserver") { - return false, fmt.Errorf("received response body without expected substring 'test-webserver': %s", string(body)) - } - Logf("Successfully reached %v", url) - return true, nil - }) +func testReachableInTime(testFunc wait.ConditionFunc, timeout time.Duration) bool { + By(fmt.Sprintf("Waiting up to %v", timeout)) + err := wait.PollImmediate(poll, timeout, testFunc) if err != nil { - Expect(err).NotTo(HaveOccurred(), "Error waiting for %s", desc) + Expect(err).NotTo(HaveOccurred(), "Error waiting") return false } return true