Skip to content

feat(workbench): add Audit Database support - #158

Merged
Lytol merged 7 commits into
mainfrom
zachhannum-audit-database
Aug 27, 2026
Merged

feat(workbench): add Audit Database support#158
Lytol merged 7 commits into
mainfrom
zachhannum-audit-database

Conversation

@zachhannum

@zachhannum zachhannum commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds spec.workbench.experimentalFeatures.auditDatabaseEnabled, which provisions a second Postgres database (distinct from the internal database) via the same EnsureDatabaseExists/PostgresDatabase machinery already used for the internal database, and renders audit-database.conf pointing Workbench at it.
  • The audit role's password is resolved server-side (via the existing product.FetchSecret helper, keyed by a new dev-audit-db-password Site secret entry) and written directly into audit-database.conf's Password field — that file, like database.conf, is rendered into a Kubernetes Secret rather than a ConfigMap, so no credentials land in plaintext CR/ConfigMap data.
  • A dedicated role/password (rather than reusing the internal DB's) is required because WORKBENCH_POSTGRES_PASSWORD is shared between database.conf and audit-database.conf and cannot carry two different passwords for two different roles.
  • Consumers must add a dev-audit-db-password key to their Site secret before enabling this flag (mirrors the existing dev-db-password key).

Test plan

  • make generate / make generate-all (deepcopy + client-go + openapi) / make manifests / make helm-generate / make copy-crds / make verify-crds — all regenerated and in sync
  • go build ./..., go vet ./...
  • make go-test — full suite green, including envtest-based reconciler tests in internal/controller/core
  • make test-kustomize, make helm-lint, make helm-template — no drift
  • New/extended unit tests: TestWorkbenchSecretConfig_GenerateSecretData (audit-database.conf rendering), TestSiteAuditDatabaseEnabled / TestSiteAuditDatabaseDisabledByDefault (Site → Workbench propagation)

Adds spec.workbench.experimentalFeatures.auditDatabaseEnabled, which
provisions a second Postgres database (distinct from the internal
database) via the same EnsureDatabaseExists/PostgresDatabase machinery
already used for the internal database, and renders audit-database.conf
pointing Workbench at it. Also sets package-audit=1 in rserver.conf.

The audit role's password is resolved server-side (via the existing
product.FetchSecret helper, keyed by a new "dev-audit-db-password" Site
secret entry) and written directly into audit-database.conf's Password
field, since that file -- like database.conf -- is rendered into a
Kubernetes Secret rather than a ConfigMap. A dedicated role/password is
required because WORKBENCH_POSTGRES_PASSWORD is shared between
database.conf and audit-database.conf and cannot carry two different
passwords for two different roles.

workbench-api-admin-enabled/-super-admin-enabled need no changes here:
they're already set whenever ApiSettings.Enabled is true.

See: https://docs.posit.co/ide/server-pro/admin/database/audit/database.html
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown

Claude finished @zachhannum's task in 1m 56s —— View job


PR Review

  • Read review guidelines
  • Review API changes (api/core/v1beta1/)
  • Review controller changes (internal/controller/core/)
  • Review CRD changes
  • Review tests
  • Post final review

Overall this is a clean, well-scoped feature that follows existing patterns. Two issues worth addressing before merge:

Critical

None.

Important

  1. Postgres identifier length (internal/controller/core/workbench.go:199) — auditDbName is derived from {name}-workbench-audit after sanitizing hyphens to underscores. Postgres enforces a 63-byte max identifier length. The WorkbenchSpec.Name field allows up to 256 characters, so a long-but-valid Workbench name could produce a db/role name that silently truncates (or errors) at the Postgres side. The existing primary database has the same theoretical exposure but is 6 characters shorter. Worth adding truncation or a validation marker (+kubebuilder:validation:MaxLength) on the field to keep names safe. An inline comment has been left.

  2. Unused AutoCreate field (api/core/v1beta1/workbench_config.go:926) — WorkbenchAuditDatabaseConfig has an AutoCreate string field that is never populated by the controller. WorkbenchDatabaseConfig (the primary database struct) does not have this field. Either remove it or wire it up. An inline comment has been left.

Suggestion

  • There's no controller-level integration test (in internal/controller/core/site_test.go) covering the AuditDatabaseEnabledWorkbench.Spec.AuditDatabaseEnabled / PackageAudit=1 flow, unlike the existing ForceAdminUiEnabled and AuditedJobs cases. The unit tests in api/ cover rendering, but a short site_test.go test would catch any future regression in the site→workbench propagation path. Not a blocker, but worth adding.

return ctrl.Result{}, err
}

auditDbName := invalidCharacters.ReplaceAllString(auditComponentName, "_")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auditDbName is {name}-workbench-audit after sanitization (e.g. my-site-workbench-audit). Postgres identifier max length is 63 bytes; the dbName for the primary database is {name}_workbench, which is already fine. But adding -audit appends 6 more characters. If a Workbench name is close to the 256-character API limit this would overflow. That limit is far higher than Postgres allows, so this is worth truncating — or at minimum, document that names must stay short enough to accommodate the suffix.

The main database path has the same potential issue, but adding 6 more characters here makes it more likely to trip in practice for longer names.

Comment thread api/core/v1beta1/workbench_config.go Outdated
Host string `json:"host,omitempty"`
Username string `json:"username,omitempty"`
Password string `json:"password,omitempty"`
AutoCreate string `json:"auto-create,omitempty"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AutoCreate is defined in the struct but never set in the controller. If this field doesn't need to be user-configurable and the operator always manages database creation via EnsureDatabaseExists, it should be removed to avoid confusion. If there's a scenario where it should be set, add the logic for it.

- Truncate the audit component name to Postgres's 63-byte identifier
  limit before deriving the database/role name from it, so a long
  Workbench name plus the "-audit" suffix can't silently overflow.
- Remove WorkbenchAuditDatabaseConfig.AutoCreate: dead field, never set
  by the controller (the operator always provisions the audit database
  itself via EnsureDatabaseExists).
- Add TestSiteAuditDatabaseEnabled / TestSiteAuditDatabaseDisabledByDefault
  covering the Site -> Workbench propagation path, mirroring the existing
  AuditedJobs/ForceAdminUiEnabled test pattern.
- Regenerate CRDs: also picks up a stale chronicleBuiltinEnabled entry
  in core.posit.team_sites.yaml left over from the earlier branch rebase
  (removed from the Go types, but not re-synced to the CRD YAML at the
  time).
@zachhannum

Copy link
Copy Markdown
Contributor Author

Addressed both review findings in 24968a0:

  1. Postgres identifier length — added truncateForPostgresIdentifier, applied before deriving the audit database/role name (and the PostgresDatabase CR name) from it, so a long Workbench name + the -audit suffix can no longer silently overflow Postgres's 63-byte limit.
  2. Unused AutoCreate field — removed. The operator always provisions the audit database itself via EnsureDatabaseExists, so there was never a code path that would set it.

Also added the suggested site_test.go coverage (TestSiteAuditDatabaseEnabled / TestSiteAuditDatabaseDisabledByDefault) for the Site → Workbench propagation path, mirroring the existing AuditedJobs/ForceAdminUiEnabled tests.

While regenerating CRDs I also caught a stale chronicleBuiltinEnabled entry that had lingered in core.posit.team_sites.yaml from the earlier rebase off main (removed from the Go types but not re-synced to the CRD YAML) — cleaned that up too.

Full make go-test suite green.

make helm-generate regenerates dist/chart/templates/crd/*.yaml from
config/crd/bases/*.yaml; the CI "Verify Helm chart is in sync with
kustomize" check caught that this hadn't been run after the site/
workbench CRD changes for Audit Database support.
make generate-client (kube_codegen) hadn't been run after the API
changes for Audit Database support; CI's "Assert no diff" check caught
the drift in client-go/applyconfiguration/.
package-audit=1 and "audit database configured" are two independent
prerequisites for the Package Audit Read API (see rstudio-pro's
wiki/features/package-audit-read-api.qmd) -- not the same feature.
AuditDatabaseEnabled provisioning the audit database should not also
silently flip on package scanning; other consumers of the audit
database (general session/usage history) don't want that side effect.

Removes WorkbenchRServerConfig.PackageAudit and the auto-set on
AuditDatabaseEnabled. package-audit=1 can be set independently via the
existing free-form rserver.conf additionalConfigs escape hatch on
consumers that want it -- it doesn't need a dedicated CRD field.
Lytol
Lytol previously approved these changes Aug 26, 2026

@Lytol Lytol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Let me know if you want any help with testing this on an internal workload.

@zachhannum

Copy link
Copy Markdown
Contributor Author

@Lytol had to make a tweak to fix the failing PR checks, would appreciate another quick review!

@Lytol
Lytol added this pull request to the merge queue Aug 27, 2026
Merged via the queue into main with commit c325a72 Aug 27, 2026
8 checks passed
@Lytol
Lytol deleted the zachhannum-audit-database branch August 27, 2026 23:06
ian-flores pushed a commit that referenced this pull request Aug 27, 2026
# [1.31.0](v1.30.2...v1.31.0) (2026-08-27)

### Features

* **workbench:** add Audit Database support ([#158](#158)) ([c325a72](c325a72))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants