Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow AWS Route53 routing policy change to/from simple to others. #3222

Merged
merged 5 commits into from
Jan 10, 2023
Merged

Allow AWS Route53 routing policy change to/from simple to others. #3222

merged 5 commits into from
Jan 10, 2023

Conversation

jessegonzalez
Copy link
Contributor

Signed-off-by: @jessegonzalez

Description

This PR allows Route53 record updates to change the routing policy to/from simple from/to other routing policies. These record modifications do not support UPSERT, but instead, require DELETE/CREATE to succeed.

The AWS provider function createUpdateChanges has been updated with another conditional to test if one of the old/new set identifiers is the empty string and the other is not.

The AWS Provider test TestAWSApplyChanges has been updated to validate the change, and validated before PR submission.

Original PR: #3159 did not include stashed changes.

Local test output 
❯ make test
go test -race -coverprofile=profile.cov ./...
?       sigs.k8s.io/external-dns        [no test files]
ok      sigs.k8s.io/external-dns/controller     5.706s  coverage: 75.9% of statements
?       sigs.k8s.io/external-dns/docs/scripts   [no test files]
ok      sigs.k8s.io/external-dns/endpoint       1.117s  coverage: 47.8% of statements
?       sigs.k8s.io/external-dns/internal/config        [no test files]
ok      sigs.k8s.io/external-dns/internal/testutils     1.045s  coverage: 31.7% of statements
ok      sigs.k8s.io/external-dns/pkg/apis/externaldns   4.749s  coverage: 98.9% of statements
ok      sigs.k8s.io/external-dns/pkg/apis/externaldns/validation        3.711s  coverage: 64.6% of statements
?       sigs.k8s.io/external-dns/pkg/tlsutils   [no test files]
ok      sigs.k8s.io/external-dns/plan   1.429s  coverage: 93.0% of statements
ok      sigs.k8s.io/external-dns/provider       2.278s  coverage: 87.7% of statements
ok      sigs.k8s.io/external-dns/provider/akamai        3.222s  coverage: 72.2% of statements
ok      sigs.k8s.io/external-dns/provider/alibabacloud  3.020s  coverage: 66.4% of statements
ok      sigs.k8s.io/external-dns/provider/aws   5.166s  coverage: 89.3% of statements
ok      sigs.k8s.io/external-dns/provider/awssd 6.545s  coverage: 77.0% of statements
ok      sigs.k8s.io/external-dns/provider/azure 4.419s  coverage: 77.2% of statements
ok      sigs.k8s.io/external-dns/provider/bluecat       4.703s  coverage: 71.5% of statements
ok      sigs.k8s.io/external-dns/provider/bluecat/gateway       1.946s  coverage: 31.2% of statements
ok      sigs.k8s.io/external-dns/provider/civo  6.563s  coverage: 85.1% of statements
ok      sigs.k8s.io/external-dns/provider/cloudflare    7.011s  coverage: 87.5% of statements
ok      sigs.k8s.io/external-dns/provider/coredns       3.852s  coverage: 50.2% of statements
ok      sigs.k8s.io/external-dns/provider/designate     5.009s  coverage: 73.6% of statements
ok      sigs.k8s.io/external-dns/provider/digitalocean  7.969s  coverage: 83.2% of statements
ok      sigs.k8s.io/external-dns/provider/dnsimple      8.855s  coverage: 76.6% of statements
ok      sigs.k8s.io/external-dns/provider/dyn   6.590s  coverage: 25.5% of statements
?       sigs.k8s.io/external-dns/provider/dyn/soap      [no test files]
ok      sigs.k8s.io/external-dns/provider/exoscale      6.727s  coverage: 70.5% of statements
ok      sigs.k8s.io/external-dns/provider/gandi 7.172s  coverage: 83.5% of statements
ok      sigs.k8s.io/external-dns/provider/godaddy       7.588s  coverage: 59.5% of statements
ok      sigs.k8s.io/external-dns/provider/google        7.178s  coverage: 80.1% of statements
ok      sigs.k8s.io/external-dns/provider/ibmcloud      7.675s  coverage: 75.0% of statements
ok      sigs.k8s.io/external-dns/provider/infoblox      6.701s  coverage: 75.3% of statements
ok      sigs.k8s.io/external-dns/provider/inmemory      6.430s  coverage: 82.6% of statements
ok      sigs.k8s.io/external-dns/provider/linode        6.966s  coverage: 84.3% of statements
ok      sigs.k8s.io/external-dns/provider/ns1   6.485s  coverage: 81.6% of statements
ok      sigs.k8s.io/external-dns/provider/oci   6.796s  coverage: 82.3% of statements
ok      sigs.k8s.io/external-dns/provider/ovh   10.195s coverage: 95.2% of statements
ok      sigs.k8s.io/external-dns/provider/pdns  6.118s  coverage: 63.2% of statements
ok      sigs.k8s.io/external-dns/provider/pihole        6.398s  coverage: 73.7% of statements
ok      sigs.k8s.io/external-dns/provider/plural        5.186s  coverage: 31.1% of statements
ok      sigs.k8s.io/external-dns/provider/rcode0        5.568s  coverage: 86.3% of statements
ok      sigs.k8s.io/external-dns/provider/rdns  4.673s  coverage: 46.8% of statements
ok      sigs.k8s.io/external-dns/provider/rfc2136       5.022s  coverage: 60.4% of statements
ok      sigs.k8s.io/external-dns/provider/safedns       4.806s  coverage: 87.2% of statements
ok      sigs.k8s.io/external-dns/provider/scaleway      4.541s  coverage: 70.3% of statements
ok      sigs.k8s.io/external-dns/provider/tencentcloud  3.930s  coverage: 84.3% of statements
?       sigs.k8s.io/external-dns/provider/tencentcloud/cloudapi [no test files]
ok      sigs.k8s.io/external-dns/provider/transip       4.064s  coverage: 40.7% of statements
ok      sigs.k8s.io/external-dns/provider/ultradns      3.857s  coverage: 81.9% of statements
ok      sigs.k8s.io/external-dns/provider/vinyldns      3.296s  coverage: 79.6% of statements
ok      sigs.k8s.io/external-dns/provider/vultr 3.905s  coverage: 77.1% of statements
ok      sigs.k8s.io/external-dns/registry       3.948s  coverage: 90.0% of statements
ok      sigs.k8s.io/external-dns/source 5.825s  coverage: 73.5% of statements

