CSS-8226 Vault k8s relation #1197
Conversation
| harness.set_leader(True) | ||
| harness.begin_with_initial_hooks() | ||
| self.harness.enable_hooks() | ||
| self.harness.charm._on_install(None) |
There was a problem hiding this comment.
It's a bit cleaner if we could use self.harness.charm.on.install.emit().
| @@ -191,15 +191,15 @@ func TestAddController(t *testing.T) { | |||
| func TestAddControllerWithVault(t *testing.T) { | |||
| c := qt.New(t) | |||
|
|
|||
| client, path, creds, ok := jimmtest.VaultClient(c, "../../") | |||
| client, path, roleID, roleSecretID, ok := jimmtest.VaultClient(c, "../../") | |||
| if err != nil { | ||
| return nil, errors.E(op, err, "unable to login to approle auth method") | ||
| } | ||
| if authInfo == nil { |
There was a problem hiding this comment.
copied from the vault reference example, so i assume it can happen
| // be made to use a vault server and JIMM will store everything in | ||
| // it's local database. | ||
| VaultSecretFile string | ||
| VaultRoleID string |
There was a problem hiding this comment.
We called it RoleId and RoleSecretId earlier, can we make them consistent? Perhaps AppRoleId and AppRoleSecretId in both areas?
There was a problem hiding this comment.
service params deal with many aspects of JIMM.. i much prefer that the field name specifically mentions which bit it is intended for..
alesstimec
force-pushed
the
vault-k8s-relation
branch
from
c44945d to
1ccb0f5
Compare
charms/jimm-k8s/src/charm.py
Outdated
| if vault_config: | ||
| config_values.update(vault_config) |
There was a problem hiding this comment.
Here we should check if vault_config is None then enter a blocked state. You might also want to check whether the config for insecure_secret_storage is set to true to allow JIMM to start without vault provided that config value is set.
| # update vault relation if exists | ||
| binding = self.model.get_binding("vault-kv") | ||
| if binding is not None: | ||
| egress_subnet = str(binding.network.interfaces[0].subnet) | ||
| self.interface.request_credentials(event.relation, egress_subnet, self.get_vault_nonce()) |
There was a problem hiding this comment.
Is it intentional to put this in the update-status hook? Considering that the hook runs every 5m by default. Couldn't we put the self.interface.request_credentials somewhere that is only triggered when the relation to Vault is made.
There was a problem hiding this comment.
yes... that's what the library suggests.
| def _on_vault_connected(self, event: vault_kv.VaultKvConnectedEvent): | ||
| relation = self.model.get_relation(event.relation_name, event.relation_id) | ||
| egress_subnet = str(self.model.get_binding(relation).network.interfaces[0].subnet) | ||
| self.vault.request_credentials(relation, egress_subnet, self.get_vault_nonce()) |
There was a problem hiding this comment.
Ah we are doing it here as well? But here we are doing self.vault.requests_credentials rather than self.interface.request_credentials above, I imagine one might be wrong?
There was a problem hiding this comment.
both are suggested by the lib
alesstimec
force-pushed
the
vault-k8s-relation
branch
7 times, most recently
from
2a6977e to
22339e4
Compare
- switches to the new vault relation interface used by the vault-k8s charm - switches vault auth to role_id, role_secret_id Note: did not change the VM charm. also means we won't be able to relate to the vault VM charm.
alesstimec
force-pushed
the
vault-k8s-relation
branch
from
8b7003a to
04a5964
Compare
- Also updated the ingress relation to the latest and it seems v1 of the relation interface is deprecated, we should move to v2 in a separate PR.
Description
Note: did not change the VM charm. also means we won't be able to relate to the vault VM charm.
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers