Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for extending forEach to cover generate rules #52

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MariamFahmy98
Copy link
Contributor

No description provided.

Comment on lines 113 to 134
generate:
synchronize: false
foreach:
- list: request.object.spec.rules
resources:
- apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
# the name must be unique
name: httproute
namespace: "{{request.object.metadata.namespace}}"
data:
spec:
hostnames:
- '{{ element.host }}'
- list: request.object.spec.ingressClassName
resources:
- apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
name: gateway
data:
spec:
gatewayClassName: '{{ element }}'
Copy link
Member

@realshuting realshuting Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the mapping for a generate rule is - one rule can support M foreach, one foreach can generate N resources. Therefore one generate rule manages M*N resources, I wonder how the complexity of the management work can be, for example, to sync changes from the rule to downstream resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity, we can limit the rule to have 1 foreach and it will only generate 1 resource. I amn't sure how we sync the changes given there is M*N managed resources rather than applying the rule again.

kind: Secret
name: secret
namespace: "{{request.object.metadata.namespace}}"
data:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support clone and cloneList in foreach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Since cloneList is used to clone multiple resources at once, then there is no need for using foreach. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think foreach with clone will have the same functionality as cloneList, so why we introduce it in clone if there is no extra functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using foreach with clone will allow us to clone the existing resource multiple times.
Do we have a real use case for that?

For example, we can clone a secret depending on the number of containers. For each container, the same secret will be cloned.

Copy link

@pinkfloydx33 pinkfloydx33 Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this handle the case of cloning a resource into multiple namespaces?

For example matching on Secret with an annotation of the form sync: a=b, using the API server to find all Namespace with label a: b and then foreach'ing that list to generate the Secrets.

This is a case we can't currently do with Kyverno (so far as I could tell). It's a task tools like KubeD or Reflector perform as their primary purpose. (Along with many others) I'm looking for a replacement for the former and was hoping I'd be able to write a ClusterPolicy for that case... but it seems I'd need foreach in a generate rule that could also clone first

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might consider using external-secrets kubernetes provider along with its PushSecret custom resource to replace many kubed use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pinkfloydx33 - this is an interesting use case.

I have crafted an example rule which looks up a list of namespaces by the label and clone the secret to multiple namespaces.

spec:
  rules:
  - name: clone-seret-to-multiple-namespace
    match:
      any:
      - resources:
          kinds:
          - Secret
          namespaces:
          - default
    context:
    - variable:
        name: selector
        jmesPath: request.object.metadata.labels.sync
        default: a=b
    - name: namespaces
      apiCall:
        urlPath: /api/v1/namespaces?labelSelector={{selector}}?limit=5
    generate:
      synchronize: false
      foreach:
        - list: "{{namespaces}}"
          cloneList:
          - source:
              namespace: default
              name: regcred
              kind: v1
              apiVersion: Secret
            target:
              namespace: "{{element}}"
              name: regcred

What do you think?

resources:
- apiVersion: v1
kind: NetworkPolicy
name: '{{ element }}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to clarify the built-in variables for generate foreach type of rule.

@realshuting
Copy link
Member

We need clarification on how synchronize behavior applies to the foreach generate rule.

Signed-off-by: ShutingZhao <[email protected]>
@realshuting
Copy link
Member

This KDP enables the generation of multiple targets for a list of values.

Another related request is to generate multiple resources from a single trigger, see kyverno/kyverno#4352.

Signed-off-by: ShutingZhao <[email protected]>
## Phase 2 - support `foreach` declaration

In the second phase, the generate rule can be expanded to support `foreach` declaration in order to iterate through given list. This could be done by adding the `foreach` key under generate pattern, and a `list` attribute which is a JMESPath expression that defines the sub-elements it processes. For every `foreach` pattern, either `dataList` or `cloneList` will be supported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would data and clone also be supported under the foreach?

Ideally all behaviors / constructs in the regular generate are also supported when using foreach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would data and clone also be supported under the foreach?

No, as the same functionalities to generate or clone single resources can be achieved using dataList or cloneList. I'm thinking whether we want to deprecate data and clone once we introduce new fields, but that requires migration of existing policies to use new structure. They are good things to discuss but it's out of the scope of this KDP. I can start a separate design discussion once we add dataList and cloneList, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with deprecating data and clone.

However, we should try and reuse the generate logic completely. The foreach is just a loop that invokes generate multiple times.

Hence, may be easier to retain support for data and clone until they are removed.

@thesuperzapper
Copy link

I still don't think this proposal solves the issues raised in kyverno/kyverno#10715

For clone rules, it's important that we can trigger off the source object, while still targeting multiple namespaces (preferably not just by listing them multiple times, but by selecting a namespace label or similar).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants