fix: unify webhook listeners on path based endpoints#304
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. :)
Signed-off-by: Pedro <parraortega.pedro@gmail.com>
Signed-off-by: Pedro Parra Ortega <parraortega.pedro@gmail.com>
Signed-off-by: Pedro Parra Ortega <parraortega.pedro@gmail.com>
|
I'll fix the cve thing. |
|
Done :) Should be on main. |
updated! thanks! |
|
will take a look at this today afternoon! Thanks |
Skarlso
left a comment
Skarlso
left a comment
There was a problem hiding this comment.
Couple of more issues, but it's getting there! :) Nice! :)
| eventListener := webhook.NewWebhookListener( | ||
| lm.webhookServer, | ||
| manifestName.Name, | ||
| lm.context, | ||
| webhookCfg, | ||
| lm.client, | ||
| lm.eventChan, | ||
| lm.logger, | ||
| ) |
There was a problem hiding this comment.
Multiple configs with the same name are overwritten.
There was a problem hiding this comment.
Multiple configs with the same name are overwritten.
configs are cluster wide resources no name colision possible as the api would reject any duplication. The only option for override would be the case were a config have multiples webhook configurations as source. Which is something that does not make too much sense to me. I see 2 options:
- Not allow multiples wehookConfig for a config.
- add a optional field in the WebhookConfig to add a suffix to the webhook path. so we would allow multiples webhooks in 1 config.
- /webhook/keeper/config1
- /webhook/keeper/config2
What do you think?
| message.retryAt = getNextRetryAt(r.config.RetryPolicy.Algorithm, message.currentRun) | ||
| } | ||
| select { | ||
| case r.retryQueue <- message: |
There was a problem hiding this comment.
You are self-sending here... This will deadlock 🤔 Your only receiver is the same go routine. I'm wondering how this even works in any tests. 🤔 This should effectively die after like... the first retry.
| } | ||
|
|
||
| // NewWebhookServer constructs a server; call Start to listen. | ||
| func NewWebhookServer(addr string, logger logr.Logger) *WebhookServer { |
There was a problem hiding this comment.
You also need to implement the Leader election function otherwise only the leader can run this. 🤔
| # When true, /metrics is served over HTTPS (and requires Kubernetes auth for scrapers). | ||
| # When false, /metrics is plain HTTP — matches default Prometheus ServiceMonitor scraping. | ||
| secure: false |
There was a problem hiding this comment.
Nope. Can't have false as default. Sorry.
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