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.

Sign up for GitHub

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

Jump to bottom

CSS-4936 Postgres as secret backend #1011

Merged
merged 13 commits into from
Jul 26, 2023
Merged

CSS-4936 Postgres as secret backend #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
332a8bf
Added Postgres as secret backend
kian99 Jul 25, 2023
35dbbba
Fix migrations and cleaned up secret implementation
kian99 Jul 25, 2023
f386ea9
Added tests
kian99 Jul 26, 2023
4ff622d
merge feature-rebac
kian99 Jul 26, 2023
b2ba450
Removed env files
kian99 Jul 26, 2023
0efd132
Revert compose changes to use Vault
kian99 Jul 26, 2023
f86dcc5
Added godocs
kian99 Jul 26, 2023
3f5c7f8
Update 1_2.sql
kian99 Jul 26, 2023
a1e669b
PR tweaks and test fixes
kian99 Jul 26, 2023
8844dff
PR Tweaks
kian99 Jul 26, 2023
8895228
Merge branch 'feature-rebac' into CSS-4936-postgres-as-secret-backend
kian99 Jul 26, 2023
cd95ba8
Tweak - move line after check
kian99 Jul 26, 2023
c63238d
Merge branch 'feature-rebac' into CSS-4936-postgres-as-secret-backend
kian99 Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions charms/jimm-k8s/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ options:
type: string
default: https://jaas.ai/models
description: URL of the Juju Dashboard for this controller.
postgres-secret-storage:
default: false
type: boolean
description: Enables the use of Postgres for secret storage if Vault is not related.
public-key:
type: string
description: The public part of JIMM's macaroon bakery keypair.
Expand Down
3 changes: 3 additions & 0 deletions charms/jimm-k8s/src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,9 @@ def _update_workload(self, event):
config_values["JIMM_WATCH_CONTROLLERS"] = "1"
config_values["JIMM_ENABLE_JWKS_ROTATOR"] = "1"

if self.config.get("postgres-secret-storage", False):
config_values["INSECURE_SECRET_STORAGE"] = "enabled" # Value doesn't matter, checks env var exists.

# remove empty configuration values
config_values = {key: value for key, value in config_values.items() if value}

Expand Down
119 changes: 77 additions & 42 deletions charms/jimm-k8s/tests/unit/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,20 @@
"public-key": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"private-key": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"vault-access-address": "10.0.1.123",
"audit-log-retention-period-in-days": "10",
}

EXPECTED_ENV = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by fix for less duplication in the k8s charm tests.

"CANDID_URL": "test-candid-url",
"JIMM_DASHBOARD_LOCATION": "https://jaas.ai/models",
"JIMM_DNS_NAME": "juju-jimm-k8s-0.juju-jimm-k8s-endpoints.None.svc.cluster.local",
"JIMM_ENABLE_JWKS_ROTATOR": "1",
"JIMM_LISTEN_ADDR": ":8080",
"JIMM_LOG_LEVEL": "info",
"JIMM_UUID": "1234567890",
"JIMM_WATCH_CONTROLLERS": "1",
"PRIVATE_KEY": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"PUBLIC_KEY": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "0",
}


Expand Down Expand Up @@ -71,19 +84,7 @@ def test_on_pebble_ready(self):
"startup": "disabled",
"override": "replace",
"command": "/root/jimmsrv",
"environment": {
"CANDID_URL": "test-candid-url",
"JIMM_DASHBOARD_LOCATION": "https://jaas.ai/models",
"JIMM_DNS_NAME": "juju-jimm-k8s-0.juju-jimm-k8s-endpoints.None.svc.cluster.local",
"JIMM_ENABLE_JWKS_ROTATOR": "1",
"JIMM_LISTEN_ADDR": ":8080",
"JIMM_LOG_LEVEL": "info",
"JIMM_UUID": "1234567890",
"JIMM_WATCH_CONTROLLERS": "1",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "10",
"PRIVATE_KEY": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"PUBLIC_KEY": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
},
"environment": EXPECTED_ENV,
}
}
},
Expand All @@ -110,19 +111,37 @@ def test_on_config_changed(self):
"startup": "disabled",
"override": "replace",
"command": "/root/jimmsrv",
"environment": {
"CANDID_URL": "test-candid-url",
"JIMM_DASHBOARD_LOCATION": "https://jaas.ai/models",
"JIMM_DNS_NAME": "juju-jimm-k8s-0.juju-jimm-k8s-endpoints.None.svc.cluster.local",
"JIMM_ENABLE_JWKS_ROTATOR": "1",
"JIMM_LISTEN_ADDR": ":8080",
"JIMM_LOG_LEVEL": "info",
"JIMM_UUID": "1234567890",
"JIMM_WATCH_CONTROLLERS": "1",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "10",
"PRIVATE_KEY": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"PUBLIC_KEY": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
},
"environment": EXPECTED_ENV,
}
}
},
)

def test_postgres_secret_storage_config(self):
container = self.harness.model.unit.get_container("jimm")
self.harness.charm.on.jimm_pebble_ready.emit(container)

self.harness.update_config(MINIMAL_CONFIG)
self.harness.update_config({"postgres-secret-storage": True})
self.harness.set_leader(True)

# Emit the pebble-ready event for jimm
self.harness.charm.on.jimm_pebble_ready.emit(container)

# Check the that the plan was updated
plan = self.harness.get_container_pebble_plan("jimm")
expected_env = EXPECTED_ENV.copy()
expected_env.update({"INSECURE_SECRET_STORAGE": "enabled"})
self.assertEqual(
plan.to_dict(),
{
"services": {
"jimm": {
"summary": "JAAS Intelligent Model Manager",
"startup": "disabled",
"override": "replace",
"command": "/root/jimmsrv",
"environment": expected_env,
}
}
},
Expand All @@ -146,7 +165,8 @@ def test_bakery_configuration(self):

# Emit the pebble-ready event for jimm
self.harness.charm.on.jimm_pebble_ready.emit(container)

expected_env = EXPECTED_ENV.copy()
expected_env.update({"BAKERY_AGENT_FILE": "/root/config/agent.json"})
# Check the that the plan was updated
plan = self.harness.get_container_pebble_plan("jimm")
self.assertEqual(
Expand All @@ -158,20 +178,7 @@ def test_bakery_configuration(self):
"startup": "disabled",
"override": "replace",
"command": "/root/jimmsrv",
"environment": {
"BAKERY_AGENT_FILE": "/root/config/agent.json",
"CANDID_URL": "test-candid-url",
"JIMM_DASHBOARD_LOCATION": "https://jaas.ai/models",
"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "0",
"JIMM_DNS_NAME": "juju-jimm-k8s-0.juju-jimm-k8s-endpoints.None.svc.cluster.local",
"JIMM_ENABLE_JWKS_ROTATOR": "1",
"JIMM_LISTEN_ADDR": ":8080",
"JIMM_LOG_LEVEL": "info",
"JIMM_UUID": "1234567890",
"JIMM_WATCH_CONTROLLERS": "1",
"PRIVATE_KEY": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
"PUBLIC_KEY": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
},
"environment": expected_env,
}
}
},
Expand All @@ -189,6 +196,34 @@ def test_bakery_configuration(self):
},
)

def test_audit_log_retention_config(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this test as I noticed nothing was testing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh ty

container = self.harness.model.unit.get_container("jimm")
self.harness.charm.on.jimm_pebble_ready.emit(container)

self.harness.update_config(MINIMAL_CONFIG)
self.harness.update_config({"audit-log-retention-period-in-days": "10"})

# Emit the pebble-ready event for jimm
self.harness.charm.on.jimm_pebble_ready.emit(container)
expected_env = EXPECTED_ENV.copy()
expected_env.update({"JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS": "10"})
# Check the that the plan was updated
plan = self.harness.get_container_pebble_plan("jimm")
self.assertEqual(
plan.to_dict(),
{
"services": {
"jimm": {
"summary": "JAAS Intelligent Model Manager",
"startup": "disabled",
"override": "replace",
"command": "/root/jimmsrv",
"environment": expected_env,
}
}
},
)

def test_dashboard_relation_joined(self):
harness = Harness(JimmOperatorCharm)
self.addCleanup(harness.cleanup)
Expand Down
4 changes: 4 additions & 0 deletions charms/jimm/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ options:
type: string
default: https://jaas.ai/models
description: URL of the Juju Dashboard for this controller.
postgres-secret-storage:
default: false
type: boolean
description: Enables the use of Postgres for secret storage if Vault is not related.
public-key:
type: string
description: The public part of JIMM's macaroon bakery keypair.
Expand Down
3 changes: 3 additions & 0 deletions charms/jimm/src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ def _on_config_changed(self, _):
"audit_retention_period": self.config.get("audit-log-retention-period-in-days", ""),
}

if self.config.get("postgres-secret-storage", False):
args["insecure_secret_storage"] = "enabled" # Value doesn't matter, only checks env var exists.

with open(self._env_filename(), "wt") as f:
f.write(self._render_template("jimm.env", **args))
if self._ready():
Expand Down
3 changes: 3 additions & 0 deletions charms/jimm/templates/jimm.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,6 @@ PRIVATE_KEY={{private_key}}
PUBLIC_KEY={{public_key}}
{% endif %}
JIMM_AUDIT_LOG_RETENTION_PERIOD_IN_DAYS={{audit_retention_period}}
{%- if insecure_secret_storage %}
INSECURE_SECRET_STORAGE=enabled
{% endif %}
28 changes: 28 additions & 0 deletions charms/jimm/tests/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,34 @@ def test_openfga_relation_changed(self):
self.assertEqual(lines[3].strip(), "OPENFGA_STORE=test-store")
self.assertEqual(lines[4].strip(), "OPENFGA_TOKEN=test-secret-token")

def test_insecure_secret_storage(self):
"""Test that the flag for insecure secret storage is only generated when explictly requested."""
config_file = os.path.join(self.harness.charm.charm_dir, "juju-jimm.env")
self.harness.update_config(
{
"candid-url": "https://candid.example.com",
"controller-admins": "user1 user2 group1",
"dns-name": "jimm.example.com",
"log-level": "debug",
"uuid": "caaa4ba4-e2b5-40dd-9bf3-2bd26d6e17aa",
"public-key": "izcYsQy3TePp6bLjqOo3IRPFvkQd2IKtyODGqC6SdFk=",
"private-key": "ly/dzsI9Nt/4JxUILQeAX79qZ4mygDiuYGqc2ZEiDEc=",
}
)
self.assertTrue(os.path.exists(config_file))
with open(config_file) as f:
lines = f.readlines()
os.unlink(config_file)
self.assertEqual(len(lines), 18)
self.assertEqual(len([match for match in lines if "INSECURE_SECRET_STORAGE" in match]), 0)
self.harness.update_config({"postgres-secret-storage": True})
self.assertTrue(os.path.exists(config_file))
with open(config_file) as f:
lines = f.readlines()
os.unlink(config_file)
self.assertEqual(len(lines), 19)
self.assertEqual(len([match for match in lines if "INSECURE_SECRET_STORAGE" in match]), 1)


class VersionHTTPRequestHandler(BaseHTTPRequestHandler):
def __init__(self, *args, **kwargs):
Expand Down
4 changes: 3 additions & 1 deletion cmd/jimmsrv/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ func start(ctx context.Context, s *service.Service) error {
zapctx.Info(ctx, "attempting to start JWKS rotator")
s.Go(func() error {
err := jimmsvc.StartJWKSRotator(ctx, time.NewTicker(time.Hour).C, time.Now().UTC().AddDate(0, 3, 0))
zapctx.Error(ctx, "failed to start JWKS rotator", zap.Error(err))
kian99 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
zapctx.Error(ctx, "failed to start JWKS rotator", zap.Error(err))
}
return err
})
}
Expand Down
3 changes: 3 additions & 0 deletions docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,13 @@ services:
# Not needed for local test (yet).
# BAKERY_AGENT_FILE: ""
JIMM_ADMINS: "[email protected]"
# Note: You can comment out the Vault ENV vars below and instead use INSECURE_SECRET_STORAGE to place secrets in Postgres.
VAULT_ADDR: "http://vault:8200"
VAULT_PATH: "/jimm-kv/"
VAULT_SECRET_FILE: "/vault/approle.json"
VAULT_AUTH_PATH: "/auth/approle/login"
# Note: By default we should use Vault as that is the primary means of secret storage.
# INSECURE_SECRET_STORAGE: "enabled"
kian99 marked this conversation as resolved.
Show resolved Hide resolved
# JIMM_DASHBOARD_LOCATION: ""
JIMM_DNS_NAME: "jimm.localhost"
JIMM_WATCH_CONTROLLERS: ""
Expand Down
5 changes: 3 additions & 2 deletions internal/db/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (d *Database) Migrate(ctx context.Context, force bool) error {
}

for {
v := dbmodel.Version{Component: dbmodel.Component}
v := dbmodel.Version{Component: dbmodel.Component, Major: 1, Minor: 0}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So to quickly discuss the changes to the migrations, when I added a migration it didn't take effect because the code didn't quite work right. Essentially the problem boiled down to the fact that our first migration was 0_0 and the default value for the struct/DB was also 0 so we had no way of knowing if a migration had taken place after the initial one was done. So the simplest solution is to start our migrations from 1. That is why you will notice our migrations now start at 1_1. This change is best left to feature-rebac since we haven't deployed it (for our staging deploy I can manually tweak things in the DB) and the schema is different to main anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, but I'm not sure how we were able to migrate up to this point? Perhaps discuss this in standup later / explain it as I don't think I fully understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first migration worked, but the second wouldn't. At the end of the first migration there was a line UPDATE versions SET major=1, minor=0 WHERE component='jimmdb'; now after that migration takes place, the code will query the versions table and see that MajorDesired==MajorSet and MinorRequired>MinorSet so it would try to run the next migration but it doesn't know what the next migration is, and that's why I had to add the Minor+=1 line. And the last thing is that when I added that line but the first migration starts at 0, we have a problem.

if err := db.FirstOrCreate(&v).Error; err != nil {
return errors.E(op, dbError(err))
}
Expand All @@ -80,10 +80,11 @@ func (d *Database) Migrate(ctx context.Context, force bool) error {
atomic.StoreUint32(&d.migrated, 1)
return nil
}
if v.Major != dbmodel.Major && !force && v.Major != 0 {
if v.Major != dbmodel.Major && !force {
return errors.E(op, errors.CodeServerConfiguration, fmt.Sprintf("database has incompatible version %d.%d", v.Major, v.Minor))
}
// The major versions are unchanged, the database can be migrated.
v.Minor += 1
schema, err := dbmodel.SQL.ReadFile(path.Join("sql", db.Name(), fmt.Sprintf("%d_%d.sql", v.Major, v.Minor)))
if err != nil {
return errors.E(op, err)
Expand Down
10 changes: 10 additions & 0 deletions internal/db/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright 2023 Canonical Ltd.

package db

var (
JwksKind = jwksKind
JwksPublicKeyTag = jwksPublicKeyTag
JwksPrivateKeyTag = jwksPrivateKeyTag
JwksExpiryTag = jwksExpiryTag
)
Loading