Infoblox provider: fix for target value duplication #2755
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
skudriavtsev
force-pushed
the
bug-duplicate-targets
branch
from
63253a0
to
2408810
Compare
k8s-ci-robot
added
size/L
There was a problem hiding this comment.
With the caveat that I'm not a "proper" maintainer here as my primary focus is the Helm chart, this PR looks good from a coding perspective and I could understand what the code was doing. I'm also not particularly familiar with Infoblox (worst reviewer ever) so I don't have much to add on the implementation other than my question about using an un-released version.
go.mod
Outdated
| @@ -27,7 +27,7 @@ require ( | |||
| github.com/google/go-cmp v0.5.7 | |||
| github.com/gophercloud/gophercloud v0.22.0 | |||
| github.com/hooklift/gowsdl v0.5.0 | |||
| github.com/infobloxopen/infoblox-go-client/v2 v2.1.1 | |||
| github.com/infobloxopen/infoblox-go-client/v2 v2.1.2-0.20220401192150-b6079a2c974a | |||
There was a problem hiding this comment.
This doesn't look like an official release, is there some context behind this?
There was a problem hiding this comment.
There was no official release of go-client so far, after v2.1.1, thus I had to use this intermediate version, which includes the new feature which will be needed in the nearest future. Current full code is here: https://github.com/skudriavtsev/external-dns-1/commits/client-cert-auth This PR contains just a portion to fix particular issue.
Thanks for the comment. Anyway, the version v2.1.2-0.20220401192150-b6079a2c974a is not right, it contains a typo. I have bumped it up already.
…rds when there is not changes in K8s
skudriavtsev
force-pushed
the
bug-duplicate-targets
branch
from
48ecbd5
to
1423e04
Compare
|
@seanmalloy cc @Raffo Can you please review this PR ? |
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Raffo, skudriavtsev 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 |
k8s-ci-robot
added
the
approved
|
The bug that this PR introduced was catastrophic. What changes to testing and review procedure could have caught it before it got merged? |
Description
An extra target (with the same value) was added after retrieving actual state of A-records before sending updates to NIOS server, thus the next time a change necessity is detected, despite there was no real changes to endpoints.
Fixes #ISSUE
Checklist