AlinsRan commented on code in PR #2909:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2909#discussion_r4534716634
##########
internal/adc/translator/annotations/plugins/plugins.go:
##########
@@ -55,20 +54,24 @@ func NewParser() annotations.IngressAnnotationsParser {
return &plugins{}
}
+// Parse returns the plugins that parsed cleanly together with the errors of
the
+// ones that did not. A handler error used to be logged and dropped here, which
+// hid a requested plugin from the admission webhook and from the operator.
func (p *plugins) Parse(e annotations.Extractor) (any, error) {
plugins := make(adctypes.Plugins)
+ var errs []error
for _, handler := range handlers {
out, err := handler.Handle(e)
if err != nil {
- log.Error(err, "Failed to handle annotation",
"handler", handler.PluginName())
+ errs = append(errs, fmt.Errorf("%s: %w",
handler.PluginName(), err))
continue
}
if out != nil {
plugins[handler.PluginName()] = out
}
}
if len(plugins) > 0 {
- return plugins, nil
+ return plugins, errors.Join(errs...)
Review Comment:
Please also remove or redact the full annotations map logged by
`TranslateIngressAnnotations` when propagating these errors. For an existing
Ingress (or one admitted with the webhook disabled) containing a valid
`enable-csrf: "true"` / `csrf-key` pair and an invalid `rewrite-target-regex`
with a corresponding template, this new error return reaches `t.Log.Error(...,
"annotations", anno)` in `internal/adc/translator/annotations.go:59`. The CSRF
key is then written to the error-level log even though the failing annotation
belongs to the rewrite plugin.
The full-map logging statement predates this PR, but plugin errors
previously stayed inside this parser and did not trigger it. I verified this
specific regression with the same test on the base and head commits: an
assertion that logs exclude the test CSRF key passes on `4d1df161` and fails on
`a8f5e549`. Please keep the error propagation, avoid logging sensitive
annotation values, and add a regression test for this case.
--
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]
