feat(sql): add User, SslCert and BackupRun, and fix nested native IDs - #168
Open
naxty wants to merge 4 commits into
Open
feat(sql): add User, SslCert and BackupRun, and fix nested native IDs#168naxty wants to merge 4 commits into
naxty wants to merge 4 commits into
Conversation
Cloud SQL had instances and databases; these are the three remaining
collections with a full create and delete.
The fix comes first in importance. Every sqladmin mutation answers with an
Operation whose targetLink names the *instance*, and extractSQLNativeID
consulted targetLink before anything else - so a nested resource was stored
under its instance's native ID, two resources shared one id, and the next sync
read the instance and reconciled the nested resource away as absent. That
defect is not new: GCP::SQL::Database has been registered for some time with no
conformance case, so nothing ever exercised the path. testdata/cloudsql-database.pkl
now covers it, and it passes.
Each of the three new types is addressed differently, and none of them by a
name a forma chooses:
User delete takes the name as a query parameter against the collection
while get takes it as a path segment, so delete is hand-written.
password is write-only and createOnly, as AlloyDB's user does.
SslCert addressed by a server-generated sha1Fingerprint, which arrives
nested under clientCert.certInfo on insert and top-level on read.
The private key is returned once and dropped rather than persisted.
BackupRun addressed by a server-generated id, which arrives as
backupContext.backupId on the create Operation. No absolute expiry,
so unlike Spanner's and Bigtable's backups its fixture cannot rot.
All three are discovered by walking the instances: discovery lists with no
properties and sqladmin has no wildcard.
MySQL's host is deliberately not modelled - it is part of a user's identity but
a DeleteRequest carries no properties to read it back from, so supporting it
means encoding it in the native ID.
cloudsql-user is crud 8/8 and discovery 4/4; cloudsql-database is crud 8/8 with
discovery running. The ssl-cert and backup-run cases are written but unrun.
GCP::SQL::Database had no parent-walking List, so discovery - which lists with no properties - asked a collection URL with no instance in it and found nothing. Its new conformance case failed with "resource did not appear in inventory", which is the second defect that one fixture has surfaced in a type that shipped without any coverage. Rather than add a fourth copy of the same walker, the three already in this package are now one. The only thing that differed between them was how an item names itself: users name sslCerts sha1Fingerprint databases name backupRuns id userProvisioner is left with just Delete, which is the one verb the generic engine genuinely cannot express for a user.
…ention sslCerts.insert answers with the certificate itself, so this code called the create synchronous and skipped polling. The certificate really is usable the moment insert returns - but Cloud SQL serialises operations per instance, so reporting the create done while its operation is still running means the next mutation is issued into a busy queue. The conformance Destroy answered 409 "Operation failed because another operation was already in progress". Certificates are now polled like every other sqladmin type. Only the native ID comes from an unusual place, and so does the operation: insert nests it beside the certificate as operation.name rather than putting it at the top level. Separately, that 409 is now retryable across the whole package, alongside the existing "is being accessed by other users" case. Both are sqladmin saying "not yet" rather than "no", and every nested type shares its instance's operation queue - so this helps user, database and backupRun too, not just the case that exposed it.
Cloud SQL does not remove a deleted backup run. The record survives as a tombstone and a get answers 200 with status "DELETED" rather than 404, so the generic Read reported the resource as still present: the conformance OOB-delete step timed out waiting for it to leave inventory. Read now reports NotFound for a tombstone, and the instance walker skips them so discovery does not offer long-dead backups as unmanaged resources to import - the quieter of the two failures, and the one no test here would have caught. The list filter is deliberately a separate function from the id extractor: a create response carries no status yet and still has to yield an id.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the three remaining Cloud SQL collections — and fixes two live defects in
GCP::SQL::Database, which shipped some time ago with no conformance case.Stacked on #167.
cloudsql-usercloudsql-databasecloudsql-ssl-certcloudsql-backup-runVerified on released formae (0.89.0) and on formae
main; themainsweep alsore-ran the pre-existing
cloudsql-instancecase (5 cases, 132 minutes, allgreen).
The fixes matter more than the types
Nested resources took their native ID from the operation's
targetLink,which names the instance. So a nested resource was stored under its instance's
native ID, two resources shared one id, and the next sync read the instance and
reconciled the nested resource away as absent.
GCP::SQL::Databasehas had thisdefect in production;
testdata/cloudsql-database.pklnow covers it.Databases were not discoverable at all — no parent-walking
List, sodiscovery asked a collection URL with no instance in it and found nothing.
409 "another operation was already in progress" is now retried. Cloud SQL
serialises operations per instance and every nested type shares that queue, so
this is ordinary contention, not a fault. Affects every Cloud SQL type.
A deleted backup run is now treated as gone. Cloud SQL keeps a tombstone and
answers
getwith 200 andstatus: "DELETED"rather than 404, so a backupdeleted outside formae never left inventory — and discovery offered long-dead
backups as unmanaged resources to import.
Notes for review
Each new type is addressed differently, and none by a name a forma chooses:
deletetakes the name as a query parameter against the collectionwhile
gettakes it as a path segment, so delete is hand-written.passwordis write-only and createOnly, as AlloyDB's user does.
sha1Fingerprint, which arrivesnested under
clientCert.certInfoon insert and top-level on read. Theprivate key is returned once and dropped rather than persisted: keeping it
would put a private key in stored state and guarantee drift on every read.
backupContext.backupId. No absolute expiry, so unlike Spanner's andBigtable's backups its fixture cannot rot.
All four instance-scoped types now share one parent-walking
List, replacingthree near-identical copies.
What I could not verify
hostis deliberately not modelled. It is part of a user'sidentity, but a
DeleteRequestcarries no properties to read it back from, sosupporting it means encoding it in the native ID. Users created without one
get MySQL's default.
cloudsql-*gets a 30-minute per-operation timeout: each case provisions itsown instance and runs 20–29 minutes for CRUD alone.
Correction (2026-08-28): this body reported CRUD as
8/8. That was wrong. TheReplace step reports
[~]— skipped, not passed: the harness only runs it when atestdata/<case>-replace.pklfixture exists, and no case in this repo has one (0 of118). Read every
8/8above as 7 passed, Replace not exercised. The same appliesto every GCP resource already on
main— the Replace path has never been coveredhere. Nothing in this PR regressed.