search

karmada-io / karmada

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

feat: agent report secret #1990

Closed CharlesQQ closed 1 year ago

CharlesQQ commented 1 year ago

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 https://github.com/karmada-io/karmada/issues/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 commented 1 year ago

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

CharlesQQ commented 1 year ago

Have you tested it? Does it solve the issue addressed by https://github.com/karmada-io/karmada/issues/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 commented 1 year ago

/cc @RainbowMango

RainbowMango commented 1 year ago

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

Poor12 commented 1 year ago

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

XiShanYongYe-Chang commented 1 year ago

/assign

CharlesQQ commented 1 year ago

@XiShanYongYe-Chang can you figure out why unit test failed, i run unit test successfully

XiShanYongYe-Chang commented 1 year ago

=== RUN TestObtainCredentialsFromMemberCluster/report_secret_enabled credential_test.go:145: ObtainCredentialsFromMemberCluster() error = failed to get serviceAccount secret for impersonation from cluster(member1), error: failed to get serviceAccount secret, error: timed out waiting for the condition, wantErr false --- FAIL: TestObtainCredentialsFromMemberCluster (60.00s) --- FAIL: TestObtainCredentialsFromMemberCluster/disable_report_secret (30.00s) --- FAIL: TestObtainCredentialsFromMemberCluster/report_secret_enabled (30.00s)

image

CharlesQQ commented 1 year ago

i run successfully

=== RUN TestObtainCredentialsFromMemberCluster --- PASS: TestObtainCredentialsFromMemberCluster (4.01s) === RUN TestObtainCredentialsFromMemberCluster/disable_report_secret --- PASS: TestObtainCredentialsFromMemberCluster/disable_report_secret (2.00s) === RUN TestObtainCredentialsFromMemberCluster/report_secret_enabled --- PASS: TestObtainCredentialsFromMemberCluster/report_secret_enabled (2.01s) PASS

XiShanYongYe-Chang commented 1 year ago

Occasional timeout.

CharlesQQ commented 1 year ago

I can't locate the error directly

image
CharlesQQ commented 1 year ago

/test ?

karmada-bot commented 1 year ago

@CharlesQQ: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to [this](https://github.com/karmada-io/karmada/pull/1990#issuecomment-1168375287): >/test ? Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
RainbowMango commented 1 year ago

Let me try on my side.

RainbowMango commented 1 year ago

Just re-triggered it as it works on my side:

go test -race -failfast -v -count=10 ./pkg/...
CharlesQQ commented 1 year ago

why unit test for ObtainCredentialsFromMemberCluster successfully on my side, but failed on CI Workflow

image

image

XiShanYongYe-Chang commented 1 year ago

Mhmm... Something seems wrong.

CharlesQQ commented 1 year ago

because of WaitForServiceAccountSecretCreation logic changed

CharlesQQ commented 1 year ago

cc @XiShanYongYe-Chang

CharlesQQ commented 1 year ago

why CI workflow not be triggered？

XiShanYongYe-Chang commented 1 year ago

why CI workflow not be triggered？

Maybe at that time, the ci stopped working, and I found that the other one pr was in the same state. We need to retrigger it.

RainbowMango commented 1 year ago

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 commented 1 year ago

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

fixed, why e2e test failed?

RainbowMango commented 1 year ago

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 commented 1 year ago

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.

RainbowMango commented 1 year ago

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 commented 1 year ago

And, before merging this patch, I want to confirm if this fixed the issue addressed by https://github.com/karmada-io/karmada/issues/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
RainbowMango commented 1 year ago

Did anything change in the force push?

CharlesQQ commented 1 year ago

Did anything change in the force push?

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

RainbowMango commented 1 year ago

/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 commented 1 year 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
XiShanYongYe-Chang commented 1 year ago

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~