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

Register annotations for Azure integration #45718

Open
sftim opened this issue Mar 28, 2024 · 22 comments
Open

Register annotations for Azure integration #45718

sftim opened this issue Mar 28, 2024 · 22 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sftim
Copy link
Contributor

sftim commented Mar 28, 2024

This is a Feature Request

What would you like to be added
Update Well-Known Labels, Annotations and Taints to include each of the annotations listed in https://cloud-provider-azure.sigs.k8s.io/topics/loadbalancer/#loadbalancer-annotations

Why is this needed
We should register (and document) all of our official annotations, even the beta ones.

Comments
/sig cloud-provider
/language en

/lifecycle frozen
/priority backlog
it's required, but not urgent

@sftim sftim added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 28, 2024
@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. language/en Issues or PRs related to English language priority/backlog Higher priority than priority/awaiting-more-evidence. labels Mar 28, 2024
@sftim
Copy link
Contributor Author

sftim commented Mar 28, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 28, 2024
@Ritikaa96
Copy link
Contributor

A full list of all annotations are listed here
There are about 30 annotations to be recorded and registered.

@Ritikaa96
Copy link
Contributor

I would like to try adding these annotations
/assign

@Ritikaa96
Copy link
Contributor

Hi @sftim , just wanted to confirm is we have to create single PR for all 30 Annotations or we can break them in several PRs?

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

just wanted to confirm is we have to create single PR for all 30 Annotations or we can break them in several PRs?

Either way is fine!

@Ritikaa96
Copy link
Contributor

Ritikaa96 commented Jun 20, 2024

great I'll try to break the list into 2-3 PRs so that not many changes occur in one and previous review help the latest PR as well. Thanks @sftim

@tengqm
Copy link
Contributor

tengqm commented Jun 20, 2024

For annotations that are not so qualified as "well-known" ones, there are two options in my mind:

  • Avoid register these annotations: No matter how many annotations there are on Azure,
    they are 3rd-party details. We will be violating our own rules for these dual-hosted
    contents.

  • Put them into a separate reference page, with a "3rd-party content" notice.
    If some Kubernetes core components/resources cannot work properly when running on Azure
    without these annotations, we can treat them as some information about "add-ons".
    We can point users to Azure documentation page, just as we did for docs on CNI, CSI.

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

These are official annotations. Switching the way we document annotations should be a separate issue from this one, which is about tracking the official Azure annotations.

@tengqm they are not dual-hosted; the code that uses them is part of Kubernetes.

@tengqm
Copy link
Contributor

tengqm commented Jun 20, 2024

These are official annotations. Switching the way we document annotations should be a separate issue from this one, which is about tracking the official Azure annotations.

They are official to Azure doesn't mean they are official to Kubernetes, right?
These annotations are already documented at https://cloud-provider-azure.sigs.k8s.io/topics/loadbalancer/#loadbalancer-annotations,
along with many other Azure-specific contents, what we proposing here is to copy them
over to k8s website?

@tengqm they are not dual-hosted; the code that uses them is part of Kubernetes.

Well ... it depends on how we define "dual-hosting". The existing annotations are well
documented at the cloud-provider-azure website. I won't vote for copying them over and I
see that as "dual-hosting" (my version of course).

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

We can hyperlink to https://cloud-provider-azure.sigs.k8s.io/topics/loadbalancer/#loadbalancer-annotations for details.

However, we should still register these annotations per the policy in https://kubernetes.io/docs/reference/labels-annotations-taints/

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

They are official to Azure doesn't mean they are official to Kubernetes, right?

No; they are official to Kubernetes because (for legacy reasons) they use a Kubernetes namespace. If we had a time machine we could go back and ask the Azure folks to use a different domain name in the annotation keys.

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

(We can still encourage the Azure provider folks to use a Microsoft domain name for the post-beta annotation and label keys, but that's again a separate issue to this one).

@Ritikaa96
Copy link
Contributor

If there is a second thought about registering these annotations or the related format, I'd like to know about it so that I can raise a PR that is still required.

@tengqm
Copy link
Contributor

tengqm commented Jun 20, 2024

We can hyperlink to https://cloud-provider-azure.sigs.k8s.io/topics/loadbalancer/#loadbalancer-annotations for details.

However, we should still register these annotations per the policy in https://kubernetes.io/docs/reference/labels-annotations-taints/

Just checked the labels-annotations-taints/_index.md file, we already have AWS specific annotations there. Most of them are about LB Services. That file is now 2385 lines of markdown.
Even if we want to add some pointers to the Azure-specific annotations, I'd still suggest
to put them into a new file under labels-annotations-taints/. We may want to move AWS-specific, NPD-specific labels/annotations out of the _index.md file as well.

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

Eventually, I'd quite like [someone] to make it more like a searchable file where you type in some text and see the matching annotations. We have so many of them now that one file isn't obviously the right way to organise it all.
I think @thockin had talked about moving the canonical registry into its own repo - perhaps with generator code. We haven't found the round tuits to make this happen.

But that's for another time.

@tengqm
Copy link
Contributor

tengqm commented Jun 20, 2024

They are official to Azure doesn't mean they are official to Kubernetes, right?

No; they are official to Kubernetes because (for legacy reasons) they use a Kubernetes namespace. If we had a time machine we could go back and ask the Azure folks to use a different domain name in the annotation keys.

We cannot change history because we are not the Big Brother. The "kubernetes.io" namespace thing is not a rule strictly followed by cloud providers. For example, take a look at these:

These annotations serve the similar purposes, but they may and may not use "kubernetes.io" domain name. Given that we have so many cloud providers out there, I'm not inclined to document things for one or two providers while ignoring others.

@sftim
Copy link
Contributor Author

sftim commented Jun 20, 2024

I'm not inclined to document things for one or two providers while ignoring others

We should document all the in-project annotations; that would include the integration with Huawei. For the OpenStack integration, they have done the right thing so we don't need to register official annotations.

This issue is about Azure, right?

@thockin
Copy link
Member

thockin commented Jun 20, 2024

Annotations which are specific to one cloud-provider should use provider specific names. It boggles my mind that so many well-intentioned people get this wrong, but it IS somewhat messy because it is azure code that lives in a k8s org.

We cannot rip those out, probably.

Providers SHOULD be chided and SHOULD add better names that trigger the same meaning.

As for documentation, I agree we should document them and make it clear that they are provider-specific. If anyone wants to help get the "names DB" off the ground, let me know. It's not HARD, it's just tedious.

@Ritikaa96
Copy link
Contributor

Even if we want to add some pointers to the Azure-specific annotations, I'd still suggest to put them into a new file under labels-annotations-taints/. We may want to move AWS-specific, NPD-specific labels/annotations out of the _index.md file as well.

I agree , from reader's perspective , having a different file will serve us good and will also help organising these annotations in discussion as they are not few but 30 in number.

@Ritikaa96
Copy link
Contributor

Eventually, I'd quite like [someone] to make it more like a searchable file where you type in some text and see the matching annotations. We have so many of them now that one file isn't obviously the right way to organise it all. I think @thockin had talked about moving the canonical registry into its own repo - perhaps with generator code. We haven't found the round tuits to make this happen.

But that's for another time.

Do let me know about the new required changes and where to register the annotations now. I'll continue the work then.

@Ritikaa96
Copy link
Contributor

Hi Any update on this discussion? Just asking so we can start working on this one.

Eventually, I'd quite like [someone] to make it more like a searchable file where you type in some text and see the matching annotations. We have so many of them now that one file isn't obviously the right way to organise it all. I think @thockin had talked about moving the canonical registry into its own repo - perhaps with generator code. We haven't found the round tuits to make this happen.

But that's for another time.

@Ritikaa96
Copy link
Contributor

Just dropping by to see if we have decided the action on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. language/en Issues or PRs related to English language lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants