feat(WebhookProvider): Let WebhookProvider return `SoftError` on response status codes >= 500 #4319

SimonKienzler · 2024-03-14T16:39:50Z

Description

#4166 introduced the SoftError as a way for providers to signal that an error is transient and should not lead to the controller bailing out of execution, to somewhat mitigate the changes made in #3009.

For some in-tree providers (AWS, Azure, and Oracle Cloud Infrastructure), SoftError support has already been implemented. This PR lets the WebhookProvider return SoftErrors for any HTTP Response with a status code >= 500 returned by webhook providers, in order to implement this for out-of-tree providers as well.

As 5xx status codes indicate a temporary problem on the provider side, it seems fitting to me that any response with such a code should be handled as a SoftError. This is in line with the definition of the SoftError, which "is meant for error propagation from providers to tell that this is a transient error." It is also in line with the webhook provider tutorial, which states that

only 5xx responses will be retried and only 20x will be considered as successful. All status codes different from those will be considered a failure on ExternalDNS's side.

This is my first PR here, happy about any feedback regarding the overall contribution process as well 😊

Checklist

Unit tests updated
End user documentation updated
- I wasn't sure if any user documentation needed updates, please advise if and where I should add something.

linux-foundation-easycla · 2024-03-14T16:39:54Z

The committers listed above are authorized under a signed CLA.

✅ login: SimonKienzler / name: Simon Kienzler (e8e5d5c, a88cae2)

k8s-ci-robot · 2024-03-14T16:39:59Z

Welcome @SimonKienzler!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-03-14T16:39:59Z

Hi @SimonKienzler. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

mloiseleur · 2024-03-18T08:10:39Z

/ok-to-test

mloiseleur · 2024-03-19T07:40:54Z

provider/webhook/webhook.go

@@ -224,7 +229,11 @@ func (p WebhookProvider) ApplyChanges(ctx context.Context, changes *plan.Changes
 	if resp.StatusCode != http.StatusNoContent {
 		applyChangesErrorsGauge.Inc()
 		log.Debugf("Failed to apply changes with code %d", resp.StatusCode)
-		return fmt.Errorf("failed to apply changes with code %d", resp.StatusCode)
+		err := fmt.Errorf("failed to apply changes with code %d", resp.StatusCode)
+		if resp.StatusCode >= http.StatusInternalServerError {


Suggested change

if resp.StatusCode >= http.StatusInternalServerError {

if resp.StatusCode >= http.StatusInternalServerError && resp.StatusCode <= http.StatusNotExtended {

I like the suggestion. IMO, the check should be consistent across all three interface functions, therefore I extracted it into a small function for convenience. WDYT?

Sounds good to me. Thanks.

mloiseleur · 2024-03-19T07:41:57Z

The PR looks technically good. I left a small suggestion.
I'm unsure if we should do it for all changes.
/lgtm
/assign @szuecs for final review

mloiseleur · 2024-03-22T13:59:04Z

/lgtm

szuecs · 2024-04-08T17:37:34Z

provider/webhook/webhook.go

 	}

 	if err := json.NewDecoder(resp.Body).Decode(&endpoints); err != nil {
-		recordsErrorsGauge.Inc()
+		adjustEndpointsErrorsGauge.Inc()


this seems to be an unrelated fix, maybe better to create a separate PR

@SimonKienzler ^

I thought it might pass as boyscouting, will open a separate PR for it.

Reverted the change and will open a separate PR for the fix soon. PTAL

szuecs · 2024-04-09T17:33:42Z

/lgtm
/approve

k8s-ci-robot · 2024-04-09T17:33:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [szuecs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 14, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 14, 2024

k8s-ci-robot requested review from mloiseleur and szuecs March 14, 2024 16:40

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 14, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 18, 2024

busilina mentioned this pull request Mar 18, 2024

Do not return a 500 error when dns zone does not exist. stackitcloud/external-dns-stackit-webhook#16

Closed

2 tasks

mloiseleur reviewed Mar 19, 2024

View reviewed changes

k8s-ci-robot assigned mloiseleur Mar 19, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 19, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2024

szuecs reviewed Apr 8, 2024

View reviewed changes

SimonKienzler added 2 commits April 9, 2024 13:51

Let the WebhookProvider return SoftError on status codes >= 500

e8e5d5c

Extract check for retryable error into function

a88cae2

SimonKienzler force-pushed the add-softerrors-to-webhook-provider branch from 5343663 to a88cae2 Compare April 9, 2024 11:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2024

SimonKienzler mentioned this pull request Apr 9, 2024

Webhook provider: Use correct error gauge in AdjustEndpoints() func #4374

Merged

k8s-ci-robot assigned szuecs Apr 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024

k8s-ci-robot merged commit 6800ee5 into kubernetes-sigs:master Apr 9, 2024
13 checks passed

SimonKienzler deleted the add-softerrors-to-webhook-provider branch April 11, 2024 07:23

SimonKienzler mentioned this pull request Apr 11, 2024

fix: provide possibility to have a soft error mode #4166

Merged

SimonKienzler mentioned this pull request Jul 4, 2024

Unable to specify SoftError for webhook providers #4479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(WebhookProvider): Let WebhookProvider return `SoftError` on response status codes >= 500 #4319

feat(WebhookProvider): Let WebhookProvider return `SoftError` on response status codes >= 500 #4319

SimonKienzler commented Mar 14, 2024

linux-foundation-easycla bot commented Mar 14, 2024 •

edited

Loading

k8s-ci-robot commented Mar 14, 2024

k8s-ci-robot commented Mar 14, 2024

mloiseleur commented Mar 18, 2024

mloiseleur Mar 19, 2024

SimonKienzler Mar 22, 2024

mloiseleur Mar 22, 2024

mloiseleur commented Mar 19, 2024

mloiseleur commented Mar 22, 2024

szuecs Apr 8, 2024

szuecs Apr 9, 2024

SimonKienzler Apr 9, 2024

SimonKienzler Apr 9, 2024

SimonKienzler Apr 9, 2024

szuecs commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

	if resp.StatusCode >= http.StatusInternalServerError {
	if resp.StatusCode >= http.StatusInternalServerError && resp.StatusCode <= http.StatusNotExtended {

feat(WebhookProvider): Let WebhookProvider return SoftError on response status codes >= 500 #4319

feat(WebhookProvider): Let WebhookProvider return SoftError on response status codes >= 500 #4319

Conversation

SimonKienzler commented Mar 14, 2024

linux-foundation-easycla bot commented Mar 14, 2024 • edited Loading

k8s-ci-robot commented Mar 14, 2024

k8s-ci-robot commented Mar 14, 2024

mloiseleur commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloiseleur commented Mar 19, 2024

mloiseleur commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

feat(WebhookProvider): Let WebhookProvider return `SoftError` on response status codes >= 500 #4319

feat(WebhookProvider): Let WebhookProvider return `SoftError` on response status codes >= 500 #4319

linux-foundation-easycla bot commented Mar 14, 2024 •

edited

Loading