Repository navigation
dbaas: add acl list, create, delete and update commands - #923
pierre-emmanuelJ wants to merge 3 commits into
Conversation
natalie-o-perret
left a comment
natalie-o-perret
left a comment
There was a problem hiding this comment.
These gremlins look blocking to me, especially the invalid empty-ACL payload, the delete-all trapdoor, and the lost-update path. Requesting changes until those are nailed down.
1f71272 to
22f2914
Compare
natalie-o-perret
left a comment
natalie-o-perret
left a comment
There was a problem hiding this comment.
The OpenSearch gremlins are gone, nice one. I found two smaller blockers in the remaining ClickHouse and Kafka paths: keeping the released ClickHouse IAM contract, and making duplicate Kafka topic ACLs addressable by ID. Details inline.
| } | ||
|
|
||
| acl, err := client.GetDBAASClickhouseAclConfig(ctx, c.Name) | ||
| dbType, err := dbaasServiceType(ctx, client, c.Name, c.Zone) |
There was a problem hiding this comment.
Could we preserve the released ClickHouse permission contract here, perhaps by falling back to the direct ACL GET when type lookup is denied?
I think this gives the existing command a bigger IAM hat than it used to wear. Released acl show went straight to get-dbaas-clickhouse-acl-config, while this adds list-dbaas-services first. Those are separate IAM operations.
Focused setup: /dbaas-service returns 403 and the existing ClickHouse ACL endpoint returns 200.
$ go test ./cmd/dbaas/acl -run '^TestReviewClickhouseShowKeepsExistingPermissionContract$' -count=1 -v
ListDBAASServices: http response: Forbidden: forbidden
--- FAIL: TestReviewClickhouseShowKeepsExistingPermissionContract
Falling back to showClickhouseAcl when lookup fails kept the old key working, and the same check passed:
--- PASS: TestReviewClickhouseShowKeepsExistingPermissionContract
PASS
ok github.com/exoscale/cli/cmd/dbaas/acl
There was a problem hiding this comment.
Done in 27a69f5: when listing the services is forbidden, acl show falls back to the direct ClickHouse ACL call, as before. Covered by TestDBAASClickhouseAclShowWithoutServiceList.
| case len(ids) == 0: | ||
| return fmt.Errorf("no matching ACL entry found for user %q in service %q", c.Username, c.Name) | ||
| case len(ids) > 1: | ||
| return fmt.Errorf("%d ACL entries match for user %q, use --permission to select one", len(ids), c.Username) |
There was a problem hiding this comment.
Could we add an ID selector here, or otherwise make an exact duplicate addressable?
I think we have a tiny Kafka ACL Hotel California: exact duplicate topic entries can check in, but this command cannot check either one out. Aiven explicitly handles multiple entries with the same topic, username, and permission, and acl list already exposes their IDs. --permission remains ambiguous for those entries.
Focused setup: the ACL GET returns two alice / orders / read entries named topic-1 and topic-duplicate.
$ go test ./cmd/dbaas/acl -run '^TestReviewDuplicateKafkaTopicAclCanBeDeletedByID$' -count=1 -v
unknown flag: --id
--- FAIL: TestReviewDuplicateKafkaTopicAclCanBeDeletedByID
After adding --id topic-duplicate, the same check sent DELETE /topic/acl-config/topic-duplicate and passed:
--- PASS: TestReviewDuplicateKafkaTopicAclCanBeDeletedByID
PASS
ok github.com/exoscale/cli/cmd/dbaas/acl
There was a problem hiding this comment.
Done in 27a69f5: acl delete NAME USERNAME --id ID, exclusive with the other selectors; the ID has to belong to USERNAME. The ambiguity error now lists the matching IDs, and the e2e scenario deletes one entry by --id.
Manage the ACLs of Kafka (topic and Schema Registry) and OpenSearch services under `exo dbaas acl`, with the service type detected from the service name. `acl show` now also covers these two types. Kafka entries cannot be updated and ClickHouse ACLs are read-only: the API has no call for either. [sc-104243] AI-assisted: true Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The OpenSearch ACL configuration can only be read and replaced as a whole: there is no atomic per-entry operation nor any revision check, so two concurrent writes could silently drop one another. Until the API offers one, `acl create` and `acl delete` only cover Kafka, and `acl update` (OpenSearch only) is removed; `acl list` and `acl show` still cover OpenSearch. This also removes the delete-all default of `acl delete` on OpenSearch and the empty `acls` PUT rejected by the API. The Kafka e2e scenario uses startup-4: startup-2 plans are no longer available on Kafka 3.9 and above. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tries `acl show` used to call the ClickHouse ACL endpoint directly. Finding the service type first needs the permission to list the services: when that lookup is forbidden, fall back to the ClickHouse ACL call, so API keys restricted to it keep working. `acl delete` gains `--id`, the entry ID shown by `acl list`: exact duplicate Kafka entries (same user, topic or resource, and permission) can't be told apart by --permission. The ambiguity error now lists the IDs of the matching entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
22f2914 to
27a69f5
Compare
This branch has not been deployed
2 participants
Description
exo dbaas aclonly hadshow, for ClickHouse. This adds the rest of the tree for the services whose ACLs the API lets us manage safely. [sc-104243]exo dbaas acl list NAMEfor Kafka (topic and Schema Registry entries) and OpenSearch (index entries),acl create|delete NAME USERNAME ...for Kafka (deletealso takes the entry--idshown byacl list), andacl shownow also covers Kafka and OpenSearch. The service is a positional name and its type is detected, as inexo dbaas user.acl showon a ClickHouse service keeps its output, at the cost of one extra call to find the service type (if the API key can't list services, it falls back to the ClickHouse ACL call, as before); on other service types it now fails with an explicit "unsupported" error instead of an API error.Checklist
(For exoscale contributors)
CHANGELOG.md)Note
AI assistance: code, tests, PR description.
🤖 Generated with Claude Code