Repository navigation
Fix token connection secret handling and auth session caching - #37
Conversation
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
| @@ -113,6 +118,24 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex | |||
| return managed.ExternalObservation{}, errors.Wrap(resource.Ignore(uperrors.IsNotFound, err), "failed to get token") | |||
| } | |||
| cr.Status.SetConditions(xpv2.Available()) | |||
There was a problem hiding this comment.
xpv2.Available() is set here, before the secret check on line 128. On the Observe error path the reconciler persists the object as-is so an MR in the unrecoverable-secret state ends up READY=True, SYNCED=False.
* same for namespaced controller
There was a problem hiding this comment.
Thanks for flagging this @jonasz-lasut. You’re right that Available=True remains alongside Synced=False on this error path.
The main reason of this decision is about status semantics. Available reflects the state of the external resource, rather than whether the controller completed the entire reconcile successfully. At this point the token exists and is available on the Upbound API. I think, this is also consistent with other provider behaviors, which set Available=True after a successful Describe/Get even though later local processing can still fail. Synced=False with the error message should represent that local failure.
Moving the call also wouldn’t fully avoid this state. If an MR already has Available=True from a previous successful reconcile, that condition remains persisted when a later reconcile fails before reaching SetConditions(Available()). So the main difference would be the first reconcile of a newly adopted MR with a missing secret.
Given that, my preference is to keep the current behavior. If we find that READY=True / SYNCED=False is causing confusion in practice, we can revisit it separately.
|
Thank you for the contribution @sergenyalcin LGTM already but I've left a few comments for your consideration |
…uard - Skip connection-secret check in Observe when the MR has a deletion timestamp; without this guard a Token with a missing secret can never be deleted — Observe keeps erroring and the finalizer is never removed - Guard session eviction in sessionClearingTransport by comparing the stored session value to the one the transport was minted with, so a stale 401 cannot evict a fresh session stored by a concurrent reconcile - Check s.Data["token"] instead of len(s.Data) == 0 to detect a missing JWT specifically rather than any absent data key - Clarify bug description in the end-to-end test comment Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
- Add comment on sessionCache explaining that entries are only evicted on 401 or near-expiry and that no proactive sweep is needed; growth is bounded by the number of distinct ProviderConfig identities - Add comment on the cache.mu lock explaining why it is held across the login network call (single-flight behaviour for concurrent cache misses) and what the right fix would be if contention ever becomes a bottleneck Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
f8bd622 to
7de0a6e
Compare
Description of your changes
Fixes two separate issues found while investigating Robot and Token resources.
Authentication session cache
The provider previously kept a single global cached session. Once populated, that session was reused regardless of organization, endpoint, or credential changes. It was also kept after the API returned 401, so an invalid session could cause repeated reconciliation failures until the provider pod was restarted.
This change:
The request that receives the 401 still fails, but the next reconciliation performs a fresh login instead of continuing to reuse the invalid session. Re-authenticating and retrying within the same request can be considered separately.
Token connection secret
The Upbound API only returns the token JWT when the token is created. If an existing Token MR is restored without its connection secret,
Observe()can find and adopt the upstream token, but cannot recover its JWT.Previously, reconciliation continued with empty connection details and could create a data=0 connection secret. The MR would still appear healthy while consumers later failed authentication with an empty token.
Observe()now checks the connection secret after confirming that the upstream token exists. If the secret is missing or empty, it returns an actionable error instead of allowing an empty secret to be published:token exists upstream but connection secret is missing or empty; the token value cannot be recovered via the API — delete the upstream token to allow automatic recreation.We considered recreating the token automatically, but that would either leave the existing upstream token orphaned or require deleting it as a side effect of Observe/Update. Since the existing JWT cannot be recovered, surfacing the state and leaving that lifecycle decision to the user is safer. This token renewal may considered as a day two operation.
Tests
Added unit coverage for both cluster-scoped and namespaced Token controllers, including:
Also added coverage for the new session cache and 401 invalidation behavior.
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Locally tested.