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

Support GVK format is case sensitive #2261

Merged
merged 9 commits into from
Sep 9, 2021
Merged

Conversation

vyankyGH
Copy link
Contributor

@vyankyGH vyankyGH commented Aug 12, 2021

Signed-off-by: Vyankatesh vyankateshkd@gmail.com

Related issue

closes #2253

Milestone of this PR

What type of PR is this

Proposed Changes

Proof Manifests

Resource.yaml

spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: deny-exec-myapp-maintenance
    match:
      resources:
        kinds:
        - "deployment"

After install policy -o yaml

spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: deny-exec-myapp-maintenance
    match:
      resources:
        kinds:
        - "Deployment"

Resource.yaml

spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: deny-exec-myapp-maintenance
    match:
      resources:
        kinds:
        - "batch.volcano.sh/v1alpha1/job"

After install policy -o yaml

spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: deny-exec-myapp-maintenance
    match:
      resources:
        kinds:
        - "batch.volcano.sh/v1alpha1/Job"

Resource.yaml

spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: deny-exec-myapp-maintenance
    match:
      resources:
        kinds:
          - configMap
          - pod
          - deployment

After install policy -o yaml

spec:
  validationFailureAction: enforce
  background: false
  rules:
  - name: deny-exec-myapp-maintenance
    match:
      resources:
        kinds:
          - ConfigMap
          - Pod
          - Deployment

Checklist

  • I have read the contributing guidelines.
  • I have added tests that prove my fix is effective or that my feature works.
  • My PR contains new or altered behavior to Kyverno and
    • I have added or changed the documentation myself in an existing PR and the link is:
    • [] I have raised an issue in kyverno/website to track the doc update and the link is:
    • [] I have read the PR documentation guide and followed the process including adding proof manifests to this PR.

Further Comments

@vyankyGH vyankyGH added the wip work in progress label Aug 12, 2021
@chipzoller
Copy link
Member

Did you happen to test plural vs singular as well? I know it's not the subject of this PR but asking.

@vyankyGH
Copy link
Contributor Author

Did you happen to test plural vs singular as well? I know it's not the subject of this PR but asking.
Yes, It will work.

@vyankyGH vyankyGH removed the wip work in progress label Aug 13, 2021
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.

Can we convert the GVK dynamically while processing the policy? Mutating the policy may not work if mutating webhook is missing or the policy creation request is somehow missed.

@realshuting realshuting added the wip work in progress label Aug 20, 2021
@vyankyGH vyankyGH removed the wip work in progress label Aug 23, 2021
@realshuting
Copy link
Member

Can we convert the GVK dynamically while processing the policy? Mutating the policy may not work if mutating webhook is missing or the policy creation request is somehow missed.

@vyankyGH - what do you think?

@vyankyGH
Copy link
Contributor Author

vyankyGH commented Sep 1, 2021

Can we convert the GVK dynamically while processing the policy? Mutating the policy may not work if mutating webhook is missing or the policy creation request is somehow missed.

@vyankyGH - what do you think?

Sorry, I missed comment. I will update to convert GVK while processing the policy.

@vyankyGH
Copy link
Contributor Author

vyankyGH commented Sep 3, 2021

Added test cases.

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.

/LGTM

@vyankyGH - can you resolve conflict?

Any doc update needed?

Comment on lines +35 to +43
if resource.GetKind() == strings.Title(kind) {
return true
}
} else if len(SplitGVK) == 2 {
if resource.GroupVersionKind().Kind == SplitGVK[1] && resource.GroupVersionKind().Version == SplitGVK[0] {
if resource.GroupVersionKind().Kind == strings.Title(SplitGVK[1]) && resource.GroupVersionKind().Version == SplitGVK[0] {
return true
}
} else {
if resource.GroupVersionKind().Group == SplitGVK[0] && resource.GroupVersionKind().Kind == SplitGVK[2] && (resource.GroupVersionKind().Version == SplitGVK[1] || resource.GroupVersionKind().Version == "*") {
if resource.GroupVersionKind().Group == SplitGVK[0] && resource.GroupVersionKind().Kind == strings.Title(SplitGVK[2]) && (resource.GroupVersionKind().Version == SplitGVK[1] || resource.GroupVersionKind().Version == "*") {
Copy link
Member

Choose a reason for hiding this comment

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

@vyankyGH - this solution won't work for resources like ConfigMap, ClusterRole, CronJob, etc. We need to leverage this util GetFormatedKind to match the GVK.

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] GVK format is case sensitive
3 participants