Adding support for DNS based injection URLs in webhook probe check - #2042
Adding support for DNS based injection URLs in webhook probe check#2042Dimss wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2042 +/- ##
===========================================
+ Coverage 53.30% 83.41% +30.11%
===========================================
Files 59 59
Lines 3236 2901 -335
===========================================
+ Hits 1725 2420 +695
+ Misses 1408 481 -927
+ Partials 103 0 -103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dgn
left a comment
dgn
left a comment
There was a problem hiding this comment.
Thanks for working on this. I don't think we should make assumptions about the URL though (ie that the /ready endpoint is available - it often isn't especially when the webhook is behind a gateway). I think a simpler way would be to just send a GET request to the actual /inject endpoint. If it returns a 4xx error, we're good. If there's a network error, we're not. It's not perfect, but it does at least give us network availability
| case config.URL != nil: | ||
| return "", errors.New("only webhooks pointing to a Service are supported") | ||
| if u, err := url.Parse(*config.URL); err == nil && u.Scheme != "" && u.Host != "" { | ||
| return fmt.Sprintf("%s://%s/ready", u.Scheme, u.Host), nil |
There was a problem hiding this comment.
a few issues I see with this approach:
- we're dropping the path entirely. if the URL is
mydomain.custom/istio/injectwe're rewriting tomydomain.custom/readywhich does not seem correct - we assume blindly that
/readyis actually exposed. that might not be the case though. especially when the/injectendpoint is behind a gateway
There was a problem hiding this comment.
Indeed, but I do not think we can get it somewhere else. In External-Remote setup (this is where we mainly need DNS based mutation webhook URL) The config.URL will be something like this https://DNS-HOST:15017/inject/cluster/cluster2/net/network1, and it will be always placed behind a Gateway. We can't use /inject/cluster/cluster2/net/network1 for the health check obviously, and we've no other place where user can define the health check path for the external mutation webhook.
In addition to that:
- The probing we do in
webhook_controller.godoes not exists in upstream - The sail-operator healthy probe is useful only on the first (create) reconcile, i.e if probe is failing sail-operator won't continue reconciliation loop (no istiod or other related resources will be created). But, if the external webhook become broken after at least one successful reconcile, sail-operator will do nothing with it.
84ca89e to
48520d5
Compare
Fixes istio-ecosystem#1934 Signed-off-by: Dmitry Kartsev <dkartsev@redhat.com>
48520d5 to
1074ee6
Compare
|
/retest |
dgn
left a comment
dgn
left a comment
There was a problem hiding this comment.
tests and lint still failing...
Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
|
@Dimss: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What this PR does / why we need it:
DNS-based injection URLs (for example, allocated by AWS LB) will now work correctly in the probe check.
Which issue(s) this PR fixes:
Fixes #1934
Related Issue/PR #
#1091