Fixes #3144

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2022
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 7, 2022
@jessegonzalez
Copy link
Contributor Author

Apologies @Raffo, and others, for the failed tests in the original PR. I overlooked committing a stashed update.

cc: @njuettner
cc: @seanmalloy
cc: @szuecs

@szuecs
Copy link
Contributor

szuecs commented Dec 7, 2022

@jessegonzalez it was my fault not running the tests. :)

@jessegonzalez-life360
Copy link

@jessegonzalez it was my fault not running the tests. :)

And now we have CI running on this build. 🚀

@nitrocode
Copy link
Contributor

cc: @szuecs @njuettner @seanmalloy (for visibility, please review when time permits)

@szuecs
Copy link
Contributor

szuecs commented Dec 19, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jessegonzalez, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2022
@szuecs
Copy link
Contributor

szuecs commented Dec 19, 2022

/assign @njuettner

@johngmyers
Copy link
Contributor

Possibly test changing the set identifier from one non-empty value to another non-empty value?

@szuecs
Copy link
Contributor

szuecs commented Jan 6, 2023

@johngmyers that's a good point, yes.
@jessegonzalez can you add test cases as mentioned?
I think it would be also good to add a test case with old and new have the same SetIdentifier to do nothing.

@jessegonzalez jessegonzalez force-pushed the fix-simple-to-from-other-routing-policy branch from cd20d2e to 569571a Compare January 10, 2023 04:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 10, 2023
@jessegonzalez
refactor(provider/aws): Refactor createUpdateChanges() with helper re…
6580182 
…quiresDeleteCreate() to see if change is UPSERT capable.
@jessegonzalez jessegonzalez force-pushed the fix-simple-to-from-other-routing-policy branch from 569571a to 6580182 Compare January 10, 2023 04:55
@jessegonzalez-life360
Copy link

jessegonzalez-life360 commented Jan 10, 2023 •
edited

@szuecs Things started to get a bit messy, so I decided to refactor func createUpdateChanges() to use another helper func requiresDeleteCreate().

I added test cases as requested.

edit: also updated provider/ovh/ovh.go to fix linting error.

szuecs
szuecs reviewed Jan 10, 2023
View reviewed changes
provider/ovh/ovh.go
"github.com/ovh/go-ovh/ovh"
log "github.com/sirupsen/logrus"
"golang.org/x/sync/errgroup"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this change of an unrelated file? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok linter error to fix is fine

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@szuecs
Copy link
Contributor

szuecs commented Jan 10, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9f599ec into kubernetes-sigs:master Jan 10, 2023
@johngmyers johngmyers mentioned this pull request Jun 7, 2023
Merged
2 tasks
@johngmyers johngmyers mentioned this pull request Sep 13, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szuecs szuecs szuecs left review comments

@jessegonzalez-life360 jessegonzalez-life360 jessegonzalez-life360 left review comments

@njuettner njuettner Awaiting requested review from njuettner

@seanmalloy seanmalloy Awaiting requested review from seanmalloy

Assignees

@szuecs szuecs

@njuettner njuettner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jessegonzalez @szuecs @jessegonzalez-life360 @nitrocode @k8s-ci-robot @johngmyers @njuettner