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

Simplify openfga setup #1323

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Simplify openfga setup #1323

merged 2 commits into from
Aug 27, 2024

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Aug 23, 2024

Description

Note: This PR builds on #1322

This PR simplifies JIMM's Docker compose file further by removing all the OpenFGA migration containers and placing their actions into an entrypoint.sh script that is called when running the OpenFGA container.

@kian99 kian99 requested a review from a team as a code owner August 23, 2024 11:44
@kian99 kian99 force-pushed the simplify-openfga-setup branch 3 times, most recently from eb31dbe to 4a0d3dd Compare August 23, 2024 12:33
SimoneDutto
SimoneDutto previously approved these changes Aug 23, 2024
Copy link
Contributor

@SimoneDutto SimoneDutto left a comment

Choose a reason for hiding this comment

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

these changes are very good in the direction of improving our dev-env. Thank you for this!

echo "Switching juju controller to $JIMM_CONTROLLER_NAME"
juju switch "$JIMM_CONTROLLER_NAME"
echo
echo "Retrieving controller info for $CONTROLLER_NAME"
./jimmctl controller-info --local "$CONTROLLER_NAME" "$CONTROLLER_YAML_PATH" --tls-hostname juju-apiserver
jimmctl controller-info --local "$CONTROLLER_NAME" "$CONTROLLER_YAML_PATH" --tls-hostname juju-apiserver
Copy link
Contributor

Choose a reason for hiding this comment

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

why are using jimmctl installed via snap even though we build it locally before?

Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove the build part all together if we suppose to always have the snap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is introduced in #1321, so we can discuss it there but the purpose of this change is to detect if jimmctl is available and if it's not only then build it. In CI, we don't necessarily want to require Go to be installed to spin up the test environment.

# An instance of JIMM used for dev, built from source with hot-reloading.
jimm-dev:
extends:
file: compose-common.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

this is very clever!

container_name: openfga
environment:
OPENFGA_AUTHN_METHOD: "preshared"
OPENFGA_AUTHN_PRESHARED_KEYS: "jimm"
OPENFGA_DATASTORE_ENGINE: "postgres"
OPENFGA_DATASTORE_URI: "postgresql://jimm:jimm@db/jimm?sslmode=disable"
command: run
volumes:
- ./openfga/authorisation_model.json:/app/authorisation_model.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you agree:
having it as a volume suggests that changing this json triggers a change in the openfga model.
Is this true?
If it isn't, i would add this file in the dockerfile as an ADD

Copy link
Contributor Author

@kian99 kian99 Aug 23, 2024

Choose a reason for hiding this comment

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

So I initially had it added inside the dockerfile, the issue is that the dockerfile is in local/openfga/ and the auth model is defined in /openfga/authorisation_model.json so I couldn't figure out how to pass that file through. Symlinks are not handled by dockerfile ADD so the volume was the next best thing.

Makefile Outdated
@@ -36,22 +36,23 @@ certs:
@cd local/traefik/certs; ./certs.sh; cd -

test-env: sys-deps certs
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity:
what is the purpose of this make rule? it is a docker compose w/o any profile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's there just for convenience and aligns with the other commands like dev-env. I still often just do docker compose up --wait -d myself.

alesstimec
alesstimec previously approved these changes Aug 27, 2024
@kian99 kian99 merged commit 73357b9 into canonical:v3 Aug 27, 2024
4 checks passed
# Cleanup old auth model from previous starts
psql -Atx "$OPENFGA_DATASTORE_URI" -c "DELETE FROM authorization_model;"
# Adds the auth model and updates its authorisation model id to be the expected hard-coded id such that our local JIMM can utilise it for queries.
wget -q -O - --header 'Content-Type: application/json' --header 'Authorization: Bearer jimm' --post-file authorisation_model.json localhost:8080/stores/01GP1254CHWJC1MNGVB0WDG1T0/authorization-models
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this

kian99 added a commit to kian99/jimm that referenced this pull request Sep 3, 2024
* simplify Docker compose and OpenFGA setup

* entrypoint tweaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants