feat: support multi receiver by matchLabels by raffis · Pull Request #482 · fluxcd/notification-controller

raffis · 2023-03-06T14:27:16Z

Current situation

At the moment one has to reference each resource by name for a webhook receiver.
While spec.resources actually is a CrossNamespaceObjectReference which supports matchLabels. However this is not implemented in the internal handling of the receiver. What I like to achieve is basically trigger a reconcile for multiple HelmCharts matching a certain label (or just all of them in a specific namespace).

This currently is unsupported in the reconcile logic while it is supported in the api:

apiVersion: notification.toolkit.fluxcd.io/v1beta2
kind: Receiver
metadata:
  name: ghcr-receiver
  namespace: flux-system
spec:
  events:
  - package
  resources:
  - apiVersion: source.toolkit.fluxcd.io/v1beta2
    kind: HelmChart
    matchLabels:
      registry: ghcr
    name: '*'
  secretRef:
    name: github-webhook-token
  type: github

Background

I like to send a webhook from github for helm packages published in ghcr. The receiver should reconcile all (or matching helm charts by label) charts and trigger a reconcile.
What I am trying to avoid is set a very low interval but instead reconcile a helmrelease immediately if a new version is published via webhook.
This works perfectly fine already but I would need to specify each chart manually in the list of resources.

Proposal

Support matchLabels and * as name the same way as this is already supported for Alerts in the notification-controller.

makkes

I don't understand how this is supposed to work. The .name field is still mandatory and this PR doesn't change the way it is handled. The PR description says you can set name to * the same way we support it with Alerts but the commits don't back this up.

In fact, name is still used to fetch a single resource and in addition matchLabels is taken into account as well.

raffis · 2023-03-13T10:34:01Z

I don't understand how this is supposed to work. The .name field is still mandatory and this PR doesn't change the way it is handled. The PR description says you can set name to * the same way we support it with Alerts but the commits don't back this up.

In fact, name is still used to fetch a single resource and in addition matchLabels is taken into account as well.

Hey please align on your side and decide what you want, #482 (comment)

makkes · 2023-03-13T10:52:31Z

I don't understand how this is supposed to work. The .name field is still mandatory and this PR doesn't change the way it is handled. The PR description says you can set name to * the same way we support it with Alerts but the commits don't back this up.
In fact, name is still used to fetch a single resource and in addition matchLabels is taken into account as well.

Hey please align on your side and decide what you want, #482 (comment)

I see 2 issues with the details of this PR:

The .spec.resources.name field is required so it can never be empty.
Even if we changed it back to * as a special case (sorry, haven't looked at the resolved comments) the receiver handler would still try to get an object of that name and fail.

A solution would be to accept * as a name and treat it specially. However, there's still two cases that would need to be covered:

.spec.resources.name == * and .spec.resources.matchLabels is missing.
.spec.resources.name == * and .spec.resources.matchLabels is present.

But then the question arises of how we handle .spec.resources.name not being * and .spec.resources.matchLabels being present. This case doesn't make sense and IMHO should be prevented at the validation level.

raffis · 2023-03-13T11:01:21Z

I don't understand how this is supposed to work. The .name field is still mandatory and this PR doesn't change the way it is handled. The PR description says you can set name to * the same way we support it with Alerts but the commits don't back this up.
In fact, name is still used to fetch a single resource and in addition matchLabels is taken into account as well.

Hey please align on your side and decide what you want, #482 (comment)

I see 2 issues with the details of this PR:
1. The `.spec.resources.name` field is required so it can never be empty.

2. Even if we changed it back to `*` as a special case (sorry, haven't looked at the resolved comments) the receiver handler would still try to get an object of that name and fail.

I will change it back then. That's what I suggested in the first place.

A solution would be to accept * as a name and treat it specially. However, there's still two cases that would need to be covered:
* `.spec.resources.name` == `*` and `.spec.resources.matchLabels` is missing.

Either we can can change it so if no matchLabels are set and its * it would select all resources. Or if no change it would just fallback to select by name which would fail to get a resource since there is none named *. I would say first one.

* `.spec.resources.name` == `*` and `.spec.resources.matchLabels` is present.

Well thats the case which is covered if I change it back how it was initially.

But then the question arises of how we handle .spec.resources.name not being * and .spec.resources.matchLabels being present. This case doesn't make sense and IMHO should be prevented at the validation level.

This would require a validation hook I reckon. But this is also possible in Alert actually. You currently can set match labels while name is not *.

somtochiama · 2023-03-13T15:37:35Z

@makkes My bad, I suggested checking for an empty name field in my last review, forgetting that the name was required.
Thanks @raffis for reverting the change

makkes

This looks much better now. Just some suggestions and please also update docs/api/notification.md to reflect the changes around the semantics of .spec.resources.

makkes

Just a last nit. Otherwise lgtm.

makkes

I think we have everything in place right now. Good job, @raffis. 🎖️

Please squash all your commits into one, then I'll make sure to get it merged.

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>

raffis · 2023-03-20T08:13:25Z

I think we have everything in place right now. Good job, @raffis. medal_military

Please squash all your commits into one, then I'll make sure to get it merged.

Done, thx for the review 🥇

makkes · 2023-03-20T15:32:34Z

@somtochiama mind to take a final look, please? 🙏🏻

makkes · 2023-03-20T15:33:38Z

Holding for next minor release.

somtochiama

LGTM!

Thanks @raffis 🚀

makkes

I'm sorry @raffis but I need to ask for one additional change: Please document the changed API in https://github.com/fluxcd/notification-controller/blob/main/docs/spec/v1beta2/receivers.md#resources. The content of that page is published at https://fluxcd.io/flux/components/notification/receiver/ so we need give clear guidance on how to use matchLabels.

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>

stefanprodan

LGTM

Thanks @raffis 🏅

This has been introduced in #482 but we actually want this feature to only be available in v1 of the API. A follow-up PR will re-add this to the v1 API. Signed-off-by: Max Jonas Werner <mail@makk.es>

This functionality has been implemented in #482 but we only want to expose it in v1 of the API. Signed-off-by: Max Jonas Werner <mail@makk.es>

This has been introduced in fluxcd#482 but we actually want this feature to only be available in v1 of the API. A follow-up PR will re-add this to the v1 API. Signed-off-by: Max Jonas Werner <mail@makk.es>

This functionality has been implemented in fluxcd#482 but we only want to expose it in v1 of the API. Signed-off-by: Max Jonas Werner <mail@makk.es>

raffis force-pushed the feat-receiver-by-labels branch from b455609 to efcff60 Compare March 6, 2023 14:31

raffis mentioned this pull request Mar 9, 2023

Manage labels and annotations for a HelmChart fluxcd/helm-controller#631

Merged

somtochiama requested changes Mar 10, 2023

View reviewed changes

Comment thread internal/server/receiver_handlers.go

Comment thread internal/server/receiver_handlers.go Outdated

raffis force-pushed the feat-receiver-by-labels branch from f8b1720 to 3a7a8ca Compare March 10, 2023 14:16

raffis requested a review from somtochiama March 10, 2023 14:17

makkes requested changes Mar 13, 2023

View reviewed changes

raffis requested review from makkes and removed request for somtochiama March 13, 2023 10:34

somtochiama reviewed Mar 13, 2023

View reviewed changes

Comment thread internal/server/receiver_handlers.go Outdated

raffis requested review from somtochiama and removed request for makkes March 14, 2023 13:42

makkes reviewed Mar 16, 2023

View reviewed changes

Comment thread internal/server/receiver_handler_test.go

Comment thread internal/server/receiver_handlers.go Outdated

raffis force-pushed the feat-receiver-by-labels branch from 8f69751 to 1ae0f47 Compare March 16, 2023 09:46

raffis requested review from makkes and removed request for somtochiama March 16, 2023 11:29

makkes reviewed Mar 17, 2023

View reviewed changes

Comment thread internal/server/receiver_handlers.go Outdated

raffis requested a review from makkes March 20, 2023 07:47

makkes approved these changes Mar 20, 2023

View reviewed changes

feat: support multi receiver by matchLabels

57f62f4

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>

raffis force-pushed the feat-receiver-by-labels branch from 036a521 to 57f62f4 Compare March 20, 2023 08:12

makkes added the hold Issues and pull requests put on hold label Mar 20, 2023

makkes added this to the 0.34 milestone Mar 20, 2023

makkes added the enhancement New feature or request label Mar 20, 2023

makkes added the area/receiver Webhook receiver related issues and PRs label Mar 20, 2023

somtochiama approved these changes Mar 20, 2023

View reviewed changes

makkes requested changes Mar 20, 2023

View reviewed changes

add docs and log when no objects are found

4fcde00

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>

somtochiama requested a review from stefanprodan March 20, 2023 21:44

docs: fix matchLabels part

ef8c973

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>

stefanprodan approved these changes Mar 23, 2023

View reviewed changes

stefanprodan requested a review from makkes March 23, 2023 09:54

makkes approved these changes Mar 23, 2023

View reviewed changes

makkes merged commit f8ab99e into fluxcd:main Mar 23, 2023

makkes removed the hold Issues and pull requests put on hold label Mar 23, 2023

makkes mentioned this pull request Mar 27, 2023

un-document receivers.spec.resources.matchLabels #501

Merged

makkes pushed a commit that referenced this pull request Mar 27, 2023

document receivers.spec.resources.matchLabels in v1 API

d558ae3

This functionality has been implemented in #482 but we only want to expose it in v1 of the API. Signed-off-by: Max Jonas Werner <mail@makk.es>

makkes pushed a commit that referenced this pull request Mar 27, 2023

document receivers.spec.resources.matchLabels in v1 API

f30a7c9

This functionality has been implemented in #482 but we only want to expose it in v1 of the API. Signed-off-by: Max Jonas Werner <mail@makk.es>

makkes pushed a commit that referenced this pull request Mar 30, 2023

document receivers.spec.resources.matchLabels in v1 API

dbeb5a3

This functionality has been implemented in #482 but we only want to expose it in v1 of the API. Signed-off-by: Max Jonas Werner <mail@makk.es>

makkes mentioned this pull request Oct 20, 2023

Namespace fallback for annotation logs #492

Closed

Conversation

raffis commented Mar 6, 2023

Current situation

Background

Proposal

Uh oh!

Uh oh!

Uh oh!

makkes left a comment

Choose a reason for hiding this comment

Uh oh!

raffis commented Mar 13, 2023

Uh oh!

makkes commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raffis commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

somtochiama commented Mar 13, 2023

Uh oh!

Uh oh!

makkes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

makkes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

makkes left a comment

Choose a reason for hiding this comment

Uh oh!

raffis commented Mar 20, 2023

Uh oh!

makkes commented Mar 20, 2023

Uh oh!

makkes commented Mar 20, 2023

Uh oh!

somtochiama left a comment

Choose a reason for hiding this comment

Uh oh!

makkes left a comment

Choose a reason for hiding this comment

Uh oh!

stefanprodan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

makkes commented Mar 13, 2023 •

edited

Loading

raffis commented Mar 13, 2023 •

edited

Loading