Add option to force enabling snippet directives #7665
Conversation
k8s-ci-robot
added
triage/accepted
|
/assign @ElvinEfendi @bowei @strongjz |
k8s-ci-robot
added
area/docs
area/helm
rikatz
requested review from
bowei,
strongjz and
ElvinEfendi
and removed request for
tao12345666333 and
ChiefAlexander
rikatz
force-pushed
the
disable-snippets
branch
2 times, most recently
from
727e35e to
55f62a6
Compare
| cfg := n.store.GetBackendConfiguration() | ||
| cfg.Resolver = n.resolver | ||
|
|
||
| for key := range ing.ObjectMeta.GetAnnotations() { |
There was a problem hiding this comment.
For those wondering about this change, as we have a bunch of "fors" in admission webhook for annotations I though it would be better to move all of them to a single iteraction (performance yeeey)
rikatz
force-pushed
the
disable-snippets
branch
2 times, most recently
from
e4e73da to
0ba62a0
Compare
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
rikatz
force-pushed
the
disable-snippets
branch
from
0ba62a0 to
9e20453
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz, strongjz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
* Add option to force enabling snippet directives (#7665) Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com> * Add missing key when cherry-picking
| @@ -46,6 +46,7 @@ The following table shows a configuration option's name, type, and the default v | |||
| |[disable-access-log](#disable-access-log)|bool|false| | |||
| |[disable-ipv6](#disable-ipv6)|bool|false| | |||
| |[disable-ipv6-dns](#disable-ipv6-dns)|bool|false| | |||
| |[enable-snippet-directives](#enable-snippet-directives)|bool|true| | |||
There was a problem hiding this comment.
what about allow-snippet-annotations?
| ## enable-snippet-directives | ||
|
|
||
| Enables Ingress to parse and add *-snippet annotations/directives created by the user. _**default:**_ `true`; | ||
| Obs.: We recommend enabling this option only if you TRUST users with permission to create Ingress objects, as this |
There was a problem hiding this comment.
What is Obs.? Maybe not shorten it? For the text I'd probably go with:
Allows snippets annotations that can inject any NGINX configuration. Set this to `false` if you do not trust the users configuring the ingresses. _**default:**_ `true`;
There was a problem hiding this comment.
yeah, agreed. I will change this one here.
Obs is a shorten for "Observação" in portuguese and my head though this made all sense in english...hahaha
I will change here for WARNING
@strongjz @cpanato enable-snippet-directives vs allow-snippet-annotations
I'm tempted now in changing to allow-snippet-annotations as Elvin proposed.
There was a problem hiding this comment.
Observação
👍🏼 i learned some portuguese
|
I wonder if it'd be better to implement the validation at https://github.com/rikatz/ingress-nginx/blob/9e20453abec1be35f110c50b779ce0f992b5f10a/internal/ingress/annotations/annotations.go#L210 |
|
lgtm |
I didn't there because the Extractor structure is not aware of the flag/configmap that enables/disables the snippet directives :/ Didn't wanted to change the whole structure for that. |
Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>
Signed-off-by: Ricardo Pchevuzinske Katz ricardo.katz@gmail.com
What this PR does / why we need it:
This PR adds a new ConfigMap option that allows admins to disable users snippets (like server-snippet and configuration-snippet).
Types of changes
Which issue/s this PR fixes
Fixes: TBD
How Has This Been Tested?
Added e2e tests and all the other stuff
/triage accepted
/kind feature