AlinsRan commented on code in PR #2813:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2813#discussion_r3654014927
##########
internal/adc/translator/annotations/plugins/plugins.go:
##########
@@ -60,8 +58,9 @@ func (p *plugins) Parse(e annotations.Extractor) (any, error)
{
for _, handler := range handlers {
out, err := handler.Handle(e)
if err != nil {
- log.Error(err, "Failed to handle annotation",
"handler", handler.PluginName())
- continue
+ // Fail closed: abort translation so the route is never
programmed
+ // while a requested plugin is missing.
+ return nil, fmt.Errorf("%s: %w", handler.PluginName(),
err)
Review Comment:
Blocking: this goes far beyond csrf. With every parser error now joined, an
Ingress also fails translation on `upstream-scheme`, `upstream-retry`, the
three timeouts (upstream.go:51-82), and a non-compiling rewrite regex
(rewrite.go:54).
An Ingress carrying a typo'd annotation that has been serving traffic fine
stops reconciling on upgrade, and after the next controller restart rebuilds
the ADC baseline the route disappears from the data plane.
Keep the fail-closed semantics, but this needs an entry in
`docs/en/latest/upgrade-guide.md`.
##########
internal/adc/translator/annotations_test.go:
##########
@@ -95,16 +95,35 @@ func TestTranslateIngressAnnotations(t *testing.T) {
name string
anno map[string]string
expected *IngressConfig
+ wantErr bool
}{
{
name: "no matching annotations",
anno: map[string]string{"upstream": "value1"},
expected: &IngressConfig{},
},
{
- name: "invalid scheme",
- anno:
map[string]string{annotations.AnnotationsUpstreamScheme: "invalid"},
- expected: &IngressConfig{},
+ // enable-csrf without a key must fail translation,
never reconcile
+ // into a route that silently lacks the csrf plugin.
+ name: "enable-csrf without csrf-key",
+ anno: map[string]string{
+ annotations.AnnotationsEnableCsrf: "true",
+ },
+ wantErr: true,
+ },
+ {
+ name: "enable-csrf with empty csrf-key",
+ anno: map[string]string{
+ annotations.AnnotationsEnableCsrf: "true",
+ annotations.AnnotationsCsrfKey: "",
+ },
+ wantErr: true,
+ },
+ {
+ // a typo'd scheme used to fall back to plaintext http
silently
+ name: "invalid scheme",
+ anno:
map[string]string{annotations.AnnotationsUpstreamScheme: "invalid"},
+ wantErr: true,
Review Comment:
The unit test proves translation errors, not the claim the PR leads with —
that the route is never programmed. Please add an e2e: apply an Ingress with
`enable-csrf: "true"` and no `csrf-key`, assert the route is absent from the
data plane.
##########
internal/adc/translator/annotations/plugins/csrf.go:
##########
@@ -39,7 +41,10 @@ func (c *csrf) Handle(e annotations.Extractor) (any, error) {
key := e.GetStringAnnotation(annotations.AnnotationsCsrfKey)
if key == "" {
- return nil, nil
+ // csrf requested but the key is missing: fail loud instead of
+ // silently dropping the plugin and programming the route
unprotected.
+ return nil, fmt.Errorf("annotation %q is enabled but %q is
missing or empty",
Review Comment:
For this specific check — `enable-csrf` set, `csrf-key` empty — the
translator is the late place to catch it. It's a purely static property of the
object, and there is already an Ingress validating webhook
(`internal/webhook/v1/ingress_webhook.go`, path
`/validate-networking-k8s-io-v1-ingress`, currently doing SSL conflict
detection). Rejecting there means `kubectl apply` fails immediately with the
reason, instead of an object that looks accepted and quietly never reconciles.
The translator check should stay as the backstop, since the webhook is
registered with `failurePolicy=Ignore`. Fine as a follow-up, but worth stating
the intent here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
