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.

Sign up for GitHub

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: add jaas validators #541

Merged
merged 3 commits into from
Aug 12, 2024
Merged

feat: add jaas validators #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1210c5
feat: add jaas validators
kian99 Aug 6, 2024
0be5485
feat: added missing godocs
kian99 Aug 6, 2024
e1d0097
feat: pr comments
kian99 Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions internal/juju/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ type Client struct {
SSHKeys sshKeysClient
Users usersClient
Secrets secretsClient

isJAAS func() bool
}

// IsJAAS returns a boolean to indicate whether the controller configured is a JAAS controller.
// JAAS controllers offer additional functionality for permission management.
func (c Client) IsJAAS() bool {
return c.isJAAS()
}

type jujuModel struct {
Expand Down Expand Up @@ -82,6 +90,12 @@ func NewClient(ctx context.Context, config ControllerConfiguration) (*Client, er
modelUUIDcache: make(map[string]jujuModel),
subCtx: tflog.NewSubsystem(ctx, LogJujuClient),
}
// Client ID and secret are only set when connecting to JAAS. Use this as a fallback
// value if connecting to the controller fails.
defaultJAASCheck := false
if config.ClientID != "" && config.ClientSecret != "" {
defaultJAASCheck = true
}

return &Client{
Applications: *newApplicationClient(sc),
Expand All @@ -93,9 +107,33 @@ func NewClient(ctx context.Context, config ControllerConfiguration) (*Client, er
SSHKeys: *newSSHKeysClient(sc),
Users: *newUsersClient(sc),
Secrets: *newSecretsClient(sc),
isJAAS: func() bool { return sc.IsJAAS(defaultJAASCheck) },
}, nil
}

var checkJAASOnce sync.Once
var isJAAS bool

// IsJAAS checks if the controller is a JAAS controller.
// It does this by checking whether it offers the "JIMM" facade which
// will only ever be offered by JAAS. The method accepts a default value
// and doesn't return an error because callers are not expected to fail if
// they can't determine whether they are connecting to JAAS.
//
// 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() {
conn, err := sc.GetConnection(nil)
if err != nil {
isJAAS = defaultVal
return
}
defer conn.Close()
isJAAS = conn.BestFacadeVersion("JIMM") != 0
kian99 marked this conversation as resolved.
Show resolved Hide resolved
})
return isJAAS
}

// GetConnection returns a juju connection for use creating juju
// api clients given the provided model name.
func (sc *sharedClient) GetConnection(modelName *string) (api.Connection, error) {
Expand Down
64 changes: 64 additions & 0 deletions internal/provider/validator_avoid_jaas.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright 2024 Canonical Ltd.
// Licensed under the Apache License, Version 2.0, see LICENCE file for details.

package provider

import (
"context"

"github.com/hashicorp/terraform-plugin-framework/datasource"
"github.com/hashicorp/terraform-plugin-framework/diag"
"github.com/hashicorp/terraform-plugin-framework/resource"

"github.com/juju/terraform-provider-juju/internal/juju"
kian99 marked this conversation as resolved.
Show resolved Hide resolved
)

var _ datasource.ConfigValidator = &AvoidJAASValidator{}
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
}

// Description returns a plain text description of the validator's behavior, suitable for a practitioner to understand its impact.
func (v AvoidJAASValidator) Description(ctx context.Context) string {
kian99 marked this conversation as resolved.
Show resolved Hide resolved
return v.MarkdownDescription(ctx)
}

// MarkdownDescription returns a markdown formatted description of the validator's behavior, suitable for a practitioner to understand its impact.
kian99 marked this conversation as resolved.
Show resolved Hide resolved
func (v AvoidJAASValidator) MarkdownDescription(_ context.Context) string {
return "Enforces that this resource should not be used with JAAS"
}

// ValidateResource performs the validation on the data source.
func (v AvoidJAASValidator) ValidateDataSource(ctx context.Context, req datasource.ValidateConfigRequest, resp *datasource.ValidateConfigResponse) {
resp.Diagnostics = v.validate()
}

// ValidateResource performs the validation on the resource.
func (v AvoidJAASValidator) ValidateResource(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) {
resp.Diagnostics = v.validate()
}

// validate runs the main validation logic of the validator, reading configuration data out of `config` and returning with diagnostics.
func (v AvoidJAASValidator) validate() diag.Diagnostics {
var diags 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() {
Copy link
Member

Choose a reason for hiding this comment

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

issue: a not nil client will hopefully be caught before this point, however please follow the known pattern along the lines of:

	// Check first if the client is configured
	if a.client == nil {
		addClientNotConfiguredError(&resp.Diagnostics, "access model", "create")
		return
	}

For both validators

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 don't think we should do that here, the validator is called multiple times throughout the process of resource creation, it is expected that the client will be nil at certain stages before the client has been created.
I'll leave a comment to that effect.

Copy link
Member

Choose a reason for hiding this comment

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

The client I'm thinking of is created as part of the jujuProvider.Config method at the very start. It's created before any resource schema is known. This block of code is recommended by terraform framework tutorials.

I'm unclear why you believe the client may be nil during resource creation?

The check is more of a paranoia check as the client should be available.

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 added the nil check because when I was investigating when the validator is called you will find that it is executed multiple times, including before the resource' configure method is called.

Only when the resource's configure method is called, is the resource's client populated (by using the client created in jujuProvider.Config) and no longer nil.

The easiest way to see this is to run one of the integration tests in debug mode and step through the code. There you can see that the client will be nil at certain points and if we were to return an error because the client was not yet configured the resource would always fail to be created because this value is always nil before the configure method is called.

hint := ""
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. "+
Copy link
Member

Choose a reason for hiding this comment

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

The sentence about JAAS is jarring because it shifts the topic from resources (the previous two sentences) to JAAS, and the connection to resources is unclear -- i.e., as a user, what I'm getting is that dedicated resources can help me take advantage of the additional enterprise features of JAAS, but it's not clear to me what "dedicated resources" is referring to and what I'm supposed to do with this information (i.e., am I suppose to not use the resource suggested in the hint?)

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 see what you mean. What if we changed this sentence to,

"JAAS offers additional enterprise features through the use of dedicated resources (those with "jaas" in the name)."

The hint is created above and supposed to lead the user to the correct resource to use - it will say something like "Try the juju_jaas_access_model resource instead."

"See https://jaas.ai/ for more details.")
}
return diags
}
56 changes: 56 additions & 0 deletions internal/provider/validator_require_jaas.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2024 Canonical Ltd.
// Licensed under the Apache License, Version 2.0, see LICENCE file for details.

package provider

import (
"context"

"github.com/hashicorp/terraform-plugin-framework/datasource"
"github.com/hashicorp/terraform-plugin-framework/diag"
"github.com/hashicorp/terraform-plugin-framework/resource"

"github.com/juju/terraform-provider-juju/internal/juju"
kian99 marked this conversation as resolved.
Show resolved Hide resolved
)

var _ datasource.ConfigValidator = &RequiresJAASValidator{}
var _ resource.ConfigValidator = &RequiresJAASValidator{}

// RequiresJAASValidator enforces that the resource can only be used with JAAS.
type RequiresJAASValidator struct {
Client *juju.Client
}

// Description returns a plain text description of the validator's behavior, suitable for a practitioner to understand its impact.
func (v RequiresJAASValidator) Description(ctx context.Context) string {
kian99 marked this conversation as resolved.
Show resolved Hide resolved
return v.MarkdownDescription(ctx)
}

// MarkdownDescription returns a markdown formatted description of the validator's behavior, suitable for a practitioner to understand its impact.
kian99 marked this conversation as resolved.
Show resolved Hide resolved
func (v RequiresJAASValidator) MarkdownDescription(_ context.Context) string {
return "Enforces that this resource can only be used with JAAS"
}

// ValidateResource performs the validation on the data source.
func (v RequiresJAASValidator) ValidateDataSource(ctx context.Context, req datasource.ValidateConfigRequest, resp *datasource.ValidateConfigResponse) {
resp.Diagnostics = v.validate()
}

// ValidateResource performs the validation on the resource.
func (v RequiresJAASValidator) ValidateResource(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) {
resp.Diagnostics = v.validate()
}

// validate runs the main validation logic of the validator, reading configuration data out of `config` and returning with diagnostics.
kian99 marked this conversation as resolved.
Show resolved Hide resolved
func (v RequiresJAASValidator) validate() diag.Diagnostics {
var diags 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() {
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.")
}

return diags
}
Loading