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.

Sign up for GitHub

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

Fix Panic for kn source apiserver and kn source binding describe with Sink URI #901

Merged
merged 5 commits into from
Jun 25, 2020
Merged

Fix Panic for kn source apiserver and kn source binding describe with Sink URI #901

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e820ac9
fix panic for apiserver and binding describe and update tests
danielhelfand Jun 22, 2020
2410198
add changelog entry
danielhelfand Jun 22, 2020
7d049f5
include namespace and kind for sink in test coverage
danielhelfand Jun 22, 2020
7a3807e
assert no URI in non Sink URI test cases
danielhelfand Jun 23, 2020
395673e
add createSinkv1 for creating Sink test data
danielhelfand Jun 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
|===
| | Description | PR

| 🐛
| Fix Panic for `kn source apiserver` and `kn source binding` describe with Sink URI
danielhelfand marked this conversation as resolved.
Show resolved Hide resolved
| https://github.com/knative/client/pull/901[#901]

| 🐛
| Fix Panic for `kn trigger describe` with Sink URI
| https://github.com/knative/client/pull/900[#900]
Expand Down
32 changes: 21 additions & 11 deletions pkg/kn/commands/source/apiserver/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

"k8s.io/client-go/tools/clientcmd"
v1alpha2 "knative.dev/eventing/pkg/apis/sources/v1alpha2"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"

kndynamic "knative.dev/client/pkg/dynamic"
Expand All @@ -29,8 +30,25 @@ import (

const testNamespace = "default"

// Helper methods
var blankConfig clientcmd.ClientConfig
var (
sinkRef = duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "Service",
Name: "testsvc",
APIVersion: "serving.knative.dev/v1",
Namespace: "default",
},
}

sinkURI = duckv1.Destination{
URI: &apis.URL{
Scheme: "https",
Host: "foo",
},
}
danielhelfand marked this conversation as resolved.
Show resolved Hide resolved

blankConfig clientcmd.ClientConfig
)

// TODO: Remove that blankConfig hack for tests in favor of overwriting GetConfig()
func init() {
Expand Down Expand Up @@ -83,20 +101,12 @@ func cleanupAPIServerMockClient() {
apiServerSourceClientFactory = nil
}

func createAPIServerSource(name, resourceKind, resourceVersion, serviceAccount, mode, service string, ceOverrides map[string]string) *v1alpha2.ApiServerSource {
func createAPIServerSource(name, resourceKind, resourceVersion, serviceAccount, mode string, ceOverrides map[string]string, sink duckv1.Destination) *v1alpha2.ApiServerSource {
resources := []v1alpha2.APIVersionKindSelector{{
APIVersion: resourceVersion,
Kind: resourceKind,
}}

sink := duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "Service",
Name: service,
APIVersion: "serving.knative.dev/v1",
Namespace: "default",
}}

return clientv1alpha2.NewAPIServerSourceBuilder(name).
Resources(resources).
ServiceAccount(serviceAccount).
Expand Down
4 changes: 2 additions & 2 deletions pkg/kn/commands/source/apiserver/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ func TestCreateApiServerSource(t *testing.T) {
apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t)

apiServerRecorder := apiServerClient.Recorder()
apiServerRecorder.CreateAPIServerSource(createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", "testsvc", map[string]string{"bla": "blub", "foo": "bar"}), nil)
apiServerRecorder.CreateAPIServerSource(createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", map[string]string{"bla": "blub", "foo": "bar"}, sinkRef), nil)

out, err := executeAPIServerSourceCommand(apiServerClient, dynamicClient, "create", "testsource", "--resource", "Event:v1", "--service-account", "testsa", "--sink", "svc:testsvc", "--mode", "Reference", "--ce-override", "bla=blub", "--ce-override", "foo=bar")
assert.NilError(t, err, "ApiServer source should be created")
util.ContainsAll(out, "created", "default", "testsource")
assert.Assert(t, util.ContainsAll(out, "created", "default", "testsource"))

apiServerRecorder.Validate()
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/kn/commands/source/apiserver/delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestApiServerSourceDelete(t *testing.T) {

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "delete", "testsource")
assert.NilError(t, err)
util.ContainsAll(out, "deleted", "testns", "testsource")
assert.Assert(t, util.ContainsAll(out, "deleted", "default", "testsource"))

apiServerRecorder.Validate()
}
Expand All @@ -47,7 +47,7 @@ func TestDeleteWithError(t *testing.T) {

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "delete", "testsource")
assert.ErrorContains(t, err, "testsource")
util.ContainsAll(out, "apiserver", "source", "testsource", "not found")
assert.Assert(t, util.ContainsAll(out, "apiserver", "source", "testsource", "not found"))

apiServerRecorder.Validate()
}
5 changes: 2 additions & 3 deletions pkg/kn/commands/source/apiserver/describe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (

// NewAPIServerDescribeCommand to describe an ApiServer source object
func NewAPIServerDescribeCommand(p *commands.KnParams) *cobra.Command {

apiServerDescribe := &cobra.Command{
Use: "describe NAME",
Short: "Show details of an api-server source",
Expand Down Expand Up @@ -110,10 +109,10 @@ func writeResources(dw printers.PrefixWriter, apiVersionKindSelectors []v1alpha2

func writeSink(dw printers.PrefixWriter, sink duckv1.Destination) {
subWriter := dw.WriteAttribute("Sink", "")
subWriter.WriteAttribute("Name", sink.Ref.Name)
subWriter.WriteAttribute("Namespace", sink.Ref.Namespace)
ref := sink.Ref
if ref != nil {
subWriter.WriteAttribute("Name", sink.Ref.Name)
subWriter.WriteAttribute("Namespace", sink.Ref.Namespace)
danielhelfand marked this conversation as resolved.
Show resolved Hide resolved
subWriter.WriteAttribute("Kind", fmt.Sprintf("%s (%s)", sink.Ref.Kind, sink.Ref.APIVersion))
danielhelfand marked this conversation as resolved.
Show resolved Hide resolved
}
uri := sink.URI
Expand Down
20 changes: 17 additions & 3 deletions pkg/kn/commands/source/apiserver/describe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ func TestSimpleDescribe(t *testing.T) {
apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t, "mynamespace")

apiServerRecorder := apiServerClient.Recorder()
sampleSource := createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", "testsvc", map[string]string{"foo": "bar"})
sampleSource := createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", map[string]string{"foo": "bar"}, sinkRef)
apiServerRecorder.GetAPIServerSource("testsource", sampleSource, nil)

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "describe", "testsource")
assert.NilError(t, err)
util.ContainsAll(out, "testsource", "testsa", "Reference", "testsvc", "Service", "Resources", "Event", "v1", "false", "Conditions", "foo", "bar")
assert.Assert(t, util.ContainsAll(out, "testsource", "testsa", "Reference", "testsvc", "Service", "Resources", "Event", "v1", "Conditions", "foo", "bar"))

apiServerRecorder.Validate()
}
Expand All @@ -46,7 +46,21 @@ func TestDescribeError(t *testing.T) {

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "describe", "testsource")
assert.ErrorContains(t, err, "testsource")
util.ContainsAll(out, "Usage", "testsource")
assert.Assert(t, util.ContainsAll(out, "Usage", "testsource"))

apiServerRecorder.Validate()
}

func TestDescribeWithSinkURI(t *testing.T) {
apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t, "mynamespace")

apiServerRecorder := apiServerClient.Recorder()
sampleSource := createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", map[string]string{"foo": "bar"}, sinkURI)
apiServerRecorder.GetAPIServerSource("testsource", sampleSource, nil)

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "describe", "testsource")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(out, "testsource", "testsa", "Reference", "Service", "Resources", "Event", "v1", "Conditions", "foo", "bar", "URI", "https", "foo"))

apiServerRecorder.Validate()
}
10 changes: 5 additions & 5 deletions pkg/kn/commands/source/apiserver/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,16 @@ func TestListAPIServerSource(t *testing.T) {
apiServerClient := v1alpha22.NewMockKnAPIServerSourceClient(t)

apiServerRecorder := apiServerClient.Recorder()
sampleSource := createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", "testsvc", nil)
sampleSource := createAPIServerSource("testsource", "Event", "v1", "testsa", "Reference", nil, sinkRef)
sampleSourceList := v1alpha2.ApiServerSourceList{}
sampleSourceList.Items = []v1alpha2.ApiServerSource{*sampleSource}

apiServerRecorder.ListAPIServerSource(&sampleSourceList, nil)

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "list")
assert.NilError(t, err, "sources should be listed")
util.ContainsAll(out, "NAME", "RESOURCES", "SINK", "AGE", "CONDITIONS", "READY", "REASON")
util.ContainsAll(out, "testsource", "Eventing:v1:false", "mysvc")
assert.Assert(t, util.ContainsAll(out, "NAME", "RESOURCES", "SINK", "AGE", "CONDITIONS", "READY", "REASON"))
assert.Assert(t, util.ContainsAll(out, "testsource", "Event:v1", "svc:testsvc"))

apiServerRecorder.Validate()
}
Expand All @@ -53,8 +53,8 @@ func TestListAPIServerSourceEmpty(t *testing.T) {

out, err := executeAPIServerSourceCommand(apiServerClient, nil, "list")
assert.NilError(t, err, "Sources should be listed")
util.ContainsNone(out, "NAME", "RESOURCES", "SINK", "AGE", "CONDITIONS", "READY", "REASON")
util.ContainsAll(out, "No", "ApiServer", "source", "found")
assert.Assert(t, util.ContainsNone(out, "NAME", "RESOURCES", "SINK", "AGE", "CONDITIONS", "READY", "REASON"))
assert.Assert(t, util.ContainsAll(out, "No", "ApiServer", "source", "found"))

apiServerRecorder.Validate()
}
27 changes: 24 additions & 3 deletions pkg/kn/commands/source/apiserver/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,34 @@ import (
"gotest.tools/assert"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
duckv1 "knative.dev/pkg/apis/duck/v1"
servingv1 "knative.dev/serving/pkg/apis/serving/v1"

dynamicfake "knative.dev/client/pkg/dynamic/fake"
"knative.dev/client/pkg/sources/v1alpha2"
"knative.dev/client/pkg/util"
)

var (
sinkRefsvc1 = duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "Service",
Name: "svc1",
APIVersion: "serving.knative.dev/v1",
Namespace: "default",
},
}

sinkRefsvc2 = duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "Service",
Name: "svc2",
APIVersion: "serving.knative.dev/v1",
Namespace: "default",
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about function sinkv1(service, namespace, string) and use that inline wherever needed ?

(could be done in a follow up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a createSinkv1 func in apiserver_test.go. Turns out this same func exists for binding already as createServiceSink. If there is a location to recommend, we could use the same func for both, but let me know what's preferred with regard to that.

)

func TestApiServerSourceUpdate(t *testing.T) {
apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t)
dynamicClient := dynamicfake.CreateFakeKnDynamicClient("default", &servingv1.Service{
Expand All @@ -37,10 +58,10 @@ func TestApiServerSourceUpdate(t *testing.T) {

apiServerRecorder := apiServerClient.Recorder()

present := createAPIServerSource("testsource", "Event", "v1", "testsa1", "Reference", "svc1", map[string]string{"bla": "blub", "foo": "bar"})
present := createAPIServerSource("testsource", "Event", "v1", "testsa1", "Reference", map[string]string{"bla": "blub", "foo": "bar"}, sinkRefsvc1)
apiServerRecorder.GetAPIServerSource("testsource", present, nil)

updated := createAPIServerSource("testsource", "Event", "v1", "testsa2", "Reference", "svc2", map[string]string{"foo": "baz"})
updated := createAPIServerSource("testsource", "Event", "v1", "testsa2", "Reference", map[string]string{"foo": "baz"}, sinkRefsvc2)
apiServerRecorder.UpdateAPIServerSource(updated, nil)

output, err := executeAPIServerSourceCommand(apiServerClient, dynamicClient, "update", "testsource", "--service-account", "testsa2", "--sink", "svc:svc2", "--ce-override", "bla-", "--ce-override", "foo=baz")
Expand All @@ -54,7 +75,7 @@ func TestApiServerSourceUpdateDeletionTimestampNotNil(t *testing.T) {
apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t)
apiServerRecorder := apiServerClient.Recorder()

present := createAPIServerSource("testsource", "Event", "v1", "testsa1", "Ref", "svc1", nil)
present := createAPIServerSource("testsource", "Event", "v1", "testsa1", "Ref", nil, sinkRefsvc1)
present.DeletionTimestamp = &metav1.Time{Time: time.Now()}
apiServerRecorder.GetAPIServerSource("testsource", present, nil)

Expand Down
2 changes: 1 addition & 1 deletion pkg/kn/commands/source/binding/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestSimpleCreateBinding(t *testing.T) {

out, err := executeSinkBindingCommand(bindingClient, dynamicClient, "create", "testbinding", "--sink", "svc:mysvc", "--subject", "deployment:apps/v1:mydeploy", "--ce-override", "bla=blub", "--ce-override", "foo=bar")
assert.NilError(t, err, "Source should have been created")
util.ContainsAll(out, "created", "default", "testbinding")
assert.Assert(t, util.ContainsAll(out, "created", "default", "testbinding"))

bindingRecorder.Validate()
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/kn/commands/source/binding/delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestSimpleDelete(t *testing.T) {

out, err := executeSinkBindingCommand(bindingClient, nil, "delete", "mybinding")
assert.NilError(t, err)
util.ContainsAll(out, "deleted", "mynamespace", "mybinding", "sink binding")
assert.Assert(t, util.ContainsAll(out, "deleted", "mynamespace", "mybinding", "Sink binding"))

bindingRecorder.Validate()
}
Expand All @@ -47,7 +47,7 @@ func TestDeleteWithError(t *testing.T) {

out, err := executeSinkBindingCommand(bindingClient, nil, "delete", "mybinding")
assert.ErrorContains(t, err, "mybinding")
util.ContainsAll(out, "no such", "mybinding")
assert.Assert(t, util.ContainsAll(out, "no such", "mybinding"))

bindingRecorder.Validate()
}
8 changes: 4 additions & 4 deletions pkg/kn/commands/source/binding/describe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,12 @@ func writeSinkBinding(dw printers.PrefixWriter, binding *v1alpha2.SinkBinding, p

func writeSink(dw printers.PrefixWriter, namespace string, sink *duckv1.Destination) {
subWriter := dw.WriteAttribute("Sink", "")
if sink.Ref.Namespace != "" && sink.Ref.Namespace != namespace {
subWriter.WriteAttribute("Namespace", sink.Ref.Namespace)
}
subWriter.WriteAttribute("Name", sink.Ref.Name)
ref := sink.Ref
if ref != nil {
subWriter.WriteAttribute("Name", sink.Ref.Name)
if sink.Ref.Namespace != "" && sink.Ref.Namespace != namespace {
subWriter.WriteAttribute("Namespace", sink.Ref.Namespace)
danielhelfand marked this conversation as resolved.
Show resolved Hide resolved
}
subWriter.WriteAttribute("Resource", fmt.Sprintf("%s (%s)", sink.Ref.Kind, sink.Ref.APIVersion))
}
uri := sink.URI
Expand Down
51 changes: 38 additions & 13 deletions pkg/kn/commands/source/binding/describe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"gotest.tools/assert"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1alpha2 "knative.dev/eventing/pkg/apis/sources/v1alpha2"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/apis/duck/v1alpha1"
"knative.dev/pkg/tracker"
Expand All @@ -30,15 +31,32 @@ import (
"knative.dev/client/pkg/util"
)

var (
sinkRef = duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "Service",
Namespace: "myservicenamespace",
Name: "mysvc",
},
}

sinkURI = duckv1.Destination{
URI: &apis.URL{
Scheme: "https",
Host: "foo",
},
}
)

func TestSimpleDescribeWitName(t *testing.T) {
bindingClient := clientv1alpha2.NewMockKnSinkBindingClient(t, "mynamespace")

bindingRecorder := bindingClient.Recorder()
bindingRecorder.GetSinkBinding("mybinding", getSinkBindingSource("myapp", map[string]string{"foo": "bar"}), nil)
bindingRecorder.GetSinkBinding("mybinding", getSinkBindingSource("myapp", map[string]string{"foo": "bar"}, sinkRef), nil)

out, err := executeSinkBindingCommand(bindingClient, nil, "describe", "mybinding")
assert.NilError(t, err)
util.ContainsAll(out, "mybinding", "myapp", "Deployment", "app/v1", "mynamespace", "mysvc", "foo", "bar")
assert.Assert(t, util.ContainsAll(out, "mysinkbinding", "myapp", "Deployment", "apps/v1", "mynamespace", "mysvc", "foo", "bar"))

bindingRecorder.Validate()
}
Expand All @@ -47,11 +65,11 @@ func TestSimpleDescribeWithSelector(t *testing.T) {
bindingClient := clientv1alpha2.NewMockKnSinkBindingClient(t, "mynamespace")

bindingRecorder := bindingClient.Recorder()
bindingRecorder.GetSinkBinding("mybinding", getSinkBindingSource("app=myapp,type=test", nil), nil)
bindingRecorder.GetSinkBinding("mybinding", getSinkBindingSource("app=myapp,type=test", nil, sinkRef), nil)

out, err := executeSinkBindingCommand(bindingClient, nil, "describe", "mybinding")
assert.NilError(t, err)
util.ContainsAll(out, "mybinding", "app:", "myapp", "type:", "test", "Deployment", "app/v1", "mynamespace", "mysvc")
assert.Assert(t, util.ContainsAll(out, "mysinkbinding", "app:", "myapp", "type:", "test", "Deployment", "apps/v1", "mynamespace", "mysvc"))

bindingRecorder.Validate()
}
Expand All @@ -64,26 +82,33 @@ func TestDescribeError(t *testing.T) {

out, err := executeSinkBindingCommand(bindingClient, nil, "describe", "mybinding")
assert.ErrorContains(t, err, "mybinding")
util.ContainsAll(out, "mybinding")
assert.Assert(t, util.ContainsAll(out, "mybinding"))

bindingRecorder.Validate()
}

func TestDescribeWithSinkURI(t *testing.T) {
bindingClient := clientv1alpha2.NewMockKnSinkBindingClient(t, "mynamespace")

bindingRecorder := bindingClient.Recorder()
bindingRecorder.GetSinkBinding("mybinding", getSinkBindingSource("myapp", map[string]string{"foo": "bar"}, sinkURI), nil)

out, err := executeSinkBindingCommand(bindingClient, nil, "describe", "mybinding")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(out, "mysinkbinding", "myapp", "Deployment", "apps/v1", "mynamespace", "foo", "bar", "URI", "https", "foo"))

bindingRecorder.Validate()
}

func getSinkBindingSource(nameOrSelector string, ceOverrides map[string]string) *v1alpha2.SinkBinding {
func getSinkBindingSource(nameOrSelector string, ceOverrides map[string]string, sink duckv1.Destination) *v1alpha2.SinkBinding {
binding := &v1alpha2.SinkBinding{
TypeMeta: v1.TypeMeta{},
ObjectMeta: v1.ObjectMeta{
Name: "mysinkbinding",
},
Spec: v1alpha2.SinkBindingSpec{
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
Kind: "Service",
Namespace: "myservicenamespace",
Name: "mysvc",
},
},
Sink: sink,
},
BindingSpec: v1alpha1.BindingSpec{
Subject: tracker.Reference{
Expand Down
Loading