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

Channel conformance tests for spec.subscriber #3050

Conversation

aliok
Copy link
Member

@aliok aliok commented Apr 26, 2020

Fixes #2988

Proposed Changes

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 26, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 26, 2020
@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Apr 26, 2020
@aliok
Copy link
Member Author

aliok commented Apr 26, 2020

Meta Channel (channel.messaging.knative.dev/v1alpha1) doesn't allow specifying the subscribers in the spec, thus the conformance test fails.

Following is a simplified example of the failure:

cat <<EOS |kubectl apply -f -
---
apiVersion: messaging.knative.dev/v1alpha1
kind: Channel
metadata:
  name: foo
spec:
  subscribable:
    subscribers:
    - UID: "1234"
      ReplyUri: "foo.bar"
EOS

ends up in

Error from server (BadRequest): error when creating "channel_v1alpha1.yaml": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: must not set the field(s): spec.subscribable.subscribers

Created #3051

}

// SPEC: each channel CRD MUST contain an array of subscribers: spec.subscribers
channelable.Spec.Subscribers = []eventingduckv1beta1.SubscriberSpec{
Copy link
Member

Choose a reason for hiding this comment

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

@lionelvillard the spec.subscribers are only populated by the Subscription, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some new info: #3051 (comment)

@aliok
Copy link
Member Author

aliok commented May 5, 2020

/retest

@matzew
Copy link
Member

matzew commented May 8, 2020

/retest

@aliok aliok force-pushed the channel_conformance_test_spec_subscriber branch from 123b020 to ad7673e Compare May 8, 2020 06:39
@aliok
Copy link
Member Author

aliok commented May 8, 2020

#3051 is fixed

This PR is rebased now, let's see what happens

UPDATE: that fixed issue is not related to this one but it is related to #3037

@lberk
Copy link
Member

lberk commented May 11, 2020

once #3129 is merged, retest and I'm hoping this conformance test should pass (passed locally 🤞 )

@lberk
Copy link
Member

lberk commented May 11, 2020

/reopen
/retest

@knative-prow-robot
Copy link
Contributor

@lberk: Reopened this PR.

In response to this:

/reopen
/retest

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.

@lberk
Copy link
Member

lberk commented May 11, 2020

/test pull-knative-eventing-integration-tests

@aliok
Copy link
Member Author

aliok commented May 12, 2020

@lberk
This PR won't pass until we change the spec and also change the tests themselves to skip the CRD spec.subscribers tests for Channel. See #3051

#3129 is again about status.subscribers (I think), not spec.subscribers.

@lberk
Copy link
Member

lberk commented May 12, 2020

I chatted with @aliok this morning, and perhaps I need a bit more clarification as I look over this PR (and how it relates to #3051)

Relevant part of the spec is:

Spec Requirements
v1alpha1 Spec
Each channel CRD MUST contain an array of subscribers: spec.subscribable.subscribers

v1beta1 Spec
Each channel CRD MUST contain an array of subscribers: spec.subscribers

Part of this conformance test sets these subscriber fields directly (which is landing in an error[1]). I think in order to correct this conformance test, we need to have the client create a subscription targeting the channel, and then verify that the channel then mirrors that subscription to the spec array. Not set the subscribers field directly.

To me this means the spec is still valid (though perhaps could be tweaked to suggest the array of subscribers should not be set directly, but via subscriptions).

Thoughts? Suggestions? @aliok @matzew @vaikas

[1] - TestChannelSpec/TestChannelSpec-{Channel_messaging.knative.dev/v1alpha1}: channel_spec_test_helper.go:119: Error updating {"Channel" "messaging.knative.dev/v1alpha1"} with subscribers in spec: "admission webhook \"validation.webhook.eventing.knative.dev\" denied the request: validation failed: must not set the field(s): spec.subscribable.subscribers"

@aliok
Copy link
Member Author

aliok commented May 12, 2020

we need to have the client create a subscription targeting the channel, and then verify that the channel then mirrors that subscription to the spec array. Not set the subscribers field directly.

Yeah, I can change the test that way.

  • Create channel
  • Create subscription
  • Wait for the subscription to settle
  • Check the Channel CR spec.subscribers

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2020
@aliok aliok force-pushed the channel_conformance_test_spec_subscriber branch from 0041836 to 28b90a9 Compare May 14, 2020 07:17
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2020
@aliok
Copy link
Member Author

aliok commented May 14, 2020

/assign @lberk

want to review?

Copy link
Contributor

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

Nice tests! A few nits.

test/conformance/helpers/channel.go Outdated Show resolved Hide resolved
})
}

func channelSpecAllowsSubscribersArray(st *testing.T, client *lib.Client, channel metav1.TypeMeta, options ...lib.SetupClientOption) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the reason behind this st variable name when other tests seem to unanimously use t.

Copy link
Member

Choose a reason for hiding this comment

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

single or specific test perhaps? I've seen tt vs t used in other spots as well iirc, I'm not sure we need to bike shed the variable name too much?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like I said, just nits

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen that's setup that way in other conformance tests and used it that way.

But I think it could make sense.

st is defined here in the test runner call: https://github.com/knative/eventing/pull/3050/files#diff-5d560f68ffc10b7bccae5ab980f0ffddR41

Let's ignore that here in this PR and if we would like to change that var name, let's do that in a separate PR for all other tests as well.

client.WaitForResourceReadyOrFail(channelName, &channel)

sampleUrl, _ := apis.ParseURL("http://example.com")
gvr, _ := meta.UnsafeGuessKindToResource(channel.GroupVersionKind())
Copy link
Contributor

@antoineco antoineco May 14, 2020

Choose a reason for hiding this comment

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

import "knative.dev/eventing/pkg/apis/messaging"

// ...
    gvr := messaging.ChannelsResource.String()

Copy link
Member Author

Choose a reason for hiding this comment

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

channel here is actually passed by the test runner and it is different in each execution of the test.

  • IMC v1alpha1
  • IMC v1beta1
  • Channel v1alpha1

Copy link
Contributor

Choose a reason for hiding this comment

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

OK makes sense now

// treat missing annotation value as v1alpha1, as written in the spec
channelable, err := getChannelAsV1Alpha1Channelable(channelName, client, channel)
if err != nil {
st.Fatalf("Unable to get channel %q to v1alpha1 duck type: %q", channel, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The error doesn't need quotes I believe. (comment applies to other occurrences as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

Converted all of them now to %s

Comment on lines +82 to +85
{
UID: "1234",
ReplyURI: sampleUrl,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it even necessary to add an element to the array for this assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is actually better to have an element inside the array instead of setting an empty array

@antoineco
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
@matzew
Copy link
Member

matzew commented May 15, 2020

/lgtm
/approve

@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 May 15, 2020
@knative-prow-robot knative-prow-robot merged commit f624e65 into knative:master May 15, 2020
@aliok aliok deleted the channel_conformance_test_spec_subscriber branch May 15, 2020 11:56
@lberk lberk mentioned this pull request Sep 1, 2020
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 tests for channel CRD spec requirements
6 participants