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

Admission registration performs webhook rules expansion modifying object on apply #107318

Closed
mbrancato opened this issue Jan 5, 2022 · 12 comments
Labels
kind/support Categorizes issue or PR as a support question. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. 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. triage/needs-information Indicates an issue needs more information in order to work on it. triage/not-reproducible Indicates an issue can not be reproduced as described.

Comments

@mbrancato
Copy link

mbrancato commented Jan 5, 2022

What happened?

When applying a MutatingWebhookConfiguration, the .webhooks[].rules[] array is being expanded by the .webhooks[].rules[].apiVersions[] values. This is causing configuration drift as the object that is in the manifest is different than the object stored by Kubernetes.

This results in the object always being modified and tools like Flux always think the object has changed:

% kubectl apply -f example.yaml
mutatingwebhookconfiguration.admissionregistration.k8s.io/webhook.domainmapping.serving.knative.dev configured
% kubectl apply -f example.yaml
mutatingwebhookconfiguration.admissionregistration.k8s.io/webhook.domainmapping.serving.knative.dev configured

So applying a config like this:

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration

webhooks:
  - admissionReviewVersions: [ "v1", "v1beta1" ]
    clientConfig:
      service:
        name: domainmapping-webhook
        namespace: knative-serving
    failurePolicy: Fail
    sideEffects: None
    name: webhook.domainmapping.serving.knative.dev
    timeoutSeconds: 10
    rules:
      - apiGroups:
          - serving.knative.dev
        apiVersions:
          - v1alpha1
          - v1beta1
        operations:
          - CREATE
          - UPDATE
        scope: "*"
        resources:
          - domainmappings

Then reading the object back, results in this:

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
webhooks:
  - rules:
      - apiGroups:
          - serving.knative.dev
        apiVersions:
          - v1alpha1
        operations:
          - CREATE
          - UPDATE
        resources:
          - domainmappings
          - domainmappings/status
        scope: '*'
      - apiGroups:
          - serving.knative.dev
        apiVersions:
          - v1beta1
        operations:
          - CREATE
          - UPDATE
        resources:
          - domainmappings
          - domainmappings/status
        scope: '*'

What did you expect to happen?

The object may be modified, fields added, but defined fields should either stay the same structure or fail validation at apply.

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

Apply an object like this:

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration

webhooks:
  - admissionReviewVersions: [ "v1", "v1beta1" ]
    clientConfig:
      service:
        name: domainmapping-webhook
        namespace: knative-serving
    failurePolicy: Fail
    sideEffects: None
    name: webhook.domainmapping.serving.knative.dev
    timeoutSeconds: 10
    rules:
      - apiGroups:
          - serving.knative.dev
        apiVersions:
          - v1alpha1
          - v1beta1
        operations:
          - CREATE
          - UPDATE
        scope: "*"
        resources:
          - domainmappings

Full example: https://github.com/knative/serving/blob/92b5a121d19ca0d9f4aa0c73da3b20e30a5e8a62/config/core/webhooks/domainmapping-defaulting.yaml

Anything else we need to know?

No response

Kubernetes version

Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.5", GitCommit:"aea7bbadd2fc0cd689de94a54e5b7b758869d691", GitTreeState:"clean", BuildDate:"2021-09-15T21:10:45Z", GoVersion:"go1.16.8", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"19+", GitVersion:"v1.19.14-gke.1900", GitCommit:"abc4e63ae76afef74b341d2dba1892471781604f", GitTreeState:"clean", BuildDate:"2021-09-07T09:21:04Z", GoVersion:"go1.15.15b5", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

GKE

OS version

Install tools

Container runtime (CRI) and and version (if applicable)

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

@mbrancato mbrancato added the kind/bug Categorizes issue or PR as related to a bug. label Jan 5, 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 Jan 5, 2022
@mbrancato
Copy link
Author

After some testing, this is also expanding by the .webhooks[].rules[].apiGroups[] and when .webhooks[].rules[].apiVersions[] is the wildcard "*" - it is expanding the wildcard into the actual versions and not storing the wildcard in the object config.

So if there are 4 values in apiGroups and 2 values in apiVersions, the single rule will turn into 8 rules.
Additionally, the wildcard is used, it will be used and then expanded as well.

@pacoxu
Copy link
Member

pacoxu commented Jan 5, 2022

/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 Jan 5, 2022
@liggitt
Copy link
Member

liggitt commented Jan 5, 2022

I can't reproduce this. After applying the webhook in the description, I see this output when fetching it:

...
webhooks:
- admissionReviewVersions:
  - v1
  - v1beta1
  clientConfig:
    service:
      name: domainmapping-webhook
      namespace: knative-serving
      port: 443
  failurePolicy: Fail
  matchPolicy: Equivalent
  name: webhook.domainmapping.serving.knative.dev
  namespaceSelector: {}
  objectSelector: {}
  reinvocationPolicy: Never
  rules:
  - apiGroups:
    - serving.knative.dev
    apiVersions:
    - v1alpha1
    - v1beta1
    operations:
    - CREATE
    - UPDATE
    resources:
    - domainmappings
    scope: '*'
  sideEffects: None
  timeoutSeconds: 10

@liggitt liggitt added triage/not-reproducible Indicates an issue can not be reproduced as described. kind/support Categorizes issue or PR as a support question. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jan 5, 2022
@liggitt
Copy link
Member

liggitt commented Jan 5, 2022

it looks like you're creating an object with this:

        resources:
          - domainmappings

and the resulting object has this:

        resources:
          - domainmappings
          - domainmappings/status

is some other controller reading and rewriting the admission configuration object? if so, is that other controller doing the expansion? there's nothing built into kubernetes that does that expansion in webhook config API objects

@liggitt liggitt added the triage/needs-information Indicates an issue needs more information in order to work on it. label Jan 5, 2022
@mbrancato
Copy link
Author

@liggitt for this to be modified immediately, it would need to be some other mutating administration webhook making the changes? I'm going to look into what others are deployed.

@liggitt
Copy link
Member

liggitt commented Jan 6, 2022

a controller could plausibly observe and quickly modify it before your next get request was issued

@liggitt
Copy link
Member

liggitt commented Jan 6, 2022

starting a watch of the object in a separate tab with kubectl get mutatingwebhookconfigurations -o yaml -w --output-watch-events --field-selector metadata.name=... before creating it would show all modification requests made to the object

@fedebongio
Copy link
Contributor

Thank you Jordan for always being on top of issues!
/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 Jan 6, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 6, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 6, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support Categorizes issue or PR as a support question. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. 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. triage/needs-information Indicates an issue needs more information in order to work on it. triage/not-reproducible Indicates an issue can not be reproduced as described.
Projects
None yet
Development

No branches or pull requests

6 participants