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.

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

Add channel metadata conformance tests #2963

Merged

Conversation

aliok
Copy link
Member

@aliok aliok commented Apr 10, 2020

Fixes #1960

Proposed Changes

Implements channel metadata conformance tests. There are 3 tests described in the issue #1960.

  1. Checking if the channel CRD is namespaced (and not cluster scoped)
  2. Checking if the channel CRD has the labels messaging.knative.dev/subscribable=true and duck.knative.dev/addressable=true
  3. Checking if the channel CRD has the category channel

1 and 3 is done by checking the ApiResource.
For 2, I had to actually get the CRD and check if it has the label.

Release Note

NONE

How to run the added tests?

go test -v -timeout 30s -tags e2e knative.dev/eventing/test/conformance -run ^TestChannelCRDMetadata$ -channels messaging.knative.dev/v1alpha1:InMemoryChannel,messaging.knative.dev/v1alpha1:Channel,messaging.knative.dev/v1beta1:InMemoryChannel

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 10, 2020
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Apr 10, 2020
@aliok aliok marked this pull request as draft April 10, 2020 21:14
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

test/conformance/helpers/channel_metadata_test_helper.go Outdated Show resolved Hide resolved
test/conformance/helpers/channel_metadata_test_helper.go Outdated Show resolved Hide resolved
@aliok
Copy link
Member Author

aliok commented Apr 10, 2020

/retest

@knative-prow-robot 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 Apr 10, 2020
@aliok
Copy link
Member Author

aliok commented Apr 10, 2020

Not sure why this fails.

Doing the following works locally for me on my Minikube:

go test -v -timeout 30s -tags e2e knative.dev/eventing/test/conformance -run ^TestChannelCRDMetadata$ -channels messaging.knative.dev/v1alpha1:InMemoryChannel,messaging.knative.dev/v1alpha1:Channel,messaging.knative.dev/v1beta1:InMemoryChannel

@aliok
Copy link
Member Author

aliok commented Apr 11, 2020

The test is unable to get the CRDs. It cannot list as well.
I did a small experiment here: #2965, specifically https://github.com/knative/eventing/pull/2965/files?file-filters%5B%5D=.go&file-filters%5B%5D=.sh&hide-deleted-files=true#diff-b48ad686db0c8498a33308be0d585ac4R50 .
Prow line: https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_eventing/2965/pull-knative-eventing-integration-tests/1249094584215015425#1:build-log.txt%3A496

The call client.Apiextensions.CustomResourceDefinitions().List(metav1.ListOptions{}) returns the server could not find the requested resource. A Get call returns the same too.

I think the service account that is used to run the tests should be updated to have at least get verb to CRDs. No idea who to ping for this.
Why this is needed is in the PR description.

@aliok aliok marked this pull request as ready for review April 11, 2020 21:30
@knative-prow-robot 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 Apr 11, 2020
@aliok
Copy link
Member Author

aliok commented Apr 14, 2020

cc @chaodaiG @n3wscott
TLDR; can we give get verb permissions for apiextensions.k8s.io CustomResourceDefinition to the service account that's running the tests on Prow?

@chaodaiG
Copy link
Contributor

@chizhg is oncall this week, please investigate what's needed to make it work

@aliok
Copy link
Member Author

aliok commented Apr 15, 2020

@chizhg

get and list permissions for apiextensions.k8s.io CustomResourceDefinition is needed at the service account that's running the tests on Prow.
I spent some time in knative/test-infra but couldn't figure it out myself.

@aliok aliok mentioned this pull request Apr 20, 2020
@aliok aliok force-pushed the channel_metadata_conformance_tests_2 branch from 1e4b450 to 64d0882 Compare April 22, 2020 07:23
@aliok
Copy link
Member Author

aliok commented Apr 22, 2020

I found the problem. apiextensions is v1beta1 in GKE, not v1.

I was checking the wrong things around RBAC, service accounts, permissions etc. the whole time 🤦‍♂️

sorry for misleading you and thanks for your help

all is green now.

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, matzew

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020
@knative-prow-robot knative-prow-robot merged commit 7a2e16a into knative:master Apr 22, 2020
@aliok aliok deleted the channel_metadata_conformance_tests_2 branch April 22, 2020 08:06
@aliok
Copy link
Member Author

aliok commented Apr 22, 2020

ping @pierDipi
if you want to do something similar to knative/eventing-contrib#1154 with these tests

@pierDipi
Copy link
Member

@aliok sure, I will let you know when it's ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conformance testing for channel metadata
7 participants