diff --git a/docs/cmd/kn_source_apiserver_create.md b/docs/cmd/kn_source_apiserver_create.md index 28df5a8f7c..4fe35c3fb1 100644 --- a/docs/cmd/kn_source_apiserver_create.md +++ b/docs/cmd/kn_source_apiserver_create.md @@ -26,8 +26,9 @@ kn source apiserver create NAME --resource RESOURCE --service-account ACCOUNTNAM "Ref" sends only the reference to the resource, "Resource" send the full resource. (default "Ref") -n, --namespace string Specify the namespace to operate in. - --resource stringArray Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true. - "isController" can be omitted and is "false" by default. + --resource stringArray Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. "Deployment:v1:true". + "APIVersion" can be omitted and is "v1" by default, e.g. "Deployment::true"; + "isController" can be omitted and is "false" by default, e.g. "Deployment:v1" or just "Deployment". --service-account string Name of the service account to use to run this source -s, --sink string Addressable sink for events ``` diff --git a/docs/cmd/kn_source_apiserver_update.md b/docs/cmd/kn_source_apiserver_update.md index 9face71071..995b936291 100644 --- a/docs/cmd/kn_source_apiserver_update.md +++ b/docs/cmd/kn_source_apiserver_update.md @@ -26,8 +26,9 @@ kn source apiserver update NAME --resource RESOURCE --service-account ACCOUNTNAM "Ref" sends only the reference to the resource, "Resource" send the full resource. (default "Ref") -n, --namespace string Specify the namespace to operate in. - --resource stringArray Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true. - "isController" can be omitted and is "false" by default. + --resource stringArray Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. "Deployment:v1:true". + "APIVersion" can be omitted and is "v1" by default, e.g. "Deployment::true"; + "isController" can be omitted and is "false" by default, e.g. "Deployment:v1" or just "Deployment". --service-account string Name of the service account to use to run this source -s, --sink string Addressable sink for events ``` diff --git a/pkg/eventing/sources/v1alpha1/apiserver_client.go b/pkg/eventing/sources/v1alpha1/apiserver_client.go index 409300d584..3a29ffd20a 100644 --- a/pkg/eventing/sources/v1alpha1/apiserver_client.go +++ b/pkg/eventing/sources/v1alpha1/apiserver_client.go @@ -15,9 +15,6 @@ package v1alpha1 import ( - "fmt" - "reflect" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kn_errors "knative.dev/client/pkg/errors" "knative.dev/eventing/pkg/apis/sources/v1alpha1" @@ -159,43 +156,6 @@ func (b *APIServerSourceBuilder) Resources(resources []v1alpha1.ApiServerResourc return b } -// AddResource which should be streamed -func (b *APIServerSourceBuilder) AddResource(version string, kind string, isController bool) *APIServerSourceBuilder { - resources := b.apiServerSource.Spec.Resources - if resources == nil { - resources = []v1alpha1.ApiServerResource{} - b.apiServerSource.Spec.Resources = resources - } - resourceRef := v1alpha1.ApiServerResource{ - APIVersion: version, - Kind: kind, - Controller: isController, - } - b.apiServerSource.Spec.Resources = append(resources, resourceRef) - return b -} - -// RemoveResource which should be streamed -func (b *APIServerSourceBuilder) RemoveResource(version string, kind string, isController bool) (*APIServerSourceBuilder, error) { - resources := b.apiServerSource.Spec.Resources - if resources == nil { - resources = []v1alpha1.ApiServerResource{} - b.apiServerSource.Spec.Resources = resources - } - resourceRef := v1alpha1.ApiServerResource{ - APIVersion: version, - Kind: kind, - Controller: isController, - } - for i, k := range resources { - if reflect.DeepEqual(k, resourceRef) { - resources = append(resources[:i], resources[i+1:]...) - return b, nil - } - } - return b, fmt.Errorf("cannot find resource %s:%s:%t to remove", version, kind, isController) -} - // ServiceAccount with which this source should operate func (b *APIServerSourceBuilder) ServiceAccount(sa string) *APIServerSourceBuilder { b.apiServerSource.Spec.ServiceAccountName = sa diff --git a/pkg/kn/commands/source/apiserver/create.go b/pkg/kn/commands/source/apiserver/create.go index dd1f432561..be750cb6db 100644 --- a/pkg/kn/commands/source/apiserver/create.go +++ b/pkg/kn/commands/source/apiserver/create.go @@ -71,9 +71,7 @@ func NewAPIServerCreateCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } - for _, k := range resources { - b.AddResource(k.ApiVersion, k.Kind, k.IsController) - } + b.Resources(resources) err = apiSourceClient.CreateAPIServerSource(b.Build()) diff --git a/pkg/kn/commands/source/apiserver/flags.go b/pkg/kn/commands/source/apiserver/flags.go index ac516b4d9b..47490874ec 100644 --- a/pkg/kn/commands/source/apiserver/flags.go +++ b/pkg/kn/commands/source/apiserver/flags.go @@ -16,6 +16,7 @@ package apiserver import ( "fmt" + "reflect" "sort" "strconv" "strings" @@ -41,15 +42,9 @@ type APIServerSourceUpdateFlags struct { Resources []string } -type resourceSpec struct { - Kind string - ApiVersion string - IsController bool -} - -// getAPIServerResourceArray is to return an array of ApiServerResource from a string. A sample is Event:v1:true,Pod:v2:false -func (f *APIServerSourceUpdateFlags) getAPIServerResourceArray() ([]resourceSpec, error) { - var resourceList []resourceSpec +// getAPIServerResourceArray is to construct an array of resources. +func (f *APIServerSourceUpdateFlags) getAPIServerResourceArray() ([]v1alpha1.ApiServerResource, error) { + var resourceList []v1alpha1.ApiServerResource for _, r := range f.Resources { resourceSpec, err := getValidResource(r) if err != nil { @@ -60,12 +55,43 @@ func (f *APIServerSourceUpdateFlags) getAPIServerResourceArray() ([]resourceSpec return resourceList, nil } -// getAPIServerResourceArray is to return an array of ApiServerResource from a string. A sample is Event:v1:true -func (f *APIServerSourceUpdateFlags) getUpdateAPIServerResourceArray() ([]resourceSpec, []resourceSpec, error) { - var added []resourceSpec - var removed []resourceSpec +// updateExistingAPIServerResourceArray is to update an array of resources. +func (f *APIServerSourceUpdateFlags) updateExistingAPIServerResourceArray(existing []v1alpha1.ApiServerResource) ([]v1alpha1.ApiServerResource, error) { + var found bool + + added, removed, err := f.getUpdateAPIServerResourceArray() + if err != nil { + return nil, err + } + + if existing == nil { + existing = []v1alpha1.ApiServerResource{} + } + + existing = append(existing, added...) + + for _, item := range removed { + found = false + for i, ref := range existing { + if reflect.DeepEqual(item, ref) { + existing = append(existing[:i], existing[i+1:]...) + found = true + break + } + } + if !found { + return nil, fmt.Errorf("cannot find resource %s:%s:%t to remove", item.Kind, item.APIVersion, item.Controller) + } + } + return existing, nil +} + +// getUpdateAPIServerResourceArray is to construct an array of resources for update action. +func (f *APIServerSourceUpdateFlags) getUpdateAPIServerResourceArray() ([]v1alpha1.ApiServerResource, []v1alpha1.ApiServerResource, error) { + var added []v1alpha1.ApiServerResource + var removed []v1alpha1.ApiServerResource - addedArray, removedArray := util.AddListAndRemovalListFromArray(f.Resources) + addedArray, removedArray := util.AddedAndRemovalListsFromArray(f.Resources) for _, r := range addedArray { resourceSpec, err := getValidResource(r) if err != nil { @@ -83,23 +109,32 @@ func (f *APIServerSourceUpdateFlags) getUpdateAPIServerResourceArray() ([]resour return added, removed, nil } -func getValidResource(resource string) (*resourceSpec, error) { - var isController = false //false as default +//getValidResource is to parse resource spec from a string +func getValidResource(resource string) (*v1alpha1.ApiServerResource, error) { + var isController bool + var version string var err error - parts := strings.Split(resource, apiVersionSplitChar) + parts := strings.SplitN(resource, apiVersionSplitChar, 3) + if len(parts[0]) == 0 { + return nil, fmt.Errorf("cannot find KIND in resource specification %s", resource) + } kind := parts[0] - if len(parts) < 2 { - return nil, fmt.Errorf("no APIVersion given for resource %s", resource) + + if len(parts) >= 2 && len(parts[1]) > 0 { + version = parts[1] + } else { + version = "v1" } - version := parts[1] - if len(parts) >= 3 { + if len(parts) >= 3 && len(parts[2]) > 0 { isController, err = strconv.ParseBool(parts[2]) if err != nil { return nil, fmt.Errorf("cannot parse controller flage in resource specification %s", resource) } + } else { + isController = false } - return &resourceSpec{Kind: kind, ApiVersion: version, IsController: isController}, nil + return &v1alpha1.ApiServerResource{Kind: kind, APIVersion: version, Controller: isController}, nil } //Add is to set parameters @@ -117,8 +152,9 @@ func (f *APIServerSourceUpdateFlags) Add(cmd *cobra.Command) { cmd.Flags().StringArrayVar(&f.Resources, "resource", []string{}, - `Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true. -"isController" can be omitted and is "false" by default.`) + `Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. "Deployment:v1:true". +"APIVersion" can be omitted and is "v1" by default, e.g. "Deployment::true"; +"isController" can be omitted and is "false" by default, e.g. "Deployment:v1" or just "Deployment".`) } // APIServerSourceListHandlers handles printing human readable table for `kn source apiserver list` command's output diff --git a/pkg/kn/commands/source/apiserver/flags_test.go b/pkg/kn/commands/source/apiserver/flags_test.go index 8d790f57e7..4cb446b885 100644 --- a/pkg/kn/commands/source/apiserver/flags_test.go +++ b/pkg/kn/commands/source/apiserver/flags_test.go @@ -18,6 +18,7 @@ import ( "testing" "gotest.tools/assert" + "knative.dev/eventing/pkg/apis/sources/v1alpha1" ) func TestGetAPIServerResourceArray(t *testing.T) { @@ -28,38 +29,55 @@ func TestGetAPIServerResourceArray(t *testing.T) { Resources: []string{"Service:serving.knative.dev/v1alpha1:true"}, } created, _ := createFlag.getAPIServerResourceArray() - wanted := []resourceSpec{{ - Kind: "Service", - ApiVersion: "serving.knative.dev/v1alpha1", - IsController: true, + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Service", + APIVersion: "serving.knative.dev/v1alpha1", + Controller: true, }} assert.DeepEqual(t, wanted, created) + }) - // default isController - createFlag = APIServerSourceUpdateFlags{ + t.Run("get single apiserver resource when isController is default", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ ServiceAccountName: "test-sa", Mode: "Ref", Resources: []string{"Service:serving.knative.dev/v1alpha1"}, } - created, _ = createFlag.getAPIServerResourceArray() - wanted = []resourceSpec{{ - Kind: "Service", - ApiVersion: "serving.knative.dev/v1alpha1", - IsController: false, + created, _ := createFlag.getAPIServerResourceArray() + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Service", + APIVersion: "serving.knative.dev/v1alpha1", + Controller: false, }} assert.DeepEqual(t, wanted, created) + }) - // default api version and isController - createFlag = APIServerSourceUpdateFlags{ + t.Run("get single apiserver resource when apiversion is default", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Ref", + Resources: []string{"Service::true"}, + } + created, _ := createFlag.getAPIServerResourceArray() + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Service", + APIVersion: "v1", + Controller: true, + }} + assert.DeepEqual(t, wanted, created) + }) + + t.Run("get single apiserver resource when isController is default and apiversion is default", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ ServiceAccountName: "test-sa", Mode: "Ref", - Resources: []string{"Service:v1"}, + Resources: []string{"Service"}, } - created, _ = createFlag.getAPIServerResourceArray() - wanted = []resourceSpec{{ - Kind: "Service", - ApiVersion: "v1", - IsController: false, + created, _ := createFlag.getAPIServerResourceArray() + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Service", + APIVersion: "v1", + Controller: false, }} assert.DeepEqual(t, wanted, created) }) @@ -71,36 +89,59 @@ func TestGetAPIServerResourceArray(t *testing.T) { Resources: []string{"Event:v1:true", "Pod:v2:false"}, } created, _ := createFlag.getAPIServerResourceArray() - wanted := []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: true, + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: true, }, { - Kind: "Pod", - ApiVersion: "v2", - IsController: false, + Kind: "Pod", + APIVersion: "v2", + Controller: false, }} assert.DeepEqual(t, wanted, created) + }) - // default api version and isController - createFlag = APIServerSourceUpdateFlags{ + t.Run("get multiple apiserver resources when isController is default and apiversion is default", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ ServiceAccountName: "test-sa", Mode: "Resource", - Resources: []string{"Event:v1", "Pod:v1"}, + Resources: []string{"Event", "Pod"}, } - created, _ = createFlag.getAPIServerResourceArray() + created, _ := createFlag.getAPIServerResourceArray() - wanted = []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: false, + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, }, { - Kind: "Pod", - ApiVersion: "v1", - IsController: false, + Kind: "Pod", + APIVersion: "v1", + Controller: false, }} assert.DeepEqual(t, wanted, created) }) + + t.Run("get apiserver resource when isController has error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Ref", + Resources: []string{"Service::xxx"}, + } + _, err := createFlag.getAPIServerResourceArray() + errorMsg := "cannot parse controller flage in resource specification Service::xxx" + assert.Error(t, err, errorMsg) + }) + + t.Run("get apiserver resources when kind has error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Ref", + Resources: []string{":v2:true"}, + } + _, err := createFlag.getAPIServerResourceArray() + errorMsg := "cannot find KIND in resource specification :v2:true" + assert.Error(t, err, errorMsg) + }) } func TestGetUpdateAPIServerResourceArray(t *testing.T) { @@ -111,15 +152,15 @@ func TestGetUpdateAPIServerResourceArray(t *testing.T) { Resources: []string{"Event:v1:true", "Pod:v2:false-"}, } added, removed, _ := createFlag.getUpdateAPIServerResourceArray() - addwanted := []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: true, + addwanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: true, }} - removewanted := []resourceSpec{{ - Kind: "Pod", - ApiVersion: "v2", - IsController: false, + removewanted := []v1alpha1.ApiServerResource{{ + Kind: "Pod", + APIVersion: "v2", + Controller: false, }} assert.DeepEqual(t, added, addwanted) assert.DeepEqual(t, removed, removewanted) @@ -132,17 +173,59 @@ func TestGetUpdateAPIServerResourceArray(t *testing.T) { } added, removed, _ = createFlag.getUpdateAPIServerResourceArray() - removewanted = []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: false, + removewanted = []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, }} - addwanted = []resourceSpec{{ - Kind: "Pod", - ApiVersion: "v1", - IsController: false, + addwanted = []v1alpha1.ApiServerResource{{ + Kind: "Pod", + APIVersion: "v1", + Controller: false, }} assert.DeepEqual(t, added, addwanted) assert.DeepEqual(t, removed, removewanted) }) } + +func TestUpdateExistingAPIServerResourceArray(t *testing.T) { + existing := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, + }, { + Kind: "Pod", + APIVersion: "v1", + Controller: false, + }} + + t.Run("update existing apiserver resources", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Resource", + Resources: []string{"Deployment:v1:true", "Pod:v1:false-"}, + } + updated, _ := createFlag.updateExistingAPIServerResourceArray(existing) + updatedWanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, + }, { + Kind: "Deployment", + APIVersion: "v1", + Controller: true, + }} + assert.DeepEqual(t, updated, updatedWanted) + }) + + t.Run("update existing apiserver resources with error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Resource", + Resources: []string{"Deployment:v1:true", "Pod:v2:false-"}, + } + _, err := createFlag.updateExistingAPIServerResourceArray(existing) + errorMsg := "cannot find resource Pod:v2:false to remove" + assert.Error(t, err, errorMsg) + }) +} diff --git a/pkg/kn/commands/source/apiserver/update.go b/pkg/kn/commands/source/apiserver/update.go index e48cf992d5..b8b279016e 100644 --- a/pkg/kn/commands/source/apiserver/update.go +++ b/pkg/kn/commands/source/apiserver/update.go @@ -74,19 +74,11 @@ func NewAPIServerUpdateCommand(p *commands.KnParams) *cobra.Command { } if cmd.Flags().Changed("resource") { - added, removed, err := apiServerUpdateFlags.getUpdateAPIServerResourceArray() + updateExisting, err := apiServerUpdateFlags.updateExistingAPIServerResourceArray(source.Spec.Resources) if err != nil { return err } - for _, k := range removed { - _, err = b.RemoveResource(k.ApiVersion, k.Kind, k.IsController) - if err != nil { - return err - } - } - for _, k := range added { - b.AddResource(k.ApiVersion, k.Kind, k.IsController) - } + b.Resources(updateExisting) } if cmd.Flags().Changed("sink") { diff --git a/pkg/util/parsing_helper.go b/pkg/util/parsing_helper.go index 27bdbf4e0d..0837a4f87e 100644 --- a/pkg/util/parsing_helper.go +++ b/pkg/util/parsing_helper.go @@ -90,8 +90,8 @@ func (m StringMap) Remove(toRemove []string) StringMap { return m } -// AddListAndRemovalListFromArray returns a list of add entries and a list of removal entries -func AddListAndRemovalListFromArray(m []string) ([]string, []string) { +// AddedAndRemovalListsFromArray returns a list of added entries and a list of removal entries +func AddedAndRemovalListsFromArray(m []string) ([]string, []string) { stringToRemove := []string{} stringToAdd := []string{} for _, key := range m { diff --git a/pkg/util/parsing_helper_test.go b/pkg/util/parsing_helper_test.go index 88352db146..e5914b505d 100644 --- a/pkg/util/parsing_helper_test.go +++ b/pkg/util/parsing_helper_test.go @@ -103,8 +103,16 @@ func TestStringMap(t *testing.T) { assert.DeepEqual(t, expectedMap, inputMap) } -func TestAddListAndRemovalListFromArray(t *testing.T) { - addList, removeList := AddListAndRemovalListFromArray([]string{"addvalue1", "remove1-", "addvalue2", "remove2-"}) +func TestAddedAndRemovalListFromArray(t *testing.T) { + addList, removeList := AddedAndRemovalListsFromArray([]string{"addvalue1", "remove1-", "addvalue2", "remove2-"}) assert.DeepEqual(t, []string{"addvalue1", "addvalue2"}, addList) assert.DeepEqual(t, []string{"remove1", "remove2"}, removeList) + + addList, removeList = AddedAndRemovalListsFromArray([]string{"remove1-"}) + assert.DeepEqual(t, []string{}, addList) + assert.DeepEqual(t, []string{"remove1"}, removeList) + + addList, removeList = AddedAndRemovalListsFromArray([]string{"addvalue1"}) + assert.DeepEqual(t, []string{"addvalue1"}, addList) + assert.DeepEqual(t, []string{}, removeList) }