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

Intercepting "pods/eviction" subresource via validating webhook is not working #110169

Closed
rafaeljesus opened this issue May 23, 2022 · 8 comments
Closed
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@rafaeljesus
Copy link

What happened?

I’ve created a validation webhook server to intercept pods/eviction requests but calling evictions via both, the go SDK client and VPA is not invoking the webhook,

The webhook config is as follows:

webhook.manifest.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations:
    cert-manager.io/inject-ca-from: <namespace>/admissionwebhooks-tls
    meta.helm.sh/release-name: <namespace>-admissionwebhooks
    meta.helm.sh/release-namespace: <namespace>
  name: admissionwebhooks.org.com
webhooks:
- admissionReviewVersions:
  - v1
  - v1beta1
  clientConfig:
    caBundle: <cert>
    service:
      name: admissionwebhooks
      namespace: <namespace>
      path: /intercept-evictions
      port: 443
  failurePolicy: Fail
  matchPolicy: Equivalent
  name: admissionwebhooks.org.com
  namespaceSelector:
    matchLabels:
      name: <namespace>
  objectSelector:
    matchLabels:
      interceptDBSetPodEvictions: "true"
  rules:
  - apiGroups:
    - ""
    apiVersions:
    - v1
    - v1beta1
    operations:
    - CREATE
    resources:
    - pods/eviction
    scope: Namespaced
  sideEffects: None

The namespaceSelector and objectSelector are pointing to the right resources,

I am testing by invoking eviction requests with the golang client, via https://github.com/ueokande/kubectl-evict/blob/d3068ae2e8c4f28051187f18ac75406ddc21c551/pkg/cmd/evict_api.go#L60, and also via VPA, which issues eviction requests,

