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

feat: provider generic annotations #3959

Closed
wants to merge 1 commit into from
Closed

feat: provider generic annotations #3959

wants to merge 1 commit into from

Conversation

farodin91
Copy link

Description
With the new webhook functionality, it is possible to add provider specific annotations for webhook provider at moment.

This solves it in a generic way without changing anything.

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 Sep 26, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @farodin91. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 26, 2023
@farodin91
Copy link
Author

@Raffo I tried to fix provider specific annotations in generic way as possible.

@johngmyers
Copy link
Contributor

I'm not sure "without changing anything" should be a design goal.

I think we're going to need to have providers filter such annotations. AdjustEndpoints could do it, but if the provider takes no explicit action then any such annotation will cause the endpoint to be handed to ApplyChanges on every sync.

The vast majority of providers don't have any provider-specific annotations and these should filter them all out.
We could make BaseProvider.AdjustEndpoints() filter out all provider-specific annotations, perhaps.

@farodin91 farodin91 force-pushed the provider-generic-annotations branch from 30bab39 to 60fc787 Compare September 29, 2023 06:59
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 29, 2023
@farodin91 farodin91 force-pushed the provider-generic-annotations branch from 60fc787 to e4deea9 Compare September 29, 2023 07:01
@farodin91
Copy link
Author

@johngmyers I added a filter out for generic annotation without filtering out old ProviderSpecific stuff.

@johngmyers
Copy link
Contributor

johngmyers commented Sep 30, 2023 •
edited
Loading

I hadn't planned on tackling the problem of provider-specific annotations quite yet, but since someone's got the desire to work on them right now I should write down my thoughts:

  • Knowledge of what the provider-specific annotations are should not be in core (as it currently is) but in the respective providers.
    • This will require extending the provider API to permit the provider to map annotations to provider-specific properties.
    • Knowledge of the existing provider-specific annotations will need to be pushed into their respective providers. A corollary of this is that the base provider shouldn't treat them specially.
    • Some existing provider-specific annotations might be core. set-identifier is because it has to be handled in core. alias might make sense for some other non-AWS provider
    • So that the webhook doesn't have to be called for each resource, this might be a call-upon-startup method returning a map from annotation key to provider-specific property key.
  • We will want to introduce stable annotations, not under the alpha.kubernetes.io. (I plan a project to graduate several annotations to stable.)
    • Stable versions of provider-specific annotations might belong under a domain belonging to the provider. On the other hand, there are providers maintained by people not affiliated by the services they talk to.

@farodin91
Copy link
Author

@johngmyers I will look into this next week.

@farodin91
Copy link
Author

farodin91 commented Oct 10, 2023 •
edited
Loading

@johngmyers I started to look into the implementation.

@farodin91 farodin91 force-pushed the provider-generic-annotations branch from e4deea9 to b076420 Compare October 11, 2023 11:07
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 11, 2023
@farodin91
Copy link
Author

farodin91 commented Oct 11, 2023 •
edited
Loading

@johngmyers My first idea based on your feedback.

  • webhook backwards combat
  • move everything to provider
  • tried to make as generic as possible

ToDo:

  • tests

@farodin91 farodin91 force-pushed the provider-generic-annotations branch from b076420 to 5092328 Compare October 11, 2023 11:11
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 11, 2023
@farodin91 farodin91 force-pushed the provider-generic-annotations branch 2 times, most recently from 9ae70ac to d49985d Compare October 11, 2023 11:50
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2023
@farodin91 farodin91 force-pushed the provider-generic-annotations branch from d49985d to 3d80bce Compare November 28, 2023 07:36
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2023
@farodin91
Copy link
Author

/assign @johngmyers

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2023
@mloiseleur
Copy link
Contributor

@farodin91 This PR is quite interesting. Do you think you can rebase it ?

@farodin91 farodin91 force-pushed the provider-generic-annotations branch from 3d80bce to f59f0fa Compare February 12, 2024 13:49
@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 ask for approval from johngmyers. 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 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2024
@farodin91
Copy link
Author

@mloiseleur Updated, if you like to give feedback and go for it.

@mloiseleur
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 12, 2024
mloiseleur
mloiseleur reviewed Feb 12, 2024
View reviewed changes
provider/ibmcloud/ibmcloud.go Outdated
func (p *IBMCloudProvider) GetProviderSpecific(_ context.Context) (apis.ProviderSpecificConfig, error) {
return apis.ProviderSpecificConfig{
PrefixTranslation: map[string]string{
"external-dns.alpha.kubernetes.io/aws-": "aws/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"external-dns.alpha.kubernetes.io/aws-": "aws/",
"external-dns.alpha.kubernetes.io/ibmcloud-": "ibmcloud/",

Copy link
Author

Choose a reason for hiding this comment

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

fixed

mloiseleur
mloiseleur reviewed Feb 12, 2024
View reviewed changes
provider/scaleway/scaleway.go Outdated
func (p *ScalewayProvider) GetProviderSpecific(_ context.Context) (apis.ProviderSpecificConfig, error) {
return apis.ProviderSpecificConfig{
PrefixTranslation: map[string]string{
"external-dns.alpha.kubernetes.io/ibmcloud-": "ibmcloud/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"external-dns.alpha.kubernetes.io/ibmcloud-": "ibmcloud/",
"external-dns.alpha.kubernetes.io/scw-": "scw/",

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, is there really scaleway specific annotations ? I cannot find any in external-dns or scaleway tutorials ?

Copy link
Author

Choose a reason for hiding this comment

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

fixed. It looks like https://github.com/kubernetes-sigs/external-dns/pull/3959/files#diff-c2c587e9de1acc152a646c08609ac201aa8f5b78494e4ec069d340ad89b2ceb2L215 . Probably never documented.

@farodin91 farodin91 force-pushed the provider-generic-annotations branch from f59f0fa to 96e911e Compare February 12, 2024 14:48
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 12, 2024
@farodin91 farodin91 force-pushed the provider-generic-annotations branch from 96e911e to 052de90 Compare May 13, 2024 09:00
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 13, 2024
@farodin91 farodin91 changed the title provider generic annotations feat: provider generic annotations May 13, 2024
@farodin91 farodin91 force-pushed the provider-generic-annotations branch from 052de90 to 6e7a92d Compare May 13, 2024 09:01
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 13, 2024
@farodin91
Copy link
Author

@mloiseleur Would you like to review?

@farodin91 farodin91 force-pushed the provider-generic-annotations branch from 6e7a92d to d987f4b Compare May 13, 2024 09:12
@farodin91 farodin91 force-pushed the provider-generic-annotations branch from d987f4b to cfa75ab Compare May 13, 2024 09:25
@farodin91
Copy link
Author

@Raffo #4458

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2024
@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-sigs/prow repository.

@Raffo
Copy link
Contributor

Raffo commented May 19, 2024

@farodin91 I totally missed this PR, sorry. Can you walk me through your changes here and how you'd like to proceed with respect to #4458 ?

@farodin91
Copy link
Author

@farodin91 I totally missed this PR, sorry. Can you walk me through your changes here and how you'd like to proceed with respect to #4458 ?

My change allows every provider to easily provide their own set of annotations. For me it was also important to support every existing custom annotation.
If you think your PR get merged, I'm fine with it as it can solve my issue.
Also, each Webhook provider can provide their own set own annotations.

@Raffo
Copy link
Contributor

Raffo commented May 27, 2024

We shipped #4458, so I'm closing this PR.

@Raffo Raffo closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mloiseleur mloiseleur mloiseleur left review comments

@njuettner njuettner Awaiting requested review from njuettner

@szuecs szuecs Awaiting requested review from szuecs

Assignees

@johngmyers johngmyers

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@farodin91 @k8s-ci-robot @johngmyers @mloiseleur @k8s-triage-robot @Raffo