List inbuilt sources if CRD access is restricted #948

navidshaikh · 2020-07-23T10:55:30Z

Description

Identify restricted access error
If server returns restricted access error, fallback to listing
only eventing inbuilt sources using their GVKs.
List every typed source and read the error
to know if eventing is installed for kn source list-types.

Changes

Add isForbiddenError in kn error factory
Add ListSourcesUsingGVKs to dynamic client
Add BuiltInSourcesGVKs to pkg/sources/v1alpha2/client.go which returns inbuilt source GVKs

Reference

Fixes #947

/lint

Fixes knative#947 - Identify restricted access error - If server returns restricted access error, fallback to listing only eventing inbuilt sources using their GVKs. - List any inbuilt source (ApiServerSource) object and read the error to know if eventing is installed for `kn source list-types`.

knative-prow-robot

@navidshaikh: 3 warnings.

In response to this:

Description

Identify restricted access error

If server returns restricted access error, fallback to listing
only eventing inbuilt sources using their GVKs.

List any inbuilt source (ApiServerSource) object and read the error
to know if eventing is installed for kn source list-types.

Changes

Add IsForbiddenError in kn error factory

Add ListSourcesUsingGVKs to dynamic client

Add BuiltInSourcesGVKs to pkg/sources/v1alpha2/client.go which returns inbuilt source GVKs

Reference

Fixes #947

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/dynamic/lib.go

pkg/errors/factory.go

pkg/sources/v1alpha2/client.go

navidshaikh · 2020-07-23T11:53:27Z

@rhuss @maximilien @daisy-ycguo: I will add the tests if the approach looks good.

rhuss

The approach lgtm, I have some questions (inline). Especially I wonder whether we can rely of numeric prefix in the error messages (i.e. considering that the API call might be used on different platforms like Kubernetes and CloudRun).

rhuss · 2020-07-24T09:53:07Z

pkg/dynamic/client.go

+	// Clear the Group and Version for list if there are multiple types of source objects found
+	// Keep the source's GVK if there is only one type of source objects found or requested via --type filter
+	if numberOfSourceTypesFound > 1 {
+		sourceList.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "", Kind: "List"})


I'm not sure whether we should set or not-set the GVK based on the result. This might lead to unexpected behaviour depending on the context where you run it. I think its better to just leave out the GVK on the list (or maybe even better, but not sure if this works), just return a slice of Unstructed instead of an UnstructuredList

This was done to show proper values if json or yaml format of list is requested and the list contains multiple types of source objects.

We'd need GVK set for the (list) object here as (json/yaml) printers require it. The question was which version/group to set if there are different types of sources found, so we set cleared group and version values and set List for kind.
If there is only one type of source object found OR user requested single type using --type filter, we keep the GVK intact.

Yes, but imagine the situation that by accident you have only one source of one type. If you list those, you the GVK set. Some time later another source of another type is added. Suddenly know the GVK gets removed or changed to an artificial "List". I think as we are dealing with a heterogenous list, that, by accident, can be also homogenous, we should treat it as a heterogenous list always. I would not mind to introduce a new type here, like we did for the Export, with an client.knative.dev group and a v1alpha1 version for now, but then use it all the time.

I would not mind to introduce a new type here, like we did for the Export, with an client.knative.dev group and a v1alpha1 version for now, but then use it all the time.

yea, we could do that, however for the other point about list of single source type, I think setting exact source type for single-type-of-source list is explicit and we could scope the client custom type for list only if there multiple types of sources.

Added #953 to track adding setting this custom client type for list. We can refine in next iteration.

pkg/dynamic/client.go

pkg/kn/commands/source/list.go

maximilien

Left some feedback. Mostly around potential dead code or lack of test coverage.

pkg/dynamic/client.go

pkg/kn/commands/source/list.go

pkg/kn/commands/source/list_types.go

daisy-ycguo

I'm fine with this PR, just a few small suggestions.

pkg/kn/commands/source/list_test.go

daisy-ycguo · 2020-07-28T16:24:30Z

pkg/dynamic/client.go

+}
+
+// ListSourcesUsingGVKs returns list of available source objects using given list of GVKs
+func (c *knDynamicClient) ListSourcesUsingGVKs(gvks *[]schema.GroupVersionKind, types ...WithType) (*unstructured.UnstructuredList, error) {


I wonder what is the best function behavior when gvks is nil: a) to return a nil list or b) to ignore gvks and use types to filter. The current behavior is a).
Yet when types is nil, the current behavior is b) to ignore types and use gvks to filter, not a) to return a nil list. It makes me a little weird.
Of course it doesn't affect the current functions. For future usage, maybe change to the same behavior when either of these two parameters is nil.

We use types only to subset the found source types. If gvks list is nil, it returns nil. Does this answer your question ?

knative-metrics-robot · 2020-07-30T04:12:23Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/dynamic/client.go	86.5%	89.7%	3.2
pkg/errors/factory.go	90.0%	95.7%	5.7
pkg/kn/commands/source/list.go	73.5%	70.3%	-3.3
pkg/kn/commands/source/list_types.go	70.8%	75.7%	4.8

navidshaikh · 2020-07-30T04:44:08Z

/retest

24.839s Ingress has not yet been reconciled.
Error: timeout: service 'hello' not ready after 600 seconds

maximilien · 2020-07-30T21:53:34Z

/test pull-knative-client-integration-tests

navidshaikh · 2020-07-31T05:00:43Z

/retest

27.413s Ingress has not yet been reconciled.

rhuss

/lgtm

Thanks !

knative-prow-robot · 2020-07-31T13:35:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

navidshaikh · 2020-08-04T10:40:44Z

/retest

* List inbuilt sources if CRD access is restricted Fixes knative#947 - Identify restricted access error - If server returns restricted access error, fallback to listing only eventing inbuilt sources using their GVKs. - List any inbuilt source (ApiServerSource) object and read the error to know if eventing is installed for `kn source list-types`. * Fix golint warnings * Remove unused imports * Verify each built in source before listing source types * Improve the check if sources are not installed in the cluster * Update finding forbidden error * Update finding errors * Add unit tests for IsForbiddenError util * Add unit tests * Add tests for dynamic pkg library * Add unit tests for case when no sources are installed * Update test name

* fix(docs): Fix tekton task link in README (#934) * Fix missing NAMESPACE column header (#951) * Fix missing NAMESPACE column header * Fix missing namespace column header for 'kn source list -A' * List inbuilt sources if CRD access is restricted (#948) * List inbuilt sources if CRD access is restricted Fixes #947 - Identify restricted access error - If server returns restricted access error, fallback to listing only eventing inbuilt sources using their GVKs. - List any inbuilt source (ApiServerSource) object and read the error to know if eventing is installed for `kn source list-types`. * Fix golint warnings * Remove unused imports * Verify each built in source before listing source types * Improve the check if sources are not installed in the cluster * Update finding forbidden error * Update finding errors * Add unit tests for IsForbiddenError util * Add unit tests * Add tests for dynamic pkg library * Add unit tests for case when no sources are installed * Update test name * Use Tekton Catalog GA structure for tasks (#966) * fix: `kn source list` command print spelling problems (#963) * Fix exit code on service delete and revision delete (#971) * Fix exit code 0 upon service delete * Mentioned bug fix in CHANGELOG.adoc * Add error check test for service not found * Fix for kn revision delete failure exit code and add test cases * Testing changes in test cases for failure in intergration tests * Fix test case error causing integration test failure * fix(volume): Generate volume name compliant with DNS Label names (#975) * fix(volume): Volume names to not contain dots Replace non alphanumberic characters with hyphen as the volumen name must be a DNS_LABEL (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names) ref: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#volume-v1-core * Generate volume name compliant with DNS Label names Volume names to follow the DNS label standard as defined in RFC 1123. This means the name must: - contain at most 63 characters - contain only lowercase alphanumeric characters or '-' - start with an alphanumeric character - end with an alphanumeric character * Set client custom GVK for source list for machine readable output (#980) - Use custom GVK {Group: client.knative.dev, Version: v1alpha1, Kind: SourceList} - Source list may contain different source types CO and machine readable output (using -o) requires List object to have GVK set, since the list contains different types of source COs, we set a custom client GVK on it. Fixes #953 * Update CHANGELOG for v0.16.1 Co-authored-by: kaustubh <[email protected]> Co-authored-by: Chris Suszynski <[email protected]> Co-authored-by: tianfeiyu <[email protected]> Co-authored-by: Himanshu Ranjan <[email protected]>

* List inbuilt sources if CRD access is restricted Fixes knative#947 - Identify restricted access error - If server returns restricted access error, fallback to listing only eventing inbuilt sources using their GVKs. - List any inbuilt source (ApiServerSource) object and read the error to know if eventing is installed for `kn source list-types`. * Fix golint warnings * Remove unused imports * Verify each built in source before listing source types * Improve the check if sources are not installed in the cluster * Update finding forbidden error * Update finding errors * Add unit tests for IsForbiddenError util * Add unit tests * Add tests for dynamic pkg library * Add unit tests for case when no sources are installed * Update test name

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 23, 2020

knative-prow-robot requested review from rhuss and sixolet July 23, 2020 10:55

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2020

knative-prow-robot reviewed Jul 23, 2020

View reviewed changes

pkg/dynamic/lib.go Show resolved Hide resolved

pkg/errors/factory.go Outdated Show resolved Hide resolved

pkg/sources/v1alpha2/client.go Outdated Show resolved Hide resolved

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 23, 2020

navidshaikh force-pushed the pr/source-list branch from e2f6ca3 to a0e527b Compare July 23, 2020 10:58

navidshaikh added 2 commits July 23, 2020 16:30

Fix golint warnings

3f36d7a

Remove unused imports

30a33ad

navidshaikh added 2 commits July 23, 2020 19:29

Verify each built in source before listing source types

0a8e024

Improve the check if sources are not installed in the cluster

eb264bc

rhuss reviewed Jul 24, 2020

View reviewed changes

navidshaikh added 4 commits July 27, 2020 12:28

Update finding forbidden error

4ee87b1

Update finding errors

5dc4893

Add unit tests for IsForbiddenError util

eeee24b

Add unit tests

381befd

navidshaikh changed the title ~~WIP: List inbuilt sources if CRD access is restricted~~ List inbuilt sources if CRD access is restricted Jul 27, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2020

Add tests for dynamic pkg library

eb58eb4

maximilien suggested changes Jul 27, 2020

View reviewed changes

pkg/dynamic/client.go Show resolved Hide resolved

pkg/kn/commands/source/list.go Show resolved Hide resolved

pkg/kn/commands/source/list_types.go Show resolved Hide resolved

pkg/kn/commands/source/list_types.go Show resolved Hide resolved

Add unit tests for case when no sources are installed

67eadfe

navidshaikh mentioned this pull request Jul 28, 2020

[source list] Add custom type for different types of source objects #953

Closed

daisy-ycguo reviewed Jul 28, 2020

View reviewed changes

Update test name

ac9988a

rhuss approved these changes Jul 31, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jul 31, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2020

knative-prow-robot merged commit cc1b68e into knative:master Aug 4, 2020

navidshaikh deleted the pr/source-list branch August 5, 2020 07:47

navidshaikh added the backport/candidate Consider this PR to be backported to the release branch label Aug 18, 2020

navidshaikh mentioned this pull request Aug 18, 2020

v0.16.1 release backports #982

Merged

navidshaikh added backported-to/0.16 and removed backport/candidate Consider this PR to be backported to the release branch labels Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List inbuilt sources if CRD access is restricted #948

List inbuilt sources if CRD access is restricted #948

navidshaikh commented Jul 23, 2020 •

edited

Loading

knative-prow-robot left a comment

navidshaikh commented Jul 23, 2020

rhuss left a comment

rhuss Jul 24, 2020

navidshaikh Jul 24, 2020

navidshaikh Jul 24, 2020

rhuss Jul 28, 2020

navidshaikh Jul 28, 2020

navidshaikh Jul 28, 2020

maximilien left a comment

daisy-ycguo left a comment

daisy-ycguo Jul 28, 2020

navidshaikh Jul 30, 2020

knative-metrics-robot commented Jul 30, 2020

navidshaikh commented Jul 30, 2020

maximilien commented Jul 30, 2020

navidshaikh commented Jul 31, 2020

rhuss left a comment

knative-prow-robot commented Jul 31, 2020

navidshaikh commented Aug 4, 2020

List inbuilt sources if CRD access is restricted #948

List inbuilt sources if CRD access is restricted #948

Conversation

navidshaikh commented Jul 23, 2020 • edited Loading

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

navidshaikh commented Jul 23, 2020

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

daisy-ycguo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jul 30, 2020

navidshaikh commented Jul 30, 2020

maximilien commented Jul 30, 2020

navidshaikh commented Jul 31, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 31, 2020

navidshaikh commented Aug 4, 2020

navidshaikh commented Jul 23, 2020 •

edited

Loading