Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Mar 10, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e5e4bfc
fixes(#606): adds --cluster-local / --no-cluster-local flags
maximilien Feb 1, 2020
df50f17
Merge branch 'master' into issue606
maximilien Feb 3, 2020
83390dd
updating from latest master and addressing review comments
maximilien Feb 4, 2020
b96455f
Merge branch 'master' into issue606
maximilien Feb 7, 2020
36e59dd
Merge branch 'master' into issue606
maximilien Feb 10, 2020
b752d0a
Merge branch 'master' into issue606
maximilien Feb 12, 2020
8582338
Merge branch 'master' into issue606
maximilien Feb 13, 2020
aeb537b
Merge branch 'master' into issue606
maximilien Mar 4, 2020
f2b27ae
Merge branch 'master' into issue606
maximilien Mar 5, 2020
596407f
Merge branch 'master' into issue606
maximilien Mar 6, 2020
e21c7d8
Merge branch 'master' into issue606
maximilien Mar 9, 2020
6fde086
Merge branch 'master' into issue606
maximilien Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@
|===
////

## v0.13.0 (2020-02-14)
maximilien marked this conversation as resolved.
Show resolved Hide resolved

[cols="1,10,3", options="header", width="100%"]
|===
| | Description | PR

| 🎁
| adds --cluster-local / --no-cluster-local flags
navidshaikh marked this conversation as resolved.
Show resolved Hide resolved
| https://github.com/knative/client/pull/629[#629]
maximilien marked this conversation as resolved.
Show resolved Hide resolved

## v0.12.0 (2020-01-29)

Expand Down
5 changes: 5 additions & 0 deletions docs/cmd/kn_service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -45,6 +48,7 @@ kn service create NAME --image IMAGE [flags]
--annotation stringArray 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-).
--async Create service and don't wait for it to become 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; sets service's route visibility to 'cluster-local'.(--no-cluster-local will remove any 'cluster-local' route visibility)
maximilien marked this conversation as resolved.
Show resolved Hide resolved
--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.
-e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-).
Expand All @@ -60,6 +64,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; sets service's route visibility to 'cluster-local'.(--no-cluster-local will remove any 'cluster-local' route visibility) (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)
-p, --port int32 The port where application listens on.
--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.
Expand Down
2 changes: 2 additions & 0 deletions docs/cmd/kn_service_update.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ kn service update NAME [flags]
--annotation stringArray 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-).
--async Update service and don't wait for it to become 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; sets service's route visibility to 'cluster-local'.(--no-cluster-local will remove any 'cluster-local' route visibility)
--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.
-e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-).
Expand All @@ -55,6 +56,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; sets service's route visibility to 'cluster-local'.(--no-cluster-local will remove any 'cluster-local' route visibility) (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)
-p, --port int32 The port where application listens on.
--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.
Expand Down
12 changes: 12 additions & 0 deletions pkg/kn/commands/service/configuration_edit_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ type ConfigurationEditFlags struct {
ServiceAccountName string
ImagePullSecrets string
Annotations []string
ClusterLocal bool

// Preferences about how to do the action.
LockToDigest bool
Expand Down Expand Up @@ -117,6 +118,10 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
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; sets service's route visibility to 'cluster-local'."+
"(--no-cluster-local will remove any 'cluster-local' route visibility)")
p.markFlagMakesRevision("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.")
Expand Down Expand Up @@ -309,6 +314,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("concurrency-target") {
err = servinglib.UpdateConcurrencyTarget(template, p.ConcurrencyTarget)
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion pkg/kn/commands/service/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 21 additions & 0 deletions pkg/kn/commands/service/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"strings"
"testing"

"gotest.tools/assert"
api_errors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/watch"
Expand Down Expand Up @@ -493,3 +494,23 @@ 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, false)

if err != nil {
t.Fatal(err)
} else if !action.Matches("create", "services") {
t.Fatalf("Bad action %v", action)
}

template, err := servinglib.RevisionTemplateOfService(created)
if err != nil {
t.Fatal(err)
}

_, present := template.Labels[config.VisibilityLabelKey]
assert.Assert(t, !present)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we please also check for the proper value of the annotation ? Maybe also in update_test.go that the annotation gets removed when using --no-cluster-local to complete the scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, slight problem adding the update_test.go. Seems like removing that label causes an API error:

Internal error occurred: admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: Saw the following changes without a name change (-old +new): spec.configuration.template
{*v1alpha1.RevisionTemplateSpec}.ObjectMeta.Labels:
	-: "map[serving.knative.dev/visibility:cluster-local]"
	+: "map[]"

Let me know if you see something obvious. Unless my cluster is not up to date. Digging more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is, that the test uses the BYO feature for revision ("Bring your own" revision name). This means you need to to change the revision name to something unique on your own (like increasing a counter). Alternatively, you can use the auto revision name generation feature from Knative serving (which results in this random prefixes, like for pods of a deployment). You can servers side name generation with -revision-name "" (which might arguable not be the best option to use). Not sure anymore why we have settled on client side provided names by default.

However, I would expect that kn update deals with percularity. Could you show me how you did the test ? Also, have you set the proper flag that the flag that you set also creates a new revision ? (look in the flags file how other do it)

}
10 changes: 10 additions & 0 deletions pkg/serving/config_changes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"knative.dev/serving/pkg/apis/autoscaling"
"knative.dev/serving/pkg/apis/serving"
servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
"knative.dev/serving/pkg/reconciler/route/config"
)

// VolumeSourceType is a type standing for enumeration of ConfigMap and Secret
Expand Down Expand Up @@ -193,6 +194,15 @@ func UpdateAutoscaleWindow(template *servingv1alpha1.RevisionTemplateSpec, windo
return UpdateRevisionTemplateAnnotation(template, autoscaling.WindowAnnotationKey, window)
}

// UpdateClusterLocal updates container serving.knative.dev/visibility annotation
func UpdateClusterLocal(service *servingv1alpha1.Service, template *servingv1alpha1.RevisionTemplateSpec, clusterLocal bool) error {
if clusterLocal {
return UpdateLabels(service, template, map[string]string{config.VisibilityLabelKey: config.VisibilityClusterLocal}, []string{})
} else {
return UpdateLabels(service, template, map[string]string{}, []string{config.VisibilityLabelKey})
}
}

// UpdateConcurrencyTarget updates container concurrency annotation
func UpdateConcurrencyTarget(template *servingv1alpha1.RevisionTemplateSpec, target int) error {
return UpdateRevisionTemplateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
Expand Down