karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 807 forks source link

propagate `Secret` of type `kubernetes.io/service-account-token` #4766

Closed a7i closed 3 weeks ago

a7i commented 1 month ago

What type of PR is this?

/kind feature

What this PR does / why we need it: Prior to kubernetes 1.24, Kubernetes controller-manager automatically created a Secret (with a long-lived token). Starting with 1.24, in order to create a Secret with long-lived token, you have to manually create a Secret and link it to the Service Account. Karmada currently doesn't support propagating this Secret as it is explicitly disabled.

Which issue(s) this PR fixes: Fixes #4752

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Yes

karmada-controller-manager: propagate `Secret` of type `kubernetes.io/service-account-token`
codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 35.00000% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 51.79%. Comparing base (ff7322a) to head (ae9f508).

Files Patch % Lines
pkg/resourceinterpreter/default/native/retain.go 35.00% 9 Missing and 4 partials :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4766 +/- ## ======================================= Coverage 51.79% 51.79% ======================================= Files 250 250 Lines 24991 25004 +13 ======================================= + Hits 12945 12952 +7 - Misses 11337 11339 +2 - Partials 709 713 +4 ``` | [Flag](https://app.codecov.io/gh/karmada-io/karmada/pull/4766/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/karmada-io/karmada/pull/4766/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io) | `51.79% <35.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=karmada-io#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

XiShanYongYe-Chang commented 1 month ago

You can add the component name karmada-controller-manager to the release-note.

XiShanYongYe-Chang commented 3 weeks ago

The retain logic is modified in the current PR. However, retain is used when resources in member clusters are updated. If resources are created, will the UID and token information in the secret on the control plane be carried to the member cluster?

Kindly ping @a7i

a7i commented 3 weeks ago

@XiShanYongYe-Chang Yes it will but we observed in our case that the member cluster kube-controller-manager will fix up the values and get ignored by karmada on update.

Is there a "global" level ignore for CREATE as well?

XiShanYongYe-Chang commented 3 weeks ago

Maybe we can do it in the karmada-webhook:

https://github.com/karmada-io/karmada/blob/eadf919b6f25dc77873178e88a55983d05b03867/pkg/webhook/work/mutating.go#L61

a7i commented 3 weeks ago

@XiShanYongYe-Chang done, I kept as two separate commits for easier review. Happy to rebase and squash if needed.

XiShanYongYe-Chang commented 3 weeks ago

It occurs a lint error:

pkg/resourceinterpreter/default/native/prune/prune.go:33:1: cyclomatic complexity 18 of func `RemoveIrrelevantField` is high (> 15) (gocyclo)
a7i commented 3 weeks ago

@XiShanYongYe-Chang let me take a stab at refactoring this function in a separate PR and I'll come back to this after that's reviewed/merged

XiShanYongYe-Chang commented 3 weeks ago

Hi @a7i, CI errors may prevent PRs from being merged.

a7i commented 3 weeks ago

Hi @a7i, CI errors may prevent PRs from being merged.

I understand, hence why I'm suggesting to put a hold on this PR, until we refactor this function first (in a separate PR).

XiShanYongYe-Chang commented 3 weeks ago

I understand, hence why I'm suggesting to put a hold on this PR, until we refactor this function first (in a separate PR).

Thanks @a7i, I got it :)

XiShanYongYe-Chang commented 3 weeks ago

Hi @a7i , now we can go on this PR.

RainbowMango commented 3 weeks ago

LGTM Thanks for your quick response!

But would you mind squash commits?

a7i commented 3 weeks ago

LGTM Thanks for your quick response!

But would you mind squash commits?

I do not mind at all! Was keeping them separate for easier review

RainbowMango commented 3 weeks ago

haha, your code is so clear that I don't have to review it by commits~

/lgtm /approve

karmada-bot commented 3 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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](https://github.com/karmada-io/karmada/blob/master/OWNERS)~~ [RainbowMango] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
RainbowMango commented 3 weeks ago

gonna heading home now, good morning to you

a7i commented 3 weeks ago

gonna heading home now, good morning to you

long day! I appreciate the thorough review and all the great feedback 🎉

RainbowMango commented 3 weeks ago

I really really hope people like you join us in maintaining this project, can we get started from the org membership?

a7i commented 2 weeks ago

Hi @RainbowMango and @XiShanYongYe-Chang I hope you would consider sponsoring me ❤️

https://github.com/karmada-io/community/issues/68

a7i commented 2 weeks ago

I have verified that my sponsors are from different member companies

perhaps @chaunceyjiang would consider 🙇🏼

RainbowMango commented 2 weeks ago

Hi @RainbowMango and @XiShanYongYe-Chang I hope you would consider sponsoring me ❤️

definitly!