Admission Controller (Webhook) for Akri Configuration(s)

[DazWilkin]

What this PR does / why we need it:

See: #180

Special notes for your reviewer:

Prematurely opening the PR to politely solicit in-flight reviews; the PR is not yet ready to be accepted.

The Webhook container can be built out-of-band:

REPO="ghcr.io/deislabs/akri"
TAGS="v1"

docker build \
--tag=${REPO}:${TAGS} \
--file=./build/containers/Dockerfile.webhook-configuration \
.

You should be able to test it standalone too by generating a self-signed cert:

openssl req \
-x509 \
-newkey rsa:2048 \
-keyout ./webhook.key \
-out ./webhook.crt \
-nodes \
-days 365 \
-subj "/CN=localhost"

Then throwing an Akri Configuration at it:

curl \
  --silent \
  --insecure \
  --cert ./webhook.crt \
  --key ./webhook.key \
  --request POST \
  --header "Content-Type: application/json" \
  --data "@/path/to/an/akri/configuration.json" \
  https://localhost:8443/validate

Issues|Concerns:

• Naming (Ha!) webhook-configuration located in ./webhooks/validating/configuration -- Thoughts?
• I'm unfamiliar with Makefile and Akri's Makefiles are non-trivial -- may I get some help integrating the webhook?
• The ./build/containers/Dockerfile.webhook-configuration.yml` works but it's not Akri-style
• The ./github/workflows/build-webhook-configuration.yml` does not work and it's not Akri-style
• Should I incorporate the webhook into ./version.sh? I think so. It is more bound to CRD versioning than Akri, but...
• Kubernetes and the Webhook require certs that can't be generated by Helm; there is a pre-Helm step if the Webhook is enabled
• Should I consider pulling the Webhook's Helm template into a separate Chart?

gripes

• There is a considerable amount of infrastructural integration needed for this Webhook (GitHub Actions, Dockerfiles, Helm changes, more...) and I would like to fix early to avoid hitting you with a huge PR.
• I'm challenged to generalize the repo sufficiently that I can run it from github.com/DazWilkin/akri instead of github.com/deislabs/akri e.g. GitHub Actions, GHCR repos etc. (this is probably my deficiency)

If applicable:

• this PR contains documentation
• this PR contains unit tests
• added code adheres to standard Rust formatting (cargo fmt)
• code builds properly (cargo build)
• code is free of common mistakes (cargo clippy)
• all Akri tests succeed (cargo test)
• inline documentation builds (cargo doc)
• version has been updated appropriately (./version.sh)

[DazWilkin]

-o Dpkg::Options::="--force-overwrite"

Fixed: 2e168fd

[bfjelds]

i think this is probably a rebase and version bump away from being able to merge :)

[DazWilkin]

Yay!! 😁

…

i think this is probably a rebase and version bump away from being able to merge :)

[bfjelds]

it looks like the rust dependency security check is picking up on a couple known vulnerabilities (in serde_yaml and hyper). can you update those? i think we've updated our other code to use hyper=0.13.10 and serde_yaml=0.8.

[DazWilkin]

it looks like the rust dependency security check is picking up on a couple known vulnerabilities (in serde_yaml and hyper). can you update those? i think we've updated our other code to use hyper=0.13.10 and serde_yaml=0.8.

Unfortunately these changes then break a bunch of other things :-(

I'll need to revisit the tooling that auto-generates the rust sources in that repo to see whether it's a trivial fix.

[bfjelds]

Unfortunately these changes then break a bunch of other things :-(

Ugh.

I'll need to revisit the tooling that auto-generates the rust sources in that repo to see whether it's a trivial fix.

Fingers triple crossed.

[DazWilkin]

Unfortunately these changes then break a bunch of other things :-(

Ugh.

I'll need to revisit the tooling that auto-generates the rust sources in that repo to see whether it's a trivial fix.

Fingers triple crossed.

Ok. I'm using OpenAPI Generator to (re)generate Rust sources using the Swagger spec for the Admission API. I originally grabbed the Swagger doscs for the Admission API v1 from here.

Today, when I regenerated the sources, a different set of dependencies is being used (no serde_yaml, hyper). My unit tests all pass so I'm going to submit this and.... hope for the best!

Now (my 'crate' version: v1.1.0):

[package]
name = "openapi"
version = "1.1.0"
authors = ["OpenAPI Generator team and contributors"]
edition = "2018"

[dependencies]
reqwest = { version = "^0.11", default-features = false, features = ["json","multipart"] }
serde = "^1.0"
serde_derive = "^1.0"
serde_json = "^1.0"
url = "1.5"

Was (my 'crate' version: v1.0.0):

[package]
name = "openapi"
version = "1.0.0"
authors = ["OpenAPI Generator team and contributors"]

[dependencies]
serde = "^1.0"
serde_derive = "^1.0"
serde_json = "^1.0"
url = "1.5"
hyper = "~0.11"
serde_yaml = "0.7"
base64 = "~0.7.0"
futures = "0.1.23"

[dev-dependencies]
tokio-core = "*"

[DazWilkin]

Because it's the first time, I just gotta put this here 😄

[DazWilkin]

Thank you for all your patience and feedback! *@bfjelds* approved this pull request.

…

[jiria]

Can we add instructions here on how to regenerate?

[DazWilkin]

Fixed: 438061a

I had this document written for myself.

I didn't add it initially because there was no similar document for the existing run-end-to-end test.

I've also added: Test K3s, Kubernetes (Kubeadm) and MicroK8s.

[jiria]

Curious, how come helm commands do not require sudo, but kubectl do? If possible, would rather not call sudo from within the script so as not to result in a prompt.

[bfjelds]

this is following the example of the existing end-to-end test code. i'm not sure i can answer the why, but from trial-and-error, sudo seemed to be needed for kubectl.

maybe we need to call the python test scripts with a sudo command so that the scripts themselves don't need to. maybe this can be part of the test cleanup issue: #240

[bfjelds]

woo hoo!! thanks @DazWilkin!!!

[DazWilkin]

Post Mortem

TL;DR I recommend smaller (faster) PRs with better automation of toil. The following is provided in the hope that it adds to Akri's success. Please don't take any of what follows as criticism. If you see criticism herein, it is my fault of poorly explaining myself.

---

Recommendations

• Encourage smaller PRs
• Automate toil (e.g. Makefiles, GitHub Actions, End-to-End tests, boilerplate) End to end tests are ridiculously complicated and maybe impossible to run locally #240
• Improve documentation
• Create Posts Mortem as part of doing business; consider reviewing these during Akri community meetings

Background

The Webhook was created to automate validation of Akri Configurations (#180). It was original written in Golang and then rewritten in Rust to be more sympathetic to Akri's codebase. This author had not written a Kubernetes (Admission) Webhook, documentation (Golang|Rust) was scant, the API is moving from v1beta to v1, and TLS provisioning 'evolved' with the fast-moving Kubernetes API.

The (Rust) Webhook is ~250 lines of code and ~500 lines of unit tests. 80 lines (check) provided by Deislabs (@bfjelds). The remainder is boilerplate code to create a TLS-supporting httpd that accepts AdmissionReview(s), extracts Akri Configurations, checks them and returns AdmissionReview(s) with a valid (true) or invalid (false).

AdmissionReview, messages are sent by Kubernetes API Server to user-defined Webhook servers. For this reason they are considered out-of-band to the core Kubernetes Swagger|OpenAPI spec (issue) and not included in e.g. kube. As a result, a detour was needed to generate these messages as Rust sources for this project (link).

Development of the (Golang) Webhook began (link mid-December 2020 and a provisional PR of the Rust implementation was submitted mid-January 2021 (link).

PR #206 comprises changes to 22 files, 83 commits and 41 days.

I estimate (generously) the effort represents <<50% core function code and >>50% toil.

The advantage of requiring everything in one PR (code, documentation, tests, CI) is clear. However, large(r) PRs are burdensome for all (many thanks to @bfjelds and @kate-goldenring for the ongoing, patient and valuable feedback, thanks guys!). Developers may be(come) discouraged by these. They suffer from "bit rot".

Supporting code (Makefiles, GitHub Actions, Dockerfiles) is error-prone, uses a myriad (possibly inextricably) and is less-well documented. These processes would benefit from more transparent documentation and proposals for automation. As part of the preliminary proposal|acceptance process, consideration could be given to whether the developer or other developers partition the work of core code development from the supporting code development. Decisions could then be made on these resource allocations as to whether to proceed. For example a protocol developer who's willing primarily to contribute protocol code may be encouraged to find a companion developer to develop the CI code.

To avoid some of the above hurdles, documentation could be considered that outlines the steps necessary to develop components for Akri, both the core functionality and expectations around testing, documentation and automation.

The Akri team may wish to consider "Rules of Engagement" that outline the project's needs|expectations. Are in-repo protocols desired vs. (thanks to @kate-goldenring gRPC adaptor) encouragement of external, community-contributed protocols preferred? Are the guidelines different for contributions to the core repro vs. acceptance criteria for community-contributed? Are protocols preferred to e.g. Webhooks? What are the project's greatest needs and priorities? What expectations are there for keeping Rust (crate) dependencies current and maintaining currency with Kubernetes versions?

[bfjelds]

This is super helpful feedback and very much appreciated. We would very much like to reduce the toil to as close to 0 as possible and have useful documentation for the rest!! Any critique that helps us towards that is 1000% requested.

I, and I suspect the team as a whole, agree entirely with each of the recommendations.