Hamza/hackweek0725 extend vault config #32

HamzaShili65 · 2025-07-24T18:13:54Z

Adds support for the remaining Vault configs to the vault_start resource. The configs added are:

- ha_storage
- user_lockout
- reporting
- cache_size
- disable_cache
- disable_mlock
- plugin_directory
- plugin_tmpdir
- plugin_file_uid
- plugin_file_permissions
- default_lease_ttl
- max_lease_ttl
- default_max_request_duration
- detect_deadlocks
- raw_storage_endpoint
- introspection_endpoint
- pid_file
- enable_response_header_hostname
- enable_response_header_raft_node_id
- log_format
- log_file
- log_rotate_duration
- log_rotate_bytes
- log_rotate_max_files
- experiments
- imprecise_lease_role_tracking
- enable_post_unseal_trace
- post_unseal_trace_directory

WIP: Add acceptance test for the remaining configs

HamzaShili65 · 2025-07-24T18:24:34Z

internal/plugin/resource_vault_start.go

 	// Manage the vault systemd service ourselves unless it has explicitly been
 	// set that we should not.
-	if manage, set := s.ManageService.Get(); !set || (set && manage) {
+	if manage, set := s.ManageService.Get(); !set || manage {


The previous expression was always True

ryancragun · 2025-07-24T19:10:15Z

Looks like there are serialization errors for the new config. We'll want to add test for all the new optional config: https://github.com/hashicorp-forge/terraform-provider-enos/blob/main/internal/plugin/resource_vault_start_test.go#L19 as well as coverage for the new attributes: https://github.com/hashicorp-forge/terraform-provider-enos/blob/main/internal/plugin/resource_vault_start_test.go#L82

ryancragun · 2025-07-24T19:38:56Z

We'll also want to set the new attributes in our scenarios. Here we configure the leader and follower nodes in our test scenarios. We should consider setting all of the new attributes. Where possible, split them between leader and follower so that we ensure coverage of the attribute but also that they are optional.

ryancragun · 2025-07-29T21:04:18Z

re: The scenario failure, if we can duplicate it with the same variants locally then it's likely either an issue with the provider that has already existed or is present when testing that version of Vault.

Things that we could do but are not required:

Upload the failure logs on fail like we do with vault
Remove unsupported versions of Vault from the test matrix. The oldest version we really ought to care about is 1.16.x+ent.

ryancragun

Things are looking good! In addition to the inline feedback we'll also want to bump the VERSION file as it would be nice to release this after we get the two PR's in.

We'll also want to add the new attributes into the enos_vault_start resources in our enos scenarios. Since these all ought to be optional I'd suggest splitting the attributes up and testing them across both instances of the resource.

ryancragun · 2025-08-06T19:19:33Z

internal/plugin/resource_vault_start_test.go

 		"usage_gauge_period":        "30m",
 		"maximum_gague_cardinality": 100,
 	})
+	vaultCfg.UI.Set(true)


I like that we're testing optional attributes being set but most of the time our issues with optional attributes arise when we think they're optional but they really are not. Perhaps we should have another test case that only provides required inputs to ensure that it plans correctly.

ryancragun · 2025-08-06T19:22:30Z

internal/plugin/vault_config_experiments.go

+	return tftypes.NewValue(s.Terraform5Type(), vals)
+}
+
+// Add Set method and configSet for experiments


Lets move the doc comment to the implementation below.

ryancragun · 2025-08-06T19:24:08Z

internal/plugin/resource_vault_start_test.go

+	vaultCfg.PluginDirectory.Set("/opt/plugins")
+	vaultCfg.PluginTmpdir.Set("/tmp/plugins")
+	vaultCfg.PluginFileUID.Set(1001)
+	vaultCfg.PluginFilePermissions.Set(0644)


We'll need to use octal notation to please the linters

ryancragun · 2025-08-06T19:28:34Z

internal/plugin/vault_config_experiments.go

+	"github.com/hashicorp/terraform-plugin-go/tftypes"
+)
+
+type vaultExperimentsConfig struct {


Given the values here, can't we use *tfStringSlice instead of creating an entirely new type?

Example: https://github.com/hashicorp-forge/terraform-provider-enos/blob/main/internal/plugin/resource_vault_init.go#L40

ryancragun · 2025-08-06T19:29:29Z

internal/plugin/resource_vault_start.go

 - ^config.seals.tertiary.type^ (String) The Vault [seal](https://developer.hashicorp.com/vault/docs/configuration/seal) type
 - ^config.seals.tertiary.attributes^ (String) The Vault [seal](https://developer.hashicorp.com/vault/docs/configuration/seal) parameters for the given seal type
 - ^config.telemetry^ (Object) The Vault [telemetry](https://developer.hashicorp.com/vault/docs/configuration/telemetry#telemetry-parameters) stanza
+- ^config.ha_storage^ (Object) The Vault [ha_storage](https://developer.hashicorp.com/vault/docs/internals/high-availability?productSlug=vault&tutorialSlug=raft&tutorialSlug=raft-ha-storage) stanza


We'll need to run make docs before merge to ensure our rendered docs are accurate.

ryancragun · 2025-08-06T19:37:01Z

internal/plugin/vault_config_reporting.go

+	"github.com/hashicorp/terraform-plugin-go/tftypes"
+)
+
+type vaultReportingLicenseConfig struct {


Did we try to use a dynamicPseudoTypeBlock for this instead of creating an individual type? I suppose the benefit here is having named attributes in a struct vs keys in a map, so if that's something we need then I understand, but at first glance it feels largely the same.

If we do end up keeping this I want to make sure that all of none of the keys are required in tests. So we'll want to test setting no reporting config, all of it, and some of it.

ryancragun · 2025-08-06T19:38:10Z

internal/plugin/vault_config_reporting.go

+	return tftypes.NewValue(tftypes.Object{AttributeTypes: attrs}, vals)
+}
+
+// Add Set methods and configSet types for reporting


Lets fix the doc comments in this file.

ryancragun · 2025-08-06T19:42:23Z

internal/plugin/vault_config_user_lockout.go

+	"github.com/hashicorp/terraform-plugin-go/tftypes"
+)
+
+type vaultUserLockoutConfig struct {


I have much the same opinion about this as I do the reporting config. It comes down to case of tradeoffs:

With the dynamic pseudo type block we can support mixed schema automatically so we don't have to deal with the optional attributes dance. It also allows us to future proof new configuration in some cases because when we render the config on the remote target we're iterating over a map of keys and writing the values, trusting in the configuration that has been passed to be valid.

With a new type for the block we now have to maintain a strict type and deal with optional attributes ourselves. We also have to individually add any new config if it changes. The upside of course is that we have strict validation, but we can also do that kind of validation with the dynamic pseudo type block if we want to, we'll just have to deal with a map instead of a struct.

If there are reasons for a new type we'll want to follow the same criteria: tests for none, some, and all of the attributes.

ryancragun · 2025-08-06T20:43:46Z

internal/plugin/resource_vault_start.go

+	}
+	if val, ok := c.PluginDirectory.Get(); ok {
+		if configMode == "file" {
+			hclBuilder.AppendAttribute("plugin_directory", val)


Do we also want to create the directory?

HamzaShili65 added 2 commits July 23, 2025 18:41

add config types for reporting, user_lockout, and experiments

0e3e721

add support for remaining vault configs

e8e3f99

HamzaShili65 commented Jul 24, 2025

View reviewed changes

fmt

aac3b2f

HamzaShili65 requested a review from ryancragun July 24, 2025 18:29

HamzaShili65 added 5 commits July 24, 2025 13:56

fmt

ecf80a4

convert user_lockout and reporting struct types to dynamic-pseudo-type

de22276

fmt

85f513d

extend interfaces to add a set method

2ed6b6c

extend unit and acceptance tests to test all vault's configs

7580008

ryancragun requested changes Aug 6, 2025

View reviewed changes

ryancragun reviewed Aug 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Hamza/hackweek0725 extend vault config #32

Hamza/hackweek0725 extend vault config #32

Uh oh!

HamzaShili65 commented Jul 24, 2025

Uh oh!

HamzaShili65 Jul 24, 2025

Uh oh!

ryancragun commented Jul 24, 2025

Uh oh!

ryancragun commented Jul 24, 2025 •

edited

Loading

Uh oh!

ryancragun commented Jul 29, 2025

Uh oh!

ryancragun left a comment

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

ryancragun Aug 6, 2025

Uh oh!

Uh oh!

Hamza/hackweek0725 extend vault config #32

Are you sure you want to change the base?

Hamza/hackweek0725 extend vault config #32

Uh oh!

Conversation

HamzaShili65 commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryancragun commented Jul 24, 2025

Uh oh!

ryancragun commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryancragun commented Jul 29, 2025

Uh oh!

ryancragun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryancragun commented Jul 24, 2025 •

edited

Loading