In addition to the pods/eviction, I also tried a few combinations in the rules.resources, but nothing seems to work:

  • pods/*
  • */eviction
  • eviction
  • pods/*/eviction

If I set "pods/status" my webhook server gets status requests though,

It sounds to me that this is still not working #75193

What did you expect to happen?

I expect my custom validation webhook server to intercept pods eviction requests

How can we reproduce it (as minimally and precisely as possible)?

If you create a validationwebhookconfiguration with:

kind: ValidatingWebhookConfiguration
webhooks:
 - rules:
  - apiGroups:
    - ""
    apiVersions:
    - v1
    - v1beta1
    operations:
    - CREATE
    resources:
    - pods/eviction

and you try to evict pods either with https://github.com/ueokande/kubectl-evict, or any other mechanism like VPA, etc. you should notice the webhook server doesn't get called

Anything else we need to know?

No response

Kubernetes version

Client Version: version.Info{Major:"1", Minor:"21+", GitVersion:"v1.21.9-dispatcher", GitCommit:"2a8027f41d28b788b001389f3091c245cd0a9a60", GitTreeState:"clean", BuildDate:"2022-01-21T20:31:13Z", GoVersion:"go1.16.12", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.10-gke.2000", GitCommit:"0823380786b063c3f71d5e7c76826a972e30550d", GitTreeState:"clean", BuildDate:"2022-03-17T09:22:22Z", GoVersion:"go1.16.14b7", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

GCP

OS version

NAME="Container-Optimized OS"
ID=cos
PRETTY_NAME="Container-Optimized OS from Google"
HOME_URL="https://cloud.google.com/container-optimized-os/docs"
BUG_REPORT_URL="https://cloud.google.com/container-optimized-os/docs/resources/support-policy#contact_us"
GOOGLE_CRASH_ID=Lakitu
GOOGLE_METRICS_PRODUCT_ID=26
KERNEL_COMMIT_ID=b53f300d0adab43140be3ea0a2167e5c7c9af8a6
VERSION=89
VERSION_ID=89
BUILD_ID=16108.604.19

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

@rafaeljesus rafaeljesus added the kind/bug Categorizes issue or PR as related to a bug. label May 23, 2022
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 23, 2022
@rafaeljesus
Copy link
Author

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 23, 2022
@sftim
Copy link
Contributor

sftim commented May 23, 2022

Can you use validating webhooks with subresources? If not, this might be more of a feature request against the docs.

@rafaeljesus
Copy link
Author

rafaeljesus commented May 23, 2022

Can you use validating webhooks with subresources? If not, this might be more of a feature request against the docs.

@sftim AFAICT, this appears to only (or mostly work) for the pods/status subresource, for example, I changed the resources: ["pods/status"] and my webhook received a request for the UPDATE /status operations, and changing it to "pods/*" also worked for the status subresouce, but if I tried to portforward, and evict the pod my wehbhook is not receiving any request

list of pods subresource for reference
pods
pods/attach
pods/binding
pods/eviction
pods/exec
pods/log
pods/portforward
pods/proxy
pods/status

@rafaeljesus
Copy link
Author

I want to reiterate that I also tested the rules with the below and still the webhook server didn't received any request

apiGroups: ["*"]
apiVersions: ["*"]
operations: ["*"]
resources:
- pods/eviction

@rafaeljesus
Copy link
Author

I've found the problem, I removed the objectSelector and now my webhook server receives eviction validation requests, again, it's interesting that if I add the pods/status into the resources field and still have the objectSelector field, my webhook server receives validation requests for the status subresource.

as a reference, the pod I am trying to intercept with the same label used in the objectSelector:

apiVersion: v1
kind: Pod
metadata:
  labels:
    app: <appname>
    interceptDBSetPodEvictions: "true"
    ... other fields omitted

as of now, I will only be able to filter by namespaceSelector and within the webhook ignore the objects I don't want

@leilajal
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 24, 2022
@liggitt
Copy link
Member

liggitt commented May 29, 2022

objectSelector selects on the labels of the object sent to the webhook. for the eviction subresource, this is the Eviction object, not the parent Pod object. It is unlikely the eviction request being submitted has the same labels as the parent Pod.

For the /status subresource, the object submitted is the Pod, so the labels are the same

/close

@k8s-ci-robot
Copy link
Contributor

@liggitt: Closing this issue.

In response to this:

objectSelector selects on the labels of the object sent to the webhook. for the eviction subresource, this is the Eviction object, not the parent Pod object. It is unlikely the eviction request being submitted has the same labels as the parent Pod.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 3, 2024
Currently, it is not possible to filter eviction requests
based on pod labels [1][2].

For this reason, the pod eviction admitter has to catch all
eviction requests in the cluster and only handle eviction
requests on virt-launcher pods.

Eviction requests on non virt-launcher pods should be approved.

Since the existing tests have global variables set by
a global BeforeEach function, it is difficult to refactor
in place.

Create a new Describe clause in order to hold the refactored
test.

Add new helper functions.

For additional details please see [3].

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809
[3] kubevirt#11286

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 3, 2024
Currently, it is not possible to filter eviction requests
based on pod labels [1][2].

For this reason, the pod eviction admitter has to catch all
eviction requests in the cluster and only handle eviction
requests on virt-launcher pods.

Eviction requests on non virt-launcher pods should be approved.

Since the existing tests have global variables set by
a global BeforeEach function, it is difficult to refactor
in place.

Create a new Describe clause in order to hold the refactored
test.

Add new helper functions.

For additional details please see [3].

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809
[3] kubevirt#11286

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 3, 2024
Currently, it is not possible to filter eviction requests
based on pod labels [1][2].

For this reason, the pod eviction admitter has to catch all
eviction requests in the cluster and only handle eviction
requests on virt-launcher pods.

Eviction requests on non virt-launcher pods should be approved.

Since the existing tests have global variables set by
a global BeforeEach function, it is difficult to refactor
in place.

Create a new Describe clause in order to hold the refactored
test.

Add new helper functions.

For additional details please see [3].

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809
[3] kubevirt#11286

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 4, 2024
Currently, it is not possible to filter eviction requests
based on pod labels [1][2].

For this reason, the pod eviction admitter has to catch all
eviction requests in the cluster and only handle eviction
requests on virt-launcher pods.

Eviction requests on non virt-launcher pods should be approved.

Since the existing tests have global variables set by
a global BeforeEach function, it is difficult to refactor
in place.

Create a new Describe clause in order to hold the refactored
test.

Add new helper functions.

For additional details please see [3].

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809
[3] kubevirt#11286

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 15, 2024
Currently, it is not possible to filter eviction requests
by pod label [1][2], thus unfortunately the admitter intercepts
all eviction requests in the cluster - including for pods that
are not virt-launchers.

The admitter checks whether an evicted pod is a virt-launcher
by checking the the existence and value of the `kubevirt.io` label.

Extract this logic into a function for better
readability.

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809

Treat the pod as a virt-launcher, only after it was verified as such.

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 30, 2024
Currently, it is not possible to filter eviction requests
by pod label [1][2], thus unfortunately the admitter intercepts
all eviction requests in the cluster - including for pods that
are not virt-launchers.

The admitter checks whether an evicted pod is a virt-launcher
by checking the the existence and value of the `kubevirt.io` label.

Extract this logic into a function for better
readability.

The value of the `kubevirt.io/domain` annotation on
the virt-launcher pod, represents the name of
its controlling VMI.
Rename the `domainName` variable to `vmiName`
in order to better describe its purpose.

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue Apr 30, 2024
Currently, it is not possible to filter eviction requests
by pod label [1][2], thus unfortunately the admitter intercepts
all eviction requests in the cluster - including for pods that
are not virt-launchers.

The admitter checks whether an evicted pod is a virt-launcher
by checking the the existence and value of the `kubevirt.io` label.

Extract this logic into a function for better
readability.

The value of the `kubevirt.io/domain` annotation on
the virt-launcher pod, represents the name of
its controlling VMI.
Rename the `domainName` variable to `vmiName`
in order to better describe its purpose.

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809

Signed-off-by: Orel Misan <omisan@redhat.com>
orelmisan added a commit to orelmisan/kubevirt that referenced this issue May 7, 2024
Currently, it is not possible to filter eviction requests
by pod label [1][2], thus unfortunately the admitter intercepts
all eviction requests in the cluster - including for pods that
are not virt-launchers.

The admitter checks whether an evicted pod is a virt-launcher
by checking the the existence and value of the `kubevirt.io` label.

Rename the `launcher` to `pod` in order to emphesize that it could
be any pod.

Extract this logic into a function for better
readability.

The value of the `kubevirt.io/domain` annotation on
the virt-launcher pod, represents the name of
its controlling VMI.
Rename the `domainName` variable to `vmiName`
in order to better describe its purpose.

[1] kubernetes/kubernetes#110169 (comment)
[2] https://kubernetes.slack.com/archives/C0EG7JC6T/p1707054818877809

Signed-off-by: Orel Misan <omisan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants