Merge pull request #16975 from liggitt/service_resource_location

pkg/registry/service/rest.go

@@ -284,6 +284,26 @@ func (rs *REST) ResourceLocation(ctx api.Context, id string) (*url.URL, http.Rou
 		return nil, nil, errors.NewBadRequest(fmt.Sprintf("invalid service request %q", id))
 	}
 
+	// If a port *number* was specified, find the corresponding service port name
+	if portNum, err := strconv.ParseInt(portStr, 10, 64); err == nil {
+		svc, err := rs.registry.GetService(ctx, svcName)
+		if err != nil {
+			return nil, nil, err
+		}
+		found := false
+		for _, svcPort := range svc.Spec.Ports {
+			if svcPort.Port == int(portNum) {
+				// use the declared port's name
+				portStr = svcPort.Name
+				found = true
+				break
+			}
+		}
+		if !found {
+			return nil, nil, errors.NewServiceUnavailable(fmt.Sprintf("no service port %d found for service %q", portNum, svcName))
+		}
+	}
+
 	eps, err := rs.endpoints.GetEndpoints(ctx, svcName)
 	if err != nil {
 		return nil, nil, err
@@ -308,15 +328,6 @@ func (rs *REST) ResourceLocation(ctx api.Context, id string) (*url.URL, http.Rou
 					Scheme: svcScheme,
 					Host:   net.JoinHostPort(ip, strconv.Itoa(port)),
 				}, rs.proxyTransport, nil
-			} else {
-				port, err := strconv.ParseInt(portStr, 10, 64)
-				if err == nil && int(port) == ss.Ports[i].Port {
-					ip := ss.Addresses[rand.Intn(len(ss.Addresses))].IP
-					return &url.URL{
-						Scheme: svcScheme,
-						Host:   net.JoinHostPort(ip, portStr),
-					}, rs.proxyTransport, nil
-				}
 			}
 		}
 	}

pkg/registry/service/rest_test.go

@@ -462,6 +462,14 @@ func TestServiceRegistryResourceLocation(t *testing.T) {
 		ObjectMeta: api.ObjectMeta{Name: "foo"},
 		Spec: api.ServiceSpec{
 			Selector: map[string]string{"bar": "baz"},
+			Ports: []api.ServicePort{
+				// Service port 9393 should route to endpoint port "p", which is port 93
+				{Name: "p", Port: 9393, TargetPort: intstr.FromString("p")},
+
+				// Service port 93 should route to unnamed endpoint port, which is port 80
+				// This is to test that the service port definition is used when determining resource location
+				{Name: "", Port: 93, TargetPort: intstr.FromInt(80)},
+			},
 		},
 	})
 	redirector := rest.Redirector(storage)
@@ -490,7 +498,7 @@ func TestServiceRegistryResourceLocation(t *testing.T) {
 		t.Errorf("Expected %v, but got %v", e, a)
 	}
 
-	// Test a name + port number.
+	// Test a name + port number (service port 93 -> target port 80)
 	location, _, err = redirector.ResourceLocation(ctx, "foo:93")
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
@@ -498,6 +506,18 @@ func TestServiceRegistryResourceLocation(t *testing.T) {
 	if location == nil {
 		t.Errorf("Unexpected nil: %v", location)
 	}
+	if e, a := "//1.2.3.4:80", location.String(); e != a {
+		t.Errorf("Expected %v, but got %v", e, a)
+	}
+
+	// Test a name + port number (service port 9393 -> target port "p" -> endpoint port 93)
+	location, _, err = redirector.ResourceLocation(ctx, "foo:9393")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	if location == nil {
+		t.Errorf("Unexpected nil: %v", location)
+	}
 	if e, a := "//1.2.3.4:93", location.String(); e != a {
 		t.Errorf("Expected %v, but got %v", e, a)
 	}

test/e2e/proxy.go

@@ -145,13 +145,19 @@ func proxyContext(version string) {
 		}
 		expectations := map[string]string{
 			svcProxyURL("", "portname1") + "/": "foo",
+			svcProxyURL("", "80") + "/":        "foo",
 			svcProxyURL("", "portname2") + "/": "bar",
+			svcProxyURL("", "81") + "/":        "bar",
 
 			svcProxyURL("http", "portname1") + "/": "foo",
+			svcProxyURL("http", "80") + "/":        "foo",
 			svcProxyURL("http", "portname2") + "/": "bar",
+			svcProxyURL("http", "81") + "/":        "bar",
 
 			svcProxyURL("https", "tlsportname1") + "/": "tls baz",
+			svcProxyURL("https", "443") + "/":          "tls baz",
 			svcProxyURL("https", "tlsportname2") + "/": "tls qux",
+			svcProxyURL("https", "444") + "/":          "tls qux",
 
 			podProxyURL("", "80") + "/":  `<a href="` + podProxyURL("", "80") + `/rewriteme">test</a>`,
 			podProxyURL("", "160") + "/": "foo",
@@ -172,9 +178,8 @@ func proxyContext(version string) {
 			subresourcePodProxyURL("https", "443") + "/": `<a href="` + subresourcePodProxyURL("https", "443") + `/tlsrewriteme">test</a>`,
 			subresourcePodProxyURL("https", "460") + "/": "tls baz",
 			subresourcePodProxyURL("https", "462") + "/": "tls qux",
+
 			// TODO: below entries don't work, but I believe we should make them work.
-			// svcPrefix + ":80": "foo",
-			// svcPrefix + ":81": "bar",
 			// podPrefix + ":dest1": "foo",
 			// podPrefix + ":dest2": "bar",
 		}
@@ -231,6 +236,8 @@ func doProxy(f *Framework, path string) (body []byte, statusCode int, d time.Dur
 	d = time.Since(start)
 	if len(body) > 0 {
 		Logf("%v: %s (%v; %v)", path, truncate(body, maxDisplayBodyLen), statusCode, d)
+	} else {
+		Logf("%v: %s (%v; %v)", path, "no body", statusCode, d)
 	}
 	return
 }