add port name (#915)

* add port name

* add port name

* review comments

* review comments

* add unit tests for port changes

* review comments for port spec changes

Author: itsmurugappan

docs/cmd/kn_service_create.md

@@ -27,8 +27,11 @@ kn service create NAME --image IMAGE
   # Create or replace environment variables of service 's1' using --force flag
   kn service create --force s1 --env TARGET=force --env FROM=examples --image knativesamples/helloworld
 
-  # Create a service with port 80
-  kn service create s2 --port 80 --image knativesamples/helloworld
+  # Create a service with port 8080
+  kn service create s2 --port 8080 --image knativesamples/helloworld
+
+  # Create a service with port 8080 and port name h2c
+  kn service create s2 --port h2c:8080 --image knativesamples/helloworld
 
   # Create or replace default resources of a service 's1' using --force flag
   # (earlier configured resource requests and limits will be replaced with default)
@@ -78,7 +81,7 @@ kn service create NAME --image IMAGE
       --no-cluster-local              Do not specify that the service be private. (--no-cluster-local will make the service publicly available) (default true)
       --no-lock-to-digest             Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                       Do not wait for 'service create' operation to be completed.
-  -p, --port int32                    The port where application listens on.
+  -p, --port string                   The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string            Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings               The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --requests-cpu string           DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).

docs/cmd/kn_service_update.md

@@ -65,7 +65,7 @@ kn service update NAME
       --no-cluster-local              Do not specify that the service be private. (--no-cluster-local will make the service publicly available) (default true)
       --no-lock-to-digest             Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                       Do not wait for 'service update' operation to be completed.
-  -p, --port int32                    The port where application listens on.
+  -p, --port string                   The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string            Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings               The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --requests-cpu string           DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).

pkg/kn/commands/service/configuration_edit_flags.go

@@ -51,7 +51,7 @@ type ConfigurationEditFlags struct {
 	ConcurrencyLimit           int
 	ConcurrencyUtilization     int
 	AutoscaleWindow            string
-	Port                       int32
+	Port                       string
 	Labels                     []string
 	LabelsService              []string
 	LabelsRevision             []string
@@ -207,7 +207,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 		"Percentage of concurrent requests utilization before scaling up.")
 	p.markFlagMakesRevision("concurrency-utilization")
 
-	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
+	command.Flags().StringVarP(&p.Port, "port", "p", "", "The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.")
 	p.markFlagMakesRevision("port")
 
 	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},

pkg/kn/commands/service/create.go

@@ -47,8 +47,11 @@ var create_example = `
   # Create or replace environment variables of service 's1' using --force flag
   kn service create --force s1 --env TARGET=force --env FROM=examples --image knativesamples/helloworld
 
-  # Create a service with port 80
-  kn service create s2 --port 80 --image knativesamples/helloworld
+  # Create a service with port 8080
+  kn service create s2 --port 8080 --image knativesamples/helloworld
+
+  # Create a service with port 8080 and port name h2c
+  kn service create s2 --port h2c:8080 --image knativesamples/helloworld
 
   # Create or replace default resources of a service 's1' using --force flag
   # (earlier configured resource requests and limits will be replaced with default)

pkg/serving/config_changes.go

@@ -40,6 +40,12 @@ type VolumeSourceType int
 const (
 	ConfigMapVolumeSourceType VolumeSourceType = iota
 	SecretVolumeSourceType
+	PortFormatErr = "The port specification '%s' is not valid. Please provide in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'."
+)
+
+var (
+	UserImageAnnotationKey = "client.knative.dev/user-image"
+	ApiTooOldError         = errors.New("the service is using too old of an API format for the operation")
 )
 
 func (vt VolumeSourceType) String() string {
@@ -50,8 +56,6 @@ func (vt VolumeSourceType) String() string {
 	return names[vt]
 }
 
-var UserImageAnnotationKey = "client.knative.dev/user-image"
-
 // UpdateEnvVars gives the configuration all the env var values listed in the given map of
 // vars.  Does not touch any environment variables not mentioned, but it can add
 // new env vars and change the values of existing ones, then sort by env key name.
@@ -236,8 +240,6 @@ func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec,
 	return nil
 }
 
-var ApiTooOldError = errors.New("the service is using too old of an API format for the operation")
-
 // EnvToMap is an utility function to translate between the API list form of env vars, and the
 // more convenient map form.
 func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) {
@@ -331,14 +333,34 @@ func UpdateContainerArg(template *servingv1.RevisionTemplateSpec, arg []string)
 	return nil
 }
 
-// UpdateContainerPort updates container with a give port
-func UpdateContainerPort(template *servingv1.RevisionTemplateSpec, port int32) error {
+// UpdateContainerPort updates container with a given name:port
+func UpdateContainerPort(template *servingv1.RevisionTemplateSpec, port string) error {
 	container, err := ContainerOfRevisionTemplate(template)
 	if err != nil {
 		return err
 	}
+
+	var containerPort int64
+	var name string
+
+	elements := strings.SplitN(port, ":", 2)
+	if len(elements) == 2 {
+		name = elements[0]
+		containerPort, err = strconv.ParseInt(elements[1], 10, 32)
+		if err != nil {
+			return fmt.Errorf(PortFormatErr, port)
+		}
+	} else {
+		name = ""
+		containerPort, err = strconv.ParseInt(elements[0], 10, 32)
+		if err != nil {
+			return fmt.Errorf(PortFormatErr, port)
+		}
+	}
+
 	container.Ports = []corev1.ContainerPort{{
-		ContainerPort: port,
+		ContainerPort: int32(containerPort),
+		Name:          name,
 	}}
 	return nil
 }

pkg/serving/config_changes_test.go

@@ -15,6 +15,7 @@
 package serving
 
 import (
+	"fmt"
 	"reflect"
 	"strconv"
 	"testing"
@@ -285,21 +286,52 @@ func TestUpdateContainerArg(t *testing.T) {
 
 func TestUpdateContainerPort(t *testing.T) {
 	template, _ := getRevisionTemplate()
-	err := UpdateContainerPort(template, 8888)
-	assert.NilError(t, err)
-	// Verify update is successful or not
-	checkPortUpdate(t, template, 8888)
-	// update template with container port info
-	template.Spec.Containers[0].Ports[0].ContainerPort = 9090
-	err = UpdateContainerPort(template, 80)
-	assert.NilError(t, err)
-	// Verify that given port overrides the existing container port
-	checkPortUpdate(t, template, 80)
-}
-
-func checkPortUpdate(t *testing.T, template *servingv1.RevisionTemplateSpec, port int32) {
-	if template.Spec.Containers[0].Ports[0].ContainerPort != port {
-		t.Error("Failed to update the container port")
+	for _, tc := range []struct {
+		name    string
+		input   string
+		isErr   bool
+		expPort int32
+		expName string
+	}{{
+		name:    "only port 8888",
+		input:   "8888",
+		expPort: int32(8888),
+	}, {
+		name:    "name and port h2c:8080",
+		input:   "h2c:8080",
+		expPort: int32(8080),
+		expName: "h2c",
+	}, {
+		name:  "error case - not correct format",
+		input: "h2c:800000000000000000",
+		isErr: true,
+	}, {
+		name:  "error case - empty port",
+		input: "h2c:",
+		isErr: true,
+	}, {
+		name:  "error case - wrong format",
+		input: "8080:h2c",
+		isErr: true,
+	}, {
+		name:  "error case - multiple :",
+		input: "h2c:8080:proto",
+		isErr: true,
+	}, {
+		name:    "empty name no error",
+		input:   ":8888",
+		expPort: int32(8888),
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := UpdateContainerPort(template, tc.input)
+			if tc.isErr {
+				assert.Error(t, err, fmt.Sprintf(PortFormatErr, tc.input))
+			} else {
+				assert.NilError(t, err)
+				assert.Equal(t, template.Spec.Containers[0].Ports[0].ContainerPort, tc.expPort)
+				assert.Equal(t, template.Spec.Containers[0].Ports[0].Name, tc.expName)
+			}
+		})
 	}
 }