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! :)
| 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.
|
@pepordev With the config thing out of the way, the rest looks okay expect for the default thing in my last comment. |
|
And the otherone with the deadlock. :_ |
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