feat: Add Firestore storage backend - #17
Conversation
Implement a pluggable storage backend using Google Cloud Firestore with transaction-based resource versioning, snapshot listeners for real-time event broadcasting, and native duplicate detection via Create(). Adds Makefile targets (firestore-start, firestore-stop, test-firestore) and a README documenting limitations and comparison with other backends. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thetechnick The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
patjlm
left a comment
There was a problem hiding this comment.
Firestore PoC Review
Based on analysis of the diff against storage.ResourceStore contract and Kubernetes API semantics.
Critical (won't compile on target branch)
1. storage.EventPruner interface doesn't exist
_ storage.EventPruner = (*FirestoreBroadcaster)(nil) references an undefined interface. This will fail to compile.
2. opts.FieldFilters doesn't exist on storage.ListOptions
Referenced in both List() and Watch() methods. ListOptions (which embeds metav1.ListOptions + custom fields) has no FieldFilters field on the target branch. Another compile error. (This field may exist on the api-aggregation-proposal branch but not on the PR's target.)
Critical (correctness bugs)
3. No optimistic concurrency on Update
Update() does not compare the incoming object's resourceVersion against the stored value. It simply overwrites with a new RV. Two concurrent updates both succeed — last-write-wins. This breaks the Kubernetes API contract where clients expect 409 Conflict on stale resourceVersion. Compare: the memory store correctly checks if newRV != existingRV and returns errors.NewConflict(...).
4. TOCTOU race in Update and Delete
Both methods do Get() then a separate write (Set() / Delete()). Between these calls, another process could modify or delete the resource. Should use Firestore transactions to make the read-check-write atomic.
Significant
5. Broadcast silently swallows event log write errors
_, _ = b.client.Collection(b.eventLogColl).Doc(docID).Set(...) — the error return is discarded. If the event log write fails, watchers miss events with no error surfaced anywhere. At minimum, log the error; ideally, propagate it.
6. Client-side filtering breaks pagination
When label/shard/field filters are active with a limit, Firestore returns limit+1 documents, but client-side filtering may eliminate most of them. A page could return far fewer items than limit even when more matching items exist, and the continue token may skip over valid matches.
7. Update overwrites createdAt
The Update method's document map includes "createdAt": time.Now(), which overwrites the original creation timestamp on every update. Should preserve the original createdAt or use a separate updatedAt field.
8. Document ID collision risk
Using _ as separator in buildDocID(filterValue, namespace, name) means namespace ns_a + name b collides with namespace ns + name a_b. The memory store uses \x00 as separator, which is invalid in Kubernetes names and thus collision-free.
9. Historical event replay capped at 1000 with no indication
If a watcher was disconnected and >1000 events occurred, it silently gets partial history. Should return 410 Gone (like compaction) to force a relist when the gap exceeds the replay window.
Design consideration: timestamp-as-resource-version
The current implementation uses a counter document for monotonic resource versions, which is bottlenecked by Firestore's ~1 sustained write/sec/document limit (README acknowledges ~10 writes/sec practical cap). An alternative is using firestore.ServerTimestamp on each document write — this eliminates the counter bottleneck since each write gets its own server-assigned timestamp independently.
However, Firestore's API does not explicitly document that server timestamps are unique or strictly monotonic across concurrent transactions. Firestore is built on Spanner internally so these guarantees likely hold, but relying on undocumented behavior in production is risky. By contrast, Spanner's PENDING_COMMIT_TIMESTAMP() explicitly guarantees this via TrueTime.
Minor
- Non-blocking broadcast drops events.
broadcastToSubscribersusesselect { case ch <- event: default: }. Full subscriber channels lose events silently. listcreatesUnstructuredListvia type assertion.list := listObj.(*unstructured.UnstructuredList)will panic if the scheme returns a different type. Usemeta.SetList()instead.- No
RemainingItemCountin pagination. The memory store sets this; the Firestore store does not. matchesFieldFiltersis expensive. Doesjson.Marshal+json.Unmarshalper object per list call. Could access fields directly on unstructured objects.
|
Correction on review findings #1 and #2 My earlier review incorrectly flagged Findings #1 and #2 ("won't compile") should be withdrawn. The remaining findings (no OCC on Update, TOCTOU races, broadcast error swallowing, pagination/filtering interaction, createdAt overwrite, document ID collision, historical replay cap) stand. |
Comprehensive study evaluating GCP datastore options for replacing etcd as the storage backend. Covers Cloud Spanner, Cloud SQL, AlloyDB, Firestore, and Bigtable with database schemas, implementation patterns, cost analysis, and PoC reviews (PR openshift-online#16, openshift-online#17). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Implement a pluggable storage backend using Google Cloud Firestore with
transaction-based resource versioning, snapshot listeners for real-time
event broadcasting, and native duplicate detection via Create().
Adds Makefile targets (firestore-start, firestore-stop, test-firestore)
and a README documenting limitations and comparison with other backends.