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

Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds, namespaceSelector, objectSelector and cert files locations #6260

Merged
merged 1 commit into from Oct 8, 2020

Conversation

kolorful
Copy link
Contributor

@kolorful kolorful commented Oct 1, 2020

This PR allows Helm Chart to customize admission webhook's annotations, timeoutSeconds, namespaceSelector, objectSelector and cert files locations.

What this PR does / why we need it:

This PR adds following capabilities to the ingress-nginx Helm Chart:

  • Allow users to set annotations, timeoutSeconds, namespaceSelector and objectSelector on ValidatingWebhookConfiguration
  • Allow users to customize values of --validating-webhook-certificate and --validating-webhook-key in Deployment and DaemontSet.
  • Fixed a duplicated command line argument --namespace $NAMESPACE in generate-deploy-scripts.sh

Why:

  • We use cert-manager to manage webhook certs and it relies on an annotation on the ValidatingWebhookConfiguration resource, which currently cannot be set.
  • The other improvement is making timeoutSeconds customizable, since it's a super useful setting.
  • I also made namespaceSelector and objectSelector configurable due to it could be handy in big clusters.
  • The last improvement is that during testing, I found there is a a duplicated command line argument --namespace $NAMESPACE in generate-deploy-scripts.sh, so I just went ahead and fixed it.
    cat << EOF | helm template $RELEASE_NAME ${DIR}/charts/ingress-nginx --namespace $NAMESPACE --namespace $NAMESPACE --values - | $DIR/hack/add-namespace.py $NAMESPACE > ${OUTPUT_FILE}
  • In general, it's nicer to be able to customize resource as much as possible.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

  • I ran the e2e test.
  • I ran the e2e test for helm-charts.
  • I ran generate-deploy-scripts.sh to make sure generated static yamls has the correct webhook related values.
  • I also ran helm install -n ingress-nginx-internal --dry-run ingress-nginx ./charts/ingress-nginx/ before and after my change with different sets values and verified via the diff results that annotations and timeoutSeconds are added correctly when specified and are not added when not specified.

With new value like:

  admissionWebhooks:
    annotations:
      foo: bar
      hello: world
    enabled: true
    failurePolicy: Fail
    timeoutSeconds: 30
    port: 8443
    certificate: "/usr/local/certificates/tls.crt"
    key: "/usr/local/certificates/tls.key"
    namespaceSelector:
      matchLabels:
        needValidation: true
    objectSelector: 
      matchExpressions:
      - key: owner
        operator: NotIn
        values:
        - noValidation

The new outputs are:

  1. in ValidatingWebhookConfiguration resource
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
  annotations: 
    foo: bar
    hello: world
  labels:
    helm.sh/chart: ingress-nginx-3.5.0
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/instance: ingress-nginx
    app.kubernetes.io/version: "0.40.2"
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: admission-webhook
  name: ingress-nginx-admission
webhooks:
  - name: validate.nginx.ingress.kubernetes.io
    rules:
      - apiGroups:
          - networking.k8s.io
        apiVersions:
          - v1beta1
          - v1
        operations:
          - CREATE
          - UPDATE
        resources:
          - ingresses
    failurePolicy: Fail
    sideEffects: None
    admissionReviewVersions:
      - v1
    clientConfig:
      service:
        namespace: ingress-nginx-internal
        name: ingress-nginx-controller-admission
        path: /networking/v1beta1/ingresses
    timeoutSeconds: 30
    namespaceSelector:
      matchLabels:
        needValidation: true
    objectSelector: 
      matchExpressions:
      - key: owner
        operator: NotIn
        values:
        - noValidation
  1. In Deployment or DaemonSet resource
         args:
            ...
            - --validating-webhook-certificate=/usr/local/certificates/tls.crt
            - --validating-webhook-key=/usr/local/certificates/tls.key

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 1, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @kolorful!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @kolorful. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2020
@aledbf
Copy link
Member

aledbf commented Oct 1, 2020

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2020
@aledbf aledbf added this to In Progress in 0.41.0 Oct 1, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 2, 2020
@kolorful kolorful changed the title Allow ingress-nginx Helm Chart to set annotations and timeoutSeconds on ValidatingWebhookConfiguration Allow ingress-nginx Helm Chart to set annotations and timeoutSeconds on ValidatingWebhookConfiguration and webhook certs on Deployment/DaemonSet Oct 2, 2020
@kolorful kolorful changed the title Allow ingress-nginx Helm Chart to set annotations and timeoutSeconds on ValidatingWebhookConfiguration and webhook certs on Deployment/DaemonSet Allow ingress-nginx Helm Chart to customize admission webhook's annotations, timeoutSeconds and cert files location Oct 2, 2020
@kolorful kolorful changed the title Allow ingress-nginx Helm Chart to customize admission webhook's annotations, timeoutSeconds and cert files location Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds and cert files location Oct 2, 2020
@kolorful kolorful changed the title Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds and cert files location Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds and cert files locations Oct 2, 2020
@kolorful
Copy link
Contributor Author

kolorful commented Oct 5, 2020

Hi @ChiefAlexander and @ElvinEfendi, it would be very appreciated if you could take a look at this code change :)

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2020
@kolorful kolorful changed the title Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds and cert files locations Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds, namespaceSelector, objectSelectorand cert files locations Oct 5, 2020
@kolorful kolorful changed the title Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds, namespaceSelector, objectSelectorand cert files locations Allow Helm Chart to customize admission webhook's annotations, timeoutSeconds, namespaceSelector, objectSelector and cert files locations Oct 5, 2020
@kolorful
Copy link
Contributor Author

kolorful commented Oct 5, 2020

/assign @ChiefAlexander

@ChiefAlexander
Copy link
Contributor

Why'd you put this onhold @aledbf?

@aledbf
Copy link
Member

aledbf commented Oct 5, 2020

Why'd you put this onhold @aledbf?

Because when I am preparing a release (last Friday) and for a couple of days (just in case a new version is required) all PRs ready to merge are put on hold.

@ChiefAlexander
Copy link
Contributor

Why'd you put this onhold @aledbf?

Because when I am preparing a release (last Friday) and for a couple of days (just in case a new version is required) all PRs ready to merge are put on hold.

Interesting. If I lgtm would that override your hold?

@ChiefAlexander
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 5, 2020
@aledbf
Copy link
Member

aledbf commented Oct 5, 2020

Interesting. If I lgtm would that override your hold?

No, tide waits until all the conditions are met

@ChiefAlexander
Copy link
Contributor

/lgtm
Thanks for the PR!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2020
@kolorful
Copy link
Contributor Author

kolorful commented Oct 5, 2020

Thank you!

@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

@kolorful, please increase the version of the chart. Otherwise, the change will not publish a new release. Thanks

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2020
@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

@kolorful
Copy link
Contributor Author

kolorful commented Oct 8, 2020

@aledbf rebased master, bumped chart version to 3.4.2 and squashed commits.

@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

@aledbf rebased master, bumped chart version to 3.4.2 and squashed commits.

This is a new feature. Please use 3.5.0

…tSeconds, namespaceSelector, objectSelector and cert files locations
@kolorful
Copy link
Contributor Author

kolorful commented Oct 8, 2020

updated to 3.5.0 :)

@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

/lgtm
/approve

@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

@kolorful thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ChiefAlexander, kolorful

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2020
@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9ba5bea into kubernetes:master Oct 8, 2020
@aledbf aledbf moved this from In Progress to done in 0.41.0 Oct 8, 2020
@kolorful
Copy link
Contributor Author

kolorful commented Oct 8, 2020

@aledbf seems like helm chart publishing failed due to Error: GET https://api.github.com/repos/kubernetes/ingress-nginx/releases/tags/ingress-nginx-3.5.0: 404 Not Found, do I need to do anything to fix it?

update: should be resolved in #6299

@aledbf
Copy link
Member

aledbf commented Oct 8, 2020

@kolorful fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants