From 635571df2afa7b7dd68b2191eaff860c6c9ce316 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Tue, 10 Sep 2024 15:50:21 +0200 Subject: [PATCH] feat: add avoid jaas validators to existing resources - Ensure certain resources cannot be used with JAAS and add tests for this. - Tweak the error messaging based on previous feedback. - Add constructors for JAAS validators. --- internal/juju/client.go | 10 ++++++-- internal/provider/resource_access_generic.go | 2 +- internal/provider/resource_access_model.go | 7 ++++++ .../provider/resource_access_model_test.go | 24 +++++++++++++++++++ internal/provider/resource_user.go | 8 +++++++ internal/provider/resource_user_test.go | 19 ++++++++++++++- internal/provider/validator_avoid_jaas.go | 23 +++++++++++++----- internal/provider/validator_require_jaas.go | 10 ++++++-- 8 files changed, 91 insertions(+), 12 deletions(-) diff --git a/internal/juju/client.go b/internal/juju/client.go index c8ba7dbe..85448b0d 100644 --- a/internal/juju/client.go +++ b/internal/juju/client.go @@ -10,6 +10,7 @@ import ( "sync" "time" + jaasApi "github.com/canonical/jimm-go-sdk/v3/api" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/juju/errors" "github.com/juju/juju/api" @@ -125,13 +126,18 @@ var isJAAS bool // IsJAAS uses a synchronisation object to only perform the check once and return the same result. func (sc *sharedClient) IsJAAS(defaultVal bool) bool { checkJAASOnce.Do(func() { + isJAAS = defaultVal conn, err := sc.GetConnection(nil) if err != nil { - isJAAS = defaultVal return } defer conn.Close() - isJAAS = conn.BestFacadeVersion("JIMM") != 0 + jc := jaasApi.NewClient(conn) + _, err = jc.ListControllers() + if err == nil { + isJAAS = true + return + } }) return isJAAS } diff --git a/internal/provider/resource_access_generic.go b/internal/provider/resource_access_generic.go index eb0c033d..9cc86e27 100644 --- a/internal/provider/resource_access_generic.go +++ b/internal/provider/resource_access_generic.go @@ -72,7 +72,7 @@ type genericJAASAccessModel struct { // ConfigValidators sets validators for the resource. func (r *genericJAASAccessResource) ConfigValidators(ctx context.Context) []resource.ConfigValidator { return []resource.ConfigValidator{ - RequiresJAASValidator{Client: r.client}, + NewRequiresJAASValidator(r.client), resourcevalidator.AtLeastOneOf( path.MatchRoot("users"), path.MatchRoot("groups"), diff --git a/internal/provider/resource_access_model.go b/internal/provider/resource_access_model.go index 1eac8fb7..8979b2c1 100644 --- a/internal/provider/resource_access_model.go +++ b/internal/provider/resource_access_model.go @@ -52,6 +52,13 @@ func (a *accessModelResource) Metadata(_ context.Context, req resource.MetadataR resp.TypeName = req.ProviderTypeName + "_access_model" } +// ConfigValidators sets validators for the resource. +func (r *accessModelResource) ConfigValidators(ctx context.Context) []resource.ConfigValidator { + return []resource.ConfigValidator{ + NewAvoidJAASValidator(r.client, "juju_jaas_access_model"), + } +} + func (a *accessModelResource) Schema(_ context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { resp.Schema = schema.Schema{ Description: "A resource that represent a Juju Access Model.", diff --git a/internal/provider/resource_access_model_test.go b/internal/provider/resource_access_model_test.go index e93858ac..e2c4cfe1 100644 --- a/internal/provider/resource_access_model_test.go +++ b/internal/provider/resource_access_model_test.go @@ -106,6 +106,30 @@ func TestAcc_ResourceAccessModel_UpgradeProvider(t *testing.T) { }) } +func TestAcc_ResourceAccessModel_ErrorWhenUsedWithJAAS(t *testing.T) { + OnlyTestAgainstJAAS(t) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: frameworkProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccResourceAccessModelFixedUser(), + ExpectError: regexp.MustCompile("This resource is not supported with JAAS"), + }, + }, + }) +} + +func testAccResourceAccessModelFixedUser() string { + return ` +resource "juju_access_model" "test" { + access = "write" + model = "foo" + users = ["bob"] +}` +} + func testAccResourceAccessModel(userName, userPassword, modelName, access string) string { return fmt.Sprintf(` resource "juju_user" "test-user" { diff --git a/internal/provider/resource_user.go b/internal/provider/resource_user.go index 4c20a209..6ba355f2 100644 --- a/internal/provider/resource_user.go +++ b/internal/provider/resource_user.go @@ -51,6 +51,14 @@ func (r *userResource) Metadata(ctx context.Context, req resource.MetadataReques resp.TypeName = req.ProviderTypeName + "_user" } +// ConfigValidators sets validators for the resource. +func (r *userResource) ConfigValidators(ctx context.Context) []resource.ConfigValidator { + return []resource.ConfigValidator{ + // There is no JAAS object that replaces the user resource since JAAS users come from an external identity provider. + NewAvoidJAASValidator(r.client, ""), + } +} + func (r *userResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { // The User resource maps to a juju user that is operated via // `juju add-user`, `juju remove-user` diff --git a/internal/provider/resource_user_test.go b/internal/provider/resource_user_test.go index 3658b962..fa3823d4 100644 --- a/internal/provider/resource_user_test.go +++ b/internal/provider/resource_user_test.go @@ -5,6 +5,7 @@ package provider import ( "fmt" + "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -26,7 +27,6 @@ func TestAcc_ResourceUser(t *testing.T) { Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(resourceName, "name", userName), ), - PreConfig: func() { testAccPreCheck(t) }, }, { Destroy: true, @@ -78,3 +78,20 @@ func TestAcc_ResourceUser_UpgradeProvider(t *testing.T) { }, }) } + +func TestAcc_ResourceUser_ErrorWhenUsedWithJAAS(t *testing.T) { + OnlyTestAgainstJAAS(t) + userName := acctest.RandomWithPrefix("tfuser") + userPassword := acctest.RandomWithPrefix("tf-test-user") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProtoV6ProviderFactories: frameworkProviderFactories, + Steps: []resource.TestStep{ + { + Config: testAccResourceUser(userName, userPassword), + ExpectError: regexp.MustCompile("This resource is not supported with JAAS"), + }, + }, + }) +} diff --git a/internal/provider/validator_avoid_jaas.go b/internal/provider/validator_avoid_jaas.go index 2e18e167..ca225052 100644 --- a/internal/provider/validator_avoid_jaas.go +++ b/internal/provider/validator_avoid_jaas.go @@ -19,8 +19,19 @@ var _ resource.ConfigValidator = &AvoidJAASValidator{} // AvoidJAASValidator enforces that the resource is not used with JAAS. // Useful to direct users to more capable resources. type AvoidJAASValidator struct { - Client *juju.Client - PreferredObject string + client *juju.Client + preferredObject string +} + +// NewAvoidJAASValidator creates a new validator that can be used with resources or +// data sources to enforce that the resource cannot be used with JAAS. +// Provide a non-empty string for preferredObject to point users to an alternate object in the error messsage. +// If preferredObject is empty, no hint on an alternate object will be offered in the error message. +func NewAvoidJAASValidator(client *juju.Client, preferredObject string) AvoidJAASValidator { + return AvoidJAASValidator{ + client: client, + preferredObject: preferredObject, + } } // Description returns a plain text description of the validator's behavior, suitable for a practitioner to understand its impact. @@ -49,15 +60,15 @@ func (v AvoidJAASValidator) validate() diag.Diagnostics { // Return without error if a nil client is detected. // This is possible since validation is called at various points throughout resource creation. - if v.Client != nil && v.Client.IsJAAS() { + if v.client != nil && v.client.IsJAAS() { hint := "" - if v.PreferredObject != "" { - hint = "Try the " + v.PreferredObject + " resource instead." + if v.preferredObject != "" { + hint = "Try the " + v.preferredObject + " resource instead." } diags.AddError("Invalid use of resource with JAAS.", "This resource is not supported with JAAS. "+ hint+ - "JAAS offers additional enterprise features through the use of dedicated resources. "+ + `JAAS offers additional enterprise features through the use of dedicated resources (those with "jaas" in the name). `+ "See https://jaas.ai/ for more details.") } return diags diff --git a/internal/provider/validator_require_jaas.go b/internal/provider/validator_require_jaas.go index 7497dbbe..ed18b43e 100644 --- a/internal/provider/validator_require_jaas.go +++ b/internal/provider/validator_require_jaas.go @@ -18,7 +18,13 @@ var _ resource.ConfigValidator = &RequiresJAASValidator{} // RequiresJAASValidator enforces that the resource can only be used with JAAS. type RequiresJAASValidator struct { - Client *juju.Client + client *juju.Client +} + +func NewRequiresJAASValidator(client *juju.Client) RequiresJAASValidator { + return RequiresJAASValidator{ + client: client, + } } // Description returns a plain text description of the validator's behavior, suitable for a practitioner to understand its impact. @@ -47,7 +53,7 @@ func (v RequiresJAASValidator) validate() diag.Diagnostics { // Return without error if a nil client is detected. // This is possible since validation is called at various points throughout resource creation. - if v.Client != nil && !v.Client.IsJAAS() { + if v.client != nil && !v.client.IsJAAS() { diags.AddError("Attempted use of resource without JAAS.", "This resource can only be used with JAAS, which offers additional enterprise features - see https://jaas.ai/ for more details.") }