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

Remove mutating webhook that doesn't do anything #4233

Closed

Conversation

garvinp-stripe
Copy link
Contributor

@garvinp-stripe garvinp-stripe commented Jul 7, 2023

Fixes
Remove mutating webhook that isn't doing anything for awsnodetemplates

Description
Currently node template doesn't have any side effects in setDefaults https://github.com/aws/karpenter/blob/86229e15ba129992b5829b14a580bcded0d5f2fb/pkg/apis/v1alpha1/awsnodetemplate_defaults.go#L22. To reduce unnecessary components and calls to Karpenter we should remove this rule.

Move metric relabelings that add additional info to the metrics into the ServiceMonitor
How was this change tested?
Ran helm template locally against the new webhook template. Mutating webhook new configuration looks like: (without defaulting against awsnodetemplates

---
# Source: karpenter/templates/webhooks.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: defaulting.webhook.karpenter.k8s.aws
  labels:
    helm.sh/chart: karpenter-0.28.0
    app.kubernetes.io/name: karpenter
    app.kubernetes.io/instance: karpenter
    app.kubernetes.io/version: "0.28.0"
    app.kubernetes.io/managed-by: Helm
webhooks:
  - name: defaulting.webhook.karpenter.k8s.aws
    admissionReviewVersions: ["v1"]
    clientConfig:
      service:
        name: karpenter
        namespace: karpenter
        port: 8443
    failurePolicy: Fail
    sideEffects: None
    rules:
      - apiGroups:
          - karpenter.sh
        apiVersions:
          - v1alpha5
        resources:
          - provisioners
          - provisioners/status
        operations:
          - CREATE
          - UPDATE
---

make apply on local cluster

Does this change impact docs?
No. I don't believe there are docs regarding this defaulting behavior

[] Yes, PR includes docs updates
[] Yes, issue opened: #
No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@garvinp-stripe garvinp-stripe requested a review from a team as a code owner July 7, 2023 21:56
@garvinp-stripe garvinp-stripe requested a review from tzneal July 7, 2023 21:56
@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 5b26939
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/64ac993df51f4200083d168b

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-7434888518a4d624f78e989de2ee1543e4f48fda. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@github-actions
Copy link
Contributor

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-380523d68d9436f190f1103fed245926f5a29214. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

@garvinp-stripe
Copy link
Contributor Author

it seems like E2E test are failing due to

2023-07-10 19:21:13 [✖]  creating OIDC provider: operation error IAM: CreateOpenIDConnectProvider, https response error StatusCode: 409, RequestID: b92d07d9-b87e-4588-b92f-e9c1e1dd2c92, LimitExceeded: Cannot exceed quota for OpenIdConnectProvidersPerAccount: 100

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@github-actions
Copy link
Contributor

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-5b2693996bd1974f7c710979a8575c8b711bc66d. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

@@ -19,17 +19,6 @@ webhooks:
failurePolicy: Fail
sideEffects: None
rules:
- apiGroups:
Copy link
Contributor

Choose a reason for hiding this comment

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

In order for this to work properly with the Knative reconciliation mechanism, we would need to update the addition of the defaulting webhook for AWSNodeTemplates on this line: https://github.com/aws/karpenter/blob/main/pkg/webhooks/webhooks.go#L44. We can do this but we are planning to remove the entire batch of webhooks in the near-future.

Is this currently blocking anything or causing issues? I'm wondering if we can wait to remove this until we remove all of the webhooks outright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok no worries not blocking. We could wait.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's alright with you, I'm going to close this since we won't take this right now. You can track the removal of the webhooks through: kubernetes-sigs/karpenter#103

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.

2 participants