fixes(#606): adds --cluster-local / --no-cluster-local flags (#629)

When specified on 'service create' the '--cluster-local' flag will
make the created service 'private' by setting its config visibility
to 'cluster-local'. This is done with label:

serving.knative.dev/visibility: cluster-local

The --no-cluster-local will remove the label.

CHANGELOG.adoc

@@ -26,6 +26,10 @@
 | Add `kn source list`
 | https://github.com/knative/client/pull/666[#666]
 
+| 🎁
+| Add --cluster-local and --no-cluster-local flags
+| https://github.com/knative/client/pull/629[#629]
+
 | 🎁
 | Add JSON/YAML output format for version command
 | https://github.com/knative/client/pull/709[#709]

docs/cmd/kn_service_create.md

@@ -37,6 +37,9 @@ kn service create NAME --image IMAGE [flags]
 
   # Create a service with annotation
   kn service create s1 --image dev.local/ns/image:v3 --annotation sidecar.istio.io/inject=false
+
+  # Create a private service (that is a service with no external endpoint)
+  kn service create s1 --image dev.local/ns/image:v3 --cluster-local
 ```
 
 ### Options
@@ -46,6 +49,7 @@ kn service create NAME --image IMAGE [flags]
       --arg stringArray              Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.
       --async                        DEPRECATED: please use --no-wait instead. Create service and don't wait for it to be ready.
       --autoscale-window string      Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
+      --cluster-local                Specify that the service be private. (--no-cluster-local will make the service publicly available
       --cmd string                   Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments.
       --concurrency-limit int        Hard Limit of concurrent requests to be processed by a single replica.
       --concurrency-target int       Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
@@ -64,6 +68,7 @@ kn service create NAME --image IMAGE [flags]
       --min-scale int                Minimal number of replicas.
       --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -n, --namespace string             Specify the namespace to operate in.
+      --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                      Create service and don't wait for it to be ready.
   -p, --port int32                   The port where application listens on.

docs/cmd/kn_service_update.md

@@ -42,6 +42,7 @@ kn service update NAME [flags]
       --arg stringArray              Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.
       --async                        DEPRECATED: please use --no-wait instead. Update service and don't wait for it to be ready.
       --autoscale-window string      Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
+      --cluster-local                Specify that the service be private. (--no-cluster-local will make the service publicly available
       --cmd string                   Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments.
       --concurrency-limit int        Hard Limit of concurrent requests to be processed by a single replica.
       --concurrency-target int       Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
@@ -59,6 +60,7 @@ kn service update NAME [flags]
       --min-scale int                Minimal number of replicas.
       --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -n, --namespace string             Specify the namespace to operate in.
+      --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                      Update service and don't wait for it to be ready.
   -p, --port int32                   The port where application listens on.

pkg/kn/commands/service/configuration_edit_flags.go

@@ -28,6 +28,7 @@ import (
 	"knative.dev/client/pkg/kn/flags"
 	servinglib "knative.dev/client/pkg/serving"
 	"knative.dev/client/pkg/util"
+	"knative.dev/serving/pkg/apis/serving"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
@@ -57,6 +58,7 @@ type ConfigurationEditFlags struct {
 	ServiceAccountName         string
 	ImagePullSecrets           string
 	Annotations                []string
+	ClusterLocal               bool
 	User                       int64
 
 	// Preferences about how to do the action.
@@ -134,6 +136,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 		"Specify command to be used as entrypoint instead of default one. "+
 			"Example: --cmd /app/start or --cmd /app/start --arg myArg to pass aditional arguments.")
 	p.markFlagMakesRevision("cmd")
+
 	command.Flags().StringArrayVarP(&p.Arg, "arg", "", []string{},
 		"Add argument to the container command. "+
 			"Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. "+
@@ -142,33 +145,50 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 
 	command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).")
 	p.markFlagMakesRevision("requests-cpu")
+
 	command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).")
 	p.markFlagMakesRevision("requests-memory")
+
 	command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).")
 	p.markFlagMakesRevision("limits-cpu")
+
 	command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "",
 		"The limits on the requested memory (e.g., 1024Mi).")
 	p.markFlagMakesRevision("limits-memory")
+
 	command.Flags().IntVar(&p.MinScale, "min-scale", 0, "Minimal number of replicas.")
 	p.markFlagMakesRevision("min-scale")
+
 	command.Flags().IntVar(&p.MaxScale, "max-scale", 0, "Maximal number of replicas.")
 	p.markFlagMakesRevision("max-scale")
+
 	command.Flags().StringVar(&p.AutoscaleWindow, "autoscale-window", "", "Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)")
 	p.markFlagMakesRevision("autoscale-window")
+
+	flags.AddBothBoolFlagsUnhidden(command.Flags(), &p.ClusterLocal, "cluster-local", "", false,
+		"Specify that the service be private. (--no-cluster-local will make the service publicly available")
+	//TODO: Need to also not change revision when already set (solution to issue #646)
+	p.markFlagMakesRevision("cluster-local")
+	p.markFlagMakesRevision("no-cluster-local")
+
 	command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0,
 		"Recommendation for when to scale up based on the concurrent number of incoming request. "+
 			"Defaults to --concurrency-limit when given.")
 	p.markFlagMakesRevision("concurrency-target")
+
 	command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0,
 		"Hard Limit of concurrent requests to be processed by a single replica.")
 	p.markFlagMakesRevision("concurrency-limit")
+
 	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
 	p.markFlagMakesRevision("port")
+
 	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},
 		"Labels to set for both Service and Revision. name=value; you may provide this flag "+
 			"any number of times to set multiple labels. "+
 			"To unset, specify the label name followed by a \"-\" (e.g., name-).")
 	p.markFlagMakesRevision("label")
+
 	command.Flags().StringArrayVarP(&p.LabelsService, "label-service", "", []string{},
 		"Service label to set. name=value; you may provide this flag "+
 			"any number of times to set multiple labels. "+
@@ -181,6 +201,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 			"To unset, specify the label name followed by a \"-\" (e.g., name-). This flag takes "+
 			"precedence over \"label\" flag.")
 	p.markFlagMakesRevision("label-revision")
+
 	command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}",
 		"The revision name to set. Must start with the service name and a dash as a prefix. "+
 			"Empty revision name will result in the server generating a name for the revision. "+
@@ -192,16 +213,19 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 		"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)")
 	// Don't mark as changing the revision.
+
 	command.Flags().StringVar(&p.ServiceAccountName,
 		"service-account",
 		"",
 		"Service account name to set. An empty argument (\"\") clears the service account. The referenced service account must exist in the service's namespace.")
 	p.markFlagMakesRevision("service-account")
+
 	command.Flags().StringArrayVar(&p.Annotations, "annotation", []string{},
 		"Service annotation to set. name=value; you may provide this flag "+
 			"any number of times to set multiple annotations. "+
 			"To unset, specify the annotation name followed by a \"-\" (e.g., name-).")
 	p.markFlagMakesRevision("annotation")
+
 	command.Flags().StringVar(&p.ImagePullSecrets,
 		"pull-secret",
 		"",
@@ -288,6 +312,7 @@ func (p *ConfigurationEditFlags) Apply(
 	if p.AnyMutation(cmd) {
 		template.Name = name
 	}
+
 	imageSet := false
 	if cmd.Flags().Changed("image") {
 		err = servinglib.UpdateImage(template, p.Image.String())
@@ -379,6 +404,16 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}
 
+	if cmd.Flags().Changed("cluster-local") || cmd.Flags().Changed("no-cluster-local") {
+		if p.ClusterLocal {
+			labels := servinglib.UpdateLabels(service.ObjectMeta.Labels, map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}, []string{})
+			service.ObjectMeta.Labels = labels // In case service.ObjectMeta.Labels was nil
+		} else {
+			labels := servinglib.UpdateLabels(service.ObjectMeta.Labels, map[string]string{}, []string{serving.VisibilityLabelKey})
+			service.ObjectMeta.Labels = labels // In case service.ObjectMeta.Labels was nil
+		}
+	}
+
 	if cmd.Flags().Changed("label") || cmd.Flags().Changed("label-service") || cmd.Flags().Changed("label-revision") {
 		labelsAllMap, err := util.MapFromArrayAllowingSingles(p.Labels, "=")
 		if err != nil {

pkg/kn/commands/service/create.go

@@ -56,7 +56,10 @@ var create_example = `
   kn service create --force s1 --image dev.local/ns/image:v1
 
   # Create a service with annotation
-  kn service create s1 --image dev.local/ns/image:v3 --annotation sidecar.istio.io/inject=false`
+  kn service create s1 --image dev.local/ns/image:v3 --annotation sidecar.istio.io/inject=false
+
+  # Create a private service (that is a service with no external endpoint)
+  kn service create s1 --image dev.local/ns/image:v3 --cluster-local`
 
 func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 	var editFlags ConfigurationEditFlags

pkg/kn/commands/service/create_test.go

@@ -22,7 +22,6 @@ import (
 	"testing"
 
 	"gotest.tools/assert"
-
 	api_errors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/watch"
@@ -36,6 +35,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/runtime"
 	clienttesting "k8s.io/client-go/testing"
+	"knative.dev/serving/pkg/apis/serving"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
@@ -379,9 +379,6 @@ func TestServiceCreateMaxMinScale(t *testing.T) {
 	}
 
 	template := &created.Spec.Template
-	if err != nil {
-		t.Fatal(err)
-	}
 
 	actualAnnos := template.Annotations
 	expectedAnnos := []string{
@@ -545,3 +542,24 @@ func TestServiceCreateWithServiceAccountName(t *testing.T) {
 		t.Fatalf("wrong service account name:%v", template.Spec.ServiceAccountName)
 	}
 }
+
+func TestServiceCreateWithClusterLocal(t *testing.T) {
+	action, created, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--cluster-local"}, false)
+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("create", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	labels := created.ObjectMeta.Labels
+
+	labelValue, present := labels[serving.VisibilityLabelKey]
+	assert.Assert(t, present)
+
+	if labelValue != serving.VisibilityClusterLocal {
+		t.Fatalf("Incorrect VisibilityClusterLocal value '%s'", labelValue)
+	}
+}

pkg/kn/commands/service/update_test.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/watch"
 	clienttesting "k8s.io/client-go/testing"
+	"knative.dev/serving/pkg/apis/serving"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
@@ -648,6 +649,74 @@ func TestServiceUpdateLabelExisting(t *testing.T) {
 	assert.DeepEqual(t, expected, actual)
 }
 
+func TestServiceUpdateNoClusterLocal(t *testing.T) {
+	original := newEmptyService()
+	original.ObjectMeta.Labels = map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}
+	originalTemplate := &original.Spec.Template
+	originalTemplate.ObjectMeta.Labels = map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}
+
+	action, updated, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo", "--no-cluster-local", "--no-wait"})
+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("update", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	expected := map[string]string{}
+	actual := updated.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+}
+
+//TODO: add check for template name not changing when issue #646 solution is merged
+func TestServiceUpdateNoClusterLocalOnPublicService(t *testing.T) {
+	original := newEmptyService()
+	original.ObjectMeta.Labels = map[string]string{}
+
+	action, updated, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo", "--no-cluster-local", "--no-wait"})
+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("update", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	expected := map[string]string{}
+	actual := updated.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+}
+
+//TODO: add check for template name not changing when issue #646 solution is merged
+func TestServiceUpdateNoClusterLocalOnPrivateService(t *testing.T) {
+	original := newEmptyService()
+	original.ObjectMeta.Labels = map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}
+	originalTemplate := &original.Spec.Template
+	originalTemplate.ObjectMeta.Labels = map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}
+
+	action, updated, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo", "--cluster-local", "--no-wait"})
+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("update", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	expected := map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}
+	actual := updated.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+
+	newTemplate := updated.Spec.Template
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	actual = newTemplate.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+}
+
 func newEmptyService() *servingv1.Service {
 	ret := &servingv1.Service{
 		TypeMeta: metav1.TypeMeta{

test/e2e/service_test.go

@@ -23,8 +23,8 @@ import (
 	"testing"
 
 	"gotest.tools/assert"
-
 	"knative.dev/client/pkg/util"
+	"knative.dev/serving/pkg/apis/serving"
 )
 
 func TestService(t *testing.T) {
@@ -38,9 +38,10 @@ func TestService(t *testing.T) {
 	r := NewKnRunResultCollector(t)
 	defer r.DumpIfFailed()
 
-	t.Log("create hello service duplicate and get service already exists error")
+	t.Log("create hello service, delete, and try to create duplicate and get service already exists error")
 	test.serviceCreate(t, r, "hello")
-	test.serviceCreateDuplicate(t, r, "hello")
+	test.serviceCreatePrivate(t, r, "hello-private")
+	test.serviceCreateDuplicate(t, r, "hello-private")
 
 	t.Log("return valid info about hello service with print flags")
 	test.serviceDescribeWithPrintFlags(t, r, "hello")
@@ -51,18 +52,49 @@ func TestService(t *testing.T) {
 
 	t.Log("delete two services with a service nonexistent")
 	test.serviceCreate(t, r, "hello")
-
 	test.serviceMultipleDelete(t, r, "hello", "bla123")
+
+	t.Log("create service private and make public")
+	test.serviceCreatePrivateUpdatePublic(t, r, "hello-private-public")
+}
+
+func (test *e2eTest) serviceCreatePrivate(t *testing.T, r *KnRunResultCollector, serviceName string) {
+	out := test.kn.Run("service", "create", serviceName,
+		"--image", KnDefaultTestImage, "--cluster-local")
+	r.AssertNoError(out)
+	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "creating", "namespace", test.kn.namespace, "ready"))
+
+	out = test.kn.Run("service", "describe", serviceName, "--verbose")
+	r.AssertNoError(out)
+	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, serving.VisibilityLabelKey, serving.VisibilityClusterLocal))
+}
+
+func (test *e2eTest) serviceCreatePrivateUpdatePublic(t *testing.T, r *KnRunResultCollector, serviceName string) {
+	out := test.kn.Run("service", "create", serviceName,
+		"--image", KnDefaultTestImage, "--cluster-local")
+	r.AssertNoError(out)
+	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "creating", "namespace", test.kn.namespace, "ready"))
+
+	out = test.kn.Run("service", "describe", serviceName, "--verbose")
+	r.AssertNoError(out)
+	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, serving.VisibilityLabelKey, serving.VisibilityClusterLocal))
+
+	out = test.kn.Run("service", "update", serviceName,
+		"--image", KnDefaultTestImage, "--no-cluster-local")
+	r.AssertNoError(out)
+	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "updated", "namespace", test.kn.namespace, "ready"))
+
+	out = test.kn.Run("service", "describe", serviceName, "--verbose")
+	r.AssertNoError(out)
+	assert.Check(t, util.ContainsNone(out.Stdout, serving.VisibilityLabelKey, serving.VisibilityClusterLocal))
 }
 
 func (test *e2eTest) serviceCreateDuplicate(t *testing.T, r *KnRunResultCollector, serviceName string) {
 	out := test.kn.Run("service", "list", serviceName)
-	out.ErrorExpected = true
 	r.AssertNoError(out)
 	assert.Check(t, strings.Contains(out.Stdout, serviceName), "The service does not exist yet")
 
 	out = test.kn.Run("service", "create", serviceName, "--image", KnDefaultTestImage)
-	out.ErrorExpected = true
 	r.AssertError(out)
 	assert.Check(t, util.ContainsAll(out.Stderr, "the service already exists"))
 }