* Add the key of the object to the log context
We don't log _what_ we convert, but only _what type_ it is.
And it's not very useful
So log all the stuff
* issues
* redo
* Add helper function to read port number from env var
* Add check for port == 0
* Add the unit test
* Update error message
Co-Authored-By: savitaashture <sashture@redhat.com>
* Panic when env var is set but invalid
* Rename the func to be PortFromEnv.
* Apply suggestions from code review on error message
Co-authored-by: Victor Agababov <vagababov@gmail.com>
* Join defers
Co-authored-by: savitaashture <sashture@redhat.com>
Co-authored-by: Victor Agababov <vagababov@gmail.com>
* Implement the K8s lifecycle in webhook.
The webhook never properly implemented the Kubernetes SIGTERM/SIGKILL
lifecycle, and doesn't even really support readiness probes today. This
change enables folks to use a block like this on their webhook container:
```yaml
readinessProbe: &probe
periodSeconds: 1
httpGet:
scheme: HTTPS
port: 8443
httpHeaders:
- name: k-kubelet-probe
value: "webhook"
livenessProbe: *probe
```
With this, the webhook won't report as `Ready` until a probe has succeeded,
and when the SIGTERM is received, we will start failing probes for a grace
period (so our Endpoint drops) before shutting down the webhook's HTTP Server.
This was uncovered by running the webhook across 10 replicas in Serving with
the "Goose" (https://github.com/knative/pkg/pull/1316) enabled for the e2e
tests. The failure mode I saw was conversion webhook requests failing across
random tests.
This also moves the Serving probe-detection function into PKG.
* Increase the log level when we start to fail probes
* Wait for go routines to terminate on all paths.
* do not record for empty metric config
* Revert "do not record for empty metric config"
This reverts commit 539a5e4dbb.
* add a comment
* fix typo
* fix tests
* revert
* revert tests
* revert
* fix conflicts
* one more test file
* Remove unused code.
* Use raw strings to avoid escaping.
* Remove unneeded type conversions.
* Preallocate slices where possible.
* Use semantic equality in psbinding reconciler.
* First hack at getting the namespaces of interest to the ps
* Have the webhook label the namespace for inclusion
* Fix unit tests
* Update with feedback
* Add namespace lister, use constants
* Add one more err logging statement
* Create IsInDelete context
* Set up context with WithinDelete
* Test for callback delete
* fix subresource update
* Return oldObj for deletes
* include delete in webhook config
* include delete in unit test
* don't log on delete
* Add new callback pattern to pkg
* include the context
* typo
* Remove the empty instance of unstructured
* initialize the unstructured var
* Eliminate the unneeded pointer
* Pass a pointer to unstructured callback
* Create a validation specific context struct
* Move callback tests to own unit test case
* Switch from converting to decoding
* Update webhook/resourcesemantics/validation/validation.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* don't wrap context and include params
* split validation files
* include 2020 copyright
* include unit test for WithKubeClient
* Don't bother updating copyright date
* Inclue a unit test for panic
* Move dryRun to context
* Include context dry run unit test
* put the request operation in the context
* eliminate circular dep
* move kubeclient test out of context_test
* dont bother iterating callback map
* Callback takes a list of supported verbs
* Remove extra type
* Ensure Callback interface is public
* Alias Operation into validation
* alias Operation right in Webhook
* Update webhook/resourcesemantics/validation/validation_admit.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* Update webhook/resourcesemantics/validation/validation_admit_test.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* Update webhook/resourcesemantics/validation/validation_admit_test.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* Update webhook/resourcesemantics/validation/validation_admit.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* Update webhook/resourcesemantics/validation/validation_admit.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* Update webhook/resourcesemantics/validation/validation_admit_test.go
Co-Authored-By: Victor Agababov <vagababov@gmail.com>
* correct parens
* minor style fixes
* Rename Callback to Func
* Fix build error
* Switch callback to take a list with a factory
* keep descriptive names
* update comment
* Drop pointer, correct comments
* Add a unit test to disallow duplicate verbs
* fix comments, struct{} for set
* switch to variadic arg for NewCallback
Co-authored-by: Victor Agababov <vagababov@gmail.com>
* Start the webhook before informers sync.
Some webhooks (e.g. conversion) are required to list resources, so by delaying those until after informers have synced, we create a deadlock when they run in the same process. This change has two key parts:
1. Start the webhook immediately when our process starts, and issue a callback from sharedmain when the informers have synced.
2. Block `Admit` calls until informers have synced (all conversions are exempt), unless they have been designated by implementing `webhook.StatelessAdmissionController`.
Our built-in admission controllers (defaulting, validation, configmap validation) have all been marked as stateless, the main case where we want to block `Admit` calls is when we require the informer to have synchronized to populate indices for Bindings.
* Add missing err declaration
In general, imo, it's prettier to return things as is, rather than the pattern that was
there.
Also handle one more error case, which was kind of ignored before (probably in practice
impossible, but idk)
* Renews a webhook cert before it expires
* Moved checks out of webhook and into certificate
* Updated error messages and others from review
* Updated error messages and changed time comparison
* Added two tests cases for checking expiration
* Fixed issue with missing "." in webhook.go
* ConversionController implementation
This controller will reconcile target CRDs with the correct
conversion webhook configuration. Specifically, the HTTP path and
CA bundle will be updated.
Additionally, the conversion controller will perform the given
conversions through a hub and spoke model utilizing the
apis.Convertible interface.
* Webhook now can host ConversionControllers
* injection/sharedmain now supports webhook.ConversionControllers
These conversion controllers will be hosted by the webhook that
the sharedmain will start
* support defaulting & include godoc
* Refactor webhook to allow adding conversion support
* pr feedback
* fix memory leak
* We can use mux.Handle
* move admission integration tests to separate file
* add ability to opt-out of bindings
* I guess a year changed...
* had opt-in / opt-out backwards
* add unit test for contexts, fix comment
* clarify comments
* remove Binding from method/const names since already in bindings package
This PR adds facilities to make it easier to create both components of a Binding
over "Pod Spec"-able resources. Rather than rehashing it all here, please look
at `./pkg/webhook/psbinding/README.md` for more details.
I'm splitting this off of another change that needed them, the three changes are:
1. Give the PostConditions callbacks in TableRow access to the Reconciler
resource. It turns out this is incredibly useful to have the `TableRow`
program an admission controller and then test that programming by calling
`Admit()`.
1. Surface the test resources in our webhook "listers", and add the testing
scheme.
1. Change the `objKey` to only use reflection as a fallback. The existing logic
doesn't work properly when a mix of real resources and unstructured.Unstructured
is used for the same resource.
By combining our validation logic into our mutating webhook we were previously allowing for mutating webhooks evaluated after our own to modify our resources into invalid shapes. There are no guarantees around ordering of mutating webhooks (that I could find), so the only way to remedy this properly is to split apart the two into separate webhook configurations:
- `defaulting`: which runs during the mutating admission webhook phase
- `validation`: which runs during the validating admission webhook phase.
The diagram in [this post](https://kubernetes.io/blog/2019/03/21/a-guide-to-kubernetes-admission-controllers/) is very helpful in illustrating the flow of webhooks.
Fixes: https://github.com/knative/pkg/issues/847
* Create a new singleton Reconciler for the webhook secret.
This change creates a new Reconciler (not yet hooked up) that ensures that the
webhook secret has the appropriate shape. I call this a "singleton Reconciler"
because this reconciler exists to effectively reconcile a single resource, which
will be a pattern for the webhook's reconcilers.
* Address linter problems
With this change folks will need to call `webhook.RegisterMetrics()` to register the opencensus view with the metrics for the webhook's StatsReporter. This is needed to avoid having `sharedmain` crashloop the activator due to linking multiple on-`init()` views that register metrics named `request_latencies`.
In general, I believe that we should move away from registering these views via `init()` and more towards the broader K8s MetricsProvider pattern.
GetCertificate allows us to start in TLS mode and dynamically fetch new certificates as they change. This will eventually allow us to decouple the cert creation process from the core webhook logic, and in a subsequent change service this from a secret lister cache.
This builds on https://github.com/knative/pkg/pull/817 and makes further
breaking changes. The options pertinent to each admission controller are
now passed to their respective constructors, which leads to a cleaner
options struct, and better prepares for greater webhook diversity.
* Stop using OwnerRefs for webhook config lifecycle
This changes the model by which we manage the lifecycle of our
`{mutating,validating}webhookconfiguration`, which previously used an owner ref
from the cluster-scoped configuration to the namespace-scoped Deployment. The
new model adds an explicit yaml file for the webhook, which omits the fields
filled in by the deployment as it starts.
A few notable elements of this change:
1. Clear out OwnerReferences explicitly (avoids the linked bug),
2. Periodically reruna `Register()` to ensure our webhook exists,
3. Simplified logic around registration (all we need now is update!).
Related: https://github.com/knative/serving/issues/5845
* Incorporate feedback from @dgerd and a few other nits I noticed.
* Change 'updater' to 'lastModifier' in webhook
Both eventing and serving curretly use lastModifier. Nothing seems to
implement the pkg 'HasSpec' interface yet so this should be a safe
change. This brings the annotation in-line with currentb behavior
Fixes#511
* Remove unnecessary defer
* Change paths in json patches
* Prevent nil StatsReporter for existing webhook package consumers
* Pass StatsReporter by pointer and have tests test constructor
* Make constructor return error instead of panicking
* Move StatsReporter to ControllerOptions to consolidate constructors
* Add metrics to webhook package
Add metricstest package for shared helper functions for testing metrics
* Address PR
* Cleanup
* Fix import paths to fix build issues
* Fix import package path for test file
* Remove unnecessary formatting from error message
* Remove helper function only used once
* Add metric name to all error messages, make checkRowTags testing helper function
* Add common histogram bucket generator function to metrics package
* Fix CheckStatsNotReported check
* Reset metrics before each test so the tests are idempotent
* Make CheckStatsNotReported conditional clearer
* #457 Duck type user annotation logic
* #457 Duck type user annotation logic - tests
* #457 Revert updater annotation key from lastModifier to updater
* #457 Rename HasSpec#GetSpec() to HasSpec#GetUntypedSpec()
* #457 Fix some indentation
* #457 Get group for user info annotations from the request
* #457 Reduce confusuion in webhook testing by using same group
* have simple tests. working on impl.
* strict setting, reflection based.
* ran codegen.
* adding license.
* update based on feedback and merge better.
* getting closer to something simpler assuming shallow reflect.
* adding validation test.
* use the json tag.
* Golang things nil typed pointers are not nil.
* Use real value of reflect invalid.
* add a missing test.
* two methods, one for update, one for single check.
* checkdep is now in apis.
* fix pkg.
* Update apis/deprecated_test.go
Co-Authored-By: n3wscott <32305648+n3wscott@users.noreply.github.com>
* add code clarity.
* include inlined struct objects recursively.
* Update commnets and add a flatten error test for inlined.
This deprecates the `apis.Immutable` and `apis.Annotatable` interfaces,
which were both awkward niche extensions of `apis.Validatable` and
`apis.SetDefaults` for specific contexts that the former set didn't
cover well.
With this change, the expectation is that types that want to check
for immutability will instead access the "baseline" object via the
context from within updates. For example:
```
func (new *Type) Validate(ctx context.Context) *apis.FieldError {
if apis.IsInUpdate(ctx) {
old := apis.GetBaseline(ctx).(*Type)
// Update specific validation based on new and old.
}
}
```
For applying user annotations, the type writer can write:
```
func (new *Type) SetDefaults(ctx context.Context) {
if apis.IsInCreate(ctx) {
ui := apis.GetUserInfo(ctx)
// Set creator annotation from ui
}
if apis.IsInUpdate(ctx) {
ui := apis.GetUserInfo(ctx)
old := apis.GetBaseline(ctx).(*Type)
// Compare old.Spec vs. new.Spec and on changes
// update the "updater" annotation from ui.
}
}
```
One of the key motivations for this refactoring was to enable us
to do more powerful validation in `apis.Validate` beyond the niche
of immutability checking (and without introducing yet-another
one-off niche interface). In the BYO Revision name PoC I abused
`apis.Immutable` to do more arbitrary before/after validation,
which with this can simply be a part of `apis.Validatable`.
See: https://github.com/knative/serving/pull/3562
The general stance on deprecating interfaces such as these will be
to deprecate them in a non-breaking way (via a comment for now). They
will be hollowed out when the functionality is removed from the webhook,
but left in because of diamond dependency problems. In this change
we remove the `apis.Annotatable` functionality and deprecate the
`apis.Immutable` functionality.
Clients of webhook can now decorate the request context with additional metadata
via:
```
ac := &AdmissionController{
... // As before
WithContext: func(ctx context.Context) context.Context {
// logic to attach stuff to ctx
}
}
```
This metadata can then be accessed off of the context in methods like
`SetDefaults` and `Validate` on types registered as webhook handlers.
Fixes: https://github.com/knative/pkg/issues/306
* Modify the webhook to allow the use of duck types.
This change enables us to define a duck type that applies to a whole class of GroupVersionKinds and leverage it to perform generic validation, defaulting, etc.
Fixes: #322
* Fix typo
* Drop webhook logic to increment spec.generation
With Kubernetes 1.11+ metadata.generation now increments properly
when the status subresource is enabled on CRDs
For more details see: https://github.com/knative/serving/issues/643
* Drop the generational duck type
* Initial commit for the webhook to set the annotations about mutator.
The user that created or updated the resource will be set in the
annotations.
* update comments
* remove debug logging
* logging :/
* logging :/, returns
* logging :/ III
* error wrap
* simplify test
* rename the test
* add pkg/errors to the deps for better errors
* do not require CRD to implement Annotatable
* review issues
* fix interface as required by review
* Cleanup the webhook code.
- add more tests
- fix formatting
- fix logging and errors
- use shorthands
- some optimizations in types
- make code more Go'ey from C'ey.
* raise test coverage
* Webhook creates a patch for all fields generated by round tripping the JSON through Golang types.
* Add unit tests for InnerDefaultResource.
* Linter errors.
* PR comments - test changes
* t.Helper()
* PR comments.
* Some fixes to the spoof.go and exporter.go
While reviewing some other CL, I saw some avenues for improving
spoof.go, to log the URL that's being fetched, which would help in test
debugging and to use switch construct, rather than nested if's.
While testing the change, I noticed some shifty loggin from the
exporter, so I fixed that as well while I was there.
* Continuation of the previous cleanups.
* Fix the issues with formatting by executing a grep
* and fix compilation error
* lowercase error
* fix the newly changed unit test
My prior change added sorting to the duck.CreatePatch method to try and stabilize the result of jsonpatch.CreatePatch, which is otherwise non-deterministic (probably walking a map?).
My bad assumption was that the patch operations this generated wouldn't conflict, e.g. it should use `replace` vs. `remove` and `add`.
Clearly this was bad because we start getting really strange errors trying to import this into knative/serving, e.g.
https://gubernator.knative.dev/build/knative-prow/pr-logs/pull/knative_serving/2646/pull-knative-serving-integration-tests/1070435951391543298/
Immutable fields with default values may now be changed iff they change is to populate their default value. This is to support defaulting in the scenario where an object was created long ago and a new field (with a default!) is added. When controllers attempt to mutate the object status today, this would create a webhook rejection! With this change, we compare against a freshly defaulted "old" object to exclude newly defaulted fields from the immutability check.
We saw this in knative/serving for the newly added TimeoutSeconds field in Revision (otherwise immutable), which I believe it leading to upgrade testing flakes since post-upgrade Revision status updates will fail.
This enables us to make additive changes without breaking downgrades.
Unfortunate side effect is that we don't get a nice typo checker in the
webhook :(
* Change VerifyType to return an error instead of panicking
Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
* Move the use of `VerifyType` in tests
Those calls to `duck.VerifyType` are done at runtime and thus could be
costly at program startup. Putting them under tests ensure we still
assert those types but during unit testing.
Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
* Prune the GenericCRD spec to what is used.
Encapsulate our change detection slightly.
* Support common spec mutations via duck typing.
This adds support for performing common mutations to objects via duck types and JSON patching.
Fixes: https://github.com/knative/pkg/issues/76
* Eliminate getSpecJSON thru schemaless duck typing.
This leverages a one-off trick to get the JSON of the spec field from arbitrary types.
In order to have a single webhook support multiple domain contexts, this reworks the `Handlers` argument to embed the `schema.GroupVersion` by wrapping the existing keys with it as a `schema.GroupVersionKind`.
This is mostly straightforward, but one oddity is that I discovered that `AdmissionRequest` gets this same tuple as the less capable `metav1.GroupVersionKind`, so there's a silly conversion we have to do.
I tried this manually vendored in serving with KPA and things worked great.
This pulls the Knative webhook logic (oriented around the interfaces in `knative/pkg/apis`) into `knative/pkg`.
The code is largely copied as-is, with `keep.go` excluded. The main changes are to the test code, which in `knative/serving` still operate in terms of the serving types.
Fixes: https://github.com/knative/pkg/issues/9
Victor Agababov