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.

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

[TXT Registry] Disable generation of old style txt records when encryption is enabled #3735

Closed

Conversation

Sewci0
Copy link
Contributor

@Sewci0 Sewci0 commented Jun 26, 2023

As mentioned in the issue, current behaviour is unable to work with AWS which requires the Delete change request to be called with the exact same record set that is currently present in Route53. This is currently not happening due to:

  1. generateTXTRecord generates both the old and new format txt records. Each record ends up being encrypted with new random nonce.
  2. Records() will only store one these in the labelsMap when iterating through the TXT labels.
  3. Call to generateTXTRecord within Records() will delete the nonce from the labels.
  4. Final call to generateTXTRecord from within ApplyChanges will have to regenerate the nonce once again, ending up with an entirely different target.

This change disables the generation of the old style txt records and old to new migrations when encryption is enabled.

Fixes #3668
Depends on #3724

Checklist

  • Unit tests updated

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Sewci0
Once this PR has been reviewed and has the lgtm label, please assign raffo for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 2023
@Sewci0 Sewci0 force-pushed the fix/txt-encrypt-aws-deletion branch from 143f5fc to 73f576b Compare June 26, 2023 23:07
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@Sewci0
Copy link
Contributor Author

Sewci0 commented Aug 31, 2023

Superseded by: #3901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When enable encryption of TXT record, Deletion of the DNS record failed.
2 participants