#644 - Policy Rule Exclude conditions should be processed as a logical AND instead of a logical OR

[shravanshetty1]

No description provided.

[shravanshetty1]

Since i am unclear on why these changes are required, i would like these changes to be reviewed before i test them .etc.

[JimBugwadia]

@shravanshetty1 here is the reason for doing this change request:

From https://github.com/nirmata/kyverno/issues/634

If a rule has this exclude block:

    exclude:
      resources:
        kinds:
        - Pod
      clusterroles:
        - cluster-admin
        - admin

the user expectation is that the kinds and clusterroles are processed as an logical AND. The current implementation was a logical OR.

Does that help clarify the intent?

[shravanshetty1]

@JimBugwadia Yes this does clarify the intent will update the PR by EOD

[JimBugwadia]

The intent is to combine the resource kind and user info checks into a single logical AND check. We still need both checks.

The error reporting should indicate the precise conditions that fail.

We can extract these checks into a separate method.

[shravanshetty1]

@JimBugwadia Requested changes have been made

[shivdudhani]

@shravanshetty1
Curious to know the reason behind using goroutines to do checks?

• As most of the checks inside the goroutines are small and fast( wondering if we will spend time just managing the goroutines for tasks like looping over a list of slices). Routines are helpful to process tasks in async but, if the tasks are really small it might be faster to process them sequentially in a single goroutine(could be an overkill, but this is a theoretical observation).
• The error is captured using a channel and each goroutine can finish at different times, so the order of messages might be different. And we also assign indices after receiving errors, so the order of messages would matter. The generated messages are being used internally, so shouldnt be a major issue. But if we do plan to use the output to the unit tests(we do not check for error text returned), then we would expect the errors to be consistent order.
• Suggestion, the channel used in each goroutine is open(i.e. send & receive), will be a good design to pass the "sending only" channel if we are just adding error values to it.

[shravanshetty1]

@realshuting @shivdudhani

1. Your correct, it seems like goroutines have alot of overhead to use it for small tasks such as these. I could not find any source about overhead on goroutines, so i decided to bench it myself.
   
   Concurrent - is the version in PR
   Single - version i created without any go routines
   Hybrid - Kept 2 of the main go routines, since they were making over 50+ instructions concurrent

Seems its better to not have go routines even for 50+ instructions.

2. Will be removing go routines - should not be an issue now

3. Will be removing go routines - should not be an issue now

[shravanshetty1]

@shravanshetty1
Curious to know the reason behind using goroutines to do checks?

• As most of the checks inside the goroutines are small and fast( wondering if we will spend time just managing the goroutines for tasks like looping over a list of slices). Routines are helpful to process tasks in async but, if the tasks are really small it might be faster to process them sequentially in a single goroutine(could be an overkill, but this is a theoretical observation).
• The error is captured using a channel and each goroutine can finish at different times, so the order of messages might be different. And we also assign indices after receiving errors, so the order of messages would matter. The generated messages are being used internally, so shouldnt be a major issue. But if we do plan to use the output to the unit tests(we do not check for error text returned), then we would expect the errors to be consistent order.
• Suggestion, the channel used in each goroutine is open(i.e. send & receive), will be a good design to pass the "sending only" channel if we are just adding error values to it.

@shivdudhani suggested changes have been made

[realshuting]

Fix #644.