add named role support (#202)

* add permissions-api database, migrations Database package along with migrations and migrate command giving permissions-api it's own database to store details into. Initial support is for Roles Get, Create, Update and Delete. Signed-off-by: Mike Mason <[email protected]> * implement role metadata database into query engine This integrates the new database which contains role metadata into the query engine as well as updates the http api to expose this new information. Signed-off-by: Mike Mason <[email protected]> * add logging and health check Signed-off-by: Mike Mason <[email protected]> * add support for updating of roles Signed-off-by: Mike Mason <[email protected]> * update method names to be more descriptive Updated CreateRole, UpdateRole and DeleteRole to CreateRoleTransaction, UpdateRoleTransaction and DeleteRoleTransaction to make it more clear that a transaction is being started. Additionally, the comments on these methods have been updated to include statements that Commit or Rollback must be called to ensure the database lifts all locks on rows which are affected. Previously, the method names made it appear as though the action was taken and completed. However this could lead to hung connections and rows. The new names make it clear that a new transaction is being started. The returning struct only has two methods Commit and Rollback in addition to the Record attribute resulting in a simple structure. Signed-off-by: Mike Mason <[email protected]> * add database changes to chart and support migrations Signed-off-by: Mike Mason <[email protected]> * implement review suggestions Signed-off-by: Mike Mason <[email protected]> * implement second round of review suggestions Signed-off-by: Mike Mason <[email protected]> * add missing rollback comments and ensure tests are checking results properly Signed-off-by: Mike Mason <[email protected]> * lock role record before updating or deleting Since we're working with multiple backends, this allows us to place a lock early and ensure a separate request doesn't conflict with an in-flight change. Signed-off-by: Mike Mason <[email protected]> * correct ListRoles to ensure it always lists roles from the database Signed-off-by: Mike Mason <[email protected]> --------- Signed-off-by: Mike Mason <[email protected]>
infratographer · Jan 19, 2024 · 28ffea5 · 28ffea5
1 parent 1d6d177
commit 28ffea5
Show file tree

Hide file tree

Showing 35 changed files with 2,051 additions and 48 deletions.
diff --git a/.devcontainer/.env b/.devcontainer/.env
@@ -19,6 +19,8 @@ IDENTITYAPI_TRACING_PROVIDER=jaeger
 IDENTITYAPI_TRACING_JAEGER_ENDPOINT=http://localhost:14268/api/traces
 IDENTITYAPI_CRDB_URI="postgresql://root@crdb:26257/identityapi_dev?sslmode=disable"
 
+PERMISSIONSAPI_CRDB_URI="postgresql://root@crdb:26257/permissionsapi?sslmode=disable"
+
 PERMISSIONSAPI_TRACING_ENABLED=true
 PERMISSIONSAPI_TRACING_PROVIDER=otlpgrpc
 PERMISSIONSAPI_TRACING_OTLP_ENDPOINT=jaeger:4317

diff --git a/.devcontainer/docker-compose.yml b/.devcontainer/docker-compose.yml
@@ -3,6 +3,7 @@ version: '3.8'
 networks:
   infradev:
 
+
 volumes:
   crdb:
     null
@@ -39,7 +40,11 @@ services:
   create_databases:
     image: cockroachdb/cockroach:v23.1.12
     restart: on-failure:5
-    command: "sql --insecure -e 'CREATE DATABASE IF NOT EXISTS spicedb;'"
+    command: |
+      sql --insecure -e '
+        CREATE DATABASE IF NOT EXISTS permissionsapi;
+        CREATE DATABASE IF NOT EXISTS spicedb;
+      '
     env_file:
       - .env
     depends_on:

diff --git a/chart/permissions-api/templates/_helpers.tpl b/chart/permissions-api/templates/_helpers.tpl
@@ -18,6 +18,11 @@
   secret:
     secretName: {{ . }}
 {{- end }}
+{{- with .Values.config.crdb.caSecretName }}
+- name: crdb-ca
+  secret:
+    secretName: {{ . }}
+{{- end }}
 {{- with .Values.config.spicedb.policyConfigMapName }}
 - name: policy-file
   configMap:
@@ -36,6 +41,10 @@
 - name: nats-creds
   mountPath: /nats
 {{- end }}
+{{- if .Values.config.crdb.caSecretName }}
+- name: crdb-ca
+  mountPath: {{ .Values.config.crdb.caMountPath }}
+{{- end }}
 {{- if .Values.config.spicedb.policyConfigMapName }}
 - name: policy-file
   mountPath: /policy
@@ -51,6 +60,11 @@
   secret:
     secretName: {{ . }}
 {{- end }}
+{{- with .Values.config.crdb.caSecretName }}
+- name: crdb-ca
+  secret:
+    secretName: {{ . }}
+{{- end }}
 {{- with .Values.config.events.nats.credsSecretName }}
 - name: nats-creds
   secret:
@@ -70,6 +84,10 @@
 - name: spicedb-ca
   mountPath: /etc/ssl/spicedb/
 {{- end }}
+{{- if .Values.config.crdb.caSecretName }}
+- name: crdb-ca
+  mountPath: {{ .Values.config.crdb.caMountPath }}
+{{- end }}
 {{- if .Values.config.events.nats.credsSecretName }}
 - name: nats-creds
   mountPath: /nats

diff --git a/chart/permissions-api/templates/config-server.yaml b/chart/permissions-api/templates/config-server.yaml
@@ -10,4 +10,4 @@ metadata:
     service: server
 data:
   config.yaml: |
-    {{- pick .Values.config "server" "oidc" "spicedb" "tracing" "events" | toYaml | nindent 4 }}
+    {{- pick .Values.config "server" "oidc" "crdb" "spicedb" "tracing" "events" | toYaml | nindent 4 }}
diff --git a/chart/permissions-api/templates/config-worker.yaml b/chart/permissions-api/templates/config-worker.yaml
@@ -10,4 +10,4 @@ metadata:
     service: worker
 data:
   config.yaml: |
-    {{- pick .Values.config "server" "events" "oidc" "spicedb" "tracing" | toYaml | nindent 4 }}
+    {{- pick .Values.config "server" "events" "oidc" "crdb" "spicedb" "tracing" | toYaml | nindent 4 }}
diff --git a/chart/permissions-api/templates/deployment-server.yaml b/chart/permissions-api/templates/deployment-server.yaml
@@ -11,6 +11,7 @@ metadata:
     {{- end }}
   {{- with .Values.deployment.annotations }}
   annotations:
+    checksum/config: {{ include (print $.Template.BasePath "/config-server.yaml") . | sha256sum }}
     {{ toYaml . | nindent 4 }}
   {{- end }}
 spec:
@@ -43,6 +44,30 @@ spec:
       securityContext:
         {{- toYaml .Values.deployment.podSecurityContext | nindent 8 }}
       {{- end }}
+      {{- if eq .Values.config.crdb.migrateHook "init" }}
+      initContainers:
+        - name: {{ include "common.names.name" . }}-migrate-database-init
+          image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
+          imagePullPolicy: {{ .Values.image.pullPolicy }}
+          args:
+            - migrate
+            - up
+            - --config
+            - /config/config.yaml
+          {{- with .Values.config.crdb.uriSecretName }}
+          env:
+            - name: PERMISSIONSAPI_CRDB_URI
+              valueFrom:
+                secretKeyRef:
+                  name: {{ . }}
+                  key: uri
+          {{- end }}
+          {{- with .Values.deployment.resources }}
+          resources:
+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+          volumeMounts: {{ include "permapi.server.volumeMounts" . | nindent 12 }}
+      {{- end }}
       containers:
         - name: {{ include "common.names.name" . }}
           image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
@@ -54,6 +79,13 @@ spec:
           env:
             - name: PERMISSIONSAPI_SERVER_LISTEN
               value: ":{{ include "permapi.listenPort" . }}"
+          {{- with .Values.config.crdb.uriSecretName }}
+            - name: PERMISSIONSAPI_CRDB_URI
+              valueFrom:
+                secretKeyRef:
+                  name: {{ . }}
+                  key: uri
+          {{- end }}
           {{- if .Values.config.spicedb.policyConfigMapName }}
             - name: PERMISSIONSAPI_SPICEDB_POLICYFILE
               value: /policy/policy.yaml

diff --git a/chart/permissions-api/templates/deployment-worker.yaml b/chart/permissions-api/templates/deployment-worker.yaml
@@ -11,6 +11,7 @@ metadata:
     {{- end }}
   {{- with .Values.deployment.annotations }}
   annotations:
+    checksum/config: {{ include (print $.Template.BasePath "/config-worker.yaml") . | sha256sum }}
     {{ toYaml . | nindent 4 }}
   {{- end }}
 spec:
@@ -54,6 +55,13 @@ spec:
           env:
             - name: PERMISSIONSAPI_SERVER_LISTEN
               value: ":{{ include "permapi.listenPort" . }}"
+          {{- with .Values.config.crdb.uriSecretName }}
+            - name: PERMISSIONSAPI_CRDB_URI
+              valueFrom:
+                secretKeyRef:
+                  name: {{ . }}
+                  key: uri
+          {{- end }}
           {{- if .Values.config.events.nats.tokenSecretName }}
             - name: PERMISSIONSAPI_EVENTS_NATS_TOKEN
               valueFrom:

diff --git a/chart/permissions-api/templates/job-migrate-database.yaml b/chart/permissions-api/templates/job-migrate-database.yaml
@@ -0,0 +1,70 @@
+{{- if has .Values.config.crdb.migrateHook (list "pre-sync" "manual") }}
+---
+apiVersion: batch/v1
+kind: Job
+metadata:
+  {{- if eq .Values.config.crdb.migrateHook "manual" }}
+  name: {{ include "common.names.name" . }}-migrate-database
+  {{- else }}
+  generateName: migrate-database-
+  annotations:
+    argocd.argoproj.io/hook: PreSync
+  {{- end }}
+spec:
+  revisionHistoryLimit: 3
+  selector:
+    matchLabels:
+      service: migrate-database
+      {{- include "common.labels.matchLabels" . | nindent 6 }}
+  template:
+    metadata:
+      labels:
+        service: migrate-database
+        {{- include "common.labels.standard" . | nindent 8 }}
+    spec:
+      restartPolicy: OnFailure
+      terminationGracePeriodSeconds: 30
+      {{- with .Values.deployment.imagePullSecrets }}
+      imagePullSecrets:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      {{- if .Values.deployment.podSecurityContext }}
+      securityContext:
+        {{- toYaml .Values.deployment.podSecurityContext | nindent 8 }}
+      {{- end }}
+      containers:
+        - name: {{ include "common.names.name" . }}
+          image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
+          imagePullPolicy: {{ .Values.image.pullPolicy }}
+          args:
+            - migrate
+            - up
+            - --config
+            - /config/config.yaml
+          {{- with .Values.config.crdb.uriSecretName }}
+          env:
+            - name: PERMISSIONSAPI_CRDB_URI
+              valueFrom:
+                secretKeyRef:
+                  name: {{ . }}
+                  key: uri
+          {{- end }}
+          {{- with .Values.deployment.resources }}
+          resources:
+            {{- toYaml . | nindent 12 }}
+          {{- end }}
+          volumeMounts: {{ include "permapi.server.volumeMounts" . | nindent 12 }}
+      {{- with .Values.deployment.nodeSelector }}
+      nodeSelector:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      {{- with .Values.deployment.affinity }}
+      affinity:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      {{- with .Values.deployment.tolerations }}
+      tolerations:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      volumes: {{ include "permapi.server.volumes" . | nindent 8 }}
+{{- end }}
diff --git a/chart/permissions-api/values.yaml b/chart/permissions-api/values.yaml
@@ -46,6 +46,39 @@ config:
     # policyConfigMapName is the name of the Config Map containing the policy file configuration
     policyConfigMapName: ""
 
+  crdb:
+    # migrateHook sets when to run database migrations. one of: pre-sync, init, manual
+    #  - pre-sync: hook runs as a job before any other changes are synced.
+    #  - init: is run as an init container to the server deployment and may run multiple times if replica count is high.
+    #  - manual: a migrate-database job will be available to triggered manually
+    migrateHook: "init"
+    # name is the database name
+    name: ""
+    # host is the database host
+    host: ""
+    # user is the auth username to the database
+    user: ""
+    # password is the auth password to the database
+    password: ""
+    # params is the connection parameters to the database
+    params: ""
+    # uri is the raw uri connection string
+    uri: ""
+    # uriSecretName if set retrieves the `uri` from the provided secret name
+    uriSecretName: ""
+    # caSecretName if defined mounts database certificates from the provided secret
+    # secrets are mounted at `caMountPath`
+    caSecretName: ""
+    # caMountPath is the path the caSecretName is mounted at
+    caMountPath: /etc/ssl/crdb/
+    connections:
+      # max_open is the maximum number of open connections to the database
+      max_open: 0
+      # max_idle is the maximum number of connections in the idle connection
+      max_idle: 0
+      # max_lifetime is the maximum amount of time a connection may be idle
+      max_lifetime: 0
+
   events:
     # zedTokenBucket is the NATS bucket to use for caching ZedTokens
     zedTokenBucket: ""

diff --git a/cmd/createrole.go b/cmd/createrole.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
+	"go.infratographer.com/x/crdbx"
 	"go.infratographer.com/x/events"
 	"go.infratographer.com/x/gidx"
 	"go.infratographer.com/x/viperx"
@@ -13,12 +14,14 @@ import (
 	"go.infratographer.com/permissions-api/internal/iapl"
 	"go.infratographer.com/permissions-api/internal/query"
 	"go.infratographer.com/permissions-api/internal/spicedbx"
+	"go.infratographer.com/permissions-api/internal/storage"
 )
 
 const (
 	createRoleFlagSubject  = "subject"
 	createRoleFlagResource = "resource"
 	createRoleFlagActions  = "actions"
+	createRoleFlagName     = "name"
 )
 
 var (
@@ -38,20 +41,23 @@ func init() {
 	flags.String(createRoleFlagSubject, "", "subject to assign to created role")
 	flags.StringSlice(createRoleFlagActions, []string{}, "actions to assign to created role")
 	flags.String(createRoleFlagResource, "", "resource to bind to created role")
+	flags.String(createRoleFlagName, "", "name of role to create")
 
 	v := viper.GetViper()
 
 	viperx.MustBindFlag(v, createRoleFlagSubject, flags.Lookup(createRoleFlagSubject))
 	viperx.MustBindFlag(v, createRoleFlagActions, flags.Lookup(createRoleFlagActions))
 	viperx.MustBindFlag(v, createRoleFlagResource, flags.Lookup(createRoleFlagResource))
+	viperx.MustBindFlag(v, createRoleFlagName, flags.Lookup(createRoleFlagName))
 }
 
 func createRole(ctx context.Context, cfg *config.AppConfig) {
 	subjectIDStr := viper.GetString(createRoleFlagSubject)
 	actions := viper.GetStringSlice(createRoleFlagActions)
 	resourceIDStr := viper.GetString(createRoleFlagResource)
+	name := viper.GetString(createRoleFlagName)
 
-	if subjectIDStr == "" || len(actions) == 0 || resourceIDStr == "" {
+	if subjectIDStr == "" || len(actions) == 0 || resourceIDStr == "" || name == "" {
 		logger.Fatal("invalid config")
 	}
 
@@ -70,6 +76,13 @@ func createRole(ctx context.Context, cfg *config.AppConfig) {
 		logger.Fatalw("failed to initialize KV", "error", err)
 	}
 
+	db, err := crdbx.NewDB(cfg.CRDB, cfg.Tracing.Enabled)
+	if err != nil {
+		logger.Fatalw("unable to initialize permissions-api database", "error", err)
+	}
+
+	store := storage.New(db, storage.WithLogger(logger))
+
 	var policy iapl.Policy
 
 	if cfg.SpiceDB.PolicyFile != "" {
@@ -97,7 +110,7 @@ func createRole(ctx context.Context, cfg *config.AppConfig) {
 		logger.Fatalw("error parsing subject ID", "error", err)
 	}
 
-	engine, err := query.NewEngine("infratographer", spiceClient, kv, query.WithPolicy(policy), query.WithLogger(logger))
+	engine, err := query.NewEngine("infratographer", spiceClient, kv, store, query.WithPolicy(policy), query.WithLogger(logger))
 	if err != nil {
 		logger.Fatalw("error creating engine", "error", err)
 	}
@@ -112,7 +125,7 @@ func createRole(ctx context.Context, cfg *config.AppConfig) {
 		logger.Fatalw("error creating subject resource", "error", err)
 	}
 
-	role, err := engine.CreateRole(ctx, resource, actions)
+	role, err := engine.CreateRole(ctx, subjectResource, resource, name, actions)
 	if err != nil {
 		logger.Fatalw("error creating role", "error", err)
 	}

diff --git a/cmd/root.go b/cmd/root.go
@@ -7,12 +7,15 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
+	"go.infratographer.com/x/crdbx"
+	"go.infratographer.com/x/goosex"
 	"go.infratographer.com/x/loggingx"
 	"go.infratographer.com/x/versionx"
 	"go.infratographer.com/x/viperx"
 	"go.uber.org/zap"
 
 	"go.infratographer.com/permissions-api/internal/config"
+	"go.infratographer.com/permissions-api/internal/storage"
 )
 
 var (
@@ -42,6 +45,16 @@ func init() {
 	rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is /etc/infratographer/permissions-api.yaml)")
 	loggingx.MustViperFlags(viper.GetViper(), rootCmd.PersistentFlags())
 
+	// Database Flags
+	crdbx.MustViperFlags(viper.GetViper(), rootCmd.Flags())
+
+	// Add migrate command
+	goosex.RegisterCobraCommand(rootCmd, func() {
+		goosex.SetBaseFS(storage.Migrations)
+		goosex.SetLogger(logger)
+		goosex.SetDBURI(globalCfg.CRDB.GetURI())
+	})
+
 	// Add version command
 	versionx.RegisterCobraCommand(rootCmd, func() { versionx.PrintVersion(logger) })