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

Update mutating webhook API Version #408

Merged
merged 4 commits into from
Dec 7, 2020

Conversation

lawliet89
Copy link
Contributor

@lawliet89 lawliet89 commented Oct 21, 2020

admissionregistration.k8s.io/v1beta1 is deprecated since 1.16 and will be removed in 1.19.

This PR will set the right API Version if it is supported.

However, there is a change in behaviour from v1beta1 to v1. Specifically, the default (unspecified) failurePolicy changes from Ignore to Fail. Should we set the default in values.yaml to Ignore to preserve the previous behaviour? Or this can be labelled a breaking change?

@jasonodonnell
Copy link
Contributor

@lawliet89 Thanks! I think we should set it to Ignore in values by default since that's the expected behavior today.

@lawliet89
Copy link
Contributor Author

lawliet89 commented Oct 22, 2020

@jasonodonnell I have set it to Ignore by default.

I also tried adding tests to see if we can test if the apiVersion is set correctly. helm template has the --api-versions flag but it seems to be an additive operation.

$ helm template       --show-only templates/injector-mutating-webhook.yaml        --set 'injector.enabled=true'       --namespace foo --api-versions "admissionregistration.k8s.io/v1beta1"       .
---
# Source: vault/templates/injector-mutating-webhook.yaml
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  name: RELEASE-NAME-vault-agent-injector-cfg
  labels:
    app.kubernetes.io/name: vault-agent-injector
    app.kubernetes.io/instance: RELEASE-NAME
    app.kubernetes.io/managed-by: Helm
webhooks:
  - name: vault.hashicorp.com
    clientConfig:
      service:
        name: RELEASE-NAME-vault-agent-injector-svc
        namespace: foo
        path: "/mutate"
      caBundle: ""
    rules:
      - operations: ["CREATE", "UPDATE"]
        apiGroups: [""]
        apiVersions: ["v1"]
        resources: ["pods"]
    failurePolicy: Ignore

@lawliet89
Copy link
Contributor Author

I have also added new fields that are required for v1 as mentioned in hashicorp/consul-helm#658 (comment)

@lawliet89
Copy link
Contributor Author

@jasonodonnell Could you review this again please?

@jasonodonnell
Copy link
Contributor

@lawliet89 looks good but I'm wondering if vault-k8s also needs to be updated? auto-tls updates the caBundle when certificates change: https://github.com/hashicorp/vault-k8s/blob/master/agent-inject/handler.go#L81-L82

@jasonodonnell
Copy link
Contributor

@lawliet89 I asked a similar question here: hashicorp/vault-k8s#168

@lawliet89
Copy link
Contributor Author

I am not too familiar with the kubernetes api library. I'm not sure if a v1 api library function can work with v1beta1 endpoint. It's not quite the same change as this PR imo.

@jasonodonnell jasonodonnell self-requested a review December 7, 2020 15:16
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

LGTM, tested out fine here.

@jasonodonnell jasonodonnell merged commit 94adad8 into hashicorp:master Dec 7, 2020
@tvoran tvoran mentioned this pull request Jan 5, 2021
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