-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix deletion of DNS Records for VirtualServices when an Error occures #3140
Fix deletion of DNS Records for VirtualServices when an Error occures #3140
Conversation
Welcome @ricoberger! |
source/istio_virtualservice.go
Outdated
gateway := sc.getGateway(ctx, gateway, virtualService) | ||
gateway, err := sc.getGateway(ctx, gateway, virtualService) | ||
if err != nil { | ||
return targets, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be: nil, err ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this I used return targets, err
, because in L303 it also returns the targets and the error
external-dns/source/istio_virtualservice.go
Line 303 in 796ebbf
tgs, err := sc.targetsFromGateway(gateway) |
What do you think, should I change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rule of thumb is to return nil in case you return an err.
So if there's no reason for having this in particular I would say please change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted it, to just return the error.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njuettner, ricoberger, 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:
Approvers can indicate their approval by writing |
/lgtm |
Description
This PR fixes a bug where external-dns deletes all DNS records when they are created via VirtualServices and the Kubernetes API returns an error (#2858).
Before this change we saw the following in the external-dns logs:
With the proposed change the error will be returned from the function where the error occurs, so that the DNS records are not deleted.
The logs with this change are looking as follows:
We tested this on our clusters for the following cases:
Fixes #2858
Checklist