Multi-target ResourceInterpreterCustomization support

[grosser]

What would you like to be added:

allow ResourceInterpreterCustomization to target multiple resources

apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: declarative-configuration-example
spec:
  targetSelector:
    apiVersion: *.custom.foo
    # no `kind`

Why is this needed:

Atm we plan to copy our ResourceInterpreterCustomization (that does "copy spec and status") for every custom-resource that we have, it would be nice to make that generic so every newly added custom-resource "just works".

In case there are multiple RIC defined I'd suggest to trigger only the most specific (alternatively trigger all)

current RIC:

apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: example
spec:
  target:
    apiVersion: foo.bar.com/v1alpha1
    kind: FooBar
  customizations:
    retention:
      luaScript: >
        function Retain(desiredObj, observedObj)
          desiredObj.spec = observedObj.spec
          desiredObj.status = observedObj.status
          return desiredObj
        end

[XiShanYongYe-Chang]

That sounds like a good suggestion, I have a few questions I'd like to continue the discussion on.

allow ResourceInterpreterCustomization to target multiple resources

Is the wildcard setting only allowed for the apiVersion field here? Do we need to limit it to prefix or postfix matches?

In case there are multiple RIC defined I'd suggest to trigger only the most specific (alternatively trigger all)

It looks like some implicit prioritization needs to be set in here, how should prioritization be considered if it's all wildcards?

[grosser]

Would be API version prefix and suffix wildcard Prioritization could just be alphabethical to keep it simple

On Mon, Oct 30, 2023, 7:03 PM Chang @.***> wrote:

That sounds like a good suggestion, I have a few questions I'd like to continue the discussion on.

allow ResourceInterpreterCustomization to target multiple resources

Is the wildcard setting only allowed for the apiVersion field here? Do we need to limit it to prefix or postfix matches?

In case there are multiple RIC defined I'd suggest to trigger only the most specific (alternatively trigger all)

It looks like some implicit prioritization needs to be set in here, how should prioritization be considered if it's all wildcards?

— Reply to this email directly, view it on GitHub https://github.com/karmada-io/karmada/issues/4195#issuecomment-1786319668, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACYZ7SIECHLWUO3TF5RHLYCBL6TAVCNFSM6AAAAAA6WWGPW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGMYTSNRWHA . You are receiving this because you authored the thread.Message ID: @.***>

[XiShanYongYe-Chang]

Invite more people to come and see /cc @RainbowMango @chaunceyjiang @GitHubxsy

[chaunceyjiang]

  targetSelector:
    apiVersion: *.custom.foo
    # no `kind`

Judging from your proposal, there are two changes. The first one is that apiVersion supports wildcards, and the second one is that kind does not need to be filled out.

The first change, I feel, is a significant modification. I'm currently unsure if it can be well supported.

The second change, in my opinion, is an excellent proposal.

Currently, a resource cannot have two ResourceInterpreterCustomization. If this proposal is introduced, how should we handle the situation where a resource has two ResourceInterpreterCustomization?

[grosser]

We could start with making kind optional. For multiple interpreters executing all in alpha name order could make sense. (Is there something that prevents conflicts atm ? ... Could I not create 2 interpreters with different names but same target ?)

On Mon, Oct 30, 2023, 8:07 PM Chauncey @.***> wrote:

targetSelector: apiVersion: *.custom.foo

no kind

Judging from your proposal, there are two changes. The first one is that apiVersion supports wildcards, and the second one is that kind does not need to be filled out.

The first change, I feel, is a significant modification. I'm currently unsure if it can be well supported.

The second change, in my opinion, is an excellent proposal.

Currently, a resource cannot have two ResourceInterpreterCustomization. If this proposal is introduced, how should we handle the situation where a resource has two ResourceInterpreterCustomization?

— Reply to this email directly, view it on GitHub https://github.com/karmada-io/karmada/issues/4195#issuecomment-1786367016, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACYZZOCRFF7R3A7BJPYZ3YCBTNPAVCNFSM6AAAAAA6WWGPW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGM3DOMBRGY . You are receiving this because you authored the thread.Message ID: @.***>

[XiShanYongYe-Chang]

Going back to the original question:

it would be nice to make that generic so every newly added custom-resource "just works".

In order to make custom resources JUST WORK, can we consider adding some default processing to the resource interpreter, so that when no arbitrary interpretation behavior of the target resource exists in the resource interpreter, it is handled according to the default processing logic? Just like reflecWholeStatus in the interpretStatus operation:

https://github.com/karmada-io/karmada/blob/961aa694bec822e6919f335ae81903afe1212d95/pkg/resourceinterpreter/default/native/default.go#L135

[grosser]

That would be great too.

On Mon, Oct 30, 2023, 8:31 PM Chang @.***> wrote:

Going back to the original question:

it would be nice to make that generic so every newly added custom-resource "just works".

In order to make custom resources JUST WORK, can we consider adding some default processing to the resource interpreter, so that when no arbitrary interpretation behavior of the target resource exists in the resource interpreter, it is handled according to the default processing logic? Just like reflecWholeStatus in the interpretStatus operation:

https://github.com/karmada-io/karmada/blob/961aa694bec822e6919f335ae81903afe1212d95/pkg/resourceinterpreter/default/native/default.go#L135

— Reply to this email directly, view it on GitHub https://github.com/karmada-io/karmada/issues/4195#issuecomment-1786382377, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACYZ2RLAE42SOOM7VHYE3YCBWHLAVCNFSM6AAAAAA6WWGPW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGM4DEMZXG4 . You are receiving this because you authored the thread.Message ID: @.***>

[chaunceyjiang]

Could I not create 2 interpreters with different names but same target ?)

If I remember correctly, it should not be possible.

https://github.com/karmada-io/karmada/blob/master/pkg/webhook/resourceinterpretercustomization/helper.go#L26

[chaunceyjiang]

In order to make custom resources JUST WORK, can we consider adding some default processing to the resource interpreter, so that when no arbitrary interpretation behavior of the target resource exists in the resource interpreter, it is handled according to the default processing logic?

It might not be the same, the Retain operation provided by this issue, if set as default behavior, could potentially have a too broad impact.

    retention:
      luaScript: >
        function Retain(desiredObj, observedObj)
          desiredObj.spec = observedObj.spec
          desiredObj.status = observedObj.status
          return desiredObj
        end

[chaunceyjiang]

I strongly agree with making 'kind' an optional setting.

[XiShanYongYe-Chang]

It might not be the same, the Retain operation provided by this issue, if set as default behavior, could potentially have a too broad impact.

It's really not a good idea to handle the situation as a default if that's what the content of the RETAIN is.

[grosser]

making apiVersion optional (when a kind is set) should also be doable or not ? ... if both are not set then we'd already have the "default" behavior possible to set

the webhook could also just deny anything the conflicts until we agree on what the behavior is

[chaunceyjiang]

I think we can first support setting kind as an option in the next release, what do you think @XiShanYongYe-Chang @RainbowMango @grosser .

[XiShanYongYe-Chang]

For apiVersion to support wildcards, are there any difficulties to be expected if it is implemented?

[XiShanYongYe-Chang]

Hi @grosser

        function Retain(desiredObj, observedObj)
          desiredObj.spec = observedObj.spec
          desiredObj.status = observedObj.status
          return desiredObj
        end

In the example you provided, why do you need to do a Retain operation on both the entire spec and status?

[chaunceyjiang]

For apiVersion to support wildcards, are there any difficulties to be expected if it is implemented?

It's hard to determine the priority.

[RainbowMango]

I think we can first support setting kind as an option in the next release, what do you think @XiShanYongYe-Chang @RainbowMango @grosser .

Before planning a feature, I want to further understand the use case.

Atm we plan to copy our ResourceInterpreterCustomization (that does "copy spec and status")

@grosser Can you explain more about why you need to copy spec and status? Do you need to do that with Kubernetes native resources?

A resource type might have status subresources, in that case, retaining the .status might not work because the configuration in .status will be ignored during updation, in other words, Karmada won't update the status. So, it is concerning if your CRD has status subresource.

[grosser]

We have 1 (possibly more, but stopped migration at first 1) custom resources that are created via karmada and then propagated, but then updated in the member cluster (spec and status).

[RainbowMango]

It makes sense that some of the fields in .spec are updated(managed) by a controller running in the member cluster, that is exactly the use case of retain to avoid conflicts, but if retaining the whole .spec, that means you unable to make changes on Karmada(since the changes can't be synced to member clusters) after the first propagation, is that you expected?

Does the custom resources define subresource status? If yes, you don't need to retain the .status here. if no, yes you might need to retain it but might not the whole .status.

[grosser]

Yeah I was afraid of updated not working, let me double-check that ... worst case we end up with different interpreter and then this while kind: * does not work 😞

[grosser]

closing until we need this again ... might not need this now that we changed our approach