fix dependencies distributor buildAttachedBinding Namespace

[yanfeng1992]

Signed-off-by: huangyanfeng huangyanfeng1992@gmail.com

What type of PR is this?
/kind bug

What this PR does / why we need it:

When creating a dependent resourceBinding, the namespace should use the namespace of the object itself to support cross-namespace references to dependent resources.

For example, we have a crd virtualmachine, create virtualmachine cr test in ns defalut, webhook get virtualmachine cr test dependece configmap test-cm in ns test-2。

Which issue(s) this PR fixes:
Fixes #
dependencies distributor buildAttachedBinding namespace need
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`:  fix build attached resourceBinding namespace

[karmada-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign rainbowmango after the PR has been reviewed.
You can assign the PR to them by writing /assign @rainbowmango in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[jwcesign]

Thanks!
So for one crd in default ns, it depends on the configmap in ns-2.
The configmap's rb will be created in default ns for the previous implementation.
After this patch, the configmap's rb will be created in ns-2 ns, Do I understand correctly?

[yanfeng1992]

Thanks! So for one crd in default ns, it depends on the configmap in ns-2. The configmap's rb will be created in default ns for the previous implementation. After this patch, the configmap's rb will be created in ns-2 ns, Do I understand correctly?

@jwcesign yes, that's right。 For the previous implementation, configmap's rb be created in default ns , config map ceate in ns-2 ns, configmap's rb will be a Orphan binding. https://github.com/yanfeng1992/karmada/blob/85fcf2f627dd12cbf9849b49090451dcaa7b2a29/pkg/dependenciesdistributor/dependencies_distributor.go#L453 Do I understand correctly?

[XiShanYongYe-Chang]

Hi @yanfeng1992, I have a question, why do you need to use resources across ns?

[yanfeng1992]

Hi @yanfeng1992, I have a question, why do you need to use resources across ns?

for example，we need a pv, pv is provided by another company， pv provider company configmap in specific ns。

Doesn't karmada plan to support use resources across ns? @XiShanYongYe-Chang

[XiShanYongYe-Chang]

for example，we need a pv, pv is provided by another company， pv provider company configmap in specific ns。

Thanks for your explanation!

Doesn't karmada plan to support use resources across ns? @XiShanYongYe-Chang

No, I just wanted to know the scene.

[XiShanYongYe-Chang]

Hi @yanfeng1992, can you help add an E2E to cover this case?

[codecov-commenter]

Codecov Report

Merging #3044 (521cfd8) into master (28223cf) will increase coverage by 1.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3044      +/-   ##
==========================================
+ Coverage   37.90%   38.97%   +1.06%     
==========================================
  Files         207      207              
  Lines       19350    19365      +15     
==========================================
+ Hits         7335     7547     +212     
+ Misses      11578    11361     -217     
- Partials      437      457      +20     

Flag	Coverage Δ
unittests	38.97% <ø> (+1.06%)	⬆️

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

Impacted Files	Coverage Δ
pkg/util/worker.go	66.66% <0.00%> (-4.77%)	⬇️
pkg/search/proxy/store/util.go	93.83% <0.00%> (-0.48%)	⬇️
pkg/webhook/configuration/validating.go	71.42% <0.00%> (ø)
...g/util/overridemanager/labelannotationoverrider.go	85.29% <0.00%> (+0.44%)	⬆️
pkg/util/validation/validation.go	74.07% <0.00%> (+5.41%)	⬆️
pkg/util/overridemanager/overridemanager.go	48.62% <0.00%> (+25.22%)	⬆️
pkg/apis/cluster/mutation/mutation.go	98.11% <0.00%> (+91.82%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[yanfeng1992]

i add an E2E case cover this case, but but ran into a problem. After the configmap is updated, it cannot be synchronized to the member cluster. need your help. @XiShanYongYe-Chang

[XiShanYongYe-Chang]

i add an E2E case cover this case, but but ran into a problem. After the configmap is updated, it cannot be synchronized to the member cluster. need your help. @XiShanYongYe-Chang

Let me take a look.

[yanfeng1992]

https://github.com/yanfeng1992/karmada/blob/69a1b993415fe13633c41bdcd95ca6621d67b8b5/pkg/dependenciesdistributor/dependencies_distributor.go#L173 @XiShanYongYe-Chang I found the problem, can we list all namespace?

[yanfeng1992]

Hi @yanfeng1992, can you help add an E2E to cover this case?

E2E done @XiShanYongYe-Chang

[RainbowMango]

Could you please squash your commits?

[yanfeng1992]

Could you please squash your commits?

done @RainbowMango

[XiShanYongYe-Chang]

Hi @yanfeng1992, how about finding the target ResourceBinding through the following path?

resource template -> ResourceBinding of resource template -> dependent ResourceBinding

Hi @yanfeng1992, first of all, I apologize for my wrong advice. I tried to solve the problem using the above thinking, but I found that there was one situation that I couldn't solve: the resource creation situation. In this case, the target ResourceBinding cannot be found.
Solving this problem may still require you to go through all the ResourceBinding objects the way you did before.

But back to the previous question, is it worth it? Ask @RainbowMango for help.

[yanfeng1992]

Hi @XiShanYongYe-Chang @RainbowMango @jwcesign
I think I have solved this problem, through the way of increasing lable mentioned by jwcesign.

There are two advantages of adding labels for resourceBinding in this way
The first, can support support use resources across ns
The second,reduces the number of rb processing

[XiShanYongYe-Chang]

Hi @yanfeng1992, I feel that this solution is still a little incomplete, and if there are a large number of ResourceBinding that meet the requirements, the time consumption is still large.

In addition, the problem itself is that cross-namespace resource distribution is likely to encounter permissions issues, because for some users, he may not have permissions on all namespaces. In this case, distributing resources across namespaces may not work.

for example，we need a pv, pv is provided by another company， pv provider company configmap in specific ns。

For this application scenario, ask for @RainbowMango to take a look.

[yanfeng1992]

hi @XiShanYongYe-Chang @RainbowMango

Hi @yanfeng1992, how about finding the target ResourceBinding through the following path?
resource template -> ResourceBinding of resource template -> dependent ResourceBinding

Your previous suggestion is correct. There was a bit of a problem with my previous implementation, which I have fixed.
And you mentioned this question: ‘the resource creation situation. In this case, the target ResourceBinding cannot be found.’, I don't think it has any effect.

I think there are two advantages to doing this now

The first, can support support use resources across ns.
The second,reduces the number of rb processing. There is no need to judge whether there is a reference relationship based on the annotation.

In addition, why do we need to use resources across namespaces. When PV is mounted, xsky needs a cm. This cm is in a fixed ns. One pv corresponds to one cm, and other vendors’ csi implementations don’t seem to have this problem

[yanfeng1992]

Do we need to continue discussing this issue? Should it be considered a new feature rather than a bug? @XiShanYongYe-Chang

[XiShanYongYe-Chang]

Do we need to continue discussing this issue? Should it be considered a new feature rather than a bug? @XiShanYongYe-Chang

I think it's a new feature. How about adding a talk in the today's community meeting: https://docs.google.com/document/d/1y6YLVC-v7cmVAdbjedoyR5WL0-q45DBRXTvz5_I7bkA/edit#heading=h.g61sgp7w0d0c

[yanfeng1992]

Do we need to continue discussing this issue? Should it be considered a new feature rather than a bug? @XiShanYongYe-Chang

I think it's a new feature. How about adding a talk in the today's community meeting: https://docs.google.com/document/d/1y6YLVC-v7cmVAdbjedoyR5WL0-q45DBRXTvz5_I7bkA/edit#heading=h.g61sgp7w0d0c

OK, we can join the meeting, do you want me to edit this document?

[XiShanYongYe-Chang]

OK, we can join the meeting, do you want me to edit this document?

Yes, you can edit it directly.

[XiShanYongYe-Chang]

And you mentioned this question: ‘the resource creation situation. In this case, the target ResourceBinding cannot be found.’, I don't think it has any effect.

I just looked at the implementation, and I feel that this problem has not been solved.

[yanfeng1992]

I feel that in the previous implementation，the resource creation situation the target ResourceBinding aslo cannot be found. @XiShanYongYe-Chang

[XiShanYongYe-Chang]

I feel that in the previous implementation，the resource creation situation the target ResourceBinding aslo cannot be found. @XiShanYongYe-Chang

When a resource is created, it queues all matched rbs.

[yanfeng1992]

I understand what you mean.
For example,we created a deployment that depends on configmap.
My implementation can't cover the following situation:
deployment rb created, configmap does not exist. when configmap is created, the target ResourceBinding cannot be found.
Do i understand correctly?

[XiShanYongYe-Chang]

I understand what you mean.
For example,we created a deployment that depends on configmap.
My implementation can't cover the following situation:
deployment rb created, configmap does not exist. when configmap is created, the target ResourceBinding cannot be found.
Do i understand correctly?

Yes, it is.

[XiShanYongYe-Chang]

Hi @yanfeng1992, what are your plans for the issue now? Pack the whole application?

[yanfeng1992]

Hi @yanfeng1992, what are your plans for the issue now? Pack the whole application?

There is currently no plan to package it into an application. I am now trying to solve the problem by adding a dependent resource label in rb. Have a look at my latest implementation?

The implementation of packaging the application into a whole package is a good suggestion. I understand this suggestion in this way. In actual development, business developers may be more inclined to use the same crd in a single cluster and multi-cluster .

@XiShanYongYe-Chang @RainbowMango

[yanfeng1992]

Please take a look at my latest commit. I feel that the resource creation situation not find the target ResourceBinding has been solved.@XiShanYongYe-Chang

[XiShanYongYe-Chang]

Hi @yanfeng1992, this patch is functionally satisfactory. However, this solution may be different from the one we discussed at the meeting on Tuesday (2023-01-31). One of the considerations is the authority, which I think should also be taken into account.

[yanfeng1992]

Many thanks to the Karmada community help with this issue. I think packaging the app is a better approach. Using resources across namespaces is not a reasonable and common scenario. I think this issue can be closed. Do you have any opinion?

@XiShanYongYe-Chang @RainbowMango

[XiShanYongYe-Chang]

@yanfeng1992 Thanks for your hard trying! 👍

[RainbowMango]

Using resources across namespaces is not a reasonable and common scenario.

My primary concern must be security. Karmada and K8S's multi-tenant concepts are based on namespaces, if a resource depends on a resource in another namespace, it'll cause a potential security risk.

Anyway, thanks for spotting and this is definitely a valuable discussion.

For your case, my suggestion would be try to feed your workload with a config in the same namespace, or declare the config in PropagationPolicy explicitly.