Skip to content

Add pgaccess feature - #526

Merged
jhrv merged 21 commits into
mainfrom
pgaccess
Oct 1, 2026
Merged

jhrv merged 21 commits into
mainfrom
pgaccess

Conversation

@jhrv

@jhrv jhrv commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

This pull request introduces support for personal Postgres access management via the API, including new authorizations, RBAC rules, GraphQL mutations and queries, and comprehensive integration tests. The changes ensure that users can request, retrieve, and audit personal Postgres access securely, with fine-grained authorization checks and proper activity logging.

API and Authorization Enhancements:

  • Added a new postgres:access:read authorization, granting team members and owners permission to read personal Postgres access status and credentials. This is enforced via a new CanReadPostgresAccess function and corresponding database migration. [1] [2]
  • Updated RBAC rules to allow get and create verbs for the postgresaccesses resource in the nais.io API group, enabling the necessary Kubernetes permissions for managing Postgres access. [1] [2]

GraphQL API Additions:

  • Introduced the createPostgresAccess mutation and postgresAccessConnection and postgresAccess queries, along with their input types and resolvers, to support creation and retrieval of personal Postgres access and connection credentials. [1] [2] [3] [4] [5]
  • Extended the activity log GraphQL resolvers to support new activity log entry types for personal Postgres access creation and connection retrieval.

Integration Tests and Fixtures:

  • Added comprehensive integration tests in create_postgres_access.lua to cover authorization, error cases (such as unknown/unavailable instances), successful access creation, credential retrieval, and auditing.
  • Introduced Kubernetes resource fixtures for various Postgres access scenarios (ready, expired, pending, missing secret) and Postgres cluster states (ready, progressing) to support the integration tests. [1] [2] [3] [4] [5] [6] [7]

These changes collectively enable secure and auditable management of personal Postgres access for team members via the API.

References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18]

@jhrv
jhrv requested a review from a team as a code owner September 17, 2026 12:27
Comment thread internal/database/migrations/0075_add_postgres_access_read_authorization.sql Outdated
Comment thread internal/persistence/postgres/models.go Outdated
Comment thread internal/persistence/postgres/node.go Outdated
Comment thread internal/persistence/postgres/queries.go Outdated
Comment thread internal/persistence/postgres/queries.go
Comment thread internal/persistence/postgres/queries.go Outdated

Copilot AI 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.

🟡 Changes recommended

The status query has unresolved authorization behavior and requires additional integration coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds secure, auditable personal Postgres access management through GraphQL, authorization, RBAC, persistence, and integration tests.

Changes:

  • Adds access creation, status, and credential retrieval.
  • Adds authorization, auditing, migration, and Kubernetes RBAC support.
  • Adds models, resolvers, generated schema code, tests, and fixtures.
File summaries
File Summary
internal/persistence/postgres/queries.go Access creation and retrieval logic
internal/persistence/postgres/queries_test.go Unit tests for access handling
internal/persistence/postgres/node.go Node identification support
internal/persistence/postgres/models.go Access models and validation
internal/persistence/postgres/activitylog.go Access audit transformations
internal/graph/schema/postgres.graphqls GraphQL schema additions
internal/graph/postgres.resolvers.go GraphQL resolvers
internal/graph/gengql/schema.generated.go Generated schema execution code
internal/graph/gengql/root_.generated.go Generated resolver metadata
internal/graph/gengql/activitylog.generated.go Generated activity-log support
internal/database/migrations/0075_add_postgres_access_read_authorization.sql Read authorization migration
internal/auth/authz/queries.go Postgres access authorization checks
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/secret_ready_access_credentials.yaml Ready credential fixture
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/postgres_progressing.yaml Progressing instance fixture
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/postgres_foobar.yaml Available instance fixture
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/postgres_access_ready.yaml Ready access fixture
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/postgres_access_pending-access.yaml Pending access fixture
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/postgres_access_missing-secret-access.yaml Missing-secret fixture
integration_tests/k8s_resources/create_postgres_access/dev/someteamname/postgres_access_expired-access.yaml Expired access fixture
integration_tests/create_postgres_access.lua Integration coverage for access flows
charts/console-backend-rbac/templates/rbac.yaml Kubernetes RBAC permissions
Review details

Files not reviewed (2)

  • internal/graph/gengql/activitylog.generated.go: Generated file
  • internal/graph/gengql/schema.generated.go: Generated file

Suppressed comments (15)

internal/graph/schema/postgres.graphqls:266

  • The newly added activity-log schema types/data type are missing the required public descriptions (and the data fields are undocumented). Add descriptions for the type and each field before exposing this schema.
type PostgresPersonalAccessCreatedActivityLogEntryData {
	username: String!
	expiresAt: Time!
	reason: String!

internal/graph/schema/postgres.graphqls:338

  • The new CreatePostgresAccessPayload type and its fields are missing the required public descriptions. Add developer-facing descriptions before exposing this payload in the schema.
type CreatePostgresAccessPayload {
	"Name of the newly created PostgresAccess resource."
	name: String!
	"Server-controlled expiry for this personal access."
	expiresAt: Time!

internal/graph/schema/postgres.graphqls:345

  • The new input type and its fields are largely undocumented; only reason has a description. Add descriptions for the input and every field, including the access-level and client-key semantics.
input CreatePostgresAccessInput {
	postgresInstance: String!
	teamSlug: Slug!
	environmentName: String!
	accessLevel: PostgresAccessLevel!

internal/graph/schema/postgres.graphqls:354

  • The new PostgresAccessLevel enum and all of its values lack descriptions. Add developer-facing descriptions for the enum and each value.
enum PostgresAccessLevel {
	READ
	READWRITE
	READWRITECREATE

internal/graph/schema/postgres.graphqls:412

  • The new PostgresAccess type and its id/name fields omit the required descriptions. Add a type description and explain these fields in the public schema.
type PostgresAccess implements Node {
	id: ID!
	name: String!

internal/graph/schema/postgres.graphqls:435

  • The new PostgresAccessState enum and all values lack descriptions. Add developer-facing descriptions for the enum and each state.
enum PostgresAccessState {
	PENDING
	READY
	FAILED
	EXPIRED

internal/graph/schema/postgres.graphqls:450

  • The new connection input type and its fields are missing public descriptions. Add descriptions for the type and each input field.
input PostgresAccessConnectionInput {
	name: String!
	teamSlug: Slug!
	environmentName: String!

internal/graph/schema/postgres.graphqls:457

  • The new connection payload type and its fields are missing public descriptions. Add descriptions for the type and each returned field, especially the sensitive credential fields.
type PostgresAccessConnectionPayload {
	password: String!
	caCertificate: String!
	serverName: String!
	tunnel: PostgresAccessConnectionTunnel!

internal/graph/schema/postgres.graphqls:463

  • The new connection tunnel type and both fields are missing public descriptions. Add descriptions explaining the endpoint and gateway key.
type PostgresAccessConnectionTunnel {
	endpoint: String!
	gatewayPublicKey: String!
}

internal/graph/schema/postgres.graphqls:438

  • The new PostgresAccessTunnel type itself lacks a description even though its fields are documented. Add a developer-facing type description.
type PostgresAccessTunnel {

internal/graph/schema/postgres.graphqls:273

  • The new personal-access-connection activity-log type and its fields are all missing public descriptions. Add descriptions for the type and every field.
type PostgresPersonalAccessConnectionActivityLogEntry implements ActivityLogEntry & Node {
	id: ID!
	actor: String!
	createdAt: Time!
	message: String!

internal/graph/schema/postgres.graphqls:451

  • The connection input and the response types below are public schema additions but have no descriptions on their types or fields. Add developer-facing descriptions for the input, payload, tunnel, and every field before publishing them.
input PostgresAccessConnectionInput {
	name: String!
	teamSlug: Slug!
	environmentName: String!
}

internal/graph/schema/postgres.graphqls:457

  • The connection payload and tunnel types expose credentials and connection metadata without GraphQL descriptions. Document the types and each field, particularly password and caCertificate, so consumers understand the sensitivity and purpose of these values.
type PostgresAccessConnectionPayload {
	password: String!
	caCertificate: String!
	serverName: String!
	tunnel: PostgresAccessConnectionTunnel!

internal/persistence/postgres/queries.go:156

  • CanReadPostgresAccess grants this operation to team members, but this additional owner check rejects every non-owner with PostgresAccess not found. That makes the new read authorization ineffective for the status query and conflicts with the migration's Team member role; keep the owner-only check for credentials if intended, but allow authorized team members to read status.
	actor := authz.ActorFromContext(ctx)
	if actor == nil || access.Username != actor.User.Identity() {
		return nil, apierror.Errorf("PostgresAccess %q not found", name)
	}

internal/persistence/postgres/queries.go:128

  • The new public status query/Node lookup is not exercised by the added integration tests; they cover only connection retrieval and helper functions. Add an integration test for postgresAccess (including pending/expired state mapping and a different-owner request) because this path contains the ownership and status exposure checks.
func GetPostgresAccessByIdent(ctx context.Context, id ident.Ident) (*PostgresAccess, error) {
	teamSlug, environmentName, name, err := parseAccessIdent(id)
	if err != nil {
		return nil, err
	}

	return GetPostgresAccess(ctx, name, teamSlug, environmentName)
  • Files reviewed: 25/28 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/graph/postgres.resolvers.go Outdated
Comment thread internal/graph/schema/postgres.graphqls

Copilot AI 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.

Comment thread internal/graph/schema/postgres.graphqls Outdated
Comment thread internal/persistence/postgres/queries.go
Comment thread internal/graph/schema/postgres.graphqls Outdated
Comment thread internal/kubernetes/fake/postgres_fixtures_test.go

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Critical database migration and missing-package blockers remain, along with API compatibility and GraphQL behavior issues.

Review effort: Lite
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (4)

Comment on lines +327 to +328
data,
github_actor_claims
"github.com/coreos/go-oidc/v3/oidc"
"github.com/google/uuid"
"github.com/nais/api/internal/auth/authz"
"github.com/nais/api/internal/auth/middleware/github"
DATABASE_TYPE_UNSPECIFIED = 0;
CLOUD_SQL = 1;
ZALANDO_POSTGRES = 2;
NAIS_POSTGRES = 2;

@thokra-nav thokra-nav 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.

En del @deprecated kommentarer, sikkert fler som jeg ikke har sett. Men hvis det er sikkert at cli, console og andre ikke bruker det, så er det greit å slette nå. Samme med kommentarene til copilot over

Comment thread internal/graph/schema/postgres.graphqls
Comment thread internal/graph/schema/postgres.graphqls
Comment thread internal/graph/schema/postgres.graphqls Outdated
Comment thread internal/graph/schema/postgres.graphqls
Comment thread internal/graph/schema/postgres.graphqls Outdated

@thokra-nav thokra-nav 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.

Så tilfeldigvis på rbac, og kom over dette

Comment thread internal/persistence/postgres/queries.go Outdated
Comment thread internal/persistence/postgres/queries.go Outdated

@thokra-nav thokra-nav 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.

Ser at vi gjør get på en del ressurser via getAccessResource uten informer, er det fordi vi har hemmeligheter i de?

Comment thread internal/graph/schema/postgres.graphqls
@jhrv

jhrv commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

@thokra-nav tror vi da har fiksa det vi snakket om.

  • fjerne kobling mot relayaccess etc, alt går via postgresaccess
  • instance -> branch

Comment on lines +45 to +47
if !reflect.DeepEqual(wantSpec, spec) {
t.Errorf("spec = %#v, want %#v", spec, wantSpec)
}

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.

Bruke go-cmp i stedet for reflect.DeepEqual? Synes den gir en tydeligere diff når noe er galt

Comment on lines +213 to 216
access, err := getPostgresAccessResource(ctx, input.Name, input.TeamSlug, input.EnvironmentName)
if err != nil {
return err
return nil, err
}

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.

Skulle vi konvertert til pgrator typen her? Så slepper vi å bruke unstructured.Nested... for å hente data, og alle if err != nil hver gang.

	accessUnstructured, err := getPostgresAccessResource(ctx, input.Name, input.TeamSlug, input.EnvironmentName)
	if err != nil {
		return nil, err
	}

	access, err := kubernetes.ToConcrete[nais_io_v1.PostgresAccess](o)
	if err != nil {
		return nil, false
	}

Comment thread internal/graph/schema/postgres.graphqls
jhrv added 2 commits October 1, 2026 07:41
   Branches are now addressed as (postgres, branch) with names local to the
   Postgres. PostgresBranch objects get a deterministic, hashed
   metadata.name derived by pgrator, which the API verifies on every lookup.

   - GraphQL: add Postgres.branch(name)/branches, make Postgres.activeBranch a
     PostgresBranch, remove TeamEnvironment.postgresBranch(name)
   - createPostgresAccess and deletePostgresBranch take postgres + branch
   - Show postgres/branch in search; branch IDs are keyed on (postgres, branch)
   - Use typed nais_io_v1 resources and go-cmp per review
   - Pin pgrator/pkg/api to 3ab0adf and merge origin/main
@jhrv
jhrv merged commit 49d23ab into main Oct 1, 2026
11 checks passed
@jhrv
jhrv deleted the pgaccess branch October 1, 2026 07:35
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.

3 participants