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:
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.
--
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]
