Add retries on webhook failure

[m4rii0]

Hi @maksim-paskal! 👋

We noticed an edge case where webhooks might fail to deliver correctly due to temporary connectivity issues with the node.

This PR adds support for retries when sending webhooks in case of errors, such as 5xx responses or connectivity problems.

Go isn't my primary language, so I’d appreciate any suggestions or improvements you might have!

Cheers,
Mario

[maksim-paskal]

@m4rii0 Thank for contributing.

What do you think about that we use here some popular library for doing retryable calls for example https://github.com/hashicorp/go-retryablehttp ?

This library is widely used for the problem that we try solve here

[m4rii0]

@m4rii0 Thank for contributing.

What do you think about that we use here some popular library for doing retryable calls for example hashicorp/go-retryablehttp ?

This library is widely used for the problem that we try solve here

Absolutelly, thanks for the suggestion!

Don't hesitate to suggest any other changes

[maksim-paskal]

Overall test coverage after this changes will be lower, I see that after switching to retryablehttp framework this block

aks-node-termination-handler/pkg/webhook/webhook.go

Lines 88 to 90 in 53c5037

	if resp.StatusCode != http.StatusOK {
	return errors.Wrap(errHTTPNotOK, fmt.Sprintf("StatusCode=%d", resp.StatusCode))
	}

is not unused. Seems that this http client throws errors before. Investigating.

[maksim-paskal]

@m4rii0 Add some test to your fork for retry logic on HTTP 500 and also check that on HTTP 400 will not be retry.

Also notice that go-retryablehttp will not retry for HTTP 501 - Is it ok for your case?

if an error is returned by the client (connection errors, etc.), or if a 500-range response code is received (except 501)

[m4rii0]

@maksim-paskal, thanks for the tests!

In our case, the issue was specifically connection refused errors, which align perfectly with the first category of errors that the library checks.

Just a quick off-topic note: I noticed that the tool currently sends an error code even when a 2xx response is received. For example, our push gateway returns a 201 Created, but the tool is showing this as an error. Ideally, it should treat all 2xx responses as successful. What do you think about it?

Here’s the related code snippet for reference:

aks-node-termination-handler/pkg/webhook/webhook.go

Lines 88 to 90 in 53c5037

	if resp.StatusCode != http.StatusOK {
	return errors.Wrap(errHTTPNotOK, fmt.Sprintf("StatusCode=%d", resp.StatusCode))
	}

Thanks again for your fast response and cooperation.

[maksim-paskal]

@m4rii0 Thanks for notice - create another issue #94 - will fix in another PR