AlinsRan commented on code in PR #2809:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2809#discussion_r3643523102
##########
internal/adc/translator/apisixconsumer.go:
##########
@@ -307,15 +308,25 @@ func (t *Translator) translateConsumerHMACAuthPlugin(tctx
*provider.TranslateCon
}
clockSkewRaw := sec.Data["clock_skew"]
- clockSkew, _ := strconv.ParseInt(string(clockSkewRaw), 10, 64)
+ var clockSkew int64
+ if len(clockSkewRaw) > 0 {
+ var err error
+ clockSkew, err = strconv.ParseInt(string(clockSkewRaw), 10, 64)
+ if err != nil {
+ return nil, fmt.Errorf("hmac-auth: invalid clock_skew
%q in secret %s/%s: %w", string(clockSkewRaw), consumerNamespace,
cfg.SecretRef.Name, err)
+ }
+ }
if clockSkew < 0 {
clockSkew = _hmacAuthClockSkewDefaultValue
}
+ // comma-separated header names, not raw bytes
signedHeadersRaw := sec.Data["signed_headers"]
- signedHeaders := make([]string, 0, len(signedHeadersRaw))
- for _, b := range signedHeadersRaw {
- signedHeaders = append(signedHeaders, string(b))
+ var signedHeaders []string
+ for _, h := range strings.Split(string(signedHeadersRaw), ",") {
Review Comment:
Comma-as-delimiter is a new convention this introduces — the Secret path now
requires `signed_headers` to be comma-separated, while the inline `Value` path
takes a native `[]string` (no delimiter). That asymmetry isn't documented
anywhere. Worth a line in the api-reference / CRD field doc so users storing
headers in a Secret know the expected format; otherwise someone who used
newlines or spaces silently gets one wrong header. Not blocking.
##########
internal/adc/translator/apisixconsumer.go:
##########
@@ -307,15 +308,25 @@ func (t *Translator) translateConsumerHMACAuthPlugin(tctx
*provider.TranslateCon
}
clockSkewRaw := sec.Data["clock_skew"]
- clockSkew, _ := strconv.ParseInt(string(clockSkewRaw), 10, 64)
+ var clockSkew int64
+ if len(clockSkewRaw) > 0 {
+ var err error
+ clockSkew, err = strconv.ParseInt(string(clockSkewRaw), 10, 64)
+ if err != nil {
+ return nil, fmt.Errorf("hmac-auth: invalid clock_skew
%q in secret %s/%s: %w", string(clockSkewRaw), consumerNamespace,
cfg.SecretRef.Name, err)
Review Comment:
Behavior change worth flagging: a pre-existing consumer whose Secret has an
unparseable `clock_skew`/`max_req_body` used to silently default and sync; now
it hard-fails translation and the consumer stops syncing. It's the right
direction (a typo shouldn't coerce to a default), and it's self-scoped to that
one consumer's own Secret, so no blast radius onto others — just calling it out
as a semantics change for the changelog. The `len(...) > 0` guard correctly
keeps the empty case on the default path.
--
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]
