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

Handle multiple organizations for credentials #793

Merged

Conversation

dmalch-hashicorp
Copy link
Contributor

@dmalch-hashicorp dmalch-hashicorp commented Mar 20, 2024

🛠️ Description

Updated the provider's logic to handle zero or multiple organizations associated with configured credentials. Added diagnostics to inform the user when no organizations or multiple organizations are found. Now, it prompts an error, requiring users to specify a particular organization in the HCP provider config block.

🏗️ Acceptance tests

  • Are there any feature flags that are required to use this functionality?
  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

@dmalch-hashicorp dmalch-hashicorp requested a review from a team as a code owner March 20, 2024 18:38
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@manish-hashicorp manish-hashicorp left a comment

Choose a reason for hiding this comment

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

Let me know your thoughts on this.

return nil, diags
}
orgID := listOrgResp.Payload.Organizations[0].ID
if orgLen > 1 {
diags.AddWarning("There is more than one organization associated with the configured credentials.", "The oldest organization is selected as the default. To switch to a specific project within an organization, configure that in the HCP provider config block.")
orgID = getOldestOrganization(listOrgResp.Payload.Organizations).ID
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should fail if creds can access multiple organizations instead of choosing the oldest one. There is no reason for us to believe that user intends to use the old one. Since we don't provide org id or name anywhere in tf plan I am worried that user may ignore this warning and end up creating resources in org that they didn't intend to. The oldest project was chosen when multi-project was added because it was likely the project user had been using but even there I feel it should've actually failed instead of guessing the project.

@binarymatt Would you still consider this a bug if the error message was clear - as in asking the user to select a project since provider cannot decipher that from credential?

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 it's important to avoid users unintentionally creating resources in the wrong organization. Failing credentials that access multiple organizations is a good option to consider.

However, there are also some potential downsides to consider, failing credentials could disrupt the user's workflow, requiring them to take additional steps to specify the organization.

Let's weigh the pros and cons of failing credentials vs. alternative solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

failing credentials could disrupt the user's workflow, requiring them to take additional steps to specify the organization

That is in fact the intention. The user has to clarify which organization the provider should operate on. Unfortunately, user cannot configure organization in the provider block which I think it should since there are now resources that are getting created in the org - like projects and groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was modified to not default to the oldest organization when multiple organizations are associated with the configured credentials. Now, it instead prompts an error, requiring users to specify a particular organization in the HCP provider config block.

@binarymatt
Copy link
Contributor

@manish-hashicorp I think an error message would be a good approach.

diags = append(diags, diag.Diagnostic{
Severity: diag.Error,
Summary: "The configured credentials do not have access to any organization.",
Detail: "Please assign at least one organization to the configured credentials to use this provider.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, there is no place to configure the organization id.

Schema: map[string]*schema.Schema{
"client_id": {
Type: schema.TypeString,
Optional: true,
Description: "The OAuth2 Client ID for API operations.",
},
"client_secret": {
Type: schema.TypeString,
Optional: true,
Description: "The OAuth2 Client Secret for API operations.",
},
"project_id": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.IsUUID,
Description: "The default project in which resources should be created.",
},
"credential_file": {
Type: schema.TypeString,
Optional: true,
Description: "The path to an HCP credential file to use to authenticate the provider to HCP. " +
"You can alternatively set the HCP_CRED_FILE environment variable to point at a credential file as well. " +
"Using a credential file allows you to authenticate the provider as a service principal via client " +
"credentials or dynamically based on Workload Identity Federation.",
},
"workload_identity": {
Type: schema.TypeList,
Optional: true,
Description: "Allows authenticating the provider by exchanging the OAuth 2.0 access token or OpenID Connect " +
"token specified in the `token_file` for a HCP service principal using Workload Identity Federation.",
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"token_file": {
Type: schema.TypeString,
Required: true,
Description: "The path to a file containing a JWT token retrieved from an OpenID Connect (OIDC) or OAuth2 provider.",
},
"resource_name": {
Type: schema.TypeString,
Required: true,
Description: "The resource_name of the Workload Identity Provider to exchange the token with.",
},
},
},
},
},
ProviderMetaSchema: map[string]*schema.Schema{
"module_name": {
Type: schema.TypeString,
Optional: true,
Description: "The name of the module used with the provider. Should be set in the terraform config block of the module.",
},
},
}
It only takes project id. We will need to update it to take org id and run through different workflows (with user creds, sp creds with diff level access) to see it works. This should fix #677.

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, I was confused because there was a field in the config that made me think that it was possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manish-hashicorp Do you have any recommendations on how to rephrase this error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really until we add support for configuring organization which would be the real fix. Looks like that config is set to be the parent of configured or fetched project id.

@dmalch-hashicorp dmalch-hashicorp force-pushed the bugfix/HCPF-1635/browser-based-login-with-multiple-orgs branch from 3475143 to b49e827 Compare March 27, 2024 13:59
Copy link
Contributor

@manish-hashicorp manish-hashicorp left a comment

Choose a reason for hiding this comment

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

Thanks for changing it to project.

Updated the provider's logic to handle zero or multiple organizations
associated with configured credentials. Added diagnostics to inform the
user when no organizations or multiple organizations are found, and
implemented selection of the oldest organization by default.
The code was modified to not default to the oldest organization when
multiple organizations are associated with the configured credentials.
Now, it instead prompts an error, requiring users to specify a
particular organization in the HCP provider config block.
This commit updates the error messages to indicate that a specific
project, rather than an organization, should be configured in the HCP
provider config block. These changes affect both the provider.go and
project_helpers.go files.
@dmalch-hashicorp dmalch-hashicorp force-pushed the bugfix/HCPF-1635/browser-based-login-with-multiple-orgs branch from b49e827 to 9881094 Compare April 4, 2024 09:20
@dmalch-hashicorp dmalch-hashicorp merged commit 125f2b2 into main Apr 4, 2024
6 checks passed
@dmalch-hashicorp dmalch-hashicorp deleted the bugfix/HCPF-1635/browser-based-login-with-multiple-orgs branch April 4, 2024 09:24
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