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

CHANGELOG.adoc

@@ -18,6 +18,10 @@
 |===
 | | Description | PR
 
+| 🎁
+| 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.
@@ -62,6 +66,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.
@@ -57,6 +58,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

@@ -52,6 +52,7 @@ type ConfigurationEditFlags struct {
 	ServiceAccountName         string
 	ImagePullSecrets           string
 	Annotations                []string
+	ClusterLocal               bool
 	User                       int64
 
 	// Preferences about how to do the action.
@@ -129,6 +130,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. "+
@@ -137,33 +139,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{},
 		"Service label to set. 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().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. "+
@@ -175,16 +194,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",
 		"",
@@ -271,6 +293,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())
@@ -362,6 +385,13 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}
 
+	if cmd.Flags().Changed("cluster-local") || cmd.Flags().Changed("no-cluster-local") {
+		err = servinglib.UpdateClusterLocal(service, template, p.ClusterLocal)
+		if err != nil {
+			return err
+		}
+	}
+
 	if cmd.Flags().Changed("label") {
 		labelsMap, 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,92 @@ 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)
+
+	newTemplate := updated.Spec.Template
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	actual = newTemplate.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{}
+	originalTemplate := &original.Spec.Template
+	originalTemplate.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)
+
+	newTemplate := updated.Spec.Template
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	actual = newTemplate.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{

pkg/serving/config_changes.go

@@ -193,6 +193,15 @@ func UpdateAutoscaleWindow(template *servingv1.RevisionTemplateSpec, window stri
 	return UpdateRevisionTemplateAnnotation(template, autoscaling.WindowAnnotationKey, window)
 }
 
+// UpdateClusterLocal updates container serving.knative.dev/visibility annotation
+func UpdateClusterLocal(service *servingv1.Service, template *servingv1.RevisionTemplateSpec, clusterLocal bool) error {
+	if clusterLocal {
+		return UpdateLabels(service, template, map[string]string{serving.VisibilityLabelKey: serving.VisibilityClusterLocal}, []string{})
+	} else {
+		return UpdateLabels(service, template, map[string]string{}, []string{serving.VisibilityLabelKey})
+	}
+}
+
 // UpdateConcurrencyTarget updates container concurrency annotation
 func UpdateConcurrencyTarget(template *servingv1.RevisionTemplateSpec, target int) error {
 	return UpdateRevisionTemplateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))