AWS: change the order of the actions, DELETE before CREATE fixes #1411

[OmerKahani]

When adding an identifier to a record without an identifier, there should be a DELETE and then CREATE. Currently, external-dns will send the CREATE before the DELETE, that will fail the batch job with error:
"SetIdentifier hello-world cannot be created as a non-weighted set exists with the same name and type"

This PR solves this validation problem by changing the sort order so DELETE will be before CREATE. AWS doc states that the action will be happening in a single operation, so there shouldn't be a problem that only the DELETE was executed.
https://docs.aws.amazon.com/Route53/latest/APIReference/API_ChangeResourceRecordSets.html#API_ChangeResourceRecordSets_RequestSyntax

fixes #1411

[k8s-ci-robot]

Welcome @OmerKahani!

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. 😃

[OmerKahani]

/assign @hjacobs

[OmerKahani]

/assign @Raffo

[Raffo]

The changes look good, but given the change in semantic, I'd love to get some more feedback from people using ExternalDNS on AWS extensively.
I'm thinking of asking @njuettner @linki @szuecs for example, feel free to reply here if you have feedback to provide.

Can we have as well a sequence of steps to replicate #1411 @OmerKahani ?

[szuecs]

Reading the docs, looks good to me, but one question I have is if the other cases that use sortChangesByActionNameType() are also fine with the change.

[OmerKahani]

The changes look good, but given the change in semantic, I'd love to get some more feedback from people using ExternalDNS on AWS extensively.
I'm thinking of asking @njuettner @linki @szuecs for example, feel free to reply here if you have feedback to provide.

Can we have as well a sequence of steps to replicate #1411 @OmerKahani ?

The easiest way to replicate it is to create an ingress without the external-dns.alpha.kubernetes.io/set-identifier annotation, and then try to add the annotation

[OmerKahani]

Reading the docs, looks good to me, but one question I have is if the other cases that use sortChangesByActionNameType() are also fine with the change.

this is the only place:
https://github.com/kubernetes-sigs/external-dns/search?q=sortChangesByActionNameType&unscoped_q=sortChangesByActionNameType

[szuecs]

Then sortChangesByActionNameType() was unused and we should have a linter check.
@njuettner @Raffo do you know if you have a linter enabled to detect dead code?

[szuecs]

I checked it and no we have not.
https://github.com/kubernetes-sigs/external-dns/blob/master/.golangci.yml#L15
We should stop using megacheck, because it’s deprecated and use staticcheck, which will become the default linter in go, as far as I know.

[OmerKahani]

Then sortChangesByActionNameType() was unused and we should have a linter check.
@njuettner @Raffo do you know if you have a linter enabled to detect dead code?

Sorry, I meant it's not been used outside this function, it is been used in this function. if cs is bigger then batch size, the function will split cs to batches, and for every batch will run sortChangesByActionNameType().

[szuecs]

I see and checked the code in the changed function. LGTM

[diogonicoleti]

@linki @Raffo we use external-aws on AWS extensively and we're having this issue too.

For me it looks good. What we need to do to have this PR merged and released?If I could help with something feel free to ask.

[Raffo]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: OmerKahani, Raffo

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 [Raffo]

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

[diogonicoleti]

@Raffo thanks for merging this one! Do you have some idea when a new release with this fix will be released? We're waiting for it to fix some issues we're having in a production service.