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

Validating webhook #60

Merged
merged 20 commits into from May 3, 2019

Conversation

Projects
None yet
5 participants
@isutton
Copy link
Contributor

commented Dec 13, 2018

To prevent Kubernetes to accept Shipper objects that do not conform with the schema, this PR introduces a validation webhook exposed so Shipper can review each create and update before it hits etcd.

The "webhook-controller" is started like any other of Shipper's controllers, and also responds to the stop signal that other controllers respect. It is a web-server that has an endpoint configured to ingest AdmissionReview payloads provided by Kubernetes Admission Controller, and return an AdmissionResponse with the validation's result.

Assets

A couple of assets were introduced to help setting it up (shipperctl integration is currently out of the scope of this PR):

  • Makefile

    A Makefile was introduced to help build Shipper's related images (shipper and shipper-state-metrics), also to build webhook-controller's key and signed certificate.

  • hack/webhook/webhook-create-signed-cert.sh

    A helper script that creates a private key for the server, and automation around Kubernetes CertificateSigningRequest system, which allows Kubernetes to sign CSRs automatically. At the end of the process, this script creates a Secret in the shipper-system namespace containing the webhook-controller's private key and signed certificate to be used by the Shipper when starting the HTTPS server.

    For the automatic signing to work in microk8s, the ${SNAP_DATA}/args/kube-controller-manager must be edited, and the following lines should be added:

    --cluster-signing-cert-file=${SNAP_DATA}/certs/ca.crt
    --cluster-signing-key-file=${SNAP_DATA}/certs/ca.key
    

    This configuration enables a Kubernetes subsystem that signs the CSR once an operator has approved it through kubectl certificate approve command.

    The original script was found at https://medium.com/ibm-cloud/diving-into-kubernetes-mutatingadmissionwebhook-6ef3c5695f74.

  • hack/webhook/webhook-patch-ca-bundle.sh

    A helper script that gathers Kubernetes' CA bundle from Kubernetes itself so it can be expanded in kubernetes/validating-webhook-configuration.yaml

  • kubernetes/validating-webhook-configuration.yaml

    Instructions for the Admission Controller to validate Shipper objects both when creating new objects or updating existing ones. By default, its failurePolicy is set to "Fail", meaning that any changes in Shipper objects when Shipper is not being executed will be fail (this seemed like a nice default for me since changes on API master won't modify the current state until Shipper is back online).

  • kubernetes/shipper.service.yaml

    Since Shipper's webhook-controller exposes a HTTPS server on port 9443, a Service is required for Kubernetes to locate Shipper's validating webhook. The object created using this manifest is referred in the Validating Webhook Configuration mentioned earlier on.

  • kubernetes/shipper.deployment.yaml

    This is the equivalent of existing and documented Shipper's deployment, with two particular tweaks: one is informing Kubernetes it exports port 9443, and the other is that a secret containing the signed certificate and server keys, produced by hack/webhook/webhook-create-signed-cert.sh are mounted and available to be used by the webhook-controller.

  • Dockerfile.shipper

    Since microk8s ships with Docker 17.03, and it is currently not advised to use microk8s Docker and another more recent Docker on the same box (at the time of this writing at least), the build argument used to specify the FROM image was removed, thus hard-coding the base image in the Dockerfile itself. Additionally, the output directory when building shipper now is ${PROJECT_ROOT}/build.

  • Dockerfile.shipper-state-metrics

    This asset was renamed from Dockerfile.metrics so it is clear what it does, and also will be useful when streamlining the created Makefile later on or using yet build system. Additionally, the output directory when building shipper now is ${PROJECT_ROOT}/build.

  • ci/package.sh

    Changed shipper-state-metrics input Dockerfile to reflect the renaming mentioned earlier.

Code

  • pkg/webhook/controller/webhook_controller.go

    That is a very simple controller that uses the same interface as others: provide a NewController function, that returns a Controller object that responds to the Run() method.

    The initializeHandlers() method installs the validation handler function in '/validate'.

    The validateHandlerFunc() method is responsible for the simple validation, which is performed by un-marshalling the AdmissionRequest raw object into the equivalent informed by the Kind informed, and in case the decoding has been successfully performed, an AdmissionResponse with its Allowed field set to true is returned.

    The validateHandlerFunc() is wrapped by the adaptHandler() function to be used in HTTP context (adaptHandler() is responsible for extracting the AdmissionReview object from the request and transform the resulting AdmissionResponse into an HTTP response object).

  • cmd/shipper/main.go

    A couple of options have been added into shipper:

    -webhook-cert - path to the TLS certificate for the webhook-controller 
    -webhook-key  - path to the TLS private key for the webhook-controller
    -webhook-addr - address to bind the webhook-controller
    -webhook-port - port to bind the webhook-controller
    

    Additionally, respective fields have been added to the configuration type used by all controllers; added also infra-structure similar to other existing controllers to start the webhook-controller.

  • pkg/apis/shipper/v1alpha1/types.go

    To make Shipper work with the validating webhook just created, it is required to use the omitempty Json tag in ReleaseEnvironment.Sidecars, otherwise the Application Controller itself couldn't create a Release.

Testing the Setup

In order to configure an existing microk8s cluster with this setup, one can use the Makefile like the following:

$ make certs shipper install

Fixes #51.

@isutton isutton changed the title WIP: Validating webhook Validating webhook Dec 14, 2018

Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved Dockerfile.metrics
Show resolved Hide resolved Dockerfile.shipper-state-metrics Outdated
Show resolved Hide resolved ci/package.sh

@isutton isutton requested a review from icanhazbroccoli Dec 14, 2018

Show resolved Hide resolved Makefile Outdated
@parhamdoustdar
Copy link
Contributor

left a comment

Looks good to me and works as intended when I tested it. We need to do some work to integrate this into how shipperctl sets up a cluster, so that our users can enjoy its benefits out of the box. However, IMO that should be in its own PR.

@parhamdoustdar

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@ksurent, we'll change the CI pipelines after this is merged and Shipper is unblocked. If you don't see any more problems, could you please approve?

Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved kubernetes/shipper.deployment.yaml Outdated
Show resolved Hide resolved pkg/controller/webhook/webhook_controller.go Outdated
Show resolved Hide resolved pkg/controller/webhook/webhook_controller.go Outdated
Show resolved Hide resolved pkg/controller/webhook/webhook_controller.go Outdated
Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved Makefile Outdated
Show resolved Hide resolved pkg/controller/webhook/webhook_controller.go Outdated

@parhamdoustdar parhamdoustdar dismissed stale reviews from icanhazbroccoli and themself via 835f60b Apr 15, 2019

@parhamdoustdar parhamdoustdar requested a review from ksurent Apr 15, 2019

isutton added some commits Dec 10, 2018

web-hook: add scaffold for web-hook controller
The web-hook controller is responsible for receiving Admission Review
requests from Kubernetes and checking whether the associated payload
is valid.
web-hook: finalize webhook controller implementation
This commit adds, in addition to the simple validation logic, a couple
of command line options to configure the webhook controller.
web-hook: add extra artifacts to enable web-hooks
A couple of artifacts have been added to the repository:

* Makefile - Because writing shell scripts around is not fun

* webhook-create-signed-cert.sh - Shell script to make Kubernetes sign
  an auto-generated RSA key, and store the signed certificate and server
  keys in a secret. Shamelessly (or proudly) copied from
  https://medium.com/ibm-cloud/diving-into-kubernetes-mutatingadmissionwebhook-6ef3c5695f74

* webhook-patch-ca-bundle.sh - Helper script to gather Kubernetes
  public certificate to add in the ValidatingWebhookConfiguration we're
  creating. Also copied from the same source as above.

* validating-webhook-configuration.yaml - Configures Kubernetes
  Admission Controller to consult Shipper's validation webhook for
  CREATE and UPDATE actions on all Shipper's objects.

* shipper.deployment.yaml - Shipper's Kubernetes deployment file,
  configured to expose port 9443 and to mount the certificates' secret
  to be used by Shipper's "web-hook controller".

* shipper.service.yaml - Shipper's Kubernetes service file, which is
  referred in validating-webhook-configuration.yaml.
types: Make Sidecars omitempty
This is required now that we properly validate (as in, decode the Json
payload to catch serialization errors) all the objects.
dockerfile: use explicit FROM base image
Currently microk8s is bundled with Docker 17.03, and the feature where
ARG is allowed before FROM in a Dockerfile only appeared in 17.05. It is
understood that this version is quite old (at least 1.5 year old) but
to reduce confusion, I'm removing those lines.

Additionally, grabs the binaries from the build directory (currently
compiled binaries are created in the project's root.
gitignore: remove 'shipper' from ignore
Any file or directory called 'shipper' was ignored earlier, and this
ignore line only existed to avoid committing the compiled 'shipper'
program in git. Having this line alone can bring some issues when
trying to modify anything inside a directory 'shipper', which is not
what we want nor expect.

isutton and others added some commits Dec 13, 2018

Makefile and friends: move webhook related files to hack/webhook and …
…Kubernetes files to kubernetes/

The Makefile now has targets to build both shipper and
shipper-state-metrics. Paths of supporting files have been adjusted as
well.
Makefile: place built binaries in the root of the project
We do have some internal processes that are currently using the original
paths, so reverting changes regarding build location.
- Changed the deployment and service and the bash script to work with
  names similar to the ones created by shipperctl. This is meant to be
  a half fix for #74.
- Fixed the issues raised by @icanhazbroccoli
Made Makefile customizable and moved `webhook` out of the `controllers`
directory

I also removed the `install` make target because I didn't find a way
of replacing `bookingcom/shipper:latest` with the specified
environment variable reliably. If people want to install Shipper on
their cluster, they can always modify the file and do `kubectl apply
-f kubernetes/deployment.shipper.yaml`.

I left `webhook` in the list of controllers in `cmd/shipper/main.go`
because it already provides a lot of boilerplate to allow users to
specify if they want to have a webhook running or not. I can't think
of a way to have the ability of enabling/disabling the webhook on the command
line without introducing a lot of repeated code.

Apart from that, I also added the check to `webhook.go` to check if
the error returned from `listenAndServe` was caused by a server close,
or if it was an actual error.

@parhamdoustdar parhamdoustdar force-pushed the isutton/admission-webhooks branch from 835f60b to 51c1f5a Apr 29, 2019

@parhamdoustdar parhamdoustdar force-pushed the isutton/admission-webhooks branch from 35309f2 to 31be55e May 1, 2019

Sidecars had been removed in master, but I had mistakenly left them in
when fixing merge conflicts. This commit fixes that.
Show resolved Hide resolved pkg/webhook/webhook.go
Show resolved Hide resolved kubernetes/validating-webhook-configuration.yaml
Show resolved Hide resolved pkg/webhook/webhook.go Outdated
- Added missing return statements to the webhook handler
- Added "clusters" to the list of resources in the webhook
configuration. Note that this will be dropped later on when we
switch to using `shipperctl` everywhere.
@ksurent

ksurent approved these changes May 2, 2019

@juliogreff juliogreff merged commit 04d2fe3 into master May 3, 2019

2 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details

@parhamdoustdar parhamdoustdar deleted the isutton/admission-webhooks branch May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.