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

#753 - Validate conflicting match and exclude #758

Merged
merged 12 commits into from
Apr 12, 2020
Merged

#753 - Validate conflicting match and exclude #758

merged 12 commits into from
Apr 12, 2020

Conversation

shravanshetty1
Copy link
Contributor

fixes #753

pkg/policy/validate.go Outdated Show resolved Hide resolved
pkg/policy/validate.go Outdated Show resolved Hide resolved
pkg/policy/validate.go Outdated Show resolved Hide resolved
}

if rule.MatchResources.ResourceDescription.Name != "" {
if rule.MatchResources.ResourceDescription.Name == rule.ExcludeResources.ResourceDescription.Name {
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 need to check the name of the resource? It's possible to have the same pod name, say "test-pod" in different namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are 2 pods with same name in different name space:
Currently we can write:

match:
	resources:
		name: test-pod
		kinds:
			-Pod
exclude:
	resources:
		name: test-pod
		kinds:
			-Pod
		namespace:
			-namespace1

With this PR user will be forced to write:

match:
	resources:
		name: test-pod
		kinds:
			-Pod
exclude:
	resources:
		namespace:
			-namespace1

Copy link
Member

Choose a reason for hiding this comment

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

If I write the following policy, it will be rejected:

match:
  resources:
    name: test-pod
    kinds:
      - Pod
    namespaces:
      - namespace1
exclude:
  resources:
    name: test-pod
    kinds:
      - Pod
    namespaces:
      - namespace2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above exclude block will never apply since its only matching pod from namespace1

Lets assume - this is what you meant:

match:
  resources:
    name: test-pod
    kinds:
      - Pod
    namespaces:
      - namespace1
      - namespace2
exclude:
  resources:
    name: test-pod
    kinds:
      - Pod
    namespaces:
      - namespace2

This will be rejected since its logically equivalent to this

match:
  resources:
    name: test-pod
    kinds:
      - Pod
    namespaces:
      - namespace1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further analysis the following policy will be rejected even though its valid

match:
  resources:
    namespaces:
      - namespace1
      - namespace2
exclude:
  resources:
    kinds:
      - Pod
    namespaces:
      - namespace2

Maybe we should reconsider this change

@shravanshetty1 shravanshetty1 changed the title #753 - Validate conflicting match and exclude [Do Not Merge] #753 - Validate conflicting match and exclude Mar 24, 2020
@shravanshetty1 shravanshetty1 reopened this Apr 4, 2020
@shravanshetty1 shravanshetty1 changed the title [Do Not Merge] #753 - Validate conflicting match and exclude [WIP] #753 - Validate conflicting match and exclude Apr 4, 2020
@shravanshetty1 shravanshetty1 changed the title [WIP] #753 - Validate conflicting match and exclude #753 - Validate conflicting match and exclude Apr 4, 2020
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.

Have you tested with all best practices? How's the performance look like if someone imports all policies?

pkg/policy/validate.go Show resolved Hide resolved
pkg/webhooks/policymutation.go Outdated Show resolved Hide resolved
@shravanshetty1
Copy link
Contributor Author

shravanshetty1 commented Apr 11, 2020

Have you tested with all best practices? How's the performance look like if someone imports all policies?

yes, this should not be a bottle neck

@realshuting realshuting merged commit b5c37f4 into kyverno:master Apr 12, 2020
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.

[BUG] Validate conflicting match and exclude
4 participants