AlinsRan commented on PR #2900:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2900#issuecomment-6078981170
**A Consumer that already holds a key can lose it to another Consumer that
edits its own Secret afterwards.**
When two Consumers share a key, this PR lets the Consumer whose object was
created earlier keep the key. That is not the same as "whoever used the key
first". Example on one Gateway:
| Step | Consumer `a` (created 10:00) | Consumer `b` (created 11:00) |
|---|---|---|
| 1 | key-auth via Secret `a-secret`, `data.key: a-own-key` | inline
key-auth `key: K`, published |
| 2 | edits `a-secret` to `data.key: K` (Secret updates go through no
webhook) | |
| 3 | reconciled: `b` is newer, so `a` keeps `K` | reconciled: `a` is older,
so `b`'s credential is **dropped** |
Result: `b` was using `K` first and loses it. Requests with `K` now
authenticate as `a`, and `a`'s consumer plugins apply to them.
For comparison:
- The webhook rejects `a` if it sets `key: K` inline, because `b` already
uses it. The Secret path gives the opposite result.
- On master, `a` and `b` both get synced. APISIX's Admin API duplicate check
would reject `a`'s write, so `b` keeps `K`. With this PR, the controller
removes `b`'s credential itself.
Repro using this PR's test helpers (passes on the PR head):
```go
func TestSecretEditTakesKey(t *testing.T) {
ctx := context.Background()
now := time.Now()
a := keyAuthConsumer("a", "gw", now.Add(-time.Hour),
secretKeyAuth("cred", "a-secret", nil))
b := keyAuthConsumer("b", "gw", now, inlineKeyAuth("cred", sharedKey))
sec := &corev1.Secret{}
sec.Name, sec.Namespace = "a-secret", consumerNS
sec.Data = map[string][]byte{"key": []byte("a-own-key")}
r := buildConsumerReconciler(t, a, b, sec)
// a edits its own Secret to b's key.
sec.Data["key"] = []byte(sharedKey)
require.NoError(t, r.Update(ctx, sec))
_, skippedA := skipDuplicates(t, r, a)
_, skippedB := skipDuplicates(t, r, b)
require.Zero(t, skippedA) // a keeps K
require.Equal(t, 1, skippedB) // b loses K
}
```
The cause is that the tie-break compares Consumer creation times:
```go
// internal/controller/consumer_controller.go
func olderConsumer(a, b *v1alpha1.Consumer) bool {
if !a.CreationTimestamp.Equal(&b.CreationTimestamp) {
return a.CreationTimestamp.Before(&b.CreationTimestamp)
}
...
}
```
The controller does not record when a key was first used, so it cannot tell
which side was first. Suggestion: when a key is shared, skip it on **every**
Consumer that uses it and report the conflict in each status. Nobody can take a
key from someone else that way. Please also drop "matching the webhook's
first-come behavior" from the description.
--
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]
