fix: unify webhook listeners on path based endpoints#304
fix: unify webhook listeners on path based endpoints#304pepordev wants to merge 2 commits intoexternal-secrets:mainfrom
Conversation
Signed-off-by: Pedro Parra Ortega <parraortega.pedro@gmail.com>
Signed-off-by: Pedro Parra Ortega <parraortega.pedro@gmail.com>
Skarlso
left a comment
Skarlso
left a comment
There was a problem hiding this comment.
Initial round done. I know this is mostly borrowed code. I tried to flag only criticals. Let's leave it off a bit better than we found it. :)
| if _, exists := lm.listeners[manifestName][key]; exists { | ||
| if source.Type == schema.WEBHOOK && lm.webhookServer != nil && source.Webhook != nil { | ||
| lm.webhookServer.Register(manifestName.Name, lm.context, source.Webhook, lm.client, lm.eventChan, lm.logger) | ||
| lm.logger.V(1).Info("updated webhook registration", "manifest", manifestName, "key", key) | ||
| } else { | ||
| lm.logger.V(1).Info("listener already exists", "key", key) | ||
| } |
There was a problem hiding this comment.
I'm pretty sure this will constantly register a webhook. I think you need to hash the webhookconfig into the listener key to avoid it.
| r := newRoute(routeCtx, configName, cfg, k8sClient, eventChan, logger) | ||
| s.routes[configName] = r | ||
| s.server.Handler = s.buildMux() |
There was a problem hiding this comment.
This is super bad. You are re-assigning a running servier. 🤔 Even with the lock here, listenAndServer does only read without this lock being aware of it. You need to add a test that actually starts the server and call it concurrently with Register a couple times and run it with -race. I'm pretty sure it will trip it immediately.
|
|
||
| if r.config != nil && r.config.RetryPolicy != nil { | ||
| select { | ||
| case r.retryQueue <- &retryMessage{event: event, currentRun: 1, retryAt: time.Now()}: |
There was a problem hiding this comment.
This is a send on close panic potential here, since close closes this channel.
You either don't close it and let the garbage collection collect the channel, or we have to redesign this.
| b, err := io.ReadAll(body) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Use a limited reader. http.MaxBytesReader(w, req.Body, N) with a sane cap is standard procedure
| _, _ = fmt.Fprintln(w, "Couldn't decode webhook payload. Send a valid json") | ||
| return | ||
| } | ||
| _ = req.Body.Close() |
There was a problem hiding this comment.
add this as a defer at the begin?
| // Path that the webhook will receive the notifications. | ||
| // If not present `/webhook` will be used. The path always expects a POST and this is not configurable | ||
| // +optional | ||
| Path string `json:"path"` | ||
|
|
||
| // Address is the address where the webhook will be served in your infrastructure. | ||
| // If not present, defaults to `:8090` | ||
| // +optional | ||
| Address string `json:"address"` | ||
|
|
| } | ||
| } | ||
|
|
||
| func (r *route) drainRetryQueue() { |
There was a problem hiding this comment.
This doesn't really do anything. Since the channel is unbuffered and cancel returns early anyways in processEvent, this will just not do anything.
| func (s *WebhookServer) Start(ctx context.Context) error { | ||
| s.logger.Info("Starting shared webhook server", "addr", s.addr) | ||
| errCh := make(chan error, 1) | ||
| go func() { | ||
| err := s.server.ListenAndServe() | ||
| errCh <- err | ||
| }() | ||
| select { | ||
| case err := <-errCh: | ||
| if err != nil && err != http.ErrServerClosed { | ||
| return err | ||
| } | ||
| return nil | ||
| case <-ctx.Done(): | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| _ = s.closeHTTPServer(shutdownCtx) | ||
| err := <-errCh | ||
| if err != nil && err != http.ErrServerClosed { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Ugh, I'd wish we rewrote this alltogether. :D The error handling is pretty prone to race and/or ignoring binding errors and not cleaning up things. But not a prio.
| Client: client, | ||
| Scheme: scheme, | ||
| webhookServer: hook, | ||
| eventChan: make(chan events.SecretRotationEvent), |
There was a problem hiding this comment.
I think this needs a sane buffer. 🤔 Especially since it's a shared channel.
|
|
||
| ## How it works | ||
|
|
||
| The controller runs a **single shared HTTP server** for all `Config` resources. The listen address is set with the controller flag **`--webhook-bind-address`** (default `:8082`). Each cluster-scoped `Config` is exposed at: |
There was a problem hiding this comment.
There is a mismatch between 8082 and whatever the helm template is setting up with 8090.. Something is fishy there. Please double check.
2 participants
fixes #242
I've test it out in our environments with multiples configurations using our keeper-security integration and it is working fine