* webhook: add options to disable resource_namespace tag in metrics
To add some context, historically, `resource_name` was removed from this
tag list due to its high potential of causing high metrics cardinality.
See [knative/pkg#1464][1] for more information.
While that's great, but it might not be sufficient for large scale use
cases where namespaces can be super dynamic (with generateName, too) or
grows fase enough. There is an issue report from
[tektoncd/pipeline#3171][2] which talks about this.
This proposal makes it possible to disable `resource_namespace` tag via
an option function. The default behavior is not changed, so no user
impact if any of existing users rely on this tag. There is no API
contract change either due to the beauty of variadic functions.
Now downstream projects can consume this by override `StatsReporter` in
webhook context options with their own preference. As a caveat here, if
downstream project does choose to override `StatsReporter`, the default
`ReportMetrics` function shouldn't be called by default as they may now
have a different set of tag keys to report. As such, this function is
only called if the default `StatsReporter` is used.
[1]: https://github.com/knative/pkg/pull/1464
[2]: https://github.com/tektoncd/pipeline/issues/3171
* webhook: add StatsReporterOptions in webhook.Options
There are two ways to customize StatsReporter:
1. Use a whole new StatsReporter implementation.
1. Or pass Option funcs to customize the default StatsReporter.
Option 1 is less practical at this time due to the metrics registration
conflict. `webhook.RegisterMetrics()` is called regardless which
StatsReporter implementation is used (which is a problem by itself). The
second option is more practical since it works well without dealing with
metrics conflicts.
The `webhook.Option` in particular allows people to discard certain
metrics tags.
* TestRegistrationStopChanFire now uses ephemeral ports
* For TLS servers dial TLS
* have server error logs appear in zap
* log the correct error
* pass ephemeral listeners to the webhook for testing
* make minimum tls version configurable
* change default min TLS version to 1.3
* change opencensus tls min version to 1.3
* Update env var name
Co-authored-by: Dave Protasowski <dprotaso@gmail.com>
* use webhook options to configure min tls version
* add unit tests for webhook tlsMinVersion option
* Update webhook/env.go
Co-authored-by: Dave Protasowski <dprotaso@gmail.com>
* address feeback
---------
Co-authored-by: Dave Protasowski <dprotaso@gmail.com>
* Add certs secret name read from env
* Rename webhookNameEnv to webhookNameEnvKey
* Add test code for certs secret name from env
* Add missing input case to test CertsSecretNameFromEnv
* Change CertsSecretName to SecretName
🐛 My previous change has revive/stylecheck warnings because I made the `warn` return value `[]error` and it wants the `error` last, which is kind of silly.
This mutes the linter on this signature, since it hits both revive and stylecheck
/kind bug
🧹 Previously we lacked a public method for turning our `apis.FieldError` multi-error into a list of constituent error messages, so when we turned things into a webhook warning we simply used the combined serialization of all of the warnings. Thanks to Nghia's recent change, we can now access the list of warnings to return as a list of errors.
/kind cleanup
* Preserve webhook namespaceSelector.matchLabels
I have a webhook with this definition and the reconciler is
removing the matchLabels field:
Current resource:
```
namespaceSelector:
matchExpressions:
- key: webhooks.knative.dev/exclude
operator: DoesNotExist
objectSelector:
matchLabels:
app.kubernetes.io/component: kafka-dispatcher
```
Applied resource:
```
namespaceSelector:
matchExpressions: [ ]
matchLabels:
app.kubernetes.io/name: knative-eventing
objectSelector:
matchLabels:
app.kubernetes.io/component: kafka-dispatcher
```
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
* Optimize cases that don't need ensureLabelSelectorRequirements
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
When a callback is registered for a gvk that isn't part of the
handlers that callback is never called since there is no webhook
rule associated with that GVK.
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
* Make request body available in admission context
Signed-off-by: Andrés Torres <andrest@vmware.com>
* Add newline at the end of file
Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
* WIP: just one option exploration.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
* Another option. New signature and new type for configuration.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
* Use interfaces and type assertions.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
* log custom config with debug.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
* Address PR feedback.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
* Drop `UserInfo` from logger tagging in webhook.
This can get big, and can contain mildly sensitive data that some users don't want showing up in logs.
If we keep this, than we should perhaps restrict what we tag to one of the less unbounded fields (e.g. don't include `Extra`)
* Switch UserInfo to contain just Username
* Add support for admission webhook warnings.
This extends `apis.FieldError` to support designating certain FieldErrors as "warnings" (or explicitly as "errors", however, this is the default for back-compat).
You can turn an `apis.FieldError` into a warning using: `fe.At(apis.WarningLevel)` or force it into an error using: `fe.At(apis.ErrorLevel)`.
You can get the errors at a particular diagnostic level using: `fe.Filter(apis.WarningLevel)`.
This change also hooks this into the admission webhook infrastructure to support surfacing the "warning" level `apis.FieldError`s via the `Warnings` section of the `AdmissionResponse`.
Fixes: #2497
* Add a comment about the use of defer.
Go 1.17 introduced a new handy API for setting env vars scoped for
a single test so we can avoid the hard to read set and reset env
loops.
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
When the admission request is for a resource with an empty string as
group, which happens on core resources, the `creator` or `lastModifier`
annotations are invalid since they become `/creator` or
`/lastModifier`.
This patch removes the `/` when group = `""`.
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
* Add support for callback defaults
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Put unstr object in ctx and set user info
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Move get callback at the top
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Panic when using delete verb
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Split tests and add callback ctx tests
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Set user info annotations
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Register Webhook Rules from callbacks
Signed-off-by: Pierangelo Di Pilato <pdipilat@redhat.com>
* Adapt unstructured objects to apis.HasSpec
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
* Change json tag name to match struct field name
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Today, we can use `Path()` of `/foo/` (trailing slash) to support prefix-matched webhooks, but unfortunately the request context is lost when `Admit()` or `Convert()` is called.
This ensures that information flows through associated with context for anyone who would like this metadata for additional processing.
* Introduce `NewContext`, deprecate `NewImplFull`.
Our generated `NewImpl` methods have long taken `context.Context`, but despite many iterations the forms we expose from our `controller` package never have. This change contains several elements:
1. Expose a new `NewContext` method that takes `context.Context` in addition to the current `NewImplFull` signature.
2. Call `NewContext` instead of the deprecated `NewImpl` from our generated controller code.
3. Call `NewContext` from all our webhook reconcilers.
* Add a Tracker to controller.Impl to cut down on downstream boilerplate.
* bump k8s deps to v1.20.7
* fix migrator test pkg
* dynamicclient now expects List types to be declare either via a scheme or manually
* fix error message comparison
* drop excess vendor licenses
* Using the injection fake dynamic client will preserve pre-1.20 behaviour
This is accomplished by preprocessing the scheme/fixtures and declaring
a custom scheme were we map our types & lists to unstructured.* types
* revert webhook factory changes
* ensure objects to the dynamic client are unstructured
* seed the default dynamic client with k8s scheme
* include duckv1 types in default fake dynamic client scheme
* use default k8s scheme
looks like eventing adds to this scheme but we should import the correct one vs the one from the fake package
* drop duckv1 from default scheme
* set APIVersion/Kind if empty
* refactor ToUnstructured helper to a new package
Prashant Rewar
zhouhaibing089
navin
cola
Stavros Kontopoulos
Hector Fernandez
Dave Protasowski
Izabela Gomes
kmahapatra
Christoph Stäbler
Hyeonki Hong
Matt Moore
Pierangelo Di Pilato
Kenny Leung
Andrés Torres
Ville Aikas
Paul Schweigert
Todd
Pierangelo Di Pilato
knative-automation
Matt Moore
Markus Thömmes
NovaHe
Evan Anderson
Jason Hall