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

Jump to bottom

Webhook YAML causes constant config drift #12474

Open
mbrancato opened this issue Jan 4, 2022 · 4 comments
Open

Webhook YAML causes constant config drift #12474

mbrancato opened this issue Jan 4, 2022 · 4 comments
Labels
good first issue help wanted kind/bug triage/accepted
Milestone
Icebox

Comments

@mbrancato
Copy link

@mbrancato mbrancato commented Jan 4, 2022

What version of Knative?

0.25

Also present in the latest code.

Expected Behavior

Applying a resource should put it into a state where it can applied multiple times without making changes to the resource. This is a primary benefit of declarative config with Kubernetes.

Actual Behavior

Certain resources are causing constant config drift, possibly because the manifests are not following the correct structure or the kubernetes API is storing these in a different structure.

Steps to Reproduce the Problem

From the Knative serving manifests, apply the MutatingWebhookConfiguration webhook.domainmapping.serving.knative.dev - it has a .webhooks.rules field like this (array size: 1):

    rules:
      - apiGroups:
          - serving.knative.dev
        apiVersions:
          - v1alpha1
          - v1beta1
        operations:
          - CREATE
          - UPDATE
        scope: "*"
        resources:
          - domainmappings

Read the resulting object, it has a .webhook.rules field like this (array size: 2):

  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: '*'
@mbrancato mbrancato added the kind/bug label Jan 4, 2022
@mbrancato
Copy link
Author

@mbrancato mbrancato commented Jan 5, 2022

The expansion is being performed against apiGroups and apiVersions. I've manually modified the install manifest as a work around, and opened a corresponding issue upstream. kubernetes/kubernetes#107318

@dprotaso
Copy link
Member

@dprotaso dprotaso commented Jan 5, 2022

Applying a resource should put it into a state where it can applied multiple times without making changes to the resource. This is a primary benefit of declarative config with Kubernetes.

Our webhook framework/libs perform this mutation for a few reasons

  1. Managing/rotating the webhook's certificate so we update the clientConfig
  2. Allowing the webhook to be built programmatically (easier for us)

It's because of 2) that you're seeing the mutation - prior our webhooks config had no rules where populated by our controllers but it resulted in a race where Knative resources could be created prior to defaulting & validation. We added the initial set of rules to resolve this - details #7576

We could change the rules to match the later update but it wouldn't address your main concern about these resources not being mutated since

  1. We must be able to update that clientConfig setting given 1) above
  2. We also make use of the AggregatedClusterRoles which have their rules updated by the API server.
  3. Cluster providers will modify your resources - ie Azure/AKS#1771

What I've seen is tools typically have some option to handle merges between what's declared and what's present on the API server - so I'd dig into that.

@dprotaso
Copy link
Member

@dprotaso dprotaso commented Feb 4, 2022

/triage accepted
/good-first-issue

I'm going to accept this issue - at a minimum we can update our rules to match what the webhook generates to avoid this diff.

But with the caveat end-users should handle merges.

@knative-prow-robot
Copy link
Contributor

@knative-prow-robot knative-prow-robot commented Feb 4, 2022

@dprotaso:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/triage accepted
/good-first-issue

I'm going to accept this issue - at a minimum we can update our rules to match what the webhook generates to avoid this diff.

But with the caveat end-users should handle merges.

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.

@knative-prow-robot knative-prow-robot added triage/accepted good first issue help wanted labels Feb 4, 2022
@dprotaso dprotaso added this to the Icebox milestone Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue help wanted kind/bug triage/accepted
Projects
None yet
Milestone
Icebox
Development

No branches or pull requests
3 participants
@dprotaso @mbrancato @knative-prow-robot