From 06ecc46fc7c0a2f33fea29750e2114d828d4c45c Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 19 Aug 2020 17:37:38 +0530 Subject: [PATCH 1/7] Update eventlistener doc to include eventlistener responsibility --- docs/eventlisteners.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/docs/eventlisteners.md b/docs/eventlisteners.md index 9af46e44db..de10bb30a2 100644 --- a/docs/eventlisteners.md +++ b/docs/eventlisteners.md @@ -24,6 +24,7 @@ using [Event Interceptors](#Interceptors). - [Labels](#labels) - [Annotations](#annotations) - [EventListener Response](#eventlistener-response) +- [How does the EventListener work?](#how-does-the-eventlistener-work) - [Examples](#examples) - [Multi-Tenant Concerns](#multi-tenant-concerns) @@ -748,6 +749,39 @@ The EventListener responds with following message after receiving the event: - `namespace` - Refers to the namespace of the EventListener - `eventID` - Refers to the uniqueID that gets assigned to each incoming request +## How does the EventListener work? + +Lets understand how an EventListener works with an example using GitHub + +* Create a sample GitHub example +```bash +kubectl create -f https://github.com/tektoncd/triggers/tree/master/examples/github +``` + +* Once the EventListener is created, the Triggers controller will create a new `Deployment` and `Service` for the EventListener. We can use `kubectl` to see them running: +```bash +kubectl get deployment +NAME READY UP-TO-DATE AVAILABLE AGE +el-github-listener-interceptor 1/1 1 1 11s + +kubectl get svc +NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE +el-github-listener-interceptor ClusterIP 10.99.188.140 8080/TCP 52s +``` +The Triggers controller uses fields from the EventListener's `spec` (which is described in the [Syntax](https://github.com/tektoncd/triggers/blob/master/docs/eventlisteners.md#syntax) section, as well as [`metadata.labels`](https://github.com/tektoncd/triggers/blob/master/docs/eventlisteners.md#labels) +in addition to some pre-configured information like (container `Image`, `Name`, `Port`) to create the **Deployment** and **Service**. + +We follow a naming convention while creating these resources. An EventListener named `foo` will create a deployment and a service both named `el-foo`. + +Once all the resources are up and running user can get a URL to send webhook events. This URL points to the service created above and points to the deployment. +```bash +kubectl get eventlistener +NAME ADDRESS AVAILABLE REASON +github-listener-interceptor http://el-github-listener-interceptor.ptest.svc.cluster.local:8080 True MinimumReplicasAvailable +``` + +Follow [GitHub example](https://github.com/tektoncd/triggers/blob/master/examples/github/README.md) to try out locally. + ## Examples For complete examples, see From 19e0cd2a71c3e97db96fce0c05cf479aa19fa025 Mon Sep 17 00:00:00 2001 From: Khurram Baig Date: Tue, 8 Sep 2020 12:50:09 +0530 Subject: [PATCH 2/7] Add TriggerCRD object validation and default Defaults and Validation for TriggerCRD object have been added. --- cmd/webhook/main.go | 1 + .../v1alpha1/event_listener_defaults.go | 14 +- .../v1alpha1/event_listener_validation.go | 118 +----- .../triggers/v1alpha1/trigger_defaults.go | 42 +++ .../v1alpha1/trigger_defaults_test.go | 86 +++++ .../triggers/v1alpha1/trigger_validation.go | 177 +++++++++ .../v1alpha1/trigger_validation_test.go | 338 ++++++++++++++++++ 7 files changed, 652 insertions(+), 124 deletions(-) create mode 100644 pkg/apis/triggers/v1alpha1/trigger_defaults.go create mode 100644 pkg/apis/triggers/v1alpha1/trigger_defaults_test.go create mode 100644 pkg/apis/triggers/v1alpha1/trigger_validation.go create mode 100644 pkg/apis/triggers/v1alpha1/trigger_validation_test.go diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 2619440346..1e4e1e5cf7 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -40,6 +40,7 @@ var types = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{ v1alpha1.SchemeGroupVersion.WithKind("EventListener"): &v1alpha1.EventListener{}, v1alpha1.SchemeGroupVersion.WithKind("TriggerBinding"): &v1alpha1.TriggerBinding{}, v1alpha1.SchemeGroupVersion.WithKind("TriggerTemplate"): &v1alpha1.TriggerTemplate{}, + v1alpha1.SchemeGroupVersion.WithKind("Trigger"): &v1alpha1.Trigger{}, } func NewDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { diff --git a/pkg/apis/triggers/v1alpha1/event_listener_defaults.go b/pkg/apis/triggers/v1alpha1/event_listener_defaults.go index 470395a9ae..db949776b0 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_defaults.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_defaults.go @@ -25,18 +25,8 @@ func (el *EventListener) SetDefaults(ctx context.Context) { if IsUpgradeViaDefaulting(ctx) { // set defaults for i := range el.Spec.Triggers { - defaultBindings(&el.Spec.Triggers[i]) - } - } -} - -// set default TriggerBinding kind for Bindings -func defaultBindings(t *EventListenerTrigger) { - if len(t.Bindings) > 0 { - for _, b := range t.Bindings { - if b.Kind == "" { - b.Kind = NamespacedTriggerBindingKind - } + triggerSpecBindingArray(el.Spec.Triggers[i].Bindings). + defaultBindings() } } } diff --git a/pkg/apis/triggers/v1alpha1/event_listener_validation.go b/pkg/apis/triggers/v1alpha1/event_listener_validation.go index 3c811ba985..c3787e1c66 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_validation.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_validation.go @@ -19,10 +19,7 @@ package v1alpha1 import ( "context" "fmt" - "net/http" - "github.com/google/cel-go/cel" - pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/apis" ) @@ -55,34 +52,17 @@ func (t *EventListenerTrigger) validate(ctx context.Context) *apis.FieldError { } // Validate optional Bindings - for i, b := range t.Bindings { - // Either Ref or Spec should be present - if b.Ref == "" && b.Spec == nil { - return apis.ErrMissingOneOf(fmt.Sprintf("bindings[%d].Ref", i), fmt.Sprintf("bindings[%d].Spec", i)) - } - - // Both Ref and Spec can't be present at the same time - if b.Ref != "" && b.Spec != nil { - return apis.ErrMultipleOneOf(fmt.Sprintf("bindings[%d].Ref", i), fmt.Sprintf("bindings[%d].Spec", i)) - } - - if b.Ref != "" && b.Kind != NamespacedTriggerBindingKind && b.Kind != ClusterTriggerBindingKind { - return apis.ErrInvalidValue(fmt.Errorf("invalid kind"), fmt.Sprintf("bindings[%d].kind", i)) - } + if err := triggerSpecBindingArray(t.Bindings).validate(ctx); err != nil { + return err } - // Validate required TriggerTemplate - // Optional explicit match if t.Template != nil { - if t.Template.APIVersion != "" { - if t.Template.APIVersion != "v1alpha1" { - return apis.ErrInvalidValue(fmt.Errorf("invalid apiVersion"), "template.apiVersion") - } - } - if t.Template.Name == "" { - return apis.ErrMissingField("template.name") + // Validate required TriggerTemplate + if err := t.Template.validate(ctx); err != nil { + return err } } + // Validate optional Interceptors for i, interceptor := range t.Interceptors { if err := interceptor.validate(ctx).ViaField(fmt.Sprintf("interceptors[%d]", i)); err != nil { return err @@ -97,89 +77,3 @@ func (t *EventListenerTrigger) validate(ctx context.Context) *apis.FieldError { return nil } - -func (i *EventInterceptor) validate(ctx context.Context) *apis.FieldError { - if i.Webhook == nil && i.GitHub == nil && i.GitLab == nil && i.CEL == nil && i.Bitbucket == nil { - return apis.ErrMissingField("interceptor") - } - - // Enforce oneof - numSet := 0 - if i.Webhook != nil { - numSet++ - } - if i.GitHub != nil { - numSet++ - } - if i.GitLab != nil { - numSet++ - } - if i.Bitbucket != nil { - numSet++ - } - - if numSet > 1 { - return apis.ErrMultipleOneOf("interceptor.webhook", "interceptor.github", "interceptor.gitlab") - } - - if i.Webhook != nil { - if i.Webhook.ObjectRef == nil || i.Webhook.ObjectRef.Name == "" { - return apis.ErrMissingField("interceptor.webhook.objectRef") - } - w := i.Webhook - if w.ObjectRef.Kind != "Service" { - return apis.ErrInvalidValue(fmt.Errorf("invalid kind"), "interceptor.webhook.objectRef.kind") - } - - // Optional explicit match - if w.ObjectRef.APIVersion != "v1" { - return apis.ErrInvalidValue(fmt.Errorf("invalid apiVersion"), "interceptor.webhook.objectRef.apiVersion") - } - - for i, header := range w.Header { - // Enforce non-empty canonical header keys - if len(header.Name) == 0 || http.CanonicalHeaderKey(header.Name) != header.Name { - return apis.ErrInvalidValue(fmt.Errorf("invalid header name"), fmt.Sprintf("interceptor.webhook.header[%d].name", i)) - } - // Enforce non-empty header values - if header.Value.Type == pipelinev1.ParamTypeString { - if len(header.Value.StringVal) == 0 { - return apis.ErrInvalidValue(fmt.Errorf("invalid header value"), fmt.Sprintf("interceptor.webhook.header[%d].value", i)) - } - } else if len(header.Value.ArrayVal) == 0 { - return apis.ErrInvalidValue(fmt.Errorf("invalid header value"), fmt.Sprintf("interceptor.webhook.header[%d].value", i)) - } - } - } - - // No github validation required yet. - // if i.GitHub != nil { - // - // } - - // No gitlab validation required yet. - // if i.GitLab != nil { - // - // } - - if i.CEL != nil { - if i.CEL.Filter == "" && len(i.CEL.Overlays) == 0 { - return apis.ErrMultipleOneOf("cel.filter", "cel.overlays") - } - env, err := cel.NewEnv() - if err != nil { - return apis.ErrInvalidValue(fmt.Errorf("failed to create a CEL env: %s", err), "cel.filter") - } - if i.CEL.Filter != "" { - if _, issues := env.Parse(i.CEL.Filter); issues != nil && issues.Err() != nil { - return apis.ErrInvalidValue(fmt.Errorf("failed to parse the CEL filter: %s", issues.Err()), "cel.filter") - } - } - for _, v := range i.CEL.Overlays { - if _, issues := env.Parse(v.Expression); issues != nil && issues.Err() != nil { - return apis.ErrInvalidValue(fmt.Errorf("failed to parse the CEL overlay: %s", issues.Err()), "cel.overlay") - } - } - } - return nil -} diff --git a/pkg/apis/triggers/v1alpha1/trigger_defaults.go b/pkg/apis/triggers/v1alpha1/trigger_defaults.go new file mode 100644 index 0000000000..5465a215e7 --- /dev/null +++ b/pkg/apis/triggers/v1alpha1/trigger_defaults.go @@ -0,0 +1,42 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "context" +) + +type triggerSpecBindingArray []*TriggerSpecBinding + +// SetDefaults sets the defaults on the object. +func (t *Trigger) SetDefaults(ctx context.Context) { + if IsUpgradeViaDefaulting(ctx) { + // set defaults + triggerSpecBindingArray(t.Spec.Bindings).defaultBindings() + } +} + +// set default TriggerBinding kind for Bindings in TriggerSpec +func (t triggerSpecBindingArray) defaultBindings() { + if len(t) > 0 { + for _, b := range t { + if b.Kind == "" { + b.Kind = NamespacedTriggerBindingKind + } + } + } +} diff --git a/pkg/apis/triggers/v1alpha1/trigger_defaults_test.go b/pkg/apis/triggers/v1alpha1/trigger_defaults_test.go new file mode 100644 index 0000000000..cb902ebd8b --- /dev/null +++ b/pkg/apis/triggers/v1alpha1/trigger_defaults_test.go @@ -0,0 +1,86 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1_test + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" +) + +func TestTriggerSetDefaults(t *testing.T) { + tests := []struct { + name string + in *v1alpha1.Trigger + want *v1alpha1.Trigger + wc func(context.Context) context.Context + }{{ + name: "default binding", + in: &v1alpha1.Trigger{ + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{ + { + Ref: "binding", + }, + { + Kind: v1alpha1.NamespacedTriggerBindingKind, + Ref: "namespace-binding", + }, + { + Kind: v1alpha1.ClusterTriggerBindingKind, + Ref: "cluster-binding", + }, + }, + }, + }, + wc: v1alpha1.WithUpgradeViaDefaulting, + want: &v1alpha1.Trigger{ + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{ + { + Kind: v1alpha1.NamespacedTriggerBindingKind, + Ref: "binding", + }, + { + Kind: v1alpha1.NamespacedTriggerBindingKind, + Ref: "namespace-binding", + }, + { + Kind: v1alpha1.ClusterTriggerBindingKind, + Ref: "cluster-binding", + }, + }, + }, + }, + }} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := tc.in + ctx := context.Background() + if tc.wc != nil { + ctx = tc.wc(ctx) + } + got.SetDefaults(ctx) + + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("SetDefaults (-want, +got) = %v", diff) + } + }) + } +} diff --git a/pkg/apis/triggers/v1alpha1/trigger_validation.go b/pkg/apis/triggers/v1alpha1/trigger_validation.go new file mode 100644 index 0000000000..ebb9127da9 --- /dev/null +++ b/pkg/apis/triggers/v1alpha1/trigger_validation.go @@ -0,0 +1,177 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "context" + "fmt" + "net/http" + + "github.com/google/cel-go/cel" + pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/pkg/apis/validate" + "knative.dev/pkg/apis" +) + +// Validate validates a Trigger +func (t *Trigger) Validate(ctx context.Context) *apis.FieldError { + if err := validate.ObjectMetadata(t.GetObjectMeta()); err != nil { + return err.ViaField("metadata") + } + return t.Spec.validate(ctx).ViaField("spec") +} + +func (t *TriggerSpec) validate(ctx context.Context) *apis.FieldError { + // Validate optional Bindings + if err := triggerSpecBindingArray(t.Bindings).validate(ctx); err != nil { + return err + } + // Validate required TriggerTemplate + if err := t.Template.validate(ctx); err != nil { + return err + } + + // Validate optional Interceptors + for i, interceptor := range t.Interceptors { + if err := interceptor.validate(ctx).ViaField(fmt.Sprintf("interceptors[%d]", i)); err != nil { + return err + } + } + + return nil +} + +func (t TriggerSpecTemplate) validate(ctx context.Context) *apis.FieldError { + // Optional explicit match + if t.APIVersion != "" { + if t.APIVersion != "v1alpha1" { + return apis.ErrInvalidValue(fmt.Errorf("invalid apiVersion"), "template.apiVersion") + } + } + if t.Name == "" { + return apis.ErrMissingField("template.name") + } + return nil + +} + +func (t triggerSpecBindingArray) validate(ctx context.Context) *apis.FieldError { + if len(t) > 0 { + for i, b := range t { + // Either Ref or Spec should be present + if b.Ref == "" && b.Spec == nil { + return apis.ErrMissingOneOf(fmt.Sprintf("bindings[%d].Ref", i), fmt.Sprintf("bindings[%d].Spec", i)) + } + + // Both Ref and Spec can't be present at the same time + if b.Ref != "" && b.Spec != nil { + return apis.ErrMultipleOneOf(fmt.Sprintf("bindings[%d].Ref", i), fmt.Sprintf("bindings[%d].Spec", i)) + } + + if b.Ref != "" && b.Kind != NamespacedTriggerBindingKind && b.Kind != ClusterTriggerBindingKind { + return apis.ErrInvalidValue(fmt.Errorf("invalid kind"), fmt.Sprintf("bindings[%d].kind", i)) + } + } + } + return nil +} + +func (i *TriggerInterceptor) validate(ctx context.Context) *apis.FieldError { + if i.Webhook == nil && i.GitHub == nil && i.GitLab == nil && i.CEL == nil && i.Bitbucket == nil { + return apis.ErrMissingField("interceptor") + } + + // Enforce oneof + numSet := 0 + if i.Webhook != nil { + numSet++ + } + if i.GitHub != nil { + numSet++ + } + if i.GitLab != nil { + numSet++ + } + if i.Bitbucket != nil { + numSet++ + } + + if numSet > 1 { + return apis.ErrMultipleOneOf("interceptor.webhook", "interceptor.github", "interceptor.gitlab") + } + + if i.Webhook != nil { + if i.Webhook.ObjectRef == nil || i.Webhook.ObjectRef.Name == "" { + return apis.ErrMissingField("interceptor.webhook.objectRef") + } + w := i.Webhook + if w.ObjectRef.Kind != "Service" { + return apis.ErrInvalidValue(fmt.Errorf("invalid kind"), "interceptor.webhook.objectRef.kind") + } + + // Optional explicit match + if w.ObjectRef.APIVersion != "v1" { + return apis.ErrInvalidValue(fmt.Errorf("invalid apiVersion"), "interceptor.webhook.objectRef.apiVersion") + } + + for i, header := range w.Header { + // Enforce non-empty canonical header keys + if len(header.Name) == 0 || http.CanonicalHeaderKey(header.Name) != header.Name { + return apis.ErrInvalidValue(fmt.Errorf("invalid header name"), fmt.Sprintf("interceptor.webhook.header[%d].name", i)) + } + // Enforce non-empty header values + if header.Value.Type == pipelinev1.ParamTypeString { + if len(header.Value.StringVal) == 0 { + return apis.ErrInvalidValue(fmt.Errorf("invalid header value"), fmt.Sprintf("interceptor.webhook.header[%d].value", i)) + } + } else if len(header.Value.ArrayVal) == 0 { + return apis.ErrInvalidValue(fmt.Errorf("invalid header value"), fmt.Sprintf("interceptor.webhook.header[%d].value", i)) + } + } + } + + // No github validation required yet. + // if i.GitHub != nil { + // + // } + + // No gitlab validation required yet. + // if i.GitLab != nil { + // + // } + + if i.CEL != nil { + if i.CEL.Filter == "" && len(i.CEL.Overlays) == 0 { + return apis.ErrMultipleOneOf("cel.filter", "cel.overlays") + } + env, err := cel.NewEnv() + if err != nil { + return apis.ErrInvalidValue(fmt.Errorf("failed to create a CEL env: %s", err), "cel.filter") + } + if i.CEL.Filter != "" { + if _, issues := env.Parse(i.CEL.Filter); issues != nil && issues.Err() != nil { + return apis.ErrInvalidValue(fmt.Errorf("failed to parse the CEL filter: %s", issues.Err()), "cel.filter") + } + } + for _, v := range i.CEL.Overlays { + if _, issues := env.Parse(v.Expression); issues != nil && issues.Err() != nil { + return apis.ErrInvalidValue(fmt.Errorf("failed to parse the CEL overlay: %s", issues.Err()), "cel.overlay") + } + } + } + return nil +} diff --git a/pkg/apis/triggers/v1alpha1/trigger_validation_test.go b/pkg/apis/triggers/v1alpha1/trigger_validation_test.go new file mode 100644 index 0000000000..74c0ca176d --- /dev/null +++ b/pkg/apis/triggers/v1alpha1/trigger_validation_test.go @@ -0,0 +1,338 @@ +/* +Copyright 2020 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1_test + +import ( + "context" + "testing" + + "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" + bldr "github.com/tektoncd/triggers/test/builder" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func Test_TriggerValidate(t *testing.T) { + tests := []struct { + name string + tr *v1alpha1.Trigger + }{{ + name: "Valid Trigger No TriggerBinding", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"))), + }, { + name: "Valid Trigger with TriggerBinding", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "TriggerBinding", "tb", "v1alpha1"), + )), + }, { + name: "Valid Trigger with ClusterTriggerBinding", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "ClusterTriggerBinding", "tb", "v1alpha1"), + )), + }, { + name: "Valid Trigger with multiple TriggerBindings", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "ClusterTriggerBinding", "tb", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "TriggerBinding", "tb", "v1alpha1"), + bldr.TriggerSpecBinding("tb3", "", "tb3", "v1alpha1"), + )), + }, { + name: "Valid Trigger Interceptor", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("svc", "v1", "Service", "namespace"), + )), + }, { + name: "Valid Trigger Interceptor With Header", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("svc", "v1", "Service", "namespace", + bldr.TriggerSpecInterceptorParam("Valid-Header-Key", "valid value"), + ), + )), + }, { + name: "Valid Trigger Interceptor With Headers", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("svc", "v1", "Service", "namespace", + bldr.TriggerSpecInterceptorParam("Valid-Header-Key1", "valid value1"), + bldr.TriggerSpecInterceptorParam("Valid-Header-Key1", "valid value2"), + bldr.TriggerSpecInterceptorParam("Valid-Header-Key2", "valid value"), + ), + )), + }, { + name: "Valid Trigger with CTR interceptor", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecCELInterceptor("body.value == 'test'"), + )), + }, { + name: "Valid Trigger with no trigger name", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + )), + }, { + name: "Valid Trigger with embedded bindings", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("", "", "", "v1alpha1", bldr.TriggerBindingParam("key", "value")), + )), + }, { + name: "Valid Trigger with CEL overlays", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecCELInterceptor("", bldr.TriggerSpecCELOverlay("body.value", "'testing'")), + )), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := test.tr.Validate(context.Background()) + if err != nil { + t.Errorf("Trigger.Validate() expected no error, but got one, Trigger: %v, error: %v", test.tr, err) + } + }) + } +} + +func TestTriggerValidate_error(t *testing.T) { + tests := []struct { + name string + tr *v1alpha1.Trigger + }{{ + name: "TriggerBinding with no spec", + tr: bldr.Trigger("name", "namespace"), + }, { + name: "Bindings missing ref", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "", APIVersion: "v1alpha1"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt"}, + }, + }, + }, { + name: "Bindings missing kind", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: "", Ref: "tb", APIVersion: "v1alpha1"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt"}, + }, + }, + }, { + name: "Template with wrong apiVersion", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "tb", APIVersion: "v1alpha1"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt", APIVersion: "invalid"}, + }, + }, + }, { + name: "Template with missing name", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "tb", APIVersion: "v1alpha1"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "", APIVersion: "v1alpha1"}, + }, + }, + }, { + name: "Interceptor Name only", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("svc", "", "", ""), + )), + }, { + name: "Interceptor Missing ObjectRef", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "tb", APIVersion: "v1alpha1"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt", APIVersion: "v1alpha1"}, + Interceptors: []*v1alpha1.TriggerInterceptor{{}}, + }, + }, + }, { + name: "Interceptor Empty ObjectRef", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "tb", APIVersion: "v1alpha1"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt", APIVersion: "v1alpha1"}, + Interceptors: []*v1alpha1.TriggerInterceptor{{ + Webhook: &v1alpha1.WebhookInterceptor{ + ObjectRef: &corev1.ObjectReference{ + Name: "", + }, + }, + }}, + }, + }, + }, { + name: "Valid Trigger with invalid TriggerBinding", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "NamespaceTriggerBinding", "tb", "v1alpha1"), + )), + }, { + name: "Interceptor Wrong APIVersion", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("foo", "v3", "Service", ""), + )), + }, { + name: "Interceptor Wrong Kind", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("foo", "v1", "Deployment", ""), + )), + }, { + name: "Interceptor Non-Canonical Header", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("foo", "v1", "Deployment", "", + bldr.TriggerSpecInterceptorParam("non-canonical-header-key", "valid value"), + ), + )), + }, { + name: "Interceptor Empty Header Name", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("foo", "v1", "Deployment", "", + bldr.TriggerSpecInterceptorParam("", "valid value"), + ), + )), + }, { + name: "Interceptor Empty Header Value", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecInterceptor("foo", "v1", "Deployment", "", + bldr.TriggerSpecInterceptorParam("Valid-Header-Key", ""), + ), + )), + }, { + name: "Multiple interceptors set", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "tb"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt"}, + Interceptors: []*v1alpha1.TriggerInterceptor{{ + GitHub: &v1alpha1.GitHubInterceptor{}, + GitLab: &v1alpha1.GitLabInterceptor{}, + Bitbucket: &v1alpha1.BitbucketInterceptor{}, + }}, + }, + }, + }, { + name: "CEL interceptor with no filter or overlays", + tr: &v1alpha1.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + }, + Spec: v1alpha1.TriggerSpec{ + Bindings: []*v1alpha1.TriggerSpecBinding{{Name: "tb", Kind: v1alpha1.NamespacedTriggerBindingKind, Ref: "tb"}}, + Template: v1alpha1.TriggerSpecTemplate{Name: "tt"}, + Interceptors: []*v1alpha1.TriggerInterceptor{{ + CEL: &v1alpha1.CELInterceptor{}, + }}, + }, + }, + }, { + name: "CEL interceptor with bad filter expression", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecCELInterceptor("body.value == 'test')"), + )), + }, { + name: "CEL interceptor with bad overlay expression", + tr: bldr.Trigger("name", "namespace", + bldr.TriggerSpec( + bldr.TriggerSpecTemplate("tt", "v1alpha1"), + bldr.TriggerSpecBinding("tb", "", "tb", "v1alpha1"), + bldr.TriggerSpecCELInterceptor("", bldr.TriggerSpecCELOverlay("body.value", "'testing')")), + )), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if err := test.tr.Validate(context.Background()); err == nil { + t.Errorf("Trigger.Validate() expected error, but get none, Trigger: %v", test.tr) + } + }) + } +} From 6c599c51a946062894799ed5f6ee45fc9c8d0686 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 9 Sep 2020 10:38:39 +0530 Subject: [PATCH 3/7] Add release links to versioned docs for v0.8.0 --- README.md | 1 + tekton/release-cheat-sheet.md | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a4cb650f22..c97f792a0f 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,7 @@ events. | Version | Docs | Examples | Getting Started | | ---------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | | [HEAD](https://github.com/tektoncd/triggers/blob/master/DEVELOPMENT.md#install-pipeline) | [Docs @ HEAD](https://github.com/tektoncd/triggers/blob/master/docs/README.md) | [Examples @ HEAD](https://github.com/tektoncd/triggers/blob/master/examples) | [Getting Started @ HEAD](https://github.com/tektoncd/triggers/blob/master/docs/getting-started#getting-started-with-triggers) | +| [v0.8.0](https://github.com/tektoncd/triggers/releases/tag/v0.8.0) | [Docs @ v0.8.0](https://github.com/tektoncd/triggers/tree/v0.8.0/docs#tekton-triggers) | [Examples @ v0.8.0](https://github.com/tektoncd/triggers/tree/v0.8.0/examples#examples) | [Getting Started @ v0.8.0](https://github.com/tektoncd/triggers/tree/v0.8.0/docs/getting-started#getting-started-with-triggers) | | [v0.7.0](https://github.com/tektoncd/triggers/releases/tag/v0.7.0) | [Docs @ v0.7.0](https://github.com/tektoncd/triggers/tree/v0.7.0/docs#tekton-triggers) | [Examples @ v0.7.0](https://github.com/tektoncd/triggers/tree/v0.7.0/examples#examples) | [Getting Started @ v0.7.0](https://github.com/tektoncd/triggers/tree/v0.7.0/docs/getting-started#getting-started-with-triggers) | | [v0.6.1](https://github.com/tektoncd/triggers/releases/tag/v0.6.1) | [Docs @ v0.6.1](https://github.com/tektoncd/triggers/tree/v0.6.1/docs#tekton-triggers) | [Examples @ v0.6.1](https://github.com/tektoncd/triggers/tree/v0.6.1/examples#examples) | [Getting Started @ v0.6.1](https://github.com/tektoncd/triggers/tree/v0.6.1/docs/getting-started#getting-started-with-triggers) | | [v0.6.0](https://github.com/tektoncd/triggers/releases/tag/v0.6.0) | [Docs @ v0.6.0](https://github.com/tektoncd/triggers/tree/v0.6.0/docs#tekton-triggers) | [Examples @ v0.6.0](https://github.com/tektoncd/triggers/tree/v0.6.0/examples#examples) | [Getting Started @ v0.6.0](https://github.com/tektoncd/triggers/tree/v0.6.0/docs/getting-started#getting-started-with-triggers) | diff --git a/tekton/release-cheat-sheet.md b/tekton/release-cheat-sheet.md index 0db3c9fa78..ee72563039 100644 --- a/tekton/release-cheat-sheet.md +++ b/tekton/release-cheat-sheet.md @@ -37,9 +37,9 @@ the triggers repo, a terminal window and a text editor. 5. Create environment variables for bash scripts in later steps. ```bash - VERSION_TAG=# UPDATE THIS. Example: v0.11.2 - PREVIOUS_VERSION_TAG=# UPDATE THIS. Example v0.11.1. Used to calculate release notes - GIT_RESOURCE_NAME=# UPDATE THIS. Example: tekton-triggers-git-v0-11-2 + VERSION_TAG=# UPDATE THIS. Example: v0.6.2 + PREVIOUS_VERSION_TAG=# UPDATE THIS. Example v0.6.0. Used to calculate release notes + GIT_RESOURCE_NAME=# UPDATE THIS. Example: tekton-triggers-git-v0-6-2 IMAGE_REGISTRY=gcr.io/tekton-releases ``` From 5a47a0012e50c774d8095ff9db39785d41c1b1a4 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Thu, 10 Sep 2020 18:07:28 +0530 Subject: [PATCH 4/7] Fix replicas validation and update operation for replica set --- docs/eventlisteners.md | 4 +-- .../v1alpha1/event_listener_defaults.go | 3 ++ .../v1alpha1/event_listener_defaults_test.go | 27 ++++++++++++++ .../v1alpha1/event_listener_validation.go | 2 +- .../event_listener_validation_test.go | 4 +-- .../v1alpha1/eventlistener/eventlistener.go | 9 +++-- .../eventlistener/eventlistener_test.go | 35 ++++++++++++++++--- 7 files changed, 71 insertions(+), 13 deletions(-) diff --git a/docs/eventlisteners.md b/docs/eventlisteners.md index de10bb30a2..7a24f589bd 100644 --- a/docs/eventlisteners.md +++ b/docs/eventlisteners.md @@ -175,9 +175,9 @@ check out the guide on [exposing EventListeners](./exposing-eventlisteners.md). ### Replicas The `replicas` field is optional. By default, the number of replicas of EventListener is 1. -If you want to deploy more than one pod, you can specify the number to this field. +If you want to deploy more than one pod, you can specify the number to `replicas` field. -**Note:** If user sets `replicas` field while creating eventlistener yaml then it won't respects replicas values edited by user manually or through any other mechanism (ex: HPA). +**Note:** If user sets `replicas` field while creating/updating eventlistener yaml then it won't respects replicas values edited by user manually on deployment or through any other mechanism (ex: HPA). ### PodTemplate diff --git a/pkg/apis/triggers/v1alpha1/event_listener_defaults.go b/pkg/apis/triggers/v1alpha1/event_listener_defaults.go index db949776b0..a78e36f512 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_defaults.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_defaults.go @@ -24,6 +24,9 @@ import ( func (el *EventListener) SetDefaults(ctx context.Context) { if IsUpgradeViaDefaulting(ctx) { // set defaults + if el.Spec.Replicas != nil && *el.Spec.Replicas == 0 { + *el.Spec.Replicas = 1 + } for i := range el.Spec.Triggers { triggerSpecBindingArray(el.Spec.Triggers[i].Bindings). defaultBindings() diff --git a/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go b/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go index c1f7cd42ec..076c11c7cf 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_defaults_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" + "knative.dev/pkg/ptr" ) func TestEventListenerSetDefaults(t *testing.T) { @@ -72,6 +73,32 @@ func TestEventListenerSetDefaults(t *testing.T) { }}, }, }, + }, { + name: "set replicas to 1 if provided replicas is 0 as part of eventlistener spec", + in: &v1alpha1.EventListener{ + Spec: v1alpha1.EventListenerSpec{ + Replicas: ptr.Int32(0), + }, + }, + wc: v1alpha1.WithUpgradeViaDefaulting, + want: &v1alpha1.EventListener{ + Spec: v1alpha1.EventListenerSpec{ + Replicas: ptr.Int32(1), + }, + }, + }, { + name: "different value for replicas other than 0", + in: &v1alpha1.EventListener{ + Spec: v1alpha1.EventListenerSpec{ + Replicas: ptr.Int32(2), + }, + }, + wc: v1alpha1.WithUpgradeViaDefaulting, + want: &v1alpha1.EventListener{ + Spec: v1alpha1.EventListenerSpec{ + Replicas: ptr.Int32(2), + }, + }, }} for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/apis/triggers/v1alpha1/event_listener_validation.go b/pkg/apis/triggers/v1alpha1/event_listener_validation.go index c3787e1c66..e1c122fb5c 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_validation.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_validation.go @@ -31,7 +31,7 @@ func (e *EventListener) Validate(ctx context.Context) *apis.FieldError { func (s *EventListenerSpec) validate(ctx context.Context) *apis.FieldError { if s.Replicas != nil { - if *s.Replicas <= 0 { + if *s.Replicas < 0 { return apis.ErrInvalidValue(*s.Replicas, "spec.replicas") } } diff --git a/pkg/apis/triggers/v1alpha1/event_listener_validation_test.go b/pkg/apis/triggers/v1alpha1/event_listener_validation_test.go index 10e93c597f..8b975fc1d9 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_validation_test.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_validation_test.go @@ -425,10 +425,10 @@ func TestEventListenerValidate_error(t *testing.T) { bldr.EventListenerTriggerName("1234567890123456789012345678901234567890123456789012345678901234"), ))), }, { - name: "user specify invalid replicas which are 0 and negative values", + name: "user specify invalid replicas", el: bldr.EventListener("name", "namespace", bldr.EventListenerSpec( - bldr.EventListenerReplicas(0), + bldr.EventListenerReplicas(-1), bldr.EventListenerTrigger("tt", "v1alpha1", bldr.EventListenerTriggerBinding("tb", "TriggerBinding", "tb", "v1alpha1"), ))), diff --git a/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go b/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go index c18e1b4f9e..971b70a6c6 100644 --- a/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go +++ b/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go @@ -343,9 +343,12 @@ func (r *Reconciler) reconcileDeployment(logger *zap.SugaredLogger, el *v1alpha1 // Determine if reconciliation has to occur updated := reconcileObjectMeta(&existingDeployment.ObjectMeta, deployment.ObjectMeta) - if existingDeployment.Spec.Replicas != deployment.Spec.Replicas { - existingDeployment.Spec.Replicas = replicas - updated = true + if *existingDeployment.Spec.Replicas != *deployment.Spec.Replicas { + if el.Spec.Replicas != nil { + existingDeployment.Spec.Replicas = replicas + updated = true + } + // if no replicas found as part of el.Spec then replicas from existingDeployment will be considered } if existingDeployment.Spec.Selector != deployment.Spec.Selector { existingDeployment.Spec.Selector = deployment.Spec.Selector diff --git a/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go b/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go index 5ab82f808e..909590ebd1 100644 --- a/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go +++ b/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go @@ -39,6 +39,7 @@ import ( "knative.dev/pkg/apis" fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake" "knative.dev/pkg/configmap" + "knative.dev/pkg/ptr" rtesting "knative.dev/pkg/reconciler/testing" ) @@ -355,6 +356,10 @@ func TestReconcile(t *testing.T) { el.Spec.PodTemplate.NodeSelector = updateNodeSelector }) + elWithReplicas := makeEL(withStatus, func(el *v1alpha1.EventListener) { + el.Spec.Replicas = ptr.Int32(2) + }) + elWithDeploymentReplicaFailure := makeEL(withStatus, func(el *v1alpha1.EventListener) { el.Status.SetCondition(&apis.Condition{ Type: apis.ConditionType(appsv1.DeploymentReplicaFailure), @@ -385,8 +390,11 @@ func TestReconcile(t *testing.T) { }) deploymentWithUpdatedReplicas := makeDeployment(func(d *appsv1.Deployment) { - var updateReplicas int32 = 5 - d.Spec.Replicas = &updateReplicas + d.Spec.Replicas = ptr.Int32(5) + }) + + deploymentWithUpdatedReplicasNotConsidered := makeDeployment(func(d *appsv1.Deployment) { + d.Spec.Replicas = ptr.Int32(2) }) deploymentMissingVolumes := makeDeployment(func(d *appsv1.Deployment) { @@ -624,8 +632,8 @@ func TestReconcile(t *testing.T) { ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, }, { - // Checks that we do not overwrite replicas changed on the deployment itself - name: "deployment-replica-update", + // Updating replicas on deployment itself is success because no replicas provided as part of eventlistener spec + name: "deployment-replica-update-success", key: reconcileKey, startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, @@ -636,7 +644,7 @@ func TestReconcile(t *testing.T) { endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, EventListeners: []*v1alpha1.EventListener{elWithStatus}, - Deployments: []*appsv1.Deployment{elDeployment}, + Deployments: []*appsv1.Deployment{deploymentWithUpdatedReplicas}, Services: []*corev1.Service{elService}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, @@ -686,6 +694,23 @@ func TestReconcile(t *testing.T) { Services: []*corev1.Service{elService}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, + }, { + // Checks that we do not overwrite replicas changed on the deployment itself when replicas provided as part of eventlistener spec + name: "deployment-replica-update-unsuccessful", + key: reconcileKey, + startResources: test.Resources{ + Namespaces: []*corev1.Namespace{namespaceResource}, + EventListeners: []*v1alpha1.EventListener{elWithReplicas}, + Deployments: []*appsv1.Deployment{deploymentWithUpdatedReplicas}, + Services: []*corev1.Service{elService}, + }, + endResources: test.Resources{ + Namespaces: []*corev1.Namespace{namespaceResource}, + EventListeners: []*v1alpha1.EventListener{elWithReplicas}, + Deployments: []*appsv1.Deployment{deploymentWithUpdatedReplicasNotConsidered}, + Services: []*corev1.Service{elService}, + ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From c05c2eb0526aad553b0e5b0a452d06429b6743a5 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Thu, 10 Sep 2020 17:07:00 -0400 Subject: [PATCH 5/7] Add a IdleTimeout for EL sink Previously, we were never closing Idle connections leading to issues described in #687. This commit adds a fixed 2 minute timeout for idle connections though later we can also add other timeouts as well as allow for users to change the timeout values. I verified this manually by building on a base image with a shell and then verifying that the number of open connections eventually go down unlike before. Signed-off-by: Dibyo Mukherjee --- cmd/eventlistenersink/main.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/cmd/eventlistenersink/main.go b/cmd/eventlistenersink/main.go index 0b42c3e041..0a9b22dfc5 100644 --- a/cmd/eventlistenersink/main.go +++ b/cmd/eventlistenersink/main.go @@ -20,6 +20,7 @@ import ( "fmt" "log" "net/http" + "time" "go.uber.org/zap" @@ -95,11 +96,23 @@ func main() { // Listen and serve logger.Infof("Listen and serve on port %s", sinkArgs.Port) - http.HandleFunc("/", r.HandleEvent) + mux := http.NewServeMux() + mux.HandleFunc("/", r.HandleEvent) + // For handling Liveness Probe - http.HandleFunc("/live", func(w http.ResponseWriter, r *http.Request) { + // TODO(dibyom): Livness, metrics etc. should be on a separate port + mux.HandleFunc("/live", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) fmt.Fprint(w, "ok") }) - logger.Fatal(http.ListenAndServe(fmt.Sprintf(":%s", sinkArgs.Port), nil)) + + srv := &http.Server{ + Addr: fmt.Sprintf(":%s", sinkArgs.Port), + IdleTimeout: 120 * time.Second, + Handler: mux, + } + + if err := srv.ListenAndServe(); err != nil { + logger.Fatalf("faiiled to start eventlistener sink: %v", err) + } } From 7622d51115eedb1907e7144ca45dc33b5483bfdb Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Thu, 10 Sep 2020 13:57:01 -0400 Subject: [PATCH 6/7] Merge annotations on created resources In #712, we added a feature to propagate annotations added to the EL to its underlying resources. However, this resulted in infinite reconcile loops since the deployment controller will add a standard revision annotation that the EL controller will keep trying to overwrite. To fix this, this commit switches the annotation propagation to merge any annotations set on the underlying resources before adding any extra annotations from the EL. Fixes #752 Signed-off-by: Dibyo Mukherjee --- .../v1alpha1/eventlistener/eventlistener.go | 25 ++++--- .../eventlistener/eventlistener_test.go | 68 +++++++------------ 2 files changed, 39 insertions(+), 54 deletions(-) diff --git a/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go b/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go index 971b70a6c6..bdfbdc3a44 100644 --- a/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go +++ b/pkg/reconciler/v1alpha1/eventlistener/eventlistener.go @@ -153,18 +153,21 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, el *v1alpha1.EventListene return nil } -func reconcileObjectMeta(oldMeta *metav1.ObjectMeta, newMeta metav1.ObjectMeta) (updated bool) { - if !reflect.DeepEqual(oldMeta.Labels, newMeta.Labels) { +func reconcileObjectMeta(existing *metav1.ObjectMeta, desired metav1.ObjectMeta) (updated bool) { + if !reflect.DeepEqual(existing.Labels, desired.Labels) { updated = true - oldMeta.Labels = newMeta.Labels + existing.Labels = desired.Labels } - if !reflect.DeepEqual(oldMeta.Annotations, newMeta.Annotations) { + + // TODO(dibyom): We should exclude propagation of some annotations such as `kubernetes.io/last-applied-revision` + if !reflect.DeepEqual(existing.Annotations, mergeMaps(existing.Annotations, desired.Annotations)) { updated = true - oldMeta.Annotations = newMeta.Annotations + existing.Annotations = desired.Annotations } - if !reflect.DeepEqual(oldMeta.OwnerReferences, newMeta.OwnerReferences) { + + if !reflect.DeepEqual(existing.OwnerReferences, desired.OwnerReferences) { updated = true - oldMeta.OwnerReferences = newMeta.OwnerReferences + existing.OwnerReferences = desired.OwnerReferences } return } @@ -252,7 +255,7 @@ func (r *Reconciler) reconcileDeployment(logger *zap.SugaredLogger, el *v1alpha1 return err } - labels := mergeLabels(el.Labels, GenerateResourceLabels(el.Name)) + labels := mergeMaps(el.Labels, GenerateResourceLabels(el.Name)) var replicas = ptr.Int32(1) if el.Spec.Replicas != nil { replicas = el.Spec.Replicas @@ -445,14 +448,14 @@ func generateObjectMeta(el *v1alpha1.EventListener) metav1.ObjectMeta { Namespace: el.Namespace, Name: el.Status.Configuration.GeneratedResourceName, OwnerReferences: []metav1.OwnerReference{*el.GetOwnerReference()}, - Labels: mergeLabels(el.Labels, GenerateResourceLabels(el.Name)), + Labels: mergeMaps(el.Labels, GenerateResourceLabels(el.Name)), Annotations: el.Annotations, } } -// mergeLabels merges the values in the passed maps into a new map. +// mergeMaps merges the values in the passed maps into a new map. // Values within m2 potentially clobber m1 values. -func mergeLabels(m1, m2 map[string]string) map[string]string { +func mergeMaps(m1, m2 map[string]string) map[string]string { merged := make(map[string]string, len(m1)+len(m2)) for k, v := range m1 { merged[k] = v diff --git a/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go b/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go index 909590ebd1..c411a0513e 100644 --- a/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go +++ b/pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go @@ -129,7 +129,6 @@ func getEventListenerTestAssets(t *testing.T, r test.Resources) (test.Assets, co // If no ops are specified, it generates a base EventListener with no triggers and no Status func makeEL(ops ...func(el *v1alpha1.EventListener)) *v1alpha1.EventListener { e := bldr.EventListener(eventListenerName, namespace, - bldr.EventListenerSpec( bldr.EventListenerServiceAccount("sa"), ), @@ -335,10 +334,7 @@ func TestReconcile(t *testing.T) { t.Fatal(err) } - elPreReoncile := makeEL() elWithStatus := makeEL(withStatus) - elWithLabels := makeEL(withStatus, withAddedLabels) - elWithAnnotations := makeEL(withStatus, withAddedAnnotations) elWithUpdatedSA := makeEL(withStatus, func(el *v1alpha1.EventListener) { el.Spec.ServiceAccountName = updatedSa @@ -368,9 +364,9 @@ func TestReconcile(t *testing.T) { elDeployment := makeDeployment() elDeploymentWithLabels := makeDeployment(func(d *appsv1.Deployment) { - d.Labels = mergeLabels(updateLabel, generatedLabels) + d.Labels = mergeMaps(updateLabel, generatedLabels) d.Spec.Selector.MatchLabels = generatedLabels - d.Spec.Template.Labels = mergeLabels(updateLabel, generatedLabels) + d.Spec.Template.Labels = mergeMaps(updateLabel, generatedLabels) }) elDeploymentWithAnnotations := makeDeployment(func(d *appsv1.Deployment) { @@ -405,7 +401,7 @@ func TestReconcile(t *testing.T) { elService := makeService() elServiceWithLabels := makeService(func(s *corev1.Service) { - s.Labels = mergeLabels(updateLabel, generatedLabels) + s.Labels = mergeMaps(updateLabel, generatedLabels) s.Spec.Selector = generatedLabels }) @@ -437,13 +433,13 @@ func TestReconcile(t *testing.T) { key: reconcileKey, startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elPreReoncile}, + EventListeners: []*v1alpha1.EventListener{makeEL()}, }, endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithStatus}, - Deployments: []*appsv1.Deployment{elDeployment}, - Services: []*corev1.Service{elService}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus)}, + Deployments: []*appsv1.Deployment{makeDeployment()}, + Services: []*corev1.Service{makeService()}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, }, { @@ -452,16 +448,16 @@ func TestReconcile(t *testing.T) { // Resources before reconcile starts: EL has extra label that deployment/svc does not startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithLabels}, - Deployments: []*appsv1.Deployment{elDeployment}, - Services: []*corev1.Service{elService}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedLabels)}, + Deployments: []*appsv1.Deployment{makeDeployment()}, + Services: []*corev1.Service{makeService()}, }, // We expect the deployment and services to propagate the extra label // but the selectors in both Service and deployment should have the same // label endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithLabels}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedLabels)}, Deployments: []*appsv1.Deployment{elDeploymentWithLabels}, Services: []*corev1.Service{elServiceWithLabels}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, @@ -472,14 +468,14 @@ func TestReconcile(t *testing.T) { // Resources before reconcile starts: EL has annotation that deployment/svc does not startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithAnnotations}, - Deployments: []*appsv1.Deployment{elDeployment}, - Services: []*corev1.Service{elService}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedAnnotations)}, + Deployments: []*appsv1.Deployment{makeDeployment()}, + Services: []*corev1.Service{makeService()}, }, // We expect the deployment and services to propagate the annotations endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithAnnotations}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus, withAddedAnnotations)}, Deployments: []*appsv1.Deployment{elDeploymentWithAnnotations}, Services: []*corev1.Service{elServiceWithAnnotation}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, @@ -566,7 +562,7 @@ func TestReconcile(t *testing.T) { ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, }, { - // Check that if a user manually updates the annotations for a service, we revert the change. + // Check that if a user manually updates the annotations for a service, we do not revert the change. name: "update-el-service-annotations", key: reconcileKey, startResources: test.Resources{ @@ -578,7 +574,7 @@ func TestReconcile(t *testing.T) { endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, EventListeners: []*v1alpha1.EventListener{elWithStatus}, - Services: []*corev1.Service{elService}, // We expect the service to drop the user added annotations + Services: []*corev1.Service{elServiceWithAnnotation}, Deployments: []*appsv1.Deployment{elDeployment}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, @@ -616,6 +612,7 @@ func TestReconcile(t *testing.T) { ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, }, { + // Check that if a user manually updates the annotations for a deployment, we do not revert the change. name: "deployment-annotation-update", key: reconcileKey, startResources: test.Resources{ @@ -627,7 +624,7 @@ func TestReconcile(t *testing.T) { endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, EventListeners: []*v1alpha1.EventListener{elWithStatus}, - Deployments: []*appsv1.Deployment{elDeployment}, + Deployments: []*appsv1.Deployment{elDeploymentWithAnnotations}, Services: []*corev1.Service{elService}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, }, @@ -669,27 +666,12 @@ func TestReconcile(t *testing.T) { key: reconcileKey, startResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithStatus}, - Deployments: []*appsv1.Deployment{deploymentMissingVolumes}, - }, - endResources: test.Resources{ - Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithStatus}, - Deployments: []*appsv1.Deployment{elDeployment}, - Services: []*corev1.Service{elService}, - ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, - }, - }, { - name: "eventlistener-config-volume-mount-update", - key: reconcileKey, - startResources: test.Resources{ - Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithStatus}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus)}, Deployments: []*appsv1.Deployment{deploymentMissingVolumes}, }, endResources: test.Resources{ Namespaces: []*corev1.Namespace{namespaceResource}, - EventListeners: []*v1alpha1.EventListener{elWithStatus}, + EventListeners: []*v1alpha1.EventListener{makeEL(withStatus)}, Deployments: []*appsv1.Deployment{elDeployment}, Services: []*corev1.Service{elService}, ConfigMaps: []*corev1.ConfigMap{loggingConfigMap}, @@ -867,7 +849,7 @@ func Test_wrapError(t *testing.T) { } } -func Test_mergeLabels(t *testing.T) { +func Test_mergeMaps(t *testing.T) { tests := []struct { name string l1, l2 map[string]string @@ -900,7 +882,7 @@ func Test_mergeLabels(t *testing.T) { }} for i := range tests { t.Run(tests[i].name, func(t *testing.T) { - actualLabels := mergeLabels(tests[i].l1, tests[i].l2) + actualLabels := mergeMaps(tests[i].l1, tests[i].l2) if diff := cmp.Diff(tests[i].expectedLabels, actualLabels); diff != "" { t.Errorf("mergeLabels() did not return expected. -want, +got: %s", diff) } @@ -909,7 +891,7 @@ func Test_mergeLabels(t *testing.T) { } func TestGenerateResourceLabels(t *testing.T) { - expectedLabels := mergeLabels(StaticResourceLabels, map[string]string{"eventlistener": eventListenerName}) + expectedLabels := mergeMaps(StaticResourceLabels, map[string]string{"eventlistener": eventListenerName}) actualLabels := GenerateResourceLabels(eventListenerName) if diff := cmp.Diff(expectedLabels, actualLabels); diff != "" { t.Errorf("mergeLabels() did not return expected. -want, +got: %s", diff) @@ -977,7 +959,7 @@ func Test_generateObjectMeta(t *testing.T) { Controller: &isController, BlockOwnerDeletion: &blockOwnerDeletion, }}, - Labels: mergeLabels(map[string]string{"k": "v"}, generatedLabels), + Labels: mergeMaps(map[string]string{"k": "v"}, generatedLabels), }, }, { name: "EventListener with Annotation", From 2cc62a2beceb14c54d8f8d7857e597d9c4048a10 Mon Sep 17 00:00:00 2001 From: savitaashture Date: Mon, 14 Sep 2020 22:52:00 +0530 Subject: [PATCH 7/7] Add release links to versioned docs for v0.8.1 --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index c97f792a0f..39089c88ac 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,7 @@ events. | Version | Docs | Examples | Getting Started | | ---------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | | [HEAD](https://github.com/tektoncd/triggers/blob/master/DEVELOPMENT.md#install-pipeline) | [Docs @ HEAD](https://github.com/tektoncd/triggers/blob/master/docs/README.md) | [Examples @ HEAD](https://github.com/tektoncd/triggers/blob/master/examples) | [Getting Started @ HEAD](https://github.com/tektoncd/triggers/blob/master/docs/getting-started#getting-started-with-triggers) | +| [v0.8.1](https://github.com/tektoncd/triggers/releases/tag/v0.8.1) | [Docs @ v0.8.1](https://github.com/tektoncd/triggers/tree/v0.8.1/docs#tekton-triggers) | [Examples @ v0.8.1](https://github.com/tektoncd/triggers/tree/v0.8.1/examples#examples) | [Getting Started @ v0.8.1](https://github.com/tektoncd/triggers/tree/v0.8.1/docs/getting-started#getting-started-with-triggers) | | [v0.8.0](https://github.com/tektoncd/triggers/releases/tag/v0.8.0) | [Docs @ v0.8.0](https://github.com/tektoncd/triggers/tree/v0.8.0/docs#tekton-triggers) | [Examples @ v0.8.0](https://github.com/tektoncd/triggers/tree/v0.8.0/examples#examples) | [Getting Started @ v0.8.0](https://github.com/tektoncd/triggers/tree/v0.8.0/docs/getting-started#getting-started-with-triggers) | | [v0.7.0](https://github.com/tektoncd/triggers/releases/tag/v0.7.0) | [Docs @ v0.7.0](https://github.com/tektoncd/triggers/tree/v0.7.0/docs#tekton-triggers) | [Examples @ v0.7.0](https://github.com/tektoncd/triggers/tree/v0.7.0/examples#examples) | [Getting Started @ v0.7.0](https://github.com/tektoncd/triggers/tree/v0.7.0/docs/getting-started#getting-started-with-triggers) | | [v0.6.1](https://github.com/tektoncd/triggers/releases/tag/v0.6.1) | [Docs @ v0.6.1](https://github.com/tektoncd/triggers/tree/v0.6.1/docs#tekton-triggers) | [Examples @ v0.6.1](https://github.com/tektoncd/triggers/tree/v0.6.1/examples#examples) | [Getting Started @ v0.6.1](https://github.com/tektoncd/triggers/tree/v0.6.1/docs/getting-started#getting-started-with-triggers) |