Initial implementation for system backend activation flags#2861
Initial implementation for system backend activation flags#2861tejashwiniingalagi wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial Terraform Plugin Framework support for managing/reading Vault system backend activation flags via a new singleton resource and companion data source.
Changes:
- Introduces
vault_activation_flagsresource to reconcile/activate flags throughsys/activation-flags/*/activate. - Introduces
vault_activation_flagsdata source to read activated and unactivated flags fromsys/activation-flags. - Registers the new resource/data source with the framework provider and adds new const field names.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/vault/sys/activation_flags_resource.go | New singleton resource implementation for activating/reconciling activation flags. |
| internal/vault/sys/activation_flags_resource_test.go | Unit tests for resource schema/metadata and shared helper functions. |
| internal/vault/sys/activation_flags_data_source.go | New data source implementation for reading activation flags state from Vault. |
| internal/vault/sys/activation_flags_data_source_test.go | Unit tests for data source schema/metadata and model population. |
| internal/provider/fwprovider/provider.go | Registers the new resource and data source with the framework provider. |
| internal/consts/consts.go | Adds activated_flags / unactivated_flags field constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return false | ||
| } | ||
|
|
||
| if vaultResp == nil { |
There was a problem hiding this comment.
readActivationFlagsState returns false when vaultResp == nil without adding a diagnostic. In Create/Update this causes an early return with no error and no state set, which can lead to an inconsistent apply outcome. Consider adding a diagnostic (e.g., errutil.VaultReadResponseNil() or a clearer "endpoint unsupported" error) when the read response is nil so callers always get a failure signal.
| if vaultResp == nil { | |
| if vaultResp == nil { | |
| diagnostics.AddError(errutil.VaultReadResponseNil()) |
| return | ||
| } | ||
|
|
||
| resp.State.RemoveResource(ctx) |
There was a problem hiding this comment.
In Read, a nil response from sys/activation-flags causes the resource to be removed from state (resp.State.RemoveResource). For a singleton sys endpoint this is more likely to mean the endpoint is unavailable/unsupported rather than a deletable object disappearing; other sys resources treat a nil read response as an error (e.g., internal/vault/sys/password_policy.go:175-180). Consider returning a diagnostic error instead of removing state when the activation flags endpoint returns nil.
| resp.State.RemoveResource(ctx) | |
| resp.Diagnostics.AddError( | |
| "Failed to read activation flags", | |
| fmt.Sprintf("Vault returned no data for %q; this singleton system endpoint may be unavailable or unsupported", activationFlagsPath), | |
| ) |
| func readCurrentActivatedFlags(ctx context.Context, cli *api.Client) ([]string, error) { | ||
| flagsState, err := readActivationFlags(ctx, cli) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return flagsState.Activated, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
readCurrentActivatedFlags is not referenced anywhere in the package. If it's not intended for near-term use, consider removing it to avoid dead code; otherwise, add a call site or a comment explaining the planned usage.
| func readCurrentActivatedFlags(ctx context.Context, cli *api.Client) ([]string, error) { | |
| flagsState, err := readActivationFlags(ctx, cli) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return flagsState.Activated, nil | |
| } |
| // Create is called during terraform apply | ||
| func (r *ActivationFlagsResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { | ||
| var data ActivationFlagsModel | ||
|
|
||
| resp.Diagnostics.Append(req.Plan.Get(ctx, &data)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| cli, err := client.GetClient(ctx, r.Meta(), data.Namespace.ValueString()) | ||
| if err != nil { | ||
| resp.Diagnostics.AddError(errutil.ClientConfigureErr(err)) | ||
| return | ||
| } | ||
|
|
||
| desiredFlags, ok := getDesiredActivatedFlags(ctx, data, &resp.Diagnostics) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| if err := reconcileActivatedFlags(ctx, cli, desiredFlags); err != nil { | ||
| resp.Diagnostics.AddError(errutil.VaultCreateErr(err)) | ||
| return | ||
| } | ||
|
|
||
| if !readActivationFlagsState(ctx, cli, &data, &resp.Diagnostics) { | ||
| return | ||
| } | ||
|
|
||
| resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) | ||
| } |
There was a problem hiding this comment.
This PR introduces a new resource with behavior that depends on Vault’s sys/activation-flags endpoints (read + per-flag activation writes), but there are no acceptance tests validating it against a real Vault instance. Since internal/vault/sys already uses TestAcc* coverage for other resources (e.g., password policy), please add at least one testacc exercising create/update/import (and namespace if applicable).
| // Read is called when the data source is refreshed | ||
| func (d *ActivationFlagsDataSource) Read(ctx context.Context, req datasource.ReadRequest, resp *datasource.ReadResponse) { | ||
| var data ActivationFlagsDataSourceModel | ||
|
|
||
| resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| cli, err := client.GetClient(ctx, d.Meta(), data.Namespace.ValueString()) | ||
| if err != nil { | ||
| resp.Diagnostics.AddError(errutil.ClientConfigureErr(err)) | ||
| return | ||
| } | ||
|
|
||
| vaultResp, err := cli.Logical().ReadWithContext(ctx, activationFlagsPath) | ||
| if err != nil { | ||
| resp.Diagnostics.AddError( | ||
| "Error Reading Activation Flags", | ||
| "Error reading activation flags from Vault: "+err.Error(), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| if vaultResp == nil { | ||
| resp.Diagnostics.AddError( | ||
| "No Activation Flags Found", | ||
| "No activation flags found at "+activationFlagsPath, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| resp.Diagnostics.Append(populateActivationFlagsDataSourceModel(ctx, &data, vaultResp.Data)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) | ||
| } |
There was a problem hiding this comment.
This new data source hits Vault’s sys/activation-flags endpoint but only has unit-level tests. The provider repo includes extensive acceptance coverage for data sources (see existing vault/data_source_*_test.go patterns); please add a testacc that reads data "vault_activation_flags" and asserts the activated/unactivated lists (and namespace behavior if relevant).
Description
Checklist
Output from acceptance testing:
Community Note
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.