knative / client-contrib Goto Github PK

As part of the migration of moving https://github.com/knative/client-contrib to the knative-sandbox organisation, we want to start with extracting/moving the kn-admin plugin first.

As suggested in knative/community#174 we propose the name kn-plugin-admin

We would need:

• A GitHub repository knative-sandbox/kn-plugin-admin
• An OWNERS file with the content from https://github.com/knative/client-contrib/blob/master/OWNERS and https://github.com/knative/client-contrib/blob/master/plugins/admin/OWNERS combined
• A CI setup similar to that of knative/client-contrib

Next, we would need the following additional repositories (but maybe let's start with this canary first):

• kn-plugin-source-kafka
• kn-plugin-source-github
• kn-plugin-curl
• kn-plugin-migration
• kn-plugin-log
• kn-plugin-sample
• kn-plugin-shared

As we have kn admin as our first 'official' kn plugin, it's time to collect features which people would like to see as part of a "kn admin" which is all about to configure a Kubernetes based Knative installation (in contrast to non-Kubernetes based implementations like Cloud Run).

This plugin is special as the persona it is adresses is not necessarily the end-user of Knative (like a developer) but an admin setting up Knative. Of course, if used in a standalone development environment (like minikube) that functionality might be helpful for everyone.

As soon we have kind of an initial set which we would like to have configured, we can revisit the command structure for this plugin.

Currently the following initial features / commands are available:

• kn admin domain set - to set Knative route domain
• kn admin registry add - add registry with credentials to enable Service deployment from it

What other use case can you imagine and which would make sense to add it to this plugin ? Please forward this issue to anybody who is interested in having features added for such a plugin.

// cc: @mattmoor @vaikas @evankanderson (TOC) @zhanggbj (kn-admin maintainer)

When I tried to use kn-admin domain set to config a custom domain, I found the below points that we can improve to provide a better user experience:

1. For the --selector description, we can add an example like: app=prod to help user understand how to use this flag, and also we can tell user the flag can be used multiple times with different selectors.
2. For the below codes, I talked with @zhanggbj and she agreed that we should check the error returned from splitByEqualSign function, and tell user the detail error message if the given selector is invalid. Also @zhanggbj suggests moving the selector parsing codes into PreRunE phrase.
   

   client-contrib/plugins/admin/pkg/command/domain/set.go

   Line 62 in f207c1f

   k, v, _ := splitByEqualSign(label)

3. When I give the wrong flag value or miss the value, the command will output the error message, but also the whole usage will be printed. I think printing usage is a little verbose for user, actually the user can use --help to show the usage details. I tried knative command and no such behavior, I think we'd better keep the same with kn command.
   Here is an example for our domain command:

☕15:25 ➜ ~/Workspace/Code/github.com/client-contrib/plugins/admin/cmd [master ✘] ./kn-admin domain set -d test.com --selector
Error: flag needs an argument: --selector
Usage:
  kn admin domain set [flags]

Flags:
  -d, --custom-domain string   desired custom domain
  -h, --help                   help for set
      --selector strings       domain selector

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

Org: knative-sandbox

Repo: kn-plugin-curl

Purpose (Description): This repo holds the kn curl plugin which makes curling Knative services with $kn curl <service-name>.

Sponsoring WG: client

Dependencies: #73

Org: knative-sandbox

Repo: kn-plugin-source-kafka

Purpose (Description): This repo holds the kn plugin for the kafka source, e.g., $kn source kafka create

Sponsoring WG: client

A developer may set some autoscaling configs like --limits-cpu or --limits-memory as what kn service create described today. However, a Knative administrator may need to update some overall configs like stable-window or panic-window-percentage or ticker-interval for the whole Knative platform. The kn-admin will list all the config we support to update by kn-admin

According the Knative 0.16 doc about autoscaling, plan support following global configs to help with admin work. (Per-revision config is not planned here, that are more for developer experience)

$ kn admin autoscaling update
- Stable-window xx
- panic-window-percentage xx
- max-scale-up-rate xx
- container-concurrency-target-default xx
- container-concurrency-target-percentage xx
- requests-per-second-target-default xx

If there're any other autoscale configs that should be controlled by Knative administrator, please feel free to leave a comment in this issue. Thanks!

Now we have kn admin registry add and kn admin registry rm to manage registry credentials on default namespace. I want to add --namespace or -n flag to kn admin registry so that registry credentials on other namespace can also be added and removed. And also add --serviceaccount or -s flag to kn admin registry to enable choosing service account.

For example:
Manage registry used by Knative service deployment

Usage:
  kn admin registry [command]

Available Commands:
  add         Add registry with credentials
  help        Help about any command
  remove      Remove registry settings

Flags:
  -h, --help                    help for registry
  -n, --namespace string        the namespace to manage registries
  -s, --serviceaccount string   the serviceaccount to save imagePullSecrets

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

Use "kn admin registry [command] --help" for more information about a command.

Currently we are using profiling data to debug knative performance issues. I think it would be nice to support download related profiling data directly using kn admin plugin.
Here are three steps to get a profiling data for knative components:

1. Enable tracing by updating the config-observability ConfigMap and setting profiling.enable = "true"
2. Enable port-forwarding to get access to Knative Serving pods.
3. Download specific profiling data at http://localhost:8008/debug/pprof/

There are 8 types of profiling data could be fetched according to https://github.com/knative/serving/blob/master/test/performance/profiling.md

• heap
• profile
• block
• trace
• allocs
• mutex
• goroutine
• threadcreate

For profile and trace, a optional seconds parameter could be specified.

I think for kn-admin plugin, we could download profiling data in one command like

• kn admin profiling --enable/--disable to enable/disable profiling.
• kn admin profiling -t/--target autoscaler --heap to download heap profile of auto scaler. Will enable profiling if it is currently not enabled. The same goes for the following commands.
• kn admin profiling -t/--target networking-istio --trace 2m for a 2 minutes CPU trace profile of networking-istio.
• kn admin profiling -t/--target activator --all to download all profiling data avaliable for all activator pods with a default 30s duration for tracing and profile.

If there are more than one pod avaliable, the default behaviour should download profiling data for all pods. Also we can set a specific pod name using -t/--target flag

• kn admin profiling -t/--target activator-5979f56548 --all to download all profiling data avaliable for pod activator-5979f56548.

The plugin should be responsible to provide a meaningful filename for downloaded profiling data. e.g.
2020-07-09T10:09:48Z-activator-5979f56548-54crk-heap.pb.gz. A -d flag could be supported to specify the download directory.

cc @zhanggbj

Noticed this during #36

I don't know what the trade offs are so I'm asking.

Hope to use this issue to discuss a template or a consistent structure for a plugin. A consistent structure can ease the management and enable the automation.

Here is a prerequisite that we agree every plugin shall be self-contained. Plugins should include all necessary files. In my mind, there are:

• license file
• metadata file (name, description, parameters, environment variables maybe, ... ) in which format ?
• README.md
• source codes and dependencies
• unit tests
• e2e tests
• OWNERS
• build scripts (possible start from the file hack/build.sh)

Welcome inputs and comments.

Now we have kn admin domain set to set custom-domain. But we are missing to unset the custom domain if it is not used or incorrect.

unset Knative route domain for service

For example:
# To unset a route domain
kn admin domain unset --custom-domain mydomain.com

Usage:
  kn admin domain unset [flags]

Flags:
  -d, --custom-domain string  Custom domain to unset
  -h, --help                   help for unset

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

Now we have kn admin registry add to add registry with credentials to enable Service deployment from it. But we are missing to remove the registry. The removal params require registry username and registry server url to remove the registry.

For example:
# To remove a registry
kn admin registry remove --username=myusername  --server=docker.io

Usage:
  kn admin registry remove [flags]

Flags:
  -h, --help              help for remove
      --server string     Registry Address
      --username string   Registry Username

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

see #33 (review)

Per discussion in
#40 (comment)

now in kn-admin, for different subcommand domain/registry, we're using add/update/etc for create/update actions. We should be consistent in kn-admin.

kn client has a story to use create/apply/remove. We can follow the same way to keep same user experience.

This is an issue to keep track of progress and PRs to get kn-source-github plugin:

• dependencies PR (to facilitate review)
• create|delete|update|describe PR
• unit tests
• integration tests

@maximilien @rhuss let's use this issue to discuss where we shall put maximilien/kn-source-pkg.

maximilien/kn-source-pkg could be used as a common package for all event sources plugins, which will ease the development of sources plugins. Now it's still under dr.max's personal repo. The kn-source-pkg is also a separate go project and shall be built dependently.

Can we move it to this repo as a library? Which folder shall we put it? Maybe put to client-contrib/common.
Let me know your thoughts. Thank you.

When I read our codes, I found that we put the command examples in cobra.Command.Long fields, like this:

Actually, cobra.Command has the field Example to set the command examples. I think we'd better use this field to describe our command examples. Like the kn command did:
https://github.com/knative/client/blob/3c5ceff655ec4543907a681c300912076fa49e53/pkg/kn/commands/route/list.go#L36

If we use Example field, the output will look like:

☕16:18 ➜ ~/Workspace/Code/github.com/client-contrib/plugins/admin/cmd [master ✘] ./kn-admin domain set --help
Set Knative route domain for service

Usage:
  kn admin domain set [flags]

Examples:

  # To set a default route domain
  kn admin domain set --custom-domain mydomain.com

  # To set a route domain for service having label app=v1
  kn admin domain set --custom-domain mydomain.com --selector app=v1


Flags:
  -d, --custom-domain string   desired custom domain
  -h, --help                   help for set
      --selector strings       domain selector

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

@zhanggbj @ZhuangYuZY @lanceliuu @12345lcr How do you think if we use the field Example? Thanks!

Now we have kn admin registry add and kn admin registry rm to manage registry credentials. I want to add kn admin registry list so that current registry credentials can be listed.

For example:
Manage registry used by Knative service deployment

Usage:
  kn admin registry [command]

Available Commands:
  add         Add registry with credentials
  help        Help about any command
  list        List current registries
  remove      Remove registry settings

Flags:
  -h, --help                    help for registry
  -n, --namespace string        the namespace to manage registries
  -s, --serviceaccount string   the serviceaccount to save imagePullSecrets

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

Use "kn admin registry [command] --help" for more information about a command.

This plugin should fully manage a GitHub source, offering all important configuration possibilities as command line options.

See the Kn eventing spec for the suggested Plugin format: https://drive.google.com/drive/folders/1QF-job3rCEqCpJLm8nHkC4mBIi4XANE1

To list the kafka sources kn source kafka list.

Because kafkasource: change bootstrapServers and Topics to accept arrays is enabled in back-end, we shall modify client to support the changes.

Or else, we will get failure when creating a kafka source:

$./kn source kafka create mykafkasrc --servers my-cluster-kafka-bootstrap.kafka.svc:9092 --topics test-topic --consumergroup test-consumer-group
Error: cannot create KafkaSource 'mykafkasrc' in namespace 'default' because: admission webhook "defaulting.webhook.kafka.sources.knative.dev" denied the request: mutation failed: cannot decode incoming new object: json: cannot unmarshal string into Go struct field KafkaSourceSpec.spec.bootstrapServers of type []string

Support plugin docs auto generation.
My idea is to generate a README.md for plugin based on two inputs:

• some manually written part in md files, e.g. plugins/*/docs/header.md, and plugins/*/docs/footer.md
• the auto generated help doc by package github.com/spf13/cobra/doc

$CHANGED_FILES in test/common.sh is empty string, so that plugins e2e-test.sh will not be invoked.

See the integration test log message of PR #12. Although PR #12 has updated plugins/hello, the plugins/hello/test/e2e-test.sh is not be invoked.

===================================
==== RUNNING INTEGRATION TESTS ====
===================================
==== Mon Apr  6 11:46:09 PDT 2020
===================================
Running integration test test/e2e-tests.sh
=== Running Plugins E2E tests =======================================
--- Plugins changed in PR: ----------------

-------------------------------------------
==================================
==== INTEGRATION TESTS PASSED ====
==================================

We have different dependency declaration for different plugins which is a good thing to keep them self-contained and independent (allowing also for an independent release cycle).

However, verndoring the dependencies is really huge, especially for source where each source support for sources from eventing-contrib would have to vendor eventing-contrib individually although it only needs the client bindings for a single source out of it. (I wonder whether we could craft go mod that way to only pick that client dependency ?)

One suggestion for this particular dependency would be to get the client API with different means (e.g. git clone directly, by cherry-picking the client interface)

However, I would discuss in general whether for an aggregated repository vendoring makes sense at all or whether we should use go mod without a vendor/ dir and using the local go mod cache for downloading dependencies ?

See the output of kn hello. Pay attention that kn-hello is used in the usage part and in the last sentence, which we definitely hope to use kn hello instead.

$ kn hello
Longer description of this fantastic plugin that can go over several lines.

Usage:
  kn-hello [command]

Available Commands:
  help        Help about any command
  print       Print the argument
  version     Prints the plugin version

Flags:
  -h, --help   help for kn-hello

Use "kn-hello [command] --help" for more information about a command.

It's an issue related with cobra. I create in our repo to track.

Now we have kn admin domain set to set custom-domain. But we are missing a list command to show custom domains.

List Knative custom domain

Usage:
  kn admin domain list [flags]

Examples:

  # To list all custom domains
  kn admin domain list

Flags:
  -h, --help         help for list
      --no-headers   When using the default output format, don't print headers (default: print headers).

Global Flags:
      --config string   config file (default is $HOME/.config/kn/plugins/admin.yaml)

From the @zhanggbj's proposal doc:

In production, usually we would like to provide https connection for applications or Knative Services. To enable https connection, kn-admin will take an existing certificate and enable https-connection with the CLI instead of creating a Secret and modifying Gateway as Knative Serving doc described.

$ kn admin https-connection enable --tls-cert ${workspace}/demo.pem --tls-key ${workspace}/demo.key
Knative serving https connection is enabled

# In fact, kn admin will create a secert in istio-system and config the routing rules in knative-ingress-gateway
$ kubectl get secret -n istio-system istio-ingressgateway-certs
$ kubectl get gateway knative-ingress-gateway --namespace knative-serving

Org: knative-sandbox

Repo: kn-plugin-source-github

Purpose (Description): This repo holds the kn plugin for the github source, e.g., $kn source github create <source-name>

Sponsoring WG: client

Dependencies: #73

Should this be useful?

kn source kafka message "MY MESSAGE" --topic TOPIC  --broker BROKER

Create a kafka source without sink and try describing it, it fails with runtime error as the nil check for Sink is missing:

Steps to reproduce:

1. Create kafka source

kn source kafka create mykafkasrc --servers my-cluster-kafka-bootstrap.kafka.svc:9092 --topics test-topic --consumergroup test-consumer-group

2. Describe it

kn source kafka describe mykafkasrc
Name:              mykafkasrc
Namespace:         default
Age:               3h
BootstrapServers:  my-cluster-kafka-bootstrap.kafka.svc:9092
Topics:            test-topic
ConsumerGroup:     test-consumer-group

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x134ad18]

