Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add named role support #202

Merged
merged 11 commits into from
Jan 19, 2024
2 changes: 2 additions & 0 deletions .devcontainer/.env
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 6 additions & 1 deletion .devcontainer/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ version: '3.8'
networks:
infradev:


volumes:
crdb:
null
Expand Down Expand Up @@ -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:
Expand Down
18 changes: 18 additions & 0 deletions chart/permissions-api/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion chart/permissions-api/templates/config-server.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
2 changes: 1 addition & 1 deletion chart/permissions-api/templates/config-worker.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
32 changes: 32 additions & 0 deletions chart/permissions-api/templates/deployment-server.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 }}"
Expand All @@ -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
Expand Down
8 changes: 8 additions & 0 deletions chart/permissions-api/templates/deployment-worker.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
70 changes: 70 additions & 0 deletions chart/permissions-api/templates/job-migrate-database.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
{{- if has .Values.config.crdb.migrateHook (list "pre-sync" "manual") }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we anticipate needing or wanting to support these three kinds of migration workflows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

manual is perhaps not needed. but pre-sync and init are useful.

pre-sync can't be used during the initial deployment as it will rely on configs and secrets that have to be synced first, and therefore won't be available. so you must use the init method. but after using the init method, you can switch to the pre-sync method which will ensure migrations only execute once per sync.

if having these options is not preferred we could get rid of it and just use init containers.

---
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
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I think it's good to not assume any user of this is using Argo specifically.

{{- 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 }}
33 changes: 33 additions & 0 deletions chart/permissions-api/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Comment on lines +50 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re: Argo. I do like the concept, though!

# 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: ""
Comment on lines +61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like something we shouldn't even let users set manually, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually like to expose all available configuration options, which this is an option that can be set in the crdbx config which is why I added it here.

# params is the connection parameters to the database
params: ""
# uri is the raw uri connection string
uri: ""
Comment on lines +65 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here re: sensitive data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re same as above

# 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: ""
Expand Down
19 changes: 16 additions & 3 deletions cmd/createrole.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand All @@ -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")
}

Expand All @@ -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 != "" {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down
13 changes: 13 additions & 0 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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) })

Expand Down
Loading
Loading