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

Add vault_config_ui_custom_message resource #2154

Merged
merged 11 commits into from
Mar 19, 2024

Conversation

marcboudreau
Copy link
Contributor

@marcboudreau marcboudreau commented Feb 23, 2024

Description

This PR introduces a new resource named vault_config_ui_custom_message to the provider to allow managing the newly introduced UI Custom Messages in Vault v.1.16.0.

Checklist

  • Added CHANGELOG entry (only for user-facing changes)
  • Acceptance tests where run against all supported Vault Versions

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccConfigUICustomMessage'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -run=TestAccConfigUICustomMessage -timeout 30m ./...
?       github.com/hashicorp/terraform-provider-vault   [no test files]
?       github.com/hashicorp/terraform-provider-vault/cmd/coverage      [no test files]
?       github.com/hashicorp/terraform-provider-vault/cmd/generate      [no test files]
?       github.com/hashicorp/terraform-provider-vault/helper    [no test files]
?       github.com/hashicorp/terraform-provider-vault/internal/consts   [no test files]
ok      github.com/hashicorp/terraform-provider-vault/codegen   0.658s [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/internal/identity/group   [no test files]
?       github.com/hashicorp/terraform-provider-vault/internal/identity/mfa     [no test files]
?       github.com/hashicorp/terraform-provider-vault/internal/pki      [no test files]
?       github.com/hashicorp/terraform-provider-vault/internal/sync     [no test files]
?       github.com/hashicorp/terraform-provider-vault/schema    [no test files]
ok      github.com/hashicorp/terraform-provider-vault/internal/identity/entity  0.859s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/internal/provider 2.097s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/testutil  1.292s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/util      1.446s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/vault     3.706s

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

@marcboudreau marcboudreau force-pushed the marcboudreau/VAULT-22504/custom-message-resource branch from 6ba3ca5 to c9be60e Compare February 23, 2024 16:30
clean up unnecessary code
@marcboudreau
Copy link
Contributor Author

I've added the do-not-merge label for now, because this change should only be adopted once Vault v.1.16.0 is release.

Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

Looking great so far! Thanks @marcboudreau ! Just a few adjustments needed from what I can see. Great work! :)

CHANGELOG.md Outdated
@@ -1,5 +1,8 @@
## Unreleased

FEATURES:
* Add new resource `vault_config_ui_custom_message`: ([#2154](https://github.com/hashicorp/terraform-provider-vault/pull/2154)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Add new resource `vault_config_ui_custom_message`: ([#2154](https://github.com/hashicorp/terraform-provider-vault/pull/2154)).
* Add new resource `vault_config_ui_custom_message`. Requires Vault 1.16+: ([#2154](https://github.com/hashicorp/terraform-provider-vault/pull/2154)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this down to the "FEATURES" block below?


func configUICustomMessageResource() *schema.Resource {
return &schema.Resource{
CreateContext: configUICustomMessageCreate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this endpoint is only available in Vault 1.16 we can add a minimum version requirement to the entire resource

Suggested change
CreateContext: configUICustomMessageCreate,
CreateContext: provider.MountCreateContextWrapper(configUICustomMessageCreate, provider.VaultVersion116),

Copy link
Contributor

Choose a reason for hiding this comment

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

If we add this, then we are safe to merge before Vault 1.16 is released. We usually do a TFVP release shortly after the Vault release. So this will allow us to not have to scramble to get things merged and in sync between the two projects. :)

}

if secret == nil || secret.Data == nil {
return diag.Errorf("response from Vault server is empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this block will tell us that the resource does not exist in Vault. In this case, we will want to unset the ID and return nil to signal to Terraform that this object should be removed from state.

Suggested change
return diag.Errorf("response from Vault server is empty")
log.Printf("[WARN] Custom message not found, removing from state")
d.SetId("")
return nil


resource.Test(t, resource.TestCase{
ProviderFactories: providerFactories,
PreCheck: func() { testutil.TestAccPreCheck(t) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PreCheck: func() { testutil.TestAccPreCheck(t) },
PreCheck: func() { testutil.TestAccPreCheck(t) },
SkipIfAPIVersionLT(t, testProvider.Meta(), provider.VaultVersion116)

Copy link
Contributor

Choose a reason for hiding this comment

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

This will make sure the test doesn't run against Vault versions that don't support this resource.

@fairclothjm fairclothjm added this to the 3.26.0 milestone Feb 23, 2024
@fairclothjm fairclothjm modified the milestones: 3.26.0, 3.27.0 Feb 28, 2024
@fairclothjm fairclothjm modified the milestones: 3.27.0, 4.0.0 Mar 6, 2024
@@ -146,7 +150,9 @@ func configUICustomMessageRead(ctx context.Context, d *schema.ResourceData, meta
}

if secret == nil || secret.Data == nil {
return diag.Errorf("response from Vault server is empty")
log.Printf("response from Vault server is empty")
Copy link
Contributor

@fairclothjm fairclothjm Mar 11, 2024

Choose a reason for hiding this comment

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

We need to prepend the log with a level. I would recommend either DEBUG or WARN:

Suggested change
log.Printf("response from Vault server is empty")
log.Printf("[DEBUG] response from Vault server is empty for %q, removing from state", id)


log.Printf("[DEBUG] Reading custom message %q", id)
secret, e := client.Sys().ReadUICustomMessage(id)
if e != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this endpoint return a 404 error if the resource isn't found or is the next if block how we know the resource does not exist in Vault? If Vault does return a 404, we should remove it from TF state (d.setId("")) here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is an example of doing this check:

if err != nil {
if util.Is404(err) {
log.Printf("[DEBUG] AppRole auth backend role %q not found, removing from state", path)
d.SetId("")
return nil
} else {
return diag.Errorf("error deleting AppRole auth backend role %q, err=%s", path, err)
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vault does indeed return an HTTP 404 error if no message with the ID provided to the ReadUICustomMessage function exists.

Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

Approved as long as we make the request changes. Thanks @marcboudreau !

@fairclothjm fairclothjm removed this from the 4.0.0 milestone Mar 13, 2024
@fairclothjm
Copy link
Contributor

Clearing out the 4.0.0 milestone for now and we will do another release closer to Vault GA.

Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @marcboudreau !

@fairclothjm fairclothjm added this to the 4.1.0 milestone Mar 19, 2024
@marcboudreau marcboudreau merged commit 7f4b29f into main Mar 19, 2024
12 checks passed
@fairclothjm fairclothjm deleted the marcboudreau/VAULT-22504/custom-message-resource branch March 19, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants