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

a7i · 2024-03-28T03:49:22Z

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 · 2024-03-28T04:05:55Z

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 ⚠️

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

Additional details and impacted files

@@           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	Coverage Δ
unittests	`51.79% <35.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chaunceyjiang

Great work, you provided a new perspective.

However, this comment #4752 (comment) also reminded me that is your change backward compatible with k8s versions below v1.24?

chaunceyjiang

Looks good to me.

XiShanYongYe-Chang · 2024-03-30T03:10:49Z

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

XiShanYongYe-Chang

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?

XiShanYongYe-Chang · 2024-04-08T12:42:23Z

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 · 2024-04-08T13:32:26Z

@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 · 2024-04-09T01:30:10Z

Maybe we can do it in the karmada-webhook:

karmada/pkg/webhook/work/mutating.go

Line 61 in eadf919

err = prune.RemoveIrrelevantField(workloadObj, prune.RemoveJobTTLSeconds)

a7i · 2024-04-09T02:56:29Z

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

XiShanYongYe-Chang · 2024-04-09T03:09:59Z

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 · 2024-04-09T03:16:08Z

@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 · 2024-04-09T03:22:50Z

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

a7i · 2024-04-09T03:25:54Z

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 · 2024-04-09T03:40:06Z

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 · 2024-04-10T08:46:46Z

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

chaunceyjiang

Nice~

/lgtm

RainbowMango · 2024-04-11T06:12:03Z

pkg/resourceinterpreter/default/native/retain_test.go

@@ -101,10 +102,77 @@ func Test_retainK8sWorkloadReplicas(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := retainWorkloadReplicas(tt.args.desired, tt.args.observed)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("reflectPodDisruptionBudgetStatus() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("retainWorkloadReplicas() error = %v, wantErr %v", err, tt.wantErr)


nice catch. thanks.

pkg/resourceinterpreter/default/native/retain.go

RainbowMango · 2024-04-11T13:37:49Z

LGTM
Thanks for your quick response!

But would you mind squash commits?

a7i · 2024-04-11T13:44:53Z

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

Signed-off-by: Amir Alavi <[email protected]>

RainbowMango · 2024-04-11T13:49:24Z

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

/lgtm
/approve

karmada-bot · 2024-04-11T13:49:31Z

[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~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RainbowMango · 2024-04-11T13:51:19Z

gonna heading home now, good morning to you

a7i · 2024-04-11T13:55:08Z

gonna heading home now, good morning to you

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

RainbowMango · 2024-04-11T14:06:27Z

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

a7i · 2024-04-19T01:31:51Z

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

karmada-io/community#68

a7i · 2024-04-19T01:32:50Z

I have verified that my sponsors are from different member companies

perhaps @chaunceyjiang would consider 🙇🏼

RainbowMango · 2024-04-19T02:48:45Z

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

definitly!

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 28, 2024

karmada-bot requested review from chaunceyjiang, whitewindmills and XiShanYongYe-Chang March 28, 2024 03:49

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 28, 2024

a7i force-pushed the propagate-service-account-token-secrets branch 2 times, most recently from 8a696f2 to ae9f508 Compare March 28, 2024 03:53

chaunceyjiang reviewed Mar 29, 2024

View reviewed changes

a7i mentioned this pull request Mar 29, 2024

Propagate Service Account Token Secret #4752

Closed

chaunceyjiang reviewed Mar 29, 2024

View reviewed changes

XiShanYongYe-Chang reviewed Mar 30, 2024

View reviewed changes

a7i mentioned this pull request Apr 9, 2024

chore: refactor prune.RemoveIrrelevantField to allow for more kind … #4821

Merged

a7i force-pushed the propagate-service-account-token-secrets branch 2 times, most recently from a56a8d9 to 81ff0aa Compare April 10, 2024 15:24

a7i requested review from chaunceyjiang and XiShanYongYe-Chang April 10, 2024 15:25

chaunceyjiang reviewed Apr 11, 2024

View reviewed changes

karmada-bot assigned chaunceyjiang Apr 11, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2024

RainbowMango reviewed Apr 11, 2024

View reviewed changes

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2024

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

7dbfc9f

Signed-off-by: Amir Alavi <[email protected]>

a7i force-pushed the propagate-service-account-token-secrets branch from a12498f to 7dbfc9f Compare April 11, 2024 13:45

a7i requested review from chaunceyjiang and RainbowMango April 11, 2024 13:46

karmada-bot assigned RainbowMango Apr 11, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2024

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2024

karmada-bot merged commit da7689f into karmada-io:master Apr 11, 2024
12 checks passed

a7i deleted the propagate-service-account-token-secrets branch April 11, 2024 17:19

a7i mentioned this pull request Sep 25, 2024

Evaluate both native and custom Interpreters #5604

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

a7i commented Mar 28, 2024 •

edited

Loading

codecov-commenter commented Mar 28, 2024

chaunceyjiang left a comment

chaunceyjiang left a comment

XiShanYongYe-Chang commented Mar 30, 2024

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented Apr 8, 2024

a7i commented Apr 8, 2024

XiShanYongYe-Chang commented Apr 9, 2024

a7i commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 9, 2024

a7i commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 9, 2024

a7i commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 10, 2024

chaunceyjiang left a comment

RainbowMango Apr 11, 2024

RainbowMango commented Apr 11, 2024

a7i commented Apr 11, 2024

RainbowMango commented Apr 11, 2024

karmada-bot commented Apr 11, 2024

RainbowMango commented Apr 11, 2024

a7i commented Apr 11, 2024

RainbowMango commented Apr 11, 2024

a7i commented Apr 19, 2024

a7i commented Apr 19, 2024

RainbowMango commented Apr 19, 2024

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

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

Conversation

a7i commented Mar 28, 2024 • edited Loading

codecov-commenter commented Mar 28, 2024

Codecov Report

chaunceyjiang left a comment

Choose a reason for hiding this comment

chaunceyjiang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Mar 30, 2024

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Apr 8, 2024

a7i commented Apr 8, 2024

XiShanYongYe-Chang commented Apr 9, 2024

a7i commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 9, 2024

a7i commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 9, 2024

a7i commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 9, 2024

XiShanYongYe-Chang commented Apr 10, 2024

chaunceyjiang left a comment

Choose a reason for hiding this comment

RainbowMango Apr 11, 2024

Choose a reason for hiding this comment

RainbowMango commented Apr 11, 2024

a7i commented Apr 11, 2024

RainbowMango commented Apr 11, 2024

karmada-bot commented Apr 11, 2024

RainbowMango commented Apr 11, 2024

a7i commented Apr 11, 2024

RainbowMango commented Apr 11, 2024

a7i commented Apr 19, 2024

a7i commented Apr 19, 2024

RainbowMango commented Apr 19, 2024

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

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

a7i commented Mar 28, 2024 •

edited

Loading