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

CSS-4936 Postgres as secret backend #1011

Merged
4 changes: 4 additions & 0 deletions charms/jimm-k8s/config.yaml
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
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
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
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
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
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
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
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
10 changes: 6 additions & 4 deletions docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,12 @@ services:
# Not needed for local test (yet).
# BAKERY_AGENT_FILE: ""
JIMM_ADMINS: "[email protected]"
VAULT_ADDR: "http://vault:8200"
VAULT_PATH: "/jimm-kv/"
VAULT_SECRET_FILE: "/vault/approle.json"
VAULT_AUTH_PATH: "/auth/approle/login"
# 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"
INSECURE_SECRET_STORAGE: "enabled"
# JIMM_DASHBOARD_LOCATION: ""
JIMM_DNS_NAME: "jimm.localhost"
JIMM_WATCH_CONTROLLERS: ""
Expand Down
5 changes: 3 additions & 2 deletions internal/db/db.go
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
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
Loading