Changes based on suggestions

* removes golang/protobuf from Gopkg.toml * alias h2c package as h2cutil * adds a comment for NewTransport * removes refactoring changes (will submit them as a separate pr) * updates sample app's readme to use buildtag when running client code
knative · Jun 18, 2018 · e7c04b2 · e7c04b2
1 parent 97a3904
commit e7c04b2
Show file tree

Hide file tree

Showing 10 changed files with 47 additions and 113 deletions.
diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/Gopkg.toml b/Gopkg.toml
@@ -44,10 +44,6 @@ required = [
   name = "gopkg.in/yaml.v2"
   version = "v2.2.1"
 
-[[override]]
-  name = "github.com/golang/protobuf"
-  version = "v1.1.0"
-
 [[constraint]]
   branch = "master"
   name = "github.com/golang/glog"

diff --git a/cmd/queue/main.go b/cmd/queue/main.go
@@ -36,7 +36,7 @@ import (
 	"github.com/knative/serving/pkg"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/autoscaler"
-	h2 "github.com/knative/serving/pkg/h2c"
+	h2cutil "github.com/knative/serving/pkg/h2c"
 	"github.com/knative/serving/pkg/logging"
 	"github.com/knative/serving/pkg/logging/logkey"
 	"github.com/knative/serving/pkg/queue"
@@ -271,7 +271,7 @@ func main() {
 
 	httpProxy = httputil.NewSingleHostReverseProxy(target)
 	h2cProxy = httputil.NewSingleHostReverseProxy(target)
-	h2cProxy.Transport = h2.NewTransport()
+	h2cProxy.Transport = h2cutil.NewTransport()
 
 	logger.Infof("Queue container is starting, concurrencyModel: %s", *concurrencyModel)
 	config, err := rest.InClusterConfig()

diff --git a/config/400-activator-service.yaml b/config/400-activator-service.yaml
@@ -35,10 +35,10 @@ metadata:
   name: activator-grpc-service
   namespace: knative-serving-system
   labels:
-    app: ela-activator
+    app: activator
 spec:
   selector:
-    app: ela-activator
+    app: activator
   ports:
   - name: grpc
     protocol: TCP

diff --git a/pkg/controller/revision/queue.go b/pkg/controller/revision/queue.go
@@ -104,10 +104,6 @@ func MakeElaQueueContainer(rev *v1alpha1.Revision, controllerConfig *ControllerC
 				Name:  "ELA_AUTOSCALER_PORT",
 				Value: strconv.Itoa(autoscalerPort),
 			},
-			{
-				Name:  "ELA_PROTOCOL",
-				Value: string(rev.Spec.Protocol),
-			},
 			{
 				Name: "ELA_POD",
 				ValueFrom: &corev1.EnvVarSource{

diff --git a/pkg/controller/revision/revision_test.go b/pkg/controller/revision/revision_test.go
@@ -305,41 +305,6 @@ func newRunningTestController(t *testing.T, elaObjects ...runtime.Object) (
 	return
 }
 
-// This function is used to verify pass through of container environment
-// variables.
-func checkEnv(env []corev1.EnvVar, name, value, fieldPath string) error {
-	nameFound := false
-	for _, e := range env {
-		if e.Name == name {
-			nameFound = true
-			if value != "" && e.Value != value {
-				return fmt.Errorf("Incorrect environment variable %s. Expected value %s. Got %s.", name, value, e.Value)
-			}
-			if fieldPath != "" {
-				if vf := e.ValueFrom; vf == nil {
-					return fmt.Errorf("Incorrect environment variable %s. Missing value source.", name)
-				} else if fr := vf.FieldRef; fr == nil {
-					return fmt.Errorf("Incorrect environment variable %s. Missing field ref.", name)
-				} else if fr.FieldPath != fieldPath {
-					return fmt.Errorf("Incorrect environment variable %s. Expected field path %s. Got %s.",
-						name, fr.FieldPath, fieldPath)
-				}
-			}
-		}
-	}
-	if !nameFound {
-		return fmt.Errorf("Missing environment variable %s", name)
-	}
-	return nil
-}
-
-func compareRevisionConditions(want []v1alpha1.RevisionCondition, got []v1alpha1.RevisionCondition) string {
-	for i := range got {
-		got[i].LastTransitionTime = metav1.NewTime(time.Time{})
-	}
-	return cmp.Diff(want, got)
-}
-
 func createRevision(elaClient *fakeclientset.Clientset, elaInformer informers.SharedInformerFactory, controller *Controller, rev *v1alpha1.Revision) {
 	elaClient.ServingV1alpha1().Revisions(rev.Namespace).Create(rev)
 	// Since syncHandler looks in the lister, we need to add it to the informer
@@ -383,6 +348,33 @@ func TestCreateRevCreatesStuff(t *testing.T) {
 
 	createRevision(elaClient, elaInformer, controller, rev)
 
+	// This function is used to verify pass through of container environment
+	// variables.
+	checkEnv := func(env []corev1.EnvVar, name, value, fieldPath string) {
+		nameFound := false
+		for _, e := range env {
+			if e.Name == name {
+				nameFound = true
+				if value != "" && e.Value != value {
+					t.Errorf("Incorrect environment variable %s. Expected value %s. Got %s.", name, value, e.Value)
+				}
+				if fieldPath != "" {
+					if vf := e.ValueFrom; vf == nil {
+						t.Errorf("Incorrect environment variable %s. Missing value source.", name)
+					} else if fr := vf.FieldRef; fr == nil {
+						t.Errorf("Incorrect environment variable %s. Missing field ref.", name)
+					} else if fr.FieldPath != fieldPath {
+						t.Errorf("Incorrect environment variable %s. Expected field path %s. Got %s.",
+							name, fr.FieldPath, fieldPath)
+					}
+				}
+			}
+		}
+		if !nameFound {
+			t.Errorf("Missing environment variable %s", name)
+		}
+	}
+
 	// Look for the revision deployment.
 	expectedDeploymentName := fmt.Sprintf("%s-deployment", rev.Name)
 	deployment, err := kubeClient.AppsV1().Deployments(testNamespace).Get(expectedDeploymentName, metav1.GetOptions{})
@@ -410,30 +402,14 @@ func TestCreateRevCreatesStuff(t *testing.T) {
 		}
 		if container.Name == "queue-proxy" {
 			foundQueueProxy = true
-			if err := checkEnv(container.Env, "ELA_NAMESPACE", testNamespace, ""); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_REVISION", "test-rev", ""); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_POD", "", "metadata.name"); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_PROTOCOL", "http", ""); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_AUTOSCALER", ctrl.GetRevisionAutoscalerName(rev), ""); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_AUTOSCALER_PORT", strconv.Itoa(autoscalerPort), ""); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_LOGGING_CONFIG", controllerConfig.QueueProxyLoggingConfig, ""); err != nil {
-				t.Error(err)
-			}
-			if err := checkEnv(container.Env, "ELA_LOGGING_LEVEL", controllerConfig.QueueProxyLoggingLevel, ""); err != nil {
-				t.Error(err)
-			}
+			checkEnv(container.Env, "ELA_NAMESPACE", testNamespace, "")
+			checkEnv(container.Env, "ELA_CONFIGURATION", config.Name, "")
+			checkEnv(container.Env, "ELA_REVISION", rev.Name, "")
+			checkEnv(container.Env, "ELA_POD", "", "metadata.name")
+			checkEnv(container.Env, "ELA_AUTOSCALER", ctrl.GetRevisionAutoscalerName(rev), "")
+			checkEnv(container.Env, "ELA_AUTOSCALER_PORT", strconv.Itoa(autoscalerPort), "")
+			checkEnv(container.Env, "ELA_LOGGING_CONFIG", controllerConfig.QueueProxyLoggingConfig, "")
+			checkEnv(container.Env, "ELA_LOGGING_LEVEL", controllerConfig.QueueProxyLoggingLevel, "")
 			if diff := cmp.Diff(expectedPreStop, container.Lifecycle.PreStop); diff != "" {
 				t.Errorf("Unexpected PreStop diff in container %q (-want +got): %v", container.Name, diff)
 			}

diff --git a/pkg/h2c/transport.go b/pkg/h2c/transport.go
@@ -8,6 +8,9 @@ import (
 	"golang.org/x/net/http2"
 )
 
+// NewTransport will reroute all https traffic to http. This is
+// to explicitly allow h2c (http2 without TLS) transport.
+// See https://github.com/golang/go/issues/14141 for more details.
 func NewTransport() http.RoundTripper {
 	return &http2.Transport{
 		AllowHTTP: true,

diff --git a/sample/grpc-ping/README.md b/sample/grpc-ping/README.md
@@ -45,5 +45,5 @@ export SERVICE_IP=`kubectl get ingress grpc-ping-ela-ingress -o jsonpath="{.stat
 1. Use the client to send message streams to the gRPC server
 
 ```
-go run client/client.go -server_addr="$SERVICE_IP:80" -server_host_override="$SERVICE_HOST"
+go run -tags=grpcping client/client.go -server_addr="$SERVICE_IP:80" -server_host_override="$SERVICE_HOST"
 ```
diff --git a/sample/grpc-ping/sample.yaml b/sample/grpc-ping/sample.yaml
@@ -22,4 +22,4 @@ spec:
     spec:
       protocol: grpc
       container:
-        image: gcr.io/cf-elafros-dog/sample/grpc-ping
+        image: github.com/knative/serving/sample/grpc-ping
diff --git a/test/request.go b/test/request.go
@@ -32,8 +32,8 @@ import (
 )
 
 const (
-	RequestInterval = 1 * time.Second
-	RequestTimeout  = 1 * time.Minute
+	requestInterval = 1 * time.Second
+	requestTimeout  = 1 * time.Minute
 )
 
 func waitForRequestToDomainState(address string, spoofDomain string, retryableCodes []int, inState func(body string) (bool, error)) error {
@@ -48,7 +48,7 @@ func waitForRequestToDomainState(address string, spoofDomain string, retryableCo
 	}
 
 	var body []byte
-	err = wait.PollImmediate(RequestInterval, RequestTimeout, func() (bool, error) {
+	err = wait.PollImmediate(requestInterval, requestTimeout, func() (bool, error) {
 		resp, err := h.Do(req)
 		if err != nil {
 			return true, err