Skip to content

Commit

Permalink
Remove finalizer if present
Browse files Browse the repository at this point in the history 
Previous versions of Triggers required a finalizer to clean up logging
configMaps. They are no longer necessary. We need to remove the finalizer from
any old EventListener objects so that they can be properly deleted in newer
versions of Triggers.

Also fixes some unit tests that started failing once I removed the bit that
ignored `Finalizers` when comparing test output.

Fixes #1243

Signed-off-by: Dibyo Mukherjee <[email protected]>
  • Loading branch information
@dibyom @tekton-robot
dibyom authored and tekton-robot committed Oct 20, 2021
1 parent 71d8609 commit a6b1276
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 104 deletions.
20 changes: 20 additions & 0 deletions pkg/reconciler/eventlistener/eventlistener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, el *v1beta1.EventListene
if el.Spec.Resources.CustomResource == nil {
el.Status.SetReadyCondition()
}
if len(el.Finalizers) > 0 {
// TODO(dibyom): Remove this in a future release once we are sure no one is using pre v0.16 resources
r.removeFinalizer(ctx, el)
}

return wrapError(serviceReconcileError, deploymentReconcileError)
}

Expand Down Expand Up @@ -362,6 +367,21 @@ func (r *Reconciler) reconcileCustomObject(ctx context.Context, el *v1beta1.Even
return nil
}

func (r *Reconciler) removeFinalizer(ctx context.Context, el *v1beta1.EventListener) {
// We used to need Finalizers in older versions of Triggers.
// They are not necessary anymore so let's remove them from any old EventListener objects
for i, f := range el.Finalizers {
if f == "eventlisteners.triggers.tekton.dev" {
el.Finalizers = append(el.Finalizers[:i], el.Finalizers[i+1:]...)
_, err := r.TriggersClientSet.TriggersV1beta1().EventListeners(el.Namespace).Update(ctx, el, metav1.UpdateOptions{})
if err != nil {
logging.FromContext(ctx).Errorf("failed to update EventListener to remove finalizer: %v", err)
}
break
}
}
}

// wrapError wraps errors together. If one of the errors is nil, the other is
// returned.
func wrapError(err1, err2 error) error {
Expand Down
183 changes: 79 additions & 104 deletions pkg/reconciler/eventlistener/eventlistener_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ func compareCondition(x, y apis.Condition) bool {
return x.Type < y.Type
}

// compareEnv compares two conditions based on their Type field.
func compareEnv(x, y corev1.EnvVar) bool {
return x.Name < y.Name
}

// getEventListenerTestAssets returns TestAssets that have been seeded with the
// given test.Resources r where r represents the state of the system
func getEventListenerTestAssets(t *testing.T, r test.Resources, c *resources.Config) (test.Assets, context.CancelFunc) {
Expand Down Expand Up @@ -218,13 +223,17 @@ func makeDeployment(ops ...func(d *appsv1.Deployment)) *appsv1.Deployment {
FailureThreshold: int32(resources.DefaultFailureThreshold),
},
Args: []string{
"-el-name", eventListenerName,
"-el-namespace", namespace,
"-port", strconv.Itoa(eventListenerContainerPort),
"readtimeout", strconv.FormatInt(resources.DefaultReadTimeout, 10),
"writetimeout", strconv.FormatInt(resources.DefaultWriteTimeout, 10),
"idletimeout", strconv.FormatInt(resources.DefaultIdleTimeout, 10),
"timeouthandler", strconv.FormatInt(resources.DefaultTimeOutHandler, 10),
"--el-name=" + eventListenerName,
"--el-namespace=" + namespace,
"--port=" + strconv.Itoa(eventListenerContainerPort),
"--readtimeout=" + strconv.FormatInt(resources.DefaultReadTimeout, 10),
"--writetimeout=" + strconv.FormatInt(resources.DefaultWriteTimeout, 10),
"--idletimeout=" + strconv.FormatInt(resources.DefaultIdleTimeout, 10),
"--timeouthandler=" + strconv.FormatInt(resources.DefaultTimeOutHandler, 10),
"--is-multi-ns=false",
"--payload-validation=true",
"--tls-cert=",
"--tls-key=",
},
Env: []corev1.EnvVar{{
Name: "K_LOGGING_CONFIG",
Expand Down Expand Up @@ -273,96 +282,49 @@ func makeDeployment(ops ...func(d *appsv1.Deployment)) *appsv1.Deployment {
}

var withTLSConfig = func(d *appsv1.Deployment) {
d.Spec.Template.Spec.Containers = []corev1.Container{{
Name: "event-listener",
Image: resources.DefaultImage,
Ports: []corev1.ContainerPort{{
ContainerPort: int32(eventListenerContainerPort),
Protocol: corev1.ProtocolTCP,
}, {
ContainerPort: int32(9000),
Protocol: corev1.ProtocolTCP,
}},
LivenessProbe: &corev1.Probe{
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/live",
Scheme: corev1.URISchemeHTTPS,
Port: intstr.FromInt(eventListenerContainerPort),
},
},
PeriodSeconds: int32(resources.DefaultPeriodSeconds),
FailureThreshold: int32(resources.DefaultFailureThreshold),
},
ReadinessProbe: &corev1.Probe{
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/live",
Scheme: corev1.URISchemeHTTPS,
Port: intstr.FromInt(eventListenerContainerPort),
},
},
PeriodSeconds: int32(resources.DefaultPeriodSeconds),
FailureThreshold: int32(resources.DefaultFailureThreshold),
},
Args: []string{
"-el-name", eventListenerName,
"-el-namespace", namespace,
"-port", strconv.Itoa(eventListenerContainerPort),
"-tls-cert", "/etc/triggers/tls/tls.pem",
"-tls-key", "/etc/triggers/tls/tls.key",
},
VolumeMounts: []corev1.VolumeMount{{
Name: "https-connection",
MountPath: "/etc/triggers/tls",
ReadOnly: true,
}},
Env: []corev1.EnvVar{{
Name: "K_LOGGING_CONFIG",
}, {
Name: "K_METRICS_CONFIG",
Value: `{"Domain":"","Component":"","PrometheusPort":0,"PrometheusHost":"","ConfigMap":null}`,
}, {
Name: "K_TRACING_CONFIG",
Value: `{"backend":"","debug":"false","sample-rate":"0"}`,
}, {
Name: "NAMESPACE",
Value: namespace,
}, {
Name: "NAME",
Value: eventListenerName,
}, {
Name: "TLS_CERT",
ValueFrom: &corev1.EnvVarSource{
SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{
Name: "tls-secret-key",
},
Key: "tls.crt",
},
},
}, {
Name: "TLS_KEY",
ValueFrom: &corev1.EnvVarSource{
SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{
Name: "tls-secret-key",
},
Key: "tls.key",
// Replace the 2 TLS args with the right values
container := &d.Spec.Template.Spec.Containers[0]

// Set probes to use HTTPS
container.LivenessProbe.HTTPGet.Scheme = corev1.URISchemeHTTPS
container.ReadinessProbe.HTTPGet.Scheme = corev1.URISchemeHTTPS

// Pass keys as container args
for i, arg := range container.Args {
if arg == "--tls-key=" {
container.Args[i] = "--tls-key=/etc/triggers/tls/tls.key"
}
if arg == "--tls-cert=" {
container.Args[i] = "--tls-cert=/etc/triggers/tls/tls.crt"
}
}
container.VolumeMounts = []corev1.VolumeMount{{
Name: "https-connection",
MountPath: "/etc/triggers/tls",
ReadOnly: true,
}}

container.Env = append(container.Env, corev1.EnvVar{
Name: "TLS_CERT",
ValueFrom: &corev1.EnvVarSource{
SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{
Name: "tls-secret-key",
},
Key: "tls.crt",
},
}, {
Name: "SYSTEM_NAMESPACE",
ValueFrom: &corev1.EnvVarSource{
FieldRef: &corev1.ObjectFieldSelector{
FieldPath: "metadata.namespace",
},
}, corev1.EnvVar{
Name: "TLS_KEY",
ValueFrom: &corev1.EnvVarSource{
SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{
Name: "tls-secret-key",
},
Key: "tls.key",
},
}, {
Name: "METRICS_PROMETHEUS_PORT",
Value: "9000",
}},
}}
},
})
d.Spec.Template.Spec.Volumes = []corev1.Volume{{
Name: "https-connection",
VolumeSource: corev1.VolumeSource{
Expand Down Expand Up @@ -411,6 +373,7 @@ func makeWithPod(ops ...func(d *duckv1.WithPod)) *duckv1.WithPod {
"--idletimeout=" + strconv.FormatInt(resources.DefaultIdleTimeout, 10),
"--timeouthandler=" + strconv.FormatInt(resources.DefaultTimeOutHandler, 10),
"--is-multi-ns=" + strconv.FormatBool(false),
"--payload-validation=" + strconv.FormatBool(true),
},
Env: []corev1.EnvVar{{
Name: "K_LOGGING_CONFIG",
Expand Down Expand Up @@ -907,12 +870,6 @@ func TestReconcile(t *testing.T) {
}
})

argsForCustomResource := makeWithPod(func(data *duckv1.WithPod) {
data.Spec.Template.Spec.Containers[0].Args = []string{
"--is-multi-ns=" + strconv.FormatBool(true),
}
})

imageForCustomResource := makeWithPod(func(data *duckv1.WithPod) {
data.Spec.Template.Spec.Containers[0].Image = resources.DefaultImage
})
Expand Down Expand Up @@ -1347,17 +1304,17 @@ func TestReconcile(t *testing.T) {
WithPod: []*duckv1.WithPod{nodeSelectorForCustomResource},
},
}, {
name: "eventlistener with added Args for custom resource",
// If a user provides custom args, we ignore them and create the EL with the standard set of args
name: "CustomResource EventListener with user provided custom args",
key: reconcileKey,
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1beta1.EventListener{elWithCustomResourceForArgs},
WithPod: []*duckv1.WithPod{argsForCustomResource},
},
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1beta1.EventListener{elWithCustomResourceForArgs},
WithPod: []*duckv1.WithPod{argsForCustomResource},
WithPod: []*duckv1.WithPod{makeWithPod()},
},
}, {
name: "eventlistener with added Image for custom resource",
Expand Down Expand Up @@ -1399,6 +1356,25 @@ func TestReconcile(t *testing.T) {
EventListeners: []*v1beta1.EventListener{elWithCustomResourceForNodeSelector},
WithPod: []*duckv1.WithPod{nodeSelectorForCustomResource},
},
}, {
name: "reconcile removes old finalizers", // See #1243
key: reconcileKey,
startResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
// The initial EL needs to have a Status set else the update from the generated Reconcile
// overwrites the update from removeFinalizer
EventListeners: []*v1beta1.EventListener{makeEL(withStatus, func(el *v1beta1.EventListener) {
el.Finalizers = append(el.Finalizers, "eventlisteners.triggers.tekton.dev")
})},
},
endResources: test.Resources{
Namespaces: []*corev1.Namespace{namespaceResource},
EventListeners: []*v1beta1.EventListener{makeEL(withStatus, func(el *v1beta1.EventListener) {
el.Finalizers = []string{} // Finalizer should be removed
})},
Deployments: []*appsv1.Deployment{makeDeployment()},
Services: []*corev1.Service{makeService()},
},
}}

for _, tt := range tests {
Expand All @@ -1419,8 +1395,7 @@ func TestReconcile(t *testing.T) {
}
if diff := cmp.Diff(tt.endResources, *actualEndResources, cmpopts.IgnoreTypes(
apis.Condition{}.LastTransitionTime.Inner.Time,
metav1.ObjectMeta{}.Finalizers,
), cmpopts.SortSlices(compareCondition)); diff != "" {
), cmpopts.SortSlices(compareCondition), cmpopts.SortSlices(compareEnv)); diff != "" {
t.Errorf("eventlistener.Reconcile() equality mismatch. Diff request body: -want +got: %s", diff)
}
})
Expand Down

0 comments on commit a6b1276

Please sign in to comment.