goroutine 1 [running]:
knative.dev/client-contrib/plugins/source-kafka/pkg/factories.writeSink(0x184c360, 0xc00042dc50, 0x0)

[...]

Error: exit status 2
Run 'kn --help' for usage

Suggested fix:

Check for nil value here

Currently, the root e2e-tests.sh does nothing except loop calling the e2e-test.sh under every single plugin. I think there are a few common set up steps that we might want to let the root e2e-tests.sh to handle:

• Set up Knative Serving and Eventing. We definitely don't want every plugin to set up and tear down Knative serving and eventing.
• Manage to call a plugin_setup() and plugin_teardown() for every plugins to set up their resources before the test and delete their resources after the test. ( This might not necessary. The plugin_setup() and plugin_teardown() could be easily put in somewhere of plugin e2e-test.sh).

@rhuss please share your thoughts to this two requirments.

see #33 (comment)

Environment

System: MacOS 10.15.4
Minikube: v1.11.0
Kubectl client: v1.15.5
Kubectl server: v1.18.2
Knative: v0.12.0

Commands to Run

1. ./kn-admin domain set -d test.com --selector app=prod --selector app=stage
2. ./kn-admin autoscaling update --scale-to-zero

Results and Logs

1. For the domain command, I checked the config-domain, the test.com with the selectors is set, but there is an unexpected line ---- between domain and selector:

test.com:
----
selector:
  app: prod
  app: stage

Events:  <none>

2. For the autoscaling command, I found that the enable-scale-to-zero is set to with boolean value instead of string value:

enable-scale-to-zero:
----
true
Events:  <none>

I talked to @zhanggbj and she said there is no redundant line: ---- and the value is set to string not boolean on kube cluster. We will take time to investigate why the behaviour is different on minikube.

A developer may set some autoscaling configs like --limits-cpu or --limits-memory as what kn service create described today. However, a Knative administrator may need to update some overall configs like stable-window or panic-window-percentage or ticker-interval for the whole Knative platform. The kn-admin will list all the config we support to update by kn-admin

$ kn admin autoscaling list

• Stable-window
• panic-window-percentage
• ticker-interval

If there're any other autoscale configs which should be controlled by Knative administrator, please feel free to leave a comment in this issue. Thanks!

Although every plugin maintains its own unit tests and e2e tests, we still need a test infrastructure to run the presubmit tests, set up an e2e environment, and etc.

Maybe follow the same test infrastructure of client repo.

Repo information

Org: knative-sandbox

Repo: kn-plugin-admin

Purpose (Description): This repo holds the kn admin plugin which can be used to configure Kubernetes based Knative installations. Currently it is contained in a subdirectory of this client-contrib repo but we decided to move out all plugins into individual repostitories.

This issue replaces #73

Sponsoring WG: client

Actions to fulfill

This area is used for the TOC to track the repo creation
process

• Add this issue to the TOC project board for review.

• Send a PR adding entries for this repo in /peribolos/knative-sandbox.yaml

  • Add the repository and a description.
  • Grant Knative Admin the admin privilege.
  • Grant the sponsoring WG the write privilege.
  • PR: knative/community#264

Once the TOC has approved the above, it will merge and Peribolos will create an empty repository.

• (golang) Send a PR to add aliases for knative.dev/$REPONAME import paths (sample).

• Have a lead from the sponsoring WG bootstrap the Git repository by pushing an
  appropriate "template" repository (basic,
  sample-controller,
  sample-source) to the new repository as
  a git remote. For example:

    git clone https://github.com/knative-sandbox/sample-controller.git
    cd sample-controller
    git remote add newrepo https://github.com/knative-sandbox/$REPONAME.git
    git push newrepo master

• Set up test-infra following the
  docs linked at the beginning.

• Create a sample PR to verify Prow (e.g. edit the boilerplate README)

Once Prow has been verified.

• Have a member of the TOC enable branch protections so that tide is a required presubmit check.

kafka source pins client dep at v0.14.0, it can be bumped to latest to support latest eventing API support.

Opened this item for discussion about client-contrib plugin release and installation

• release

1. Will client-contrib plugins release with kn client or separately?
2. Inside client-contrib release, each plugin will maintain its versions and client-contrib has a list of all versions?

• installation

1. For now, a plugin and be added to kn client like here https://github.com/knative/client/blob/master/docs/README.md#options,
   but usually plugin may have its own repo and end-users can list and install/download a plugin, will client-contribe have something like this?

For kn-admin plugin, use go test framework to fill up Unit Test and e2e test.

For kn-source-kafka and kn-source-github and various planned plugins, @daisy-ycguo and I have a common set of code we use as kn-source-pkg. We would like to have this here.

While kn-source-pkg is structured like a plugin, it is not. So putting it in this repo’s plugins directory would not be correct. So I am suggesting we create a top level lib directory (like we did for client and put it there. This could also serve for a good place for other such common code.

Thoughts? Please respond soon. I will leave this open after Thursday like the other recent global issues for this repo.

As mentioned in knative/client#520, we shall create a sample source plugin "Kafka" to validate the implementation of typed plugin sources knative/client#519.

We're currently using whitelist terminology due to the strimzi API.

Let's make sure we watch upstream's efforts and remove once possible. strimzi/strimzi-kafka-operator#3289

If the kubectl context is not set or run kubectl config unset current-context to unset it, we will get runtime error when run kn admin, for example:

☕10:37 ➜ ~/ [chao ✘] k config current-context
error: current-context is not set

☕10:36 ➜ ~/ [chao ✘] ./cmd autoscaling update --scale-to-zero
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xb0 pc=0x1da8c10]

goroutine 1 [running]:
knative.dev/client-contrib/plugins/admin/pkg/command/autoscaling.NewAutoscalingUpdateCommand.func2(0xc0002e4f00, 0xc0002c7ff0, 0x0, 0x1, 0x0, 0x0)
	/Users/chao/Workspace/Code/github.com/client-contrib/plugins/admin/pkg/command/autoscaling/update.go:66 +0xa0
github.com/spf13/cobra.(*Command).execute(0xc0002e4f00, 0xc0002c7fe0, 0x1, 0x1, 0xc0002e4f00, 0xc0002c7fe0)
	/Users/chao/Workspace/Code/go/pkg/mod/github.com/spf13/[email protected]/command.go:826 +0x460
github.com/spf13/cobra.(*Command).ExecuteC(0xc000197400, 0x0, 0x0, 0xc000197400)
	/Users/chao/Workspace/Code/go/pkg/mod/github.com/spf13/[email protected]/command.go:914 +0x2fb
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/chao/Workspace/Code/go/pkg/mod/github.com/spf13/[email protected]/command.go:864
main.main()
	/Users/chao/Workspace/Code/github.com/client-contrib/plugins/admin/cmd/kn-admin.go:24 +0x40

I checked the codes in AdminParams:

// Initialize generate the clientset for params
func (params *AdminParams) Initialize() error {
	if params.ClientSet == nil {
		restConfig, err := params.RestConfig()
		if err != nil {
			return err
		}

		params.ClientSet, err = kubernetes.NewForConfig(restConfig)
		if err != nil {
			fmt.Println("failed to create client:", err)
			os.Exit(1)
		}
	}
	return nil
}

// rootCmd represents the base command when called without any subcommands
func NewAdminCommand(params ...pkg.AdminParams) *cobra.Command {
	p := &pkg.AdminParams{}
	p.Initialize()

	rootCmd := &cobra.Command{
		Use:   "kn\u00A0admin",
		Short: "A plugin of kn client to manage Knative",
		Long:  `kn admin: a plugin of kn client to manage Knative for administrators`,

...

I think we can do some improvements for the above functions so that we can avoid outputting runtime error if kubectl context is not set:

1. In Initialize(), we'd better return error when fail to create ClientSet
2. In NewAdminCommand(), we should check the error from p.Initialize(), if error is returned, we can print the error message and exit to run command.

Currently we have e2e test entrypoint in test/e2e-tests.sh, and it will iterate over all plugin directories to find and execute e2e test scripts. Based on such settings I have the following questions:

1. Do we have necessary permissions to create clusters in GKE as the same of knative/client?
2. Does every plugin need to set up clusters for its own and do teardown afterwards or to share the same cluster instead?