feat: agent report secret #1990

CharlesQQ · 2022-06-12T05:41:16Z

Signed-off-by: charlesQQ charles_ali@qq.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
Allowed karmada-agent report secret for Pull mode cluster

Which issue(s) this PR fixes:
Part of #1946

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-agent`: Introduced `--report-secrets` flag to allow secrets to be reported to the Karmada control plane during registering.

RainbowMango · 2022-06-13T08:30:56Z

Thanks @CharlesQQ
Have you tested it? Does it solve the issue addressed by #1946?

CharlesQQ · 2022-06-14T08:32:08Z

Have you tested it? Does it solve the issue addressed by #1946?

yes,I test it, karmada search can fetch cluster's resource in pull mode successfully, but sometimes, when restart karmada, karmada-search-controller might print log, whatever in push or pull mode:

I0614 15:13:55.311227      27 controller.go:170] Cluster member2 is not registered
W0614 15:56:54.931730      27 cache.go:96] SingleClusterInformerManager for cluster(member2) is nil.

I think it might aggregated-apiserver restarted and not started completely when karamda-search already started; so is might be necessary aggregated-apiserver running healthy when restarting karmada-search?

CharlesQQ · 2022-06-21T10:14:35Z

/cc @RainbowMango

RainbowMango · 2022-06-22T11:30:16Z

Sorry @CharlesQQ , I missed this PR, so busy these days.
I'll look at by this week.

cmd/agent/app/agent.go

pkg/util/credential.go

cmd/agent/app/agent.go

pkg/util/credential.go

Poor12 · 2022-06-24T01:13:11Z

Generally looks good to me.
cc @RainbowMango to check.

RainbowMango · 2022-06-24T03:04:28Z

cmd/agent/app/options/options.go

+	fs.StringVar(&o.ClusterProvider, "cluster-provider", "", "Provider of the joining cluster.")
+	fs.StringVar(&o.ClusterRegion, "cluster-region", "", "The region of the joining cluster.")
+	fs.StringVar(&o.ClusterZone, "cluster-zone", "", "The zone of the joining cluster")


Please don't combine these flags with this PR. This PR focus on reporting the secret thing.

You can do it by a separated PR. By the way, the --cluster-zone might need to update to --cluster-zones and it accepts a string. (The API part also needs an update).

We can add provider, region, zone flags and related code in the next pr.

Hi @CharlesQQ Why are the three flags still in this PR?

cmd/agent/app/options/options.go

XiShanYongYe-Chang

Thanks~

pkg/karmadactl/join.go

pkg/util/credential.go

cmd/agent/app/agent.go

XiShanYongYe-Chang · 2022-06-27T03:28:49Z

cmd/agent/app/agent.go

+			}
+		}
+	}
+	if opts.IsKubeImpersonatorEnabled() {


What if opts.IsKubeImpersonatorEnabled() if false?

What if opts.IsKubeImpersonatorEnabled() if false?

if IsKubeCredentialsEnabled and IsKubeImpersonatorEnabled is false, mutateFunc is nil, and return err

When IsKubeCredentialsEnabled and IsKubeImpersonatorEnabled are false, we need to create a Cluster object without them.

The logic here still seems to be a bit problematic. When IsKubeImpersonatorEnabled is false, mutateFunc will be nil.

Sorry, I don't understand clearly. When mutate is nill, do we just create an empty Cluster object?

When mutate is nill, do we just create an empty Cluster object?

Oh, that's really a problem

review again pls？ @XiShanYongYe-Chang

When report-secrets is {"KubeCredentials", "KubeImpersonator"}, only impersonator secret will be report.

I suggest to update like this:

--- a/cmd/agent/app/agent.go +++ b/cmd/agent/app/agent.go @@ -312,8 +312,7 @@ func startServiceExportController(ctx controllerscontext.Context) (bool, error) func generateClusterInControllerPlane(opts util.ClusterRegisterOption) (*clusterv1alpha1.Cluster, error) { clusterObj := &clusterv1alpha1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: opts.ClusterName}} - var mutateFunc, tempFunc func(cluster *clusterv1alpha1.Cluster) - tempFunc = func(cluster *clusterv1alpha1.Cluster) { + mutateFunc := func(cluster *clusterv1alpha1.Cluster) { cluster.Spec.SyncMode = clusterv1alpha1.Pull cluster.Spec.APIEndpoint = opts.ClusterAPIEndpoint cluster.Spec.ProxyURL = opts.ProxyServerAddress @@ -340,28 +339,22 @@ func generateClusterInControllerPlane(opts util.ClusterRegisterOption) (*cluster cluster.Spec.ProxyURL = url.String() } } - } - if opts.IsKubeCredentialsEnabled() { - mutateFunc = func(cluster *clusterv1alpha1.Cluster) { - tempFunc(cluster) + + if opts.IsKubeCredentialsEnabled() { cluster.Spec.SecretRef = &clusterv1alpha1.LocalSecretReference{ Namespace: opts.Secret.Namespace, Name: opts.Secret.Name, } } - } - if opts.IsKubeImpersonatorEnabled() { - mutateFunc = func(cluster *clusterv1alpha1.Cluster) { - tempFunc(cluster) + + if opts.IsKubeImpersonatorEnabled() { cluster.Spec.ImpersonatorSecretRef = &clusterv1alpha1.LocalSecretReference{ Namespace: opts.ImpersonatorSecret.Namespace, Name: opts.ImpersonatorSecret.Name, } } } - if mutateFunc == nil { - mutateFunc = tempFunc - }

XiShanYongYe-Chang · 2022-06-27T03:30:57Z

/assign

RainbowMango

Thanks @CharlesQQ for your hard work, and sorry for the delay.
I can focus on it this week.

Just a suggestion for get it easy to move this PR forward,
please keep PR as small as you can, and don't couple too much refactor work in it.

pkg/util/cluster.go

pkg/util/cluster_test.go

cmd/agent/app/agent.go

RainbowMango · 2022-07-06T03:28:00Z

Thanks for your hard work.
Generally looks good to me.
But why the cluster-provider, --cluster-region and --cluster-zone are still in this PR?

CharlesQQ · 2022-07-06T05:52:19Z

But why the cluster-provider, --cluster-region and --cluster-zone are still in this PR?

fixed, why e2e test failed?

RainbowMango · 2022-07-06T06:13:04Z

Message: failed to create a ClusterClient: Secret "karmada-member1" not found

I'm not sure, but please rebase your code with the master, and push again.

XiShanYongYe-Chang · 2022-07-06T06:18:28Z

It's maybe not an occasional mistake:

Status:
  Conditions:
    Last Transition Time:  2022-07-06T05:31:55Z
    Message:               failed to create a ClusterClient: Secret "karmada-member1" not found
    Reason:                StatusCollectionFailed
    Status:                False
    Type:                  Ready
Events:                    <none>

For the Push mode cluster, we need to create both of those two secrets by default.

XiShanYongYe-Chang

We need to report KubeImpersonator by default, our CI needs this.

XiShanYongYe-Chang · 2022-07-06T09:44:59Z

cmd/agent/app/agent.go

+			}
+		}
+	}
+	if opts.IsKubeImpersonatorEnabled() {


When report-secrets is {"KubeCredentials", "KubeImpersonator"}, only impersonator secret will be report.

I suggest to update like this:

--- a/cmd/agent/app/agent.go +++ b/cmd/agent/app/agent.go @@ -312,8 +312,7 @@ func startServiceExportController(ctx controllerscontext.Context) (bool, error) func generateClusterInControllerPlane(opts util.ClusterRegisterOption) (*clusterv1alpha1.Cluster, error) { clusterObj := &clusterv1alpha1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: opts.ClusterName}} - var mutateFunc, tempFunc func(cluster *clusterv1alpha1.Cluster) - tempFunc = func(cluster *clusterv1alpha1.Cluster) { + mutateFunc := func(cluster *clusterv1alpha1.Cluster) { cluster.Spec.SyncMode = clusterv1alpha1.Pull cluster.Spec.APIEndpoint = opts.ClusterAPIEndpoint cluster.Spec.ProxyURL = opts.ProxyServerAddress @@ -340,28 +339,22 @@ func generateClusterInControllerPlane(opts util.ClusterRegisterOption) (*cluster cluster.Spec.ProxyURL = url.String() } } - } - if opts.IsKubeCredentialsEnabled() { - mutateFunc = func(cluster *clusterv1alpha1.Cluster) { - tempFunc(cluster) + + if opts.IsKubeCredentialsEnabled() { cluster.Spec.SecretRef = &clusterv1alpha1.LocalSecretReference{ Namespace: opts.Secret.Namespace, Name: opts.Secret.Name, } } - } - if opts.IsKubeImpersonatorEnabled() { - mutateFunc = func(cluster *clusterv1alpha1.Cluster) { - tempFunc(cluster) + + if opts.IsKubeImpersonatorEnabled() { cluster.Spec.ImpersonatorSecretRef = &clusterv1alpha1.LocalSecretReference{ Namespace: opts.ImpersonatorSecret.Namespace, Name: opts.ImpersonatorSecret.Name, } } } - if mutateFunc == nil { - mutateFunc = tempFunc - }

pkg/util/cluster.go

XiShanYongYe-Chang

Thank you!

/lgtm

RainbowMango · 2022-07-07T02:10:49Z

Great! Seems we are ready to move this forward now.
Please help update the release-note part of the PR description since the flag name has changed.

And, before merging this patch, I want to confirm if this fixed the issue addressed by #1946, can you help to confirm that? (Please test with the latest patch)

CharlesQQ · 2022-07-07T06:12:34Z

And, before merging this patch, I want to confirm if this fixed the issue addressed by #1946, can you help to confirm that?

yes, I test it,and karamda-search can fetch resource from cluster in pull mode sueecessfully, but if restart karamda and karmada-aggregated-apiserver not healthy, log print following message:

I0707 11:10:53.182584      25 controller.go:170] Cluster member1 is not registered
I0707 11:10:53.182596      25 controller.go:170] Cluster member2 is not registered

Signed-off-by: charlesQQ <charles_ali@qq.com>

RainbowMango · 2022-07-07T10:29:24Z

Did anything change in the force push?

CharlesQQ · 2022-07-07T10:35:24Z

Did anything change in the force push?

yes,has code conflict in agent/app/options, because of last merge add new flag ProfileOpts

XiShanYongYe-Chang

/lgtm

👍

RainbowMango · 2022-07-08T02:55:15Z

/approve

I can see the provider/region/zone logic still in this PR.
Given this PR has been delayed for a long time, let's merge it now.
But if we are not going to introduce the provider/region/zone logs, please remember to remove the redundant logic before v1.3. @XiShanYongYe-Chang

karmada-bot · 2022-07-08T02:55:29Z

[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

XiShanYongYe-Chang · 2022-07-08T02:59:29Z

But if we are not going to introduce the provider/region/zone logs, please remember to remove the redundant logic before v1.3. @XiShanYongYe-Chang

Ok~

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 12, 2022

karmada-bot requested review from Garrybest and lonelyCZ June 12, 2022 05:41

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 12, 2022

CharlesQQ force-pushed the cluster-pull-secret branch 2 times, most recently from 4285a60 to 3b194dc Compare June 12, 2022 08:14

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 14, 2022

CharlesQQ force-pushed the cluster-pull-secret branch 2 times, most recently from ac9e92e to 1def733 Compare June 14, 2022 08:24

karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 14, 2022

karmada-bot requested a review from RainbowMango June 21, 2022 10:14

Poor12 reviewed Jun 23, 2022

View reviewed changes

cmd/agent/app/agent.go Outdated Show resolved Hide resolved

pkg/util/credential.go Outdated Show resolved Hide resolved

pkg/util/credential.go Outdated Show resolved Hide resolved

CharlesQQ force-pushed the cluster-pull-secret branch 2 times, most recently from 441d900 to 50f65db Compare June 23, 2022 09:27

Poor12 reviewed Jun 23, 2022

View reviewed changes

cmd/agent/app/agent.go Outdated Show resolved Hide resolved

pkg/util/credential.go Outdated Show resolved Hide resolved

CharlesQQ force-pushed the cluster-pull-secret branch from 50f65db to 45fa371 Compare June 23, 2022 16:53

RainbowMango reviewed Jun 24, 2022

View reviewed changes

CharlesQQ force-pushed the cluster-pull-secret branch from 45fa371 to eedf381 Compare June 25, 2022 16:59

karmada-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 25, 2022

CharlesQQ force-pushed the cluster-pull-secret branch 2 times, most recently from 73e2225 to 6c92a44 Compare June 26, 2022 01:38

XiShanYongYe-Chang reviewed Jun 27, 2022

View reviewed changes

karmada-bot assigned XiShanYongYe-Chang Jun 27, 2022

RainbowMango reviewed Jul 5, 2022

View reviewed changes

pkg/util/cluster.go Outdated Show resolved Hide resolved

pkg/util/cluster.go Outdated Show resolved Hide resolved

pkg/util/cluster_test.go Show resolved Hide resolved

cmd/agent/app/agent.go Outdated Show resolved Hide resolved

CharlesQQ force-pushed the cluster-pull-secret branch 2 times, most recently from 58a7b88 to 2f7acb6 Compare July 5, 2022 16:56

CharlesQQ force-pushed the cluster-pull-secret branch from 2f7acb6 to aedb2ea Compare July 6, 2022 05:15

XiShanYongYe-Chang reviewed Jul 6, 2022

View reviewed changes

CharlesQQ force-pushed the cluster-pull-secret branch 3 times, most recently from d42cedb to a397f78 Compare July 6, 2022 13:42

XiShanYongYe-Chang reviewed Jul 7, 2022

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022

feat: agent report secret

8738617

Signed-off-by: charlesQQ <charles_ali@qq.com>

CharlesQQ force-pushed the cluster-pull-secret branch from a397f78 to 8738617 Compare July 7, 2022 06:26

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022

XiShanYongYe-Chang reviewed Jul 8, 2022

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2022

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2022

karmada-bot merged commit 821a185 into karmada-io:master Jul 8, 2022

RainbowMango mentioned this pull request Jul 8, 2022

feat: add cluster agent register flag #2152

Merged

nuclearwu mentioned this pull request Aug 8, 2022

fix karmada-agent can not ensure default namespace issue #2328

Merged

feat: agent report secret #1990

feat: agent report secret #1990

Conversation

CharlesQQ commented Jun 12, 2022 • edited Loading

RainbowMango commented Jun 13, 2022

CharlesQQ commented Jun 14, 2022 • edited Loading

CharlesQQ commented Jun 21, 2022

RainbowMango commented Jun 22, 2022

Poor12 commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Jun 27, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango commented Jul 6, 2022

CharlesQQ commented Jul 6, 2022

RainbowMango commented Jul 6, 2022

XiShanYongYe-Chang commented Jul 6, 2022 • edited Loading

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango commented Jul 7, 2022

CharlesQQ commented Jul 7, 2022 • edited Loading

RainbowMango commented Jul 7, 2022

CharlesQQ commented Jul 7, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango commented Jul 8, 2022

karmada-bot commented Jul 8, 2022

XiShanYongYe-Chang commented Jul 8, 2022

CharlesQQ commented Jun 12, 2022 •

edited

Loading

CharlesQQ commented Jun 14, 2022 •

edited

Loading

CharlesQQ Jun 27, 2022 •

edited

Loading

CharlesQQ Jun 28, 2022 •

edited

Loading

XiShanYongYe-Chang commented Jul 6, 2022 •

edited

Loading

CharlesQQ commented Jul 7, 2022 •

edited

Loading