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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(operators): support subset checking for in and notin #1555

Merged
merged 2 commits into from
Feb 10, 2021
Merged

feat(operators): support subset checking for in and notin #1555

merged 2 commits into from
Feb 10, 2021

Conversation

RinkiyaKeDad
Copy link
Contributor

Signed-off-by: Arsh Sharma arshsharma461@gmail.com

Related issue

Fixes #1367

What type of PR is this?

/kind feature

Proposed changes

Added checking if a set of strings is in an allowed list of strings when using In and NotIn operators

Checklist

Further comments

I wrote the setExistsInArray function based on keyExistsInArray function already present. I had a doubt here, if we have a return statement in default case wouldn't this mean that the return statement outside the switch block will never get executed?
Shouldn't we simply just have this is the default case:

default:
	return true, false

I'm a bit new to Golang so sorry if this is something obvious 馃槄 Thanks!

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>

return false, checkSubset(key, valueSlice)

case string:
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious how would this be declared in the policy?

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 the example @JimBugwadia gave in the issue we could have something like:

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: restrict-external-ips
spec:
  validationFailureAction: audit
  rules:
  - name: check-ips
    match:
      resources:
        kinds:
        - Service
    validate:
      message: "externalIPs are not allowed"
      deny:
        key: {{ request.object.spec.externalIPs }}
        operator: NotIn
        value: "1.1.1.1"

Where the keys are a set of strings and the value is just a string. Another case would be when the value is a string representing JSON like in the keyExistsInArray function. Though I'm not sure how that works.

Copy link
Member

Choose a reason for hiding this comment

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

I guess you can define the array in JSON format "['a', 'b']" then it is parsed to JSON string array.

for _, val := range key {
count, found := set[val]
if !found {
return false
Copy link
Member

Choose a reason for hiding this comment

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

Seems that checkSubset returns true if all keys are found in the value, while for NotIn in this use case, it expects "any of the three matches; not all three together".

We should define the expected behavior for set operations of In/NotIn.

cc @chipzoller @JimBugwadia

Copy link
Member

Choose a reason for hiding this comment

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

Given 2 sets, S1 and S2:

  • S1 In S2: means all values of S1 are in S2
  • S1 NotIn S2: means none of the values of S1 are in S2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will write two separate functions for In and NotIn based on this definition then.

@realshuting
Copy link
Member

I had a doubt here, if we have a return statement in default case wouldn't this mean that the return statement outside the switch block will never get executed?

@RinkiyaKeDad Yes you are right, the return statement outside of the switch block will never get executed. But the function has return values declared, removing the last return would result in a compile error missing return at end of function".

@RinkiyaKeDad
Copy link
Contributor Author

@RinkiyaKeDad Yes you are right, the return statement outside of the switch block will never get executed. But the function has return values declared, removing the last return would result in a compile error missing return at end of function".

Thanks for clearing that @realshuting. I didn't include a return statement outside of the switch block in the function I wrote because I got a warning in my editor saying unreachable code.

problem

I should ignore that and add a return statement regardless, right?

Also shouldn't these two lines of code be replaced with:

return true, false

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

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

Looks good.


return false, checkSubset(key, valueSlice)

case string:
Copy link
Member

Choose a reason for hiding this comment

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

I guess you can define the array in JSON format "['a', 'b']" then it is parsed to JSON string array.

@realshuting
Copy link
Member

Yes you are right, the return statement outside of the switch block will never get executed.

I take it back since I realized that in cases []interface{} and string we do not return if conditions aren't matched. So we need the last return statement.

@realshuting realshuting merged commit 596bc9b into kyverno:main Feb 10, 2021
vyankyGH pushed a commit to vyankyGH/kyverno that referenced this pull request Feb 16, 2021
* feat(operators): support subset checking for in and notin

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>

* feat(operators): fixed NotIn function

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>
realshuting added a commit that referenced this pull request Feb 17, 2021
* fix link (#1566)

Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* update icon in chart.yaml

Signed-off-by: Shuting Zhao <shutting06@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* Adding default policies for restricted mode and adding notes to helm install (#1556)

* Adding default policies for restricted mode, taking validationFailureAction from values.yaml and adding notes on helm install

Signed-off-by: Raj Das <mail.rajdas@gmail.com>

* Adding emoji

Signed-off-by: Raj Das <mail.rajdas@gmail.com>

* Update NOTES.txt

* minor fix

Signed-off-by: Raj Das <mail.rajdas@gmail.com>

* adding to readme

Signed-off-by: Raj Das <mail.rajdas@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* update links and formatting in PR template (#1573)

* update links and formatting in PR template

Signed-off-by: Chip Zoller <chipzoller@gmail.com>

* update policy submission request template

Signed-off-by: Chip Zoller <chipzoller@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* fix: restricting empty value to pass through the validation checks (#1574)

Signed-off-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* Actually fix contributor link in PR template (#1575)

* update links and formatting in PR template

Signed-off-by: Chip Zoller <chipzoller@gmail.com>

* update policy submission request template

Signed-off-by: Chip Zoller <chipzoller@gmail.com>

* actually fix contrib guidelines

Signed-off-by: Chip Zoller <chipzoller@gmail.com>

* actually fix contrib guidelines

Signed-off-by: Chip Zoller <chipzoller@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* code improvement (#1567)

* code improvement

Signed-off-by: NoSkillGirl <singhpooja240393@gmail.com>

* added if conditions

Signed-off-by: NoSkillGirl <singhpooja240393@gmail.com>

* fixed unit test cases

Signed-off-by: NoSkillGirl <singhpooja240393@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* feat(operators): support subset checking for in and notin (#1555)

* feat(operators): support subset checking for in and notin

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>

* feat(operators): fixed NotIn function

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* panic fix (#1601)

Signed-off-by: NoSkillGirl <singhpooja240393@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* update kyverno cli test cmd

Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* code indentation

Signed-off-by: vyankatesh <vyankatesh@neualto.com>

* change  help text

Signed-off-by: vyankatesh <vyankatesh@neualto.com>

Co-authored-by: Dekel <dekelb@users.noreply.github.com>
Co-authored-by: Shuting Zhao <shutting06@gmail.com>
Co-authored-by: Raj Babu Das <mail.rajdas@gmail.com>
Co-authored-by: Chip Zoller <chipzoller@gmail.com>
Co-authored-by: Yashvardhan Kukreja <yash.kukreja.98@gmail.com>
Co-authored-by: Pooja Singh <36136335+NoSkillGirl@users.noreply.github.com>
Co-authored-by: Arsh Sharma <56963264+RinkiyaKeDad@users.noreply.github.com>
Co-authored-by: vyankatesh <vyankatesh@neualto.com>
vyankyGH pushed a commit to vyankyGH/kyverno that referenced this pull request Feb 23, 2021
* feat(operators): support subset checking for in and notin

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>

* feat(operators): fixed NotIn function

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Signed-off-by: vyankatesh <vyankatesh@neualto.com>
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.

Support set operations for In and NotIn
3 participants