webhook source in configurations create new servers per config

[pepordev]

I've been testing the solution with several webhooks configurations we have and the actual solution creates a new server for each config which could cause some issues.

• Port colissions
• Port not being accesible as those need to be exposed

Proposal

I would suggest that reloader does only expose 1 server for the webhook sources and then based on the path for each config reloader process or skip configurations.

The webhook configuration would only require the secretIdentifierOnPayload and use a configurable path and port

Example

---
apiVersion: reloader.external-secrets.io/v1alpha1
kind: Config
metadata:
  name: webhook-to-external-secrets-from-keeper
spec:
  notificationSources:
    - type: Webhook
      webhook:
        secretIdentifierOnPayload: record_uid

---
apiVersion: reloader.external-secrets.io/v1alpha1
kind: Config
metadata:
  name: webhook-to-external-secrets-from-xxx
spec:
  notificationSources:
    - type: Webhook
      webhook:
        secretIdentifierOnPayload: data.0.uid

if a payload comes in with an existing path record_uid then it only process the webhook-to-external-secrets-from-keeper and skip the other one.

What do you think? let me know your thoughts, i'm willing on contribute to it...

[pepordev]

any thoughts @Skarlso @moolen @gusfcarvalho ?

[Skarlso]

Sorry for missing this, I was trying to get reloader into shape. :D

[Skarlso]

Still working on a release and a helmchart for reloader. :D After that I'll be able to see what's up here. :)

[Skarlso]

Helm chart and release is done. I'm going to do this next. :)

[pepordev]

Helm chart and release is done. I'm going to do this next. :)

i can work on this if you agree on the proposal, or if you have any other proposal

[Skarlso]

Hm. This requires very careful coding. In general, I'm okay with a single server handling things, but in practice it will be a bitch to get it right.

Just a few things at the top of my head:

• Per route auth configuration ( right now it's easy since it's a new server on each config )
• Auto-generated unique paths to avoid multi-tenancy problems with path collisions
• Panic recovery middleware
• Graceful route removal once a config is deleted
• Retry queue isolation
• One failing handler MUST NOT influence others
• When a config is deleted obviously don't kill the server ( ties into route removal point above ).

So there is a LOT to consider here.

[pepordev]

Hm. This requires very careful coding. In general, I'm okay with a single server handling things, but in practice it will be a bitch to get it right.

Just a few things at the top of my head:

• Per route auth configuration ( right now it's easy since it's a new server on each config )
• Auto-generated unique paths to avoid multi-tenancy problems with path collisions
• Panic recovery middleware
• Graceful route removal once a config is deleted
• Retry queue isolation
• One failing handler MUST NOT influence others
• When a config is deleted obviously don't kill the server ( ties into route removal point above ).

So there is a LOT to consider here.

what i was thinking is something like this implementation:

• A single WebhookServer starts eagerly at boot on the --webhook-bind-address flag (cmd/main.go:78, currently declared but unused).
• Routes are auto-derived from the Config CR name: POST /webhook/"config-name". Since Config is cluster-scoped, names are unique, so path collisions are impossible and each config will listen in their own endpoint.
• No Provider/CreateListener indirection for webhooks. The Manager calls WebhookServer.Register() directly when it encounters a webhook source, which returns a schema.Listener-compatible object for lifecycle tracking.
• Each registered handler retains its own authentication, identifierPathOnPayload, retry queue, and event channel.

this would remove both Address and Path fields from WebhookConfig:

• Address — now controller-level via --webhook-bind-address
• Path — auto-derived as /webhook/"config-name"
• Keep: SecretIdentifierOnPayload, Auth, RetryPolicy

[Skarlso]

Sounds okay. Keep in mind garbage collection as well as path removal when unregistering.