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

docs(proposal): annotations support graduation to beta #5080

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ivankatliarchuk
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk commented Feb 9, 2025

Description

No issues to date. But there is definitely an ask and appetite to improve annotation processing. It may take a while but worth to have a plan in place, so community would be able to help to improve annotation processing.

Currently 10 open pull requests 10 open https://github.com/kubernetes-sigs/external-dns/pulls?q=is%3Apr+is%3Aopen+annotation

And 40+ open https://github.com/kubernetes-sigs/external-dns/issues?q=is%3Aissue%20state%3Aopen%20annotations

Mane goals

  • worth to have/initiate at least a discussion
  • standard interface and we could have some automation to generate docs from code
  • easy to add/deprecate/graduate an annotation or group of annotations

The proposal to merge to master 2025-Mar-09 with the decision

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign raffo for approval. For more information see the 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 Feb 9, 2025
@ivankatliarchuk
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 9, 2025
Copy link
Contributor

@szuecs szuecs left a comment

Choose a reason for hiding this comment

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

Thanks for the effort I think this can enhance safety and usability of external-dns!

I think there are some things clearly missing:

  1. How do we make sure that annotation graduation has a level of objective quality increase?
  2. How do we handle this with providers that are out of tree, which is one of our top priorities?

As long we have no good answers to these questions we should not invest time into an implementation.


- **Deprecation Policy and Migration Path**: Establish a clear deprecation policy for outdated annotations. Implement mechanisms to log warnings when deprecated annotations are used and provide comprehensive migration guides to assist users in transitioning to supported annotations.

- **Conflict Detection and Resolution**: Enhance the annotation processing logic to detect conflicting annotations proactively. Implement validation rules that either prevent conflicts at the time of deployment or resolve them in a predictable manner, ensuring consistent behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you suggest to implement a validation webhook or something else?


- No automated documentation for available annotations.
- Unclear strategy for supporting different API versions.
- No defined transition path from `external-dns.alpha.kubernetes.io` annotations to a stable format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, maybe we should define alpha/beta/stable groups.

- No defined transition path from `external-dns.alpha.kubernetes.io` annotations to a stable format.
- Lack of standardization among annotations.

By adopting a structured approach similar to `ingress-nginx`, we can address these issues and improve the overall functionality and user experience of `external-dns`.
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 link or tell more about their approach? I did not find anything browsing their GH repo

Copy link
Contributor

Choose a reason for hiding this comment

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


- Introduce automated documentation for supported annotations.
- Define a strategy for handling multiple API versions in annotations.
- Ensure backward compatibility where possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Backward compatibility is the most important IMO, so I would drop the "where possible".
I think a minor release version increase and a deprecation note for a lifecycle change.
I would suggest something like: alpha -> alpha+beta -> beta -> beta + stable -> stable and each "->" is one minor version increase. So I would suggest that a controller update will have both working "alpha+beta", but log warning for "alpha" annotations in the group of "alpha+beta" graduation step. After all annotations a migrated to "beta" in a cluster that are read by external-dns instance there is no warning log and the next controller update to "beta" is fine.


### Non-Goals

- Establish a migration plan from `external-dns.alpha.kubernetes.io` to `external-dns.beta.kubernetes.io`.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should have a clear idea how to execute the change, so I would like to have it also as goal.

- _As a maintainer_, I want to define and communicate a clear lifecycle for annotation versions, so that contributors and users understand when alpha annotations will be deprecated and how to migrate.
- _As a maintainer_, I want to ensure that annotation behavior is consistent across supported DNS providers (e.g., AWS Route 53, Cloudflare, oogle DNS), so that users do not experience unexpected inconsistencies depending on their provider.
- _As a maintainer_, I want to establish validation rules that reject conflicting or redundant annotations at runtime, so that users do not face unpredictable behavior due to overlapping DNS rules.
- _As a maintainer_, I want to collaborate with other Kubernetes SIGs (e.g., SIG-Network, SIG-Auth) to align annotation standards,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We are a project of sig-network so "other Kubernetes SIGs" is not sig-network.
  2. Where do you see collaboration needs? Is there any trial to standardize annotations?


### Behavior

- `external-dns` should recognize both `alpha` and `beta` annotations where applicable.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it exactly mean?


- `external-dns` should recognize both `alpha` and `beta` annotations where applicable.
- Warnings should be logged when deprecated annotations are used.
- Warnings should be logged when annotation is not supported by source or provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the provider is out of tree we don't know their deployed version nor if an annotation is supported or exists.
How do you make sure it works?

- `external-dns` should recognize both `alpha` and `beta` annotations where applicable.
- Warnings should be logged when deprecated annotations are used.
- Warnings should be logged when annotation is not supported by source or provider.
- Future major versions should drop support for `alpha` annotations after a defined period.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to use minor versions instead of major versions. In Go major versions are breaking changes for exposed functions/types/... and if we have 50 annotations and increase in small groups we will soon have v50, which IMO makes no sense.

### Alternative 2: Keep Annotations in Alpha Permanently

- Pros: No migration burden for users.
- Cons: Lack of stability signals to users, discouraging adoption.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how we can really make sure that alpha is better than beta. What is the signal of quality to use to make this change?

@ivankatliarchuk
Copy link
Contributor Author

Not abandonned. Still working out things

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants