knative / eventing

Event-driven application platform for Kubernetes
https://knative.dev/docs/eventing
Apache License 2.0
1.4k stars 586 forks source link

Discussion: How to add CLI meta data to Source CRDs ? #1940

Closed rhuss closed 3 years ago

rhuss commented 4 years ago

Problem

For helping kn (or any other CLI) it would be very helpful when a Source CRD would carry some extra meta information for mapping cli-options (like --access-token or --event-types for a GitHubSource) to the property within the CR (e.g. .spec.accessToken.secreteKeyRef for --access-token and .spect.eventTypes for --event-types).

Fortunately, OpenAPI schema supports so-called Specification Extensions in a Schema Object. These are fields starting with a x-

So ideally a GitHubSource CRD could look like in the example below (where I picked up the existing GitHub source, and added the required meta information to the properties which are supposed to be mapped. I also moved the registry types away from the schema to annotations, but that's not important here)

GitHub Source with `x-cli-` annotations ```yaml apiVersion: apiextensions.k8s.io/v1beta1 kind: CustomResourceDefinition metadata: name: githubsources.sources.eventing.knative.dev labels: contrib.eventing.knative.dev/release: devel eventing.knative.dev/source: "true" knative.dev/crd-install: "true" annotations: registry.knative.dev/check_suite.type: dev.knative.source.github.check_suite registry.knative.dev/commit_comment.type: dev.knative.source.github.commit_comment registry.knative.dev/create.type: dev.knative.source.github.create registry.knative.dev/delete.type: dev.knative.source.github.delete registry.knative.dev/deployment.type: dev.knative.source.github.deployment registry.knative.dev/deployment_status.type: dev.knative.source.github.deployment_status registry.knative.dev/fork.type: dev.knative.source.github.fork registry.knative.dev/gollum.type: dev.knative.source.github.gollum registry.knative.dev/installation.type: dev.knative.source.github.installation registry.knative.dev/integration_installation.type: dev.knative.source.github.integration_installation registry.knative.dev/issue_comment.type: dev.knative.source.github.issue_comment registry.knative.dev/issues.type: dev.knative.source.github.issues registry.knative.dev/member.type: dev.knative.source.github.member registry.knative.dev/milestone.type: dev.knative.source.github.milestone registry.knative.dev/org_block.type: dev.knative.source.github.org_block registry.knative.dev/ping.type: dev.knative.source.github.ping registry.knative.dev/project_column.type: dev.knative.source.github.project_column registry.knative.dev/public.type: dev.knative.source.github.public registry.knative.dev/pull_request_review.type: dev.knative.source.github.pull_request_review registry.knative.dev/push.type: dev.knative.source.github.push registry.knative.dev/repository.type: dev.knative.source.github.repository registry.knative.dev/team.type: dev.knative.source.github.team registry.knative.dev/watch.type: dev.knative.source.github.watch spec: group: sources.eventing.knative.dev names: categories: - all - knative - eventing - sources kind: GitHubSource plural: githubsources scope: Namespaced subresources: status: {} validation: openAPIV3Schema: properties: spec: properties: accessToken: properties: secretKeyRef: # Name of a cli option mapping to this field x-cli-option: "access-token" # Type which triggers a special treatment of the option to # create the embedded opject with a `key:` and `name:` field x-cli-type: "map-selector" # Help message which can be used by a CLI on the help page x-cli-help: "GitHub access token. The value of this option is the name of a secret and key within secret to GitHub access, dot separated (e.g. --secret-token=mygithubsecret.mykey) " type: object type: object eventTypes: items: enum: - check_suite - commit_comment - create - delete - deployment - deployment_status - fork - gollum - installation - integration_installation - issue_comment - issues - label - member - membership - milestone - organization - org_block - page_build - ping - project_card - project_column - project - public - pull_request - pull_request_review - pull_request_review_comment - push - release - repository - status - team - team_add - watch type: string minItems: 1 x-cli-option: "event-type" x-cli-help: "GitHub events to watch for. This is a comma separated list of options. Alternatively this option can be provided multiple times." type: array ownerAndRepository: x-cli-option: "owner" minLength: 1 type: string secretToken: properties: secretKeyRef: x-cli-option: "secret-token" x-cli-type: "map-selector" x-cli-help: "GitHub secret token. The value of this option is the name of a secret and key within secret to GitHub access, dot separated (e.g. --secret-token=mygithubsecret.mykey) " type: object type: object serviceAccountName: x-cli-option: "service-account" x-cli-help: "Service account" type: string sink: type: object required: - ownerAndRepository - eventTypes - accessToken - secretToken type: object status: properties: conditions: items: properties: lastTransitionTime: # we use a string in the stored object but a wrapper object # at runtime. type: string message: type: string reason: type: string severity: type: string status: type: string type: type: string required: - type - status type: object type: array sinkUri: type: string webhookIDKey: type: string type: object version: v1alpha1 ```

Unfortunately, Kubernetes does not support this (see https://github.com/kubernetes/kubernetes/issues/82942 and also the API docs for CRDs where it seems only a fixed set of x-kubernetes- fields are supported

😞

The question is how to proceed:

An solution would be also to put the mapping into CRD annotations with the cli-option as key and the JSON path expression as value (or having a single annotation which has a JSON mapping as value), e.g

annotations:
  client.knative.dev/access-token.option=.spec.accessToken.secretRef
  client.knative.dev/access-token.help="GitHub access token. The value of this option is the name of a secret and key within secret to GitHub access, dot separated (e.g. --secret-token=mygithubsecret.mykey)"
  client.knative.dev/access-token.type="map-selector"

Tbh, I would prefer a hook into the schema as it feels that it belongs to the schema as an 'annotation' kind.

What do you think about the proposal or is there any other way to help the client to create CRs from flat command line options ?

aaron-lerner commented 4 years ago

An issue I see with this is that I don't think all CLIs will be able to be able to dynamically add flags for each x-cli-option.

rhuss commented 4 years ago

Yes, but they alway could ;-).

This meta-data is helpful for CLI authors if they want to support dynamic cli options, based on the CR at hand. This is supported by any language, but it could be that your favourite options parsing library doesn't support this. However, you can still do the CLI options parsing manually, this is not a principal limitation for a client.

rhuss commented 4 years ago

Following the lines of the discussion starting at https://github.com/knative/eventing/issues/1550#issuecomment-535267670 and the fact that we won't see support for arbitrary x-cli- schema fields in K8s, I suggest the following approach:

We introduce in the Source Schema Spec the support of an annotation client.knative.dev/options which takes the form:

annotations:
  client.knative.dev/options: |
    [
      {
       "name" : "access-token",
       "path" : ".spec.accessToken.secretRef",
       "help" : "GitHub access token. The value of this option is the name of a secret and key within secret to GitHub access, dot separated (e.g. --access-token=mygithubsecret.mykey)"
       "type" : "secret-reference"
      },
      {
       "name" : "event-type",
       "path" : ".spec.eventTypes",
       "help" : "GitHub event type. This option can be given multiple times for registering multiple event types"
      },
      ....
    ]
rhuss commented 4 years ago

@adamharwayne if this sounds ok, I would add this to the "Kn eventing" proposal, too.

aslom commented 4 years ago

This is great idea to make CLI easier to use.

vaikas commented 4 years ago

We should also add this to the Source Spec after it lands, I don't want to hold off on that before merging, but once it lands on the spec, we can close this? https://github.com/knative/eventing/pull/1856

vaikas commented 4 years ago

I'd like to get the Source Spec changes in for v0.10.0-M2 and finish the CRDs to support this with little more leeway, if we can get everything done for 10-M2, awesome :)

n3wscott commented 3 years ago

YEAH!!! WOOO with the Manual CRD via Discovery API.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

rhuss commented 3 years ago

/remove-lifecycle stale

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

rhuss commented 3 years ago

Let's close this as it seems to going into the direction to leverage the Discovery API to hold those CLI related meta-data, so let's continue as part of the conversation how to integrate this meta-data into the Source type