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

feat: allow querying models owned by other users #577

Closed
wants to merge 2 commits into from

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Sep 16, 2024

Description

This PR aims to solve #504 to handle scenarios where a user has access to multiple models with the same name.
Juju namespaces models by username, i.e. user-1 and user-2 can both have a model called "foo". Currently the provider doesn't handle such cases properly.

This PR addresses the issue by:

  • Adjust the model cache to understand that multiple models with the same name but different owners can exist.
  • Allow a model name to be either <model-name> or <model-owner>/<model-name>. This keeps backwards compatibility with the existing provider semantics.

Consider a scenario where a user-1 creates a model and grants user-2 with admin access to that model. User-2 can now write "user-1/model-name" in their plan to reference user-1's model. This is mainly useful when you are not creating the model in your plan, just referencing one that already exists. See "Additional notes" below for the scenario where the plan uses a reference to a model rather than hardcoding strings.

The concrete changes implemented:

  • Move model cache into a separate package
  • Rework model cache to handle multiple models with the same name but different owners
  • Small tweak to testAccPreCheck to use sync.Once to fix potential race condition
  • Add test for deploying to a model owned by a different user

Fixes: #504, CSS-10769

Type of change

  • Logic changes in resources (the API interaction with Juju has been changed)
  • Bug fix (non-breaking change which fixes an issue)
  • Requires a documentation update

QA steps

The change allows for the following TF plan. Assuming you are the admin user on the controller,

resource "juju_model" "this" {
	name = "test"
}
	
resource "juju_application" "this" {
	name = "myApp"
	model = "admin/${juju_model.this.name}"
	charm {
	name = "jameinel-ubuntu-lite"
	}
	trust = true
	expose{}
}

If you have access to 2 models called test you can specify the model by switching the model owner from admin.

Additional notes

I am still looking at how best to update the documentation.

There is 1 scenario worth mentioning:

  • When referencing a model like juju_model.model-name.name, the model name does not contain the owner. If the Juju user has access to 2 models with this name, there will be an error returned to the user. The fix for this would be to do something like "<username>/${juju_model.this.name}" in your plan as is done in the tests.

There are two acceptance tests for the new behaviour.

  1. A simple test that deploys an application referencing a model by a fully specified name
  2. A more involved test that creates a new Juju user and creates a model as that user. Then grant the original user with access to the model and deploying to the model with Terraform. This test requires shelling out to the CLI. Possibly this is overdoing things and not necessary but open to opinions here.

@kian99 kian99 force-pushed the allow-access-to-shared-models branch from c4df570 to 1fb3651 Compare September 16, 2024 15:42
- Move model cache into a separate package
- Rework model cache to handle multiple models with the same name but different owners
- Small tweak to testAccPreCheck to use sync.Once to fix potential race condition
- Add test for deploying to a model owned by a different user
@kian99 kian99 force-pushed the allow-access-to-shared-models branch from 50327d1 to 2fe0d91 Compare September 16, 2024 20:27
- add missing copyright headers
- add package godoc
- add --force flag to CLI logout commands
@kian99 kian99 force-pushed the allow-access-to-shared-models branch from 2fe0d91 to 5c961df Compare September 16, 2024 21:46
@@ -809,6 +1000,50 @@ func testAccResourceApplicationBasic_Minimal(modelName, charmName string) string
`, modelName, charmName)
}

func testAccResourceApplicationWithExistingModel(appName, username, modelName string) string {
Copy link
Member

Choose a reason for hiding this comment

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

This does not test an existing model - you need to add a model data source to the plan.

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 create the model outside of Terraform for the reasons mentioned in the tests,

// We do this outside of Terraform because we don't currently support
// creating a model owned by a different user than the one making the connection.

Then test we can deploy an application into that model.


resource "juju_application" "this" {
name = "{{.AppName}}"
model = "{{.Username}}/${juju_model.this.name}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned this is a step in the opposite direction from where we want to be with referencing a model inside of other resources, by using the model UUID. The idea is that if we have the model idea, no cache will be necessary.

Also, looking at this example, you've created a model XYZ for the default user, then are deploying a an application to model XYZ owned by a different user. The linkage inferred by using juju_model.this.name appears to be incorrect.

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 agree that if we don't reference models by name and rather by UUID then this problem goes away. Happy to close this PR in favour of that change but I was aiming for something that maintains backwards compatability.

Also, looking at this example, you've created a model XYZ for the default user, then are deploying a an application to model XYZ owned by a different user. The linkage inferred by using juju_model.this.name appears to be incorrect.

In this case the "different user" is the same as the "default user", we pass in "admin" who is the user making the connection. If we were to pass in a different user the test should fail with "model not found", I can add that as a test case.
I'm not sure I understand what you mean by "linkage inferred by using juju_model.this.name appears to be incorrect."?

@kian99
Copy link
Contributor Author

kian99 commented Sep 18, 2024

Closing in favour of an approach where we use model UUIDs as reference with a possibility of maintaining backwards compatability.

@kian99 kian99 closed this Sep 18, 2024
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.

Incorrect behaviour when multiple models have the same name
2 participants