-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Proxy in Webhook client #88
Comments
@jan-mrm, thanks for raising this issue. It's a quick fix. The webhook uses a custom HTTP client, and proxy support is not included. I will enable this. |
@jan-mrm please test my change (you need Linux amd64 node), I don't have any proxy for test my change. image: paskalmaksim/aks-node-termination-handler:dev
imagePullPolicy: Always
env:
- name: "HTTPS_PROXY"
value: "http://someProxy:somePort"
args:
- "-webhook.url=https://myWebhook"
- "-webhook.template-file=/files/webhook.json"
- "-webhook.contentType=application/json"
- "-webhook.method=POST"
- "-webhook.timeout=30s" If test will be OK - I will release this change. |
@maksim-paskal thank you, I'm able to test it tomorrow and will report back |
@maksim-paskal works like a charm for me 🙂 appreciate that 👍 |
not sure if that change might be some sort of "breaking" for users since they do not expect the proxy to be used at the moment? just a thought, but it would be known in the release notes, so it should be good. |
@jan-mrm thanks for quick response, one question about your usage of proxy. For context, this tool can make two type of http request:
You need PROXY only for webhooks or also for Azure Metadata Service? |
I'd only need it for the Webhook request. In my case the Azure Metadata Service would not need to go through the proxy (I would even put it into the NO_PROXY if the proxy would be used for that request) |
Ok, I will use proxy settings (default NO_PROXY) only for Webhooks requests. You are right, I think this change will not break any user experience. I make some changes, to enable proxy only for webhooks. Can you test it again? You need to restart your pods kubectl -n kube-system delete pods -lapp=aks-node-termination-handler |
still works for me. But one additional hint: I do have to set an env var NO_PROXY (which is ok, just an fyi) since the proxy is also used for cluster requests {"error":"error in startReadingEvents: error in getting azure resource name: error in Clientset.CoreV1().Nodes().Get: Get "https://CLUSTERIP:443/api/v1/nodes/MYNODE\": AuthorizedOnly","file":"github.com/maksim-paskal/aks-node-termination-handler/cmd/main.go:86","func":"main.main","level":"fatal","msg":"","time":"SOMETIME"} |
but having to set a NO_PROXY env var was already the case in the current release, its not newly introduced by the Webhook Proxy change 🙂 |
@jan-mrm Yeap, thanks. Fixed - please try: image: paskalmaksim/aks-node-termination-handler:dev
imagePullPolicy: Always
args:
- "-webhook.url=https://myWebhook"
- "-webhook.template-file=/files/webhook.json"
- "-webhook.contentType=application/json"
- "-webhook.method=POST"
- "-webhook.timeout=30s"
- "-webhook.http-proxy=http://someProxy:1234" I add new flag |
@maksim-paskal lgtm 👍 |
@maksim-paskal thank you for implementing it so quickly 🙂👍 |
Hey, I was wondering if the webhook client should already honor the
HTTPS_PROXY
environment variable or not?I set
and it does seem to be used, since if I do not set the
NO_PROXY
correctly the pod cannot start.But the webhook requests seem to not go to the configured proxy.
Can a proxy be configured for the webhook client already, if so how?
Else this would be a feature request if possible :)
The text was updated successfully, but these errors were encountered: