Add validating admission webhook #129

przemeklal · 2018-08-22T14:42:12Z

Add validating admission webhook:

Add validating admission webhook HTTP server application
Handle incoming AdmissionReview requests and validate their correctness, handle errors if any
Validate Network Attachment Definition objects
Send AdmissionReview response with allowed/denied decision and its reason
In case of any other errors (malformed HTTP request, empty body, etc.) send proper HTTP error code
Use TLS encryption
Add some basic unit tests for Network Attachment Definition objects validation
Build Docker image with webhook application

Add deployment files for validating admission webhook:

Add script for automated certificates and secret generation
Add deployment, service and webhook configuration specification files

Add documentation for validating admission webhook.

Fixes #119.

Signed-off-by: Przemyslaw Lal przemyslawx.lal@intel.com

* Add validating admission webhook HTTP server application * Handle incoming AdmissionReview requests and validate their correctness, handle errors if any * Validate Network Attachment Definition objects * Send AdmissionReview response with allowed/denied decision and its reason * In case of any other errors (malformed HTTP request, empty body, etc.) send proper HTTP error code * Use TLS encryption * Add some basic unit tests for Network Attachment Definition objects validation * Build Docker image with webhook application Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

* Add script for automated certtificates and secret generation * Add pod, service and webhook configuration specification files Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

coveralls · 2018-08-22T14:48:34Z

Pull Request Test Coverage Report for Build 421

78 of 140 (55.71%) changed or added relevant lines in 1 file are covered.
236 unchanged lines in 3 files lost coverage.
Overall coverage increased (+2.7%) to 47.721%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
webhook/webhook.go	78	140	55.71%

Files with Coverage Reduction	New Missed Lines	%
types/conf.go	57	27.71%
multus/multus.go	75	52.87%
k8sclient/k8sclient.go	104	48.86%

Totals
Change from base Build 345:	2.7%
Covered Lines:	492
Relevant Lines:	1031

💛 - Coveralls

rkamudhan

Thank a lot @przemek-lal . I will test it :)

rkamudhan

Deployment comments: likely to flexible with all env

webhook/Dockerfile

rkamudhan

Added comments

rkamudhan · 2018-08-28T09:12:04Z

webhook/build

@@ -0,0 +1 @@
+docker build -t multus-webhook .


Include the proxy build args here

docker build --build-arg http_proxy=$(http_proxy) \ --build-arg HTTP_PROXY=$(HTTP_PROXY) \ --build-arg https_proxy=$(https_proxy) \ --build-arg HTTPS_PROXY=$(HTTPS_PROXY) \ --build-arg no_proxy=$(no_proxy) \ --build-arg NO_PROXY=$(NO_PROXY) \

change it something like this

#!/usr/bin/env bash set -e docker build --build-arg http_proxy=${http_proxy} \ --build-arg HTTP_PROXY=${HTTP_PROXY} \ --build-arg https_proxy=${https_proxy} \ --build-arg HTTPS_PROXY=${HTTPS_PROXY} \ --build-arg no_proxy=${no_proxy} \ --build-arg NO_PROXY=${NO_PROXY} -t multus-webhook .

rkamudhan

Please change the build script to fit all the deployment env

rkamudhan

Include proxy env

rkamudhan · 2018-08-28T09:56:53Z

webhook/build

@@ -0,0 +1 @@
+docker build -t multus-webhook .


change it something like this

#!/usr/bin/env bash set -e docker build --build-arg http_proxy=${http_proxy} \ --build-arg HTTP_PROXY=${HTTP_PROXY} \ --build-arg https_proxy=${https_proxy} \ --build-arg HTTPS_PROXY=${HTTPS_PROXY} \ --build-arg no_proxy=${no_proxy} \ --build-arg NO_PROXY=${NO_PROXY} -t multus-webhook .

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

przemeklal · 2018-08-28T11:17:18Z

okay, proxy env variables added

rkamudhan · 2018-10-11T09:24:11Z

Regarding the ./certs.sh

I am getting the following error:

 ./certs.sh
Generating private RSA key...
Generating CSR configuration file...
Sending CSR to Kubernetes...
certificatesigningrequest.certificates.k8s.io/multus-webhook-service.default created
Approving CSR...
certificatesigningrequest.certificates.k8s.io/multus-webhook-service.default approved
Waiting for the certificate to be issued..................
Error: certificate not issued. Verify that the API for signing certificates is enabled.

przemeklal · 2018-10-11T09:35:11Z

Error message says it all:
Error: certificate not issued. Verify that the API for signing certificates is enabled.

As stated in the Manage TLS Certificates in a Cluster guide, in order to sign certificate with Kubernetes CA you need to pass the --cluster-signing-cert-file and --cluster-signing-key-file parameters to the controller manager with paths to your Certificate Authority’s keypair.

przemeklal · 2018-10-11T09:38:10Z

deployment/webhook/pod.yaml

+# limitations under the License.
+
+apiVersion: v1
+kind: Pod


This should be a Deployment instead of a Pod. I'll update the PR.

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

rkamudhan · 2018-10-11T11:53:42Z

@przemek-lal Thanks . I will check these option in my controller. Could you please update the readme file on this. I believe most of the developers, are not aware of these items.

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

przemeklal · 2018-10-11T12:37:43Z

Okay, I made a couple of updates:

moved the webhook app from pod to a deployment (should improve availability after reboots, Docker restarts, K8s nodes going down etc.)
explicitly mentioned the K8s controller manager arguments which need to be configured in order to enable TLS certs management in a cluster in the webhook doc
added more tests to bring up coverage so coveralls doesn't fail and beyond that some minor error handling improvements

rkamudhan · 2018-10-12T15:00:33Z

In my set-up, I am getting only this log, and multus webhook is allowing the invalid net-attach-def

# kubectl logs -l app=multus-webhook
2018-10-12T14:53:49Z [debug] Starting Multus webhook server

Will sync up to get the more details.

dougbtv · 2018-10-19T18:25:21Z

Looking into a fix for this, but, a few things...

Firstly, If I run the ./certs.sh as a regular user, where my root user doesn't have kubernetes access... I'm presented with the following error:

[centos@kube-master-1 webhook]$ ./certs.sh
Generating private RSA key...
Generating CSR configuration file...
Sending CSR to Kubernetes...
certificatesigningrequest.certificates.k8s.io/multus-webhook-service.default created
Approving CSR...
certificatesigningrequest.certificates.k8s.io/multus-webhook-service.default approved
Waiting for the certificate to be issued...
Certificate issued succesfully.
Creating secret...
Error from server (NotFound): secrets "multus-webhook-secret" not found
secret/multus-webhook-secret created
Patching configuration file with certificate...
./certs.sh: line 91: configuration.yaml: Permission denied
File configuration.yaml patched.

Secondarily, I'm wondering if there's a way we can do this without the installation requiring a shell script to be run before we start this up. Looking for a way to automate this process and reduce the steps to have this installed, maybe through a daemonset / init containers / etc.

ghost · 2018-10-22T10:26:24Z

Hey Doug, I think that the TLS bootstrapping could be done in a Job. It would create a private key, sign it with K8s CA, then create a secret and validating admission controller configuration. So there would be literally zero input from the user required apart from creating the job by running kubectl command. Would this make sense? In this case no bash scripts would be needed, so no problems with the permissions you had.

przemeklal · 2018-11-07T15:40:46Z

Hey guys, I just opened a new PR #186 which contains slightly updated validating webhook code and completely changes deployment procedure. It also adds feature for mutating pods basing on the network objects annotations presence, more details in that PR.

dougbtv

Guys, it's time! OK to merge this up, sorry if the delay was on my end.

przemeklal added 3 commits August 22, 2018 15:36

Add deployment files for validating admission webhook

282b1e5

* Add script for automated certtificates and secret generation * Add pod, service and webhook configuration specification files Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

Add documentation for validating admission webhook

b90a9b8

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

przemeklal added the enhancement label Aug 22, 2018

przemeklal requested a review from rkamudhan August 22, 2018 14:44

rkamudhan requested review from dougbtv and s1061123 August 22, 2018 15:29

rkamudhan reviewed Aug 22, 2018

View reviewed changes

rkamudhan reviewed Aug 28, 2018

View reviewed changes

webhook/Dockerfile Show resolved Hide resolved

rkamudhan reviewed Aug 28, 2018

View reviewed changes

rkamudhan suggested changes Aug 28, 2018

View reviewed changes

Add proxy env variables to docker build script

096b463

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

rkamudhan approved these changes Oct 11, 2018

View reviewed changes

przemeklal commented Oct 11, 2018

View reviewed changes

run webhook as a deployment

7ec612b

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

przemeklal added 4 commits October 11, 2018 13:05

improve error handling in webhook

cc4fa7b

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

add more webhook tests

0851385

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

webhook documentation updates

e8ca145

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

fix indentation

3b21ad5

Signed-off-by: Przemyslaw Lal <przemyslawx.lal@intel.com>

rkamudhan added the release-v3 PRs to make it in release branch label Oct 18, 2018

dougbtv approved these changes Nov 12, 2018

View reviewed changes

rkamudhan approved these changes Nov 12, 2018

View reviewed changes

rkamudhan merged commit bf89318 into k8snetworkplumbingwg:master Nov 12, 2018

przemeklal mentioned this pull request Dec 10, 2018

Add validating admission controller k8snetworkplumbingwg/net-attach-def-admission-controller#1

Merged

dougbtv removed the release-v3 PRs to make it in release branch label Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validating admission webhook #129

Add validating admission webhook #129

przemeklal commented Aug 22, 2018 •

edited

Loading

coveralls commented Aug 22, 2018 •

edited

Loading

rkamudhan left a comment •

edited

Loading

rkamudhan left a comment

rkamudhan left a comment

rkamudhan Aug 28, 2018

rkamudhan Aug 28, 2018

rkamudhan left a comment

rkamudhan left a comment

rkamudhan Aug 28, 2018

przemeklal commented Aug 28, 2018

rkamudhan commented Oct 11, 2018

przemeklal commented Oct 11, 2018

przemeklal Oct 11, 2018

rkamudhan commented Oct 11, 2018

przemeklal commented Oct 11, 2018

rkamudhan commented Oct 12, 2018 •

edited

Loading

dougbtv commented Oct 19, 2018

ghost commented Oct 22, 2018

przemeklal commented Nov 7, 2018

dougbtv left a comment

Add validating admission webhook #129

Add validating admission webhook #129

Conversation

przemeklal commented Aug 22, 2018 • edited Loading

coveralls commented Aug 22, 2018 • edited Loading

Pull Request Test Coverage Report for Build 421

💛 - Coveralls

rkamudhan left a comment • edited Loading

Choose a reason for hiding this comment

rkamudhan left a comment

Choose a reason for hiding this comment

rkamudhan left a comment

Choose a reason for hiding this comment

rkamudhan Aug 28, 2018

Choose a reason for hiding this comment

rkamudhan Aug 28, 2018

Choose a reason for hiding this comment

rkamudhan left a comment

Choose a reason for hiding this comment

rkamudhan left a comment

Choose a reason for hiding this comment

rkamudhan Aug 28, 2018

Choose a reason for hiding this comment

przemeklal commented Aug 28, 2018

rkamudhan commented Oct 11, 2018

przemeklal commented Oct 11, 2018

przemeklal Oct 11, 2018

Choose a reason for hiding this comment

rkamudhan commented Oct 11, 2018

przemeklal commented Oct 11, 2018

rkamudhan commented Oct 12, 2018 • edited Loading

dougbtv commented Oct 19, 2018

ghost commented Oct 22, 2018

przemeklal commented Nov 7, 2018

dougbtv left a comment

Choose a reason for hiding this comment

przemeklal commented Aug 22, 2018 •

edited

Loading

coveralls commented Aug 22, 2018 •

edited

Loading

rkamudhan left a comment •

edited

Loading

rkamudhan commented Oct 12, 2018 •

edited

Loading