diff --git a/.agents/skills/acceptance-testing/SKILL.md b/.agents/skills/acceptance-testing/SKILL.md index b6ed039f..f60dcf7d 100644 --- a/.agents/skills/acceptance-testing/SKILL.md +++ b/.agents/skills/acceptance-testing/SKILL.md @@ -28,6 +28,8 @@ nohup bash -c "TF_ACC=1 go test -count=1 ./internal/provider/ -parallel 8 -timeo - Target a single test: `-run 'TestAccBuildingBlock$/'`. - Always tee/redirect output to `/tmp` and **read the log** to investigate — do not re-run the suite piped through `grep`. +- `ApplyAndTest` sets `MESHSTACK_SKIP_VERSION_CHECK` so the provider's anticipated-next-release + version floor can run ahead of the local `develop` backend — expected, no action needed. ## 2. Investigate diff --git a/.agents/skills/scratch-config-testing/SKILL.md b/.agents/skills/scratch-config-testing/SKILL.md new file mode 100644 index 00000000..d4b90b03 --- /dev/null +++ b/.agents/skills/scratch-config-testing/SKILL.md @@ -0,0 +1,87 @@ +--- +name: scratch-config-testing +description: Build and run a meshStack Terraform config in the git-ignored scratch/ dir against a local meshStack, using the dev-built provider binary via dev_overrides. Use when reproducing or debugging a provider bug or a failing acceptance test as a standalone, re-runnable config — by dumping a test's HCL or hand-writing one. +--- + +# scratch-config-testing + +Turn an acceptance test's generated HCL (or hand-written HCL) into a **standalone, +re-runnable** Terraform config under git-ignored `scratch/`, run with the locally-built +provider against a local meshStack. Complements the **acceptance-testing** skill (runs the +suite) and **meshstack-services** skill (brings up the backend). The `testconfig` builders +make a dumped config self-contained: applied to an empty meshStack it creates its full +dependency chain (workspace → dependent resources). + +## Prerequisites + +1. Local meshStack up — see the **meshstack-services** skill. +2. Provider env vars exported (the `http://localhost` guard applies): + + ```bash + set -a && source .env && set +a # MESHSTACK_ENDPOINT, MESHSTACK_API_KEY, MESHSTACK_API_SECRET + ``` + +## One-time setup + +Build the provider to the repo root, then point Terraform at it via a dev-override CLI config +kept separate from your global `~/.terraformrc`: + +```bash +task build # -> ./terraform-provider-meshstack +cat > "$HOME/.terraformrc.dev" <&1 | ``` To bring up the backend services, see the **`meshstack-services`** skill; to run/debug the -full acceptance suite, see the **`acceptance-testing`** skill. +full acceptance suite, see the **`acceptance-testing`** skill. To reproduce a single test (or a +bug) as a standalone, re-runnable config against a local meshStack — dumping a test's HCL with +`MESHSTACK_SCRATCH_DUMP=1` or hand-writing one — see the **`scratch-config-testing`** skill. ### Acceptance test authoring (testconfig, builders, state checks) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff1db829..30cb85c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,14 @@ -# v0.21.1 +# v0.22.0 + +Requires meshStack 2026.24.0 or later due to roll out of one shared meshcloud hosted building block runner. + +BREAKING CHANGES: +- `meshstack_building_block_definition`: For manual building blocks, `version_spec.outputs` is now computed from the API and must be omitted from configuration — the backend derives one output per input. Configuring an output with any `assignment_type` other than `PLATFORM_TENANT_ID` is now rejected. Remove `outputs` blocks from manual building block definitions; you may still declare an output with `assignment_type = "PLATFORM_TENANT_ID"` to mark which output carries the tenant id. FIXES: +- `meshstack_building_block_definition`: Fixed "Provider produced inconsistent result after apply" for manual building blocks whose outputs the backend auto-generates from inputs (e.g. `SINGLE_SELECT`/`STATIC` inputs), including when toggling `version_spec.draft` from `false` to `true` together with input changes ([#131](https://github.com/meshcloud/terraform-provider-meshstack/issues/131), [#176](https://github.com/meshcloud/terraform-provider-meshstack/issues/176)). Outputs are now reconciled from the API response. +- `meshstack_building_block_definition`: Rotating a sensitive input's secret on a released (immutable) version now fails with a clear "Updating a version_spec in non-draft state is not allowed" error instead of an opaque "Failed to determine content hash ... [plaintext]" error ([#196](https://github.com/meshcloud/terraform-provider-meshstack/issues/196)). Set `version_spec.draft = true` to create a new draft version; the secret rotation can be applied in the same step. +- `meshstack_building_block_definition`: Fixed an erroneous "Failed to determine content hash" error for definitions whose `version_spec.inputs`/`version_spec.outputs` contain an entry named `plaintext`. The secret safeguard now inspects the typed secret values instead of matching the literal `plaintext` JSON key, so user-chosen input/output names are no longer mistaken for secrets. - When no `runner_ref` is provided, the new shared building block runner UUID `98520496-627d-43e6-82da-ce499179ff3f` is used which is suitable for all implementation types. Existing `building_block_definition` resources will see a plan change addressing this migration to a single shared runner. Using the old shared runner UUIDs is deprecated but handled gracefully by the API. diff --git a/client/client.go b/client/client.go index 9a1f3668..956b5aaf 100644 --- a/client/client.go +++ b/client/client.go @@ -11,7 +11,7 @@ import ( "github.com/meshcloud/terraform-provider-meshstack/client/version" ) -var MinMeshStackVersion = version.MustParse("2026.23.0") +var MinMeshStackVersion = version.MustParse("2026.24.0") // HttpError represents an HTTP error response with status code. // This error is returned when an HTTP request fails with a non-2XX status code. diff --git a/docs/index.md b/docs/index.md index 69ccd3f9..0e5c7cf5 100644 --- a/docs/index.md +++ b/docs/index.md @@ -9,7 +9,7 @@ description: |- The meshStack terraform provider is an open-source tool, licensed under the MPL-2.0, and is actively maintained by meshcloud GmbH. The provider exposes APIs of meshStack to manage resources as code. -**Note:** This provider version requires meshStack version 2026.23.0 or higher. The provider automatically validates version compatibility during initialization. +**Note:** This provider version requires meshStack version 2026.24.0 or higher. The provider automatically validates version compatibility during initialization. ## Dependency wiring patterns diff --git a/docs/resources/building_block_definition.md b/docs/resources/building_block_definition.md index 117a2eed..d284845e 100644 --- a/docs/resources/building_block_definition.md +++ b/docs/resources/building_block_definition.md @@ -215,14 +215,8 @@ resource "meshstack_building_block_definition" "example_03_manual" { manual = {} } - # Output keys must match with inputs, as the backend copies over inputs to outputs - outputs = { - approval_required = { - display_name = "Approval Required" - type = "BOOLEAN" - assignment_type = "NONE" - } - } + # Outputs are omitted for manual building blocks: the backend derives them from the inputs + # (one output per input), so version_spec.outputs is computed and must not be set here. } } ``` @@ -395,7 +389,7 @@ Optional: - `dependency_refs` (Attributes Set) Set of refs to building block definitions this definition depends on. Prefer reusable refs from `meshstack_building_block_definition..ref` or `one(data.meshstack_building_block_definitions..building_block_definitions).ref`. (see [below for nested schema](#nestedatt--version_spec--dependency_refs)) - `inputs` (Attributes Map) Map of input definitions for the building block. Keys are input names, values are input configuration objects. Inputs define parameters that building blocks can receive. (see [below for nested schema](#nestedatt--version_spec--inputs)) - `only_apply_once_per_tenant` (Boolean) Whether this building block can only be applied once per tenant. -- `outputs` (Attributes Map) Map of output definitions for the building block. Keys are output names, values are output configuration objects. Outputs define values that building blocks produce and can be consumed by other building blocks. (see [below for nested schema](#nestedatt--version_spec--outputs)) +- `outputs` (Attributes Map) Map of output definitions for the building block. Keys are output names, values are output configuration objects. Outputs define values that building blocks produce and can be consumed by other building blocks. If implementation type is `manual`, outputs are computed from the API response, so omit this attribute entirely unless you want to specify a static `assignment_type = "PLATFORM_TENANT_ID"` as part of a landing zone. (see [below for nested schema](#nestedatt--version_spec--outputs)) - `permissions` (Set of String) Set of API permissions required by this building block. Will provide building block runs with an ephemeral API token with the specified workspace permissions. See [Workspace Permissions](https://docs.meshcloud.io/api/authentication/api-permissions/) for available values and [documentation on ephemeral API keys](https://docs.dev.meshcloud.io/concepts/building-block/#ephemeral-api-keys). - `runner_ref` (Attributes) Reference to the runner to run the implementation. If omitted, the pre-defined shared runner is used suitable for the given `implementation` choice (see [below for nested schema](#nestedatt--version_spec--runner_ref)) diff --git a/examples/resources/meshstack_building_block_definition/resource_03_manual.tf b/examples/resources/meshstack_building_block_definition/resource_03_manual.tf index f4b29cc8..14adaa92 100644 --- a/examples/resources/meshstack_building_block_definition/resource_03_manual.tf +++ b/examples/resources/meshstack_building_block_definition/resource_03_manual.tf @@ -24,13 +24,7 @@ resource "meshstack_building_block_definition" "example_03_manual" { manual = {} } - # Output keys must match with inputs, as the backend copies over inputs to outputs - outputs = { - approval_required = { - display_name = "Approval Required" - type = "BOOLEAN" - assignment_type = "NONE" - } - } + # Outputs are omitted for manual building blocks: the backend derives them from the inputs + # (one output per input), so version_spec.outputs is computed and must not be set here. } } diff --git a/examples/resources/meshstack_building_block_v2/test-support_01_workspace.tf b/examples/resources/meshstack_building_block_v2/test-support_01_workspace.tf index e00caf76..e4686092 100644 --- a/examples/resources/meshstack_building_block_v2/test-support_01_workspace.tf +++ b/examples/resources/meshstack_building_block_v2/test-support_01_workspace.tf @@ -50,33 +50,7 @@ resource "meshstack_building_block_definition" "example" { manual = {} } - # Output keys must match with inputs, as the backend copies over inputs to outputs for manual implementations. - outputs = { - name = { - display_name = "Name" - type = "STRING" - assignment_type = "NONE" - } - size = { - display_name = "Size" - type = "INTEGER" - assignment_type = "NONE" - } - environment = { - display_name = "Environment" - type = "STRING" - assignment_type = "NONE" - } - region = { - display_name = "Region" - type = "STRING" - assignment_type = "NONE" - } - enable_monitoring = { - display_name = "Enable Monitoring" - type = "BOOLEAN" - assignment_type = "NONE" - } - } + # Outputs are omitted for manual building blocks: the backend derives them from the inputs, so + # version_spec.outputs is computed and must not be set here. } } diff --git a/examples/resources/meshstack_building_block_v2/test-support_02_tenant.tf b/examples/resources/meshstack_building_block_v2/test-support_02_tenant.tf index 2b488254..67910b11 100644 --- a/examples/resources/meshstack_building_block_v2/test-support_02_tenant.tf +++ b/examples/resources/meshstack_building_block_v2/test-support_02_tenant.tf @@ -40,23 +40,7 @@ resource "meshstack_building_block_definition" "bb_v2_tenant_bbd" { manual = {} } - # Output keys must match with inputs, as the backend copies over inputs to outputs for manual implementations. - outputs = { - name = { - display_name = "Name" - type = "STRING" - assignment_type = "NONE" - } - size = { - display_name = "Size" - type = "INTEGER" - assignment_type = "NONE" - } - environment = { - display_name = "Environment" - type = "STRING" - assignment_type = "NONE" - } - } + # Outputs are omitted for manual building blocks: the backend derives them from the inputs, so + # version_spec.outputs is computed and must not be set here. } } diff --git a/internal/clientmock/mock_buildingblock_definition_version.go b/internal/clientmock/mock_buildingblock_definition_version.go index c3868c83..9bf86235 100644 --- a/internal/clientmock/mock_buildingblock_definition_version.go +++ b/internal/clientmock/mock_buildingblock_definition_version.go @@ -28,6 +28,7 @@ func (m meshBuildingBlockDefinitionVersionClient) Create(_ context.Context, owne versionUuid := uuid.NewString() // Compute hashes for all secrets in the spec backendSecretBehavior(true, &versionSpec, nil) + applyManualOutputBehavior(&versionSpec) // Set version number if not already set if versionSpec.VersionNumber == nil { @@ -51,6 +52,7 @@ func (m meshBuildingBlockDefinitionVersionClient) Update(_ context.Context, uuid if existing, ok := m.Store.Get(uuid); ok { // Compute hashes for all secrets in the spec backendSecretBehavior(false, &versionSpec, &existing.Spec) + applyManualOutputBehavior(&versionSpec) if existing.Metadata.OwnedByWorkspace != ownedByWorkspace { return nil, fmt.Errorf("mismatching workspace ownership: %s (existing) != %s (expected)", existing.Metadata.OwnedByWorkspace, ownedByWorkspace) } @@ -60,6 +62,49 @@ func (m meshBuildingBlockDefinitionVersionClient) Update(_ context.Context, uuid return nil, fmt.Errorf("building block definition version not found: %s", uuid) } +// applyManualOutputBehavior mirrors the real backend's ManualBuildingBlockCreationModule: for manual +// building blocks the outputs are derived from the inputs (one output per input, assignment type NONE, +// with SINGLE_SELECT/MULTI_SELECT/LIST input types translated to output-compatible types). Any output the +// caller marked PLATFORM_TENANT_ID keeps that assignment for the matching input; all other supplied +// outputs are ignored, matching the backend. +func applyManualOutputBehavior(versionSpec *client.MeshBuildingBlockDefinitionVersionSpec) { + if versionSpec.Implementation.Manual == nil { + return + } + platformTenantIdKeys := map[string]bool{} + for key, output := range versionSpec.Outputs { + if output.AssignmentType == client.MeshBuildingBlockDefinitionOutputAssignmentTypePlatformTenantID.Unwrap() { + platformTenantIdKeys[key] = true + } + } + outputs := make(map[string]client.MeshBuildingBlockDefinitionOutput, len(versionSpec.Inputs)) + for key, input := range versionSpec.Inputs { + assignmentType := client.MeshBuildingBlockDefinitionOutputAssignmentTypeNone.Unwrap() + if platformTenantIdKeys[key] { + assignmentType = client.MeshBuildingBlockDefinitionOutputAssignmentTypePlatformTenantID.Unwrap() + } + outputs[key] = client.MeshBuildingBlockDefinitionOutput{ + DisplayName: input.DisplayName, + Type: translateManualInputTypeToOutput(input.Type), + AssignmentType: assignmentType, + } + } + versionSpec.Outputs = outputs +} + +// translateManualInputTypeToOutput mirrors backend ManualIOTypeTranslation: SINGLE_SELECT, MULTI_SELECT +// and LIST cannot be output types and are translated; all other types are kept as-is. +func translateManualInputTypeToOutput(inputType client.MeshBuildingBlockIOType) client.MeshBuildingBlockIOType { + switch inputType { + case client.MeshBuildingBlockIOTypeSingleSelect.Unwrap(): + return client.MeshBuildingBlockIOTypeString.Unwrap() + case client.MeshBuildingBlockIOTypeMultiSelect.Unwrap(), client.MeshBuildingBlockIOTypeList.Unwrap(): + return client.MeshBuildingBlockIOTypeCode.Unwrap() + default: + return inputType + } +} + func (m meshBuildingBlockDefinitionVersionClient) getNextVersionNumber() int { maxNum := 0 for _, v := range m.Store.Values() { diff --git a/internal/provider/acctest/testconfig/build_building_block_definition.go b/internal/provider/acctest/testconfig/build_building_block_definition.go index 967fa1b6..3282f90f 100644 --- a/internal/provider/acctest/testconfig/build_building_block_definition.go +++ b/internal/provider/acctest/testconfig/build_building_block_definition.go @@ -76,6 +76,43 @@ func BBDWithIntegration(t *testing.T, suffix string) (config Config, buildingBlo ).Join(workspaceConfig, integrationConfig), buildingBlockDefinitionAddr } +// BBDGithubTwoIntegrations builds a github_workflows BBD wired to a first test integration ("A"), +// plus a second github integration ("B") owned by the same workspace. It returns the config, the +// BBD address, and integration B's address so a test can switch the BBD's integration_ref from A to +// B (e.g. to assert that re-drafting a released version with a new integration keeps the released +// version immutable). Both integrations reuse the github integration test-support file; the second +// is renamed and given a distinct display name so the backend treats it as a different integration. +func BBDGithubTwoIntegrations(t *testing.T) (config Config, buildingBlockDefinitionAddr Traversal, integrationBAddr Traversal) { + t.Helper() + workspaceConfig, workspaceAddr := Workspace(t) + exampleResource := Resource{Name: "building_block_definition", Suffix: "_02_github_workflows"} + + var integrationAAddr Traversal + integrationAConfig := exampleResource.TestSupportConfig(t, "_integration").WithFirstBlock( + ExtractAddress(&integrationAAddr), + OwnedByWorkspace(workspaceAddr), + ) + integrationBConfig := exampleResource.TestSupportConfig(t, "_integration").WithFirstBlock( + RenameKey("github_b"), + ExtractAddress(&integrationBAddr), + OwnedByWorkspace(workspaceAddr), + Descend("spec", "display_name")(SetString("GitHub Integration B")), + ) + + return exampleResource.Config(t).WithFirstBlock( + ExtractAddress(&buildingBlockDefinitionAddr), + OwnedByWorkspace(workspaceAddr), + Descend("version_spec", "implementation", "github_workflows", "integration_ref")( + SetAddr(integrationAAddr, "ref"), + ), + // Depend on integration A explicitly: once the BBD switches its integration_ref to B, the + // released version still pins A on the backend (which refuses integration deletion while + // referenced). This keeps A scheduled for destruction after the BBD so teardown succeeds; B is + // already implicitly ordered via the integration_ref reference. + Descend("depends_on")(SetRawExpr("[%s]", integrationAAddr)), + ).Join(workspaceConfig, integrationAConfig, integrationBConfig), buildingBlockDefinitionAddr, integrationBAddr +} + // BBDManual builds a BBD config with manual implementation type, owned by a new test workspace. func BBDManual(t *testing.T) (config Config, buildingBlockDefinitionAddr Traversal) { t.Helper() diff --git a/internal/provider/building_block_definition_resource.go b/internal/provider/building_block_definition_resource.go index 26374737..7de292f4 100644 --- a/internal/provider/building_block_definition_resource.go +++ b/internal/provider/building_block_definition_resource.go @@ -3,6 +3,7 @@ package provider import ( "context" "fmt" + "maps" "slices" "github.com/hashicorp/terraform-plugin-framework/diag" @@ -16,10 +17,11 @@ import ( ) var ( - _ resource.Resource = &buildingBlockDefinitionResource{} - _ resource.ResourceWithConfigure = &buildingBlockDefinitionResource{} - _ resource.ResourceWithImportState = &buildingBlockDefinitionResource{} - _ resource.ResourceWithModifyPlan = &buildingBlockDefinitionResource{} + _ resource.Resource = &buildingBlockDefinitionResource{} + _ resource.ResourceWithConfigure = &buildingBlockDefinitionResource{} + _ resource.ResourceWithImportState = &buildingBlockDefinitionResource{} + _ resource.ResourceWithModifyPlan = &buildingBlockDefinitionResource{} + _ resource.ResourceWithValidateConfig = &buildingBlockDefinitionResource{} ) func NewBuildingBlockDefinitionResource() resource.Resource { @@ -85,7 +87,10 @@ func (r *buildingBlockDefinitionResource) Create(ctx context.Context, req resour // Updating the empty created version with provided configuration to complete creation versionUuid := createdEmptyVersion.Metadata.Uuid - createVersionSpecDto := plan.VersionSpec.ToClientDto(bbdUuid) + createVersionSpecDto := versionSpecDtoFromPlan(ctx, plan, bbdUuid, req.Config, &resp.Diagnostics) + if resp.Diagnostics.HasError() { + return + } createdVersionDto, err := r.buildingBlockDefinitionVersionClient.Update(ctx, versionUuid, createdDto.Metadata.OwnedByWorkspace, createVersionSpecDto) if err != nil { resp.Diagnostics.AddError("Error updating initial version", fmt.Sprintf( @@ -144,6 +149,116 @@ func (r *buildingBlockDefinitionResource) Read(ctx context.Context, req resource buildingBlockDefinitionConverterOptions().Append(buildingBlockDefinitionVersionConverterOptions(ctx, nil, nil, req.State)...)...)...) } +func (r *buildingBlockDefinitionResource) ValidateConfig(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) { + manualPath := path.Root("version_spec").AtName("implementation").AtName("manual") + outputsPath := path.Root("version_spec").AtName("outputs") + + var manual types.Object + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, manualPath, &manual)...) + var outputs types.Map + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, outputsPath, &outputs)...) + if resp.Diagnostics.HasError() { + return + } + + // Manual building blocks derive their outputs from the inputs on the backend (one output per input, + // assignment type NONE). The backend only honors outputs the user marks with assignment type + // PLATFORM_TENANT_ID; everything else it regenerates. So configuring any non-PLATFORM_TENANT_ID output + // for a manual building block is rejected here - omit it and let it be computed (see issues #131, #176). + if manual.IsNull() || manual.IsUnknown() || outputs.IsNull() || outputs.IsUnknown() { + return + } + for key, assignmentType := range outputAssignmentTypes(outputs) { + if assignmentType == client.MeshBuildingBlockDefinitionOutputAssignmentTypePlatformTenantID.String() { + continue + } + // An omitted assignment_type defaults to NONE, which the backend ignores just like an explicit + // NONE, so reject it too instead of silently accepting config that does nothing. + configured := fmt.Sprintf("assignment_type %q", assignmentType) + if assignmentType == "" { + configured = fmt.Sprintf("no assignment_type (defaults to %s)", client.MeshBuildingBlockDefinitionOutputAssignmentTypeNone) + } + resp.Diagnostics.AddAttributeError( + outputsPath.AtMapKey(key), + "manual building block outputs may only assign PLATFORM_TENANT_ID", + fmt.Sprintf("Manual building block definitions derive their outputs from the inputs automatically. "+ + "Output %q has %s; remove it so it can be computed from the API response. "+ + "Only outputs with assignment_type %s may be configured (to mark which output carries the tenant id).", + key, configured, client.MeshBuildingBlockDefinitionOutputAssignmentTypePlatformTenantID), + ) + } +} + +// outputAssignmentTypes returns the assignment_type of each output in the given map (keyed by output name), +// skipping null/unknown maps and elements. An omitted or null assignment_type yields an empty string (it +// defaults to NONE, so it must be surfaced rather than dropped); an unknown assignment_type is skipped +// because it cannot be validated yet. +func outputAssignmentTypes(outputs types.Map) map[string]string { + result := map[string]string{} + if outputs.IsNull() || outputs.IsUnknown() { + return result + } + for key, elem := range outputs.Elements() { + obj, ok := elem.(types.Object) + if !ok || obj.IsNull() || obj.IsUnknown() { + continue + } + assignmentType, ok := obj.Attributes()["assignment_type"].(types.String) + if ok && assignmentType.IsUnknown() { + continue + } + // A missing or null assignment_type defaults to NONE; represent it as "" so callers can reject it. + if !ok || assignmentType.IsNull() { + result[key] = "" + continue + } + result[key] = assignmentType.ValueString() + } + return result +} + +// platformTenantIdOutputKeysEqual reports whether the set of output keys assigned PLATFORM_TENANT_ID is +// the same in both maps. Used to detect whether a manual building block's configured tenant-id output +// changed, which (besides an inputs change) is the only way its reconciled outputs can change. +func platformTenantIdOutputKeysEqual(a, b types.Map) bool { + tenantIdKeys := func(m types.Map) map[string]struct{} { + keys := map[string]struct{}{} + for key, assignmentType := range outputAssignmentTypes(m) { + if assignmentType == client.MeshBuildingBlockDefinitionOutputAssignmentTypePlatformTenantID.String() { + keys[key] = struct{}{} + } + } + return keys + } + return maps.Equal(tenantIdKeys(a), tenantIdKeys(b)) +} + +// versionSpecDtoFromPlan builds the version_spec client DTO from the plan, applying the manual-output +// override: manual building blocks have backend-derived outputs (left unknown in the plan), so the +// configured PLATFORM_TENANT_ID hints are sourced straight from config (see manualConfiguredOutputs). +// Non-manual implementations configure outputs explicitly and are left untouched. Shared by Create and +// Update; check diags for errors after calling. +func versionSpecDtoFromPlan(ctx context.Context, plan buildingBlockDefinition, bbdUuid string, config generic.AttributeGetter, diags *diag.Diagnostics) client.MeshBuildingBlockDefinitionVersionSpec { + dto := plan.VersionSpec.ToClientDto(bbdUuid) + if dto.Implementation.Manual != nil { + dto.Outputs = manualConfiguredOutputs(ctx, config, diags) + } + return dto +} + +// manualConfiguredOutputs reads the user-configured version_spec.outputs (the PLATFORM_TENANT_ID hints) +// from config so they can be sent to the backend even though the planned outputs value is left unknown +// for manual building blocks. Returns an empty map when outputs are omitted. +func manualConfiguredOutputs(ctx context.Context, config generic.AttributeGetter, diags *diag.Diagnostics) map[string]client.MeshBuildingBlockDefinitionOutput { + outputs := generic.GetAttribute[map[string]client.MeshBuildingBlockDefinitionOutput]( + ctx, config, path.Root("version_spec").AtName("outputs"), diags, generic.WithSetUnknownValueToZero()) + if outputs == nil { + // The backend rejects a null outputs property, so send an empty map when none are configured. + outputs = map[string]client.MeshBuildingBlockDefinitionOutput{} + } + return outputs +} + func (r *buildingBlockDefinitionResource) ModifyPlan(ctx context.Context, req resource.ModifyPlanRequest, resp *resource.ModifyPlanResponse) { if req.Plan.Raw.IsNull() { // do nothing in case of delete @@ -188,9 +303,60 @@ func (r *buildingBlockDefinitionResource) ModifyPlan(ctx context.Context, req re return } + // Creating or rotating a secret changes the version_spec (the secret machinery above already detected + // this as versionSpecSecretsChanged). On a released (non-draft) version that stays released, the + // version_spec is immutable, so reject it here at plan time with a clear, actionable error instead of + // letting the content-hash safeguard fail opaquely on the planned plaintext later in Update (issue #196). + // Flipping draft=false->true (plan draft becomes true) creates a new draft version and is allowed. + planDraftKnownReleased := plan.VersionSpec.Draft.Value != nil && !plan.VersionSpec.Draft.Get() + if versionSpecSecretsChanged && !state.VersionSpec.Draft.Get() && planDraftKnownReleased { + resp.Diagnostics.AddError("Error updating version_spec", fmt.Sprintf( + "Updating a version_spec in non-draft state is not allowed. "+ + "Rotating a secret on the released version %d changes the version_spec, which is immutable. "+ + "Set version_spec.draft = true to create a new draft version; the secret rotation can be applied in the same step.", + state.VersionLatest.Number.Get(), + )) + return + } + + // Manual building blocks have backend-derived outputs: the backend mirrors every input into an output + // (assignment type NONE), preserving only outputs the user marked PLATFORM_TENANT_ID (see issues #131 + // and #176, and ValidateConfig). We cannot fully predict the reconciled outputs at plan time, so + // whenever the inputs or the configured PLATFORM_TENANT_ID outputs change we leave outputs - and the + // content hash that includes them - unknown and let the apply reconcile them from the API response. + // Otherwise we reuse the reconciled value from state, which also avoids a perpetual diff between the + // (partial) configured outputs and the (full) stored outputs. Non-manual implementations configure + // outputs explicitly and the backend does not derive them, so they are left untouched here. + outputsPath := path.Root("version_spec").AtName("outputs") + inputsPath := path.Root("version_spec").AtName("inputs") + manualPath := path.Root("version_spec").AtName("implementation").AtName("manual") + versionSpecOutputsUncertain := false + var manual types.Object + resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, manualPath, &manual)...) + if !manual.IsNull() && !manual.IsUnknown() { + var planInputs, stateInputs, configOutputs, stateOutputs types.Map + resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, inputsPath, &planInputs)...) + resp.Diagnostics.Append(req.State.GetAttribute(ctx, inputsPath, &stateInputs)...) + resp.Diagnostics.Append(req.Config.GetAttribute(ctx, outputsPath, &configOutputs)...) + resp.Diagnostics.Append(req.State.GetAttribute(ctx, outputsPath, &stateOutputs)...) + if resp.Diagnostics.HasError() { + return + } + versionSpecOutputsUncertain = !planInputs.Equal(stateInputs) || + !platformTenantIdOutputKeysEqual(configOutputs, stateOutputs) + if versionSpecOutputsUncertain { + resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, outputsPath, types.MapUnknown(stateOutputs.ElementType(ctx)))...) + } else { + resp.Diagnostics.Append(resp.Plan.SetAttribute(ctx, outputsPath, stateOutputs)...) + } + if resp.Diagnostics.HasError() { + return + } + } + // Determine this very carefully and leave it unknown if the underlying version_spec has unknown values somewhere deep down versionSpecContentHash := func() (result generic.NullIsUnknown[string]) { - if versionSpecSecretsChanged { + if versionSpecSecretsChanged || versionSpecOutputsUncertain { return } versionSpecPath := path.Root("version_spec") @@ -300,7 +466,10 @@ func (r *buildingBlockDefinitionResource) Update(ctx context.Context, req resour // Handle version_spec update logic - versionSpecDto := plan.VersionSpec.ToClientDto(bbdUuid) + versionSpecDto := versionSpecDtoFromPlan(ctx, plan, bbdUuid, req.Config, &resp.Diagnostics) + if resp.Diagnostics.HasError() { + return + } var updatedVersionDto *client.MeshBuildingBlockDefinitionVersion switch { @@ -317,7 +486,9 @@ func (r *buildingBlockDefinitionResource) Update(ctx context.Context, req resour } case !state.VersionSpec.Draft: // state (and plan) are in draft=false (aka released), so one should not change version_spec at all - // this makes released or in-review versions immutable + // this makes released or in-review versions immutable. A secret rotation on a released version is + // already rejected at plan time in ModifyPlan with a clear message (issue #196), so it never reaches + // here; any remaining version_spec change is caught by the content-hash comparison below. versionSpecDtoContentHash := versionContentHash(versionSpecDto, &resp.Diagnostics) if resp.Diagnostics.HasError() { return diff --git a/internal/provider/building_block_definition_resource_model.go b/internal/provider/building_block_definition_resource_model.go index a1e6bbb5..5b260a34 100644 --- a/internal/provider/building_block_definition_resource_model.go +++ b/internal/provider/building_block_definition_resource_model.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "reflect" "slices" "github.com/hashicorp/terraform-plugin-framework/diag" @@ -18,6 +19,7 @@ import ( "github.com/meshcloud/terraform-provider-meshstack/internal/types/generic" "github.com/meshcloud/terraform-provider-meshstack/internal/types/secret" "github.com/meshcloud/terraform-provider-meshstack/internal/util/hash" + reflectwalk "github.com/meshcloud/terraform-provider-meshstack/internal/util/reflect" ) type buildingBlockDefinition struct { @@ -339,6 +341,14 @@ func (model *buildingBlockDefinition) SetFromVersionClientDtos(diags *diag.Diagn func versionContentHash(versionSpecDto client.MeshBuildingBlockDefinitionVersionSpec, diags *diag.Diagnostics) string { if result, err := func() (string, error) { + // Safeguard against accidentally hashing plaintext secret values (the backend only ever returns + // hashes, so plaintext would make the hash unstable). Detection is on the typed DTO so user data - + // e.g. an input named "plaintext" or a STATIC argument whose JSON carries a "plaintext" key - is + // never mistaken for a secret. Callers leave the hash unknown when a secret rotates (issue #196). + if versionSpecContainsPlaintextSecret(versionSpecDto) { + return "", errors.New("version_spec carries a plaintext secret value, which must not be hashed") + } + // Ignore version, state, and buildingBlockDefinitionRef fields by setting them to constant values, always! versionSpecDto.VersionNumber = nil versionSpecDto.State = nil @@ -356,11 +366,7 @@ func versionContentHash(versionSpecDto client.MeshBuildingBlockDefinitionVersion return "", err } - versionSpecHash, err := hash.Hasher{}.Hash(converted, - // Safeguard against accidentally hashing plaintext values - // (should never happen as backend never returns plaintext values) - hash.DisallowMapKeys("plaintext", "buildingBlockDefinitionRef"), - ) + versionSpecHash, err := hash.Hasher{}.Hash(converted) if err != nil { return "", err } @@ -376,3 +382,19 @@ func versionContentHash(versionSpecDto client.MeshBuildingBlockDefinitionVersion return result } } + +// versionSpecContainsPlaintextSecret reports whether the typed version_spec DTO carries a secret whose +// plaintext value is set, i.e. a secret is being created or rotated. It walks the typed DTO and only +// inspects clientTypes.Secret values, so arbitrary user JSON (which lands in the variant's "any" branch, +// never in a Secret) is never mistaken for a secret. The backend only ever returns hashes, so a true +// result always stems from a locally-planned value. +func versionSpecContainsPlaintextSecret(versionSpecDto client.MeshBuildingBlockDefinitionVersionSpec) bool { + found := false + _ = reflectwalk.Walk(reflect.ValueOf(versionSpecDto), func(_ reflectwalk.WalkPath, v reflect.Value) error { + if s, ok := v.Interface().(clientTypes.Secret); ok && s.Plaintext != nil { + found = true + } + return nil + }) + return found +} diff --git a/internal/provider/building_block_definition_resource_model_test.go b/internal/provider/building_block_definition_resource_model_test.go index 851c7966..7ad21311 100644 --- a/internal/provider/building_block_definition_resource_model_test.go +++ b/internal/provider/building_block_definition_resource_model_test.go @@ -64,5 +64,53 @@ func Test_versionContentHash_plaintextSecret(t *testing.T) { var diags diag.Diagnostics versionContentHash(versionSpec, &diags) require.Len(t, diags, 1) - assert.Contains(t, diags[0].Detail(), "key path *[implementation]*[terraform]*[sshPrivateKey][plaintext] matches one of disallowed keys [plaintext") + assert.Contains(t, diags[0].Detail(), "version_spec carries a plaintext secret value, which must not be hashed") +} + +// Test_versionContentHash_userDataNamedPlaintext guards against the false positive from issue #196: an +// input named "plaintext" is a user-chosen map key, not a secret, so it must hash successfully rather than +// trip the plaintext safeguard (the old JSON-key-based safeguard rejected it). +func Test_versionContentHash_userDataNamedPlaintext(t *testing.T) { + const userDataJson = `{ + "inputs": { + "plaintext": { + "displayName": "Plaintext", + "type": "STRING", + "assignmentType": "STATIC", + "argument": "hello" + } + }, + "implementation": {"terraform": {}} + }` + var versionSpec client.MeshBuildingBlockDefinitionVersionSpec + require.NoError(t, json.Unmarshal([]byte(userDataJson), &versionSpec)) + var diags diag.Diagnostics + actualHash := versionContentHash(versionSpec, &diags) + require.Empty(t, diags) + assert.NotEmpty(t, actualHash) +} + +// Test_versionContentHash_ignoresPerVersionFields pins the invariant that the content hash is independent +// of the per-BBD buildingBlockDefinitionRef and the per-version versionNumber/state (all stripped before +// hashing). Were they hashed, the same version_spec would hash differently across BBDs/versions, breaking +// the released-version immutability and change-detection comparisons. This is the focused, behavioral +// replacement for the old DisallowMapKeys("buildingBlockDefinitionRef") safeguard, which - like the +// "plaintext" one - would have misfired on a user-chosen input/output named "buildingBlockDefinitionRef". +func Test_versionContentHash_ignoresPerVersionFields(t *testing.T) { + var base, mutated client.MeshBuildingBlockDefinitionVersionSpec + require.NoError(t, json.Unmarshal(versionSpecJson, &base)) + require.NoError(t, json.Unmarshal(versionSpecJson, &mutated)) + + mutated.BuildingBlockDefinitionRef = &client.BuildingBlockDefinitionRef{ + Kind: client.MeshObjectKind.BuildingBlockDefinition, + Uuid: "a-different-bbd-uuid", + } + mutated.VersionNumber = new(int64(99)) + mutated.State = client.MeshBuildingBlockDefinitionVersionStateReleased.Ptr() + + var diags diag.Diagnostics + baseHash := versionContentHash(base, &diags) + mutatedHash := versionContentHash(mutated, &diags) + require.Empty(t, diags) + assert.Equal(t, baseHash, mutatedHash, "buildingBlockDefinitionRef/versionNumber/state must not affect the content hash") } diff --git a/internal/provider/building_block_definition_resource_schema.go b/internal/provider/building_block_definition_resource_schema.go index 9569e064..92587666 100644 --- a/internal/provider/building_block_definition_resource_schema.go +++ b/internal/provider/building_block_definition_resource_schema.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/mapdefault" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/mapplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/objectplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/setdefault" @@ -405,11 +406,15 @@ func (r *buildingBlockDefinitionResource) Schema(_ context.Context, _ resource.S "inputs": inputsAttribute, "outputs": schema.MapNestedAttribute{ MarkdownDescription: "Map of output definitions for the building block. Keys are output names, values are output configuration objects. " + - "Outputs define values that building blocks produce and can be consumed by other building blocks.", + "Outputs define values that building blocks produce and can be consumed by other building blocks. " + + "If implementation type is " + client.MeshBuildingBlockImplementationTypeManual.Markdown() + + ", outputs are computed from the API response, so omit this attribute entirely unless you want to specify a static `assignment_type = \"PLATFORM_TENANT_ID\"` as part of a landing zone.", Optional: true, Computed: true, - Default: mapdefault.StaticValue(types.MapValueMust(nestedObjectToObjectType(outputs), nil)), NestedObject: outputs, + PlanModifiers: []planmodifier.Map{ + mapplanmodifier.UseStateForUnknown(), + }, }, "implementation": schema.SingleNestedAttribute{ diff --git a/internal/provider/building_block_definition_resource_test.go b/internal/provider/building_block_definition_resource_test.go index a1df8800..1a694a43 100644 --- a/internal/provider/building_block_definition_resource_test.go +++ b/internal/provider/building_block_definition_resource_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/hashicorp/terraform-plugin-testing/compare" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/knownvalue" "github.com/hashicorp/terraform-plugin-testing/plancheck" @@ -339,6 +340,347 @@ func TestAccBuildingBlockDefinition(t *testing.T) { }, }) }) + + // Regression test for issue #131: releasing a version and then flipping it back to draft + // together with a version_spec implementation change must NOT alter the already-released + // version. The backend previously shared the implementation object across versions, so editing + // the new draft retroactively mutated the released version, making its content_hash change + // during apply ("Provider produced inconsistent result after apply"). Uses the Terraform + // implementation because its implementation carries mutable fields (e.g. pre_run_script); + // the manual implementation could not surface this. + t.Run("06_release_redraft_implementation_change", func(t *testing.T) { + config, addr := testconfig.BBDTerraform(t) + + // The released version's content_hash must be identical before and after the new draft + // is created from it. + releasedHashStable := statecheck.CompareValue(compare.ValuesSame()) + releasedHashPath := tfjsonpath.New("version_latest_release").AtMapKey("content_hash") + + redraftWithImplChange := config.WithFirstBlock( + testconfig.Descend("version_spec", "implementation", "terraform", "pre_run_script")( + testconfig.SetString(`echo "changed for the second version"`), + ), + ) + + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + // Step 1: Create draft v1 + { + Config: config.String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(1, versionStateDraft)), + }, + }, + // Step 2: Release v1 + { + Config: releaseBBDVersion(t, config).String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(1, versionStateReleased)), + releasedHashStable.AddStateValue(addr.String(), releasedHashPath), + }, + }, + // Step 3: Flip draft false->true AND change the implementation -> new draft v2. + // The released v1 must stay immutable (same content_hash, no inconsistent result). + { + Config: redraftWithImplChange.String(), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(addr.String(), plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(2, versionStateDraft)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("versions"), knownvalue.ListExact([]knownvalue.Check{ + expectedVersion(1, versionStateReleased), + expectedVersion(2, versionStateDraft), + })), + releasedHashStable.AddStateValue(addr.String(), releasedHashPath), + }, + }, + }, + }) + }) + + // Regression test for issues #131 and #176: manual building blocks derive their outputs from inputs + // on the backend (SINGLE_SELECT/STATIC inputs auto-generate outputs). The config omits version_spec.outputs + // entirely (it is computed). Releasing and then re-drafting together with an input change must reconcile + // the derived outputs without "Provider produced inconsistent result after apply", and must not change + // the already-released version. + t.Run("07_manual_computed_outputs", func(t *testing.T) { + config, addr := testconfig.BBDManual(t) + withInputs := func(inputs string) testconfig.Config { + return config.WithFirstBlock(testconfig.Descend("version_spec", "inputs")(testconfig.SetRawExpr("%s", inputs))) + } + + // approval (BOOLEAN) mirrors to a BOOLEAN output; region (SINGLE_SELECT) mirrors to a STRING output. + base := withInputs(`{ + approval = { display_name = "Approval", type = "BOOLEAN", assignment_type = "PLATFORM_OPERATOR_MANUAL_INPUT" } + region = { display_name = "Region", type = "SINGLE_SELECT", assignment_type = "USER_INPUT", selectable_values = ["eu", "us"] } + }`) + redraft := withInputs(`{ + approval = { display_name = "Approval", type = "BOOLEAN", assignment_type = "PLATFORM_OPERATOR_MANUAL_INPUT" } + region = { display_name = "Region", type = "SINGLE_SELECT", assignment_type = "USER_INPUT", selectable_values = ["eu", "us"] } + ticket = { display_name = "Ticket", type = "STRING", assignment_type = "STATIC", argument = jsonencode("T-1") } + }`) + + outputCheck := func(displayName, ioType string) knownvalue.Check { + return xknownvalue.MapExact(map[string]knownvalue.Check{ + "display_name": knownvalue.StringExact(displayName), + "type": knownvalue.StringExact(ioType), + "assignment_type": knownvalue.StringExact("NONE"), + }) + } + baseOutputs := knownvalue.MapExact(map[string]knownvalue.Check{ + "approval": outputCheck("Approval", "BOOLEAN"), + "region": outputCheck("Region", "STRING"), + }) + redraftOutputs := knownvalue.MapExact(map[string]knownvalue.Check{ + "approval": outputCheck("Approval", "BOOLEAN"), + "region": outputCheck("Region", "STRING"), + "ticket": outputCheck("Ticket", "STRING"), + }) + + releasedHashStable := statecheck.CompareValue(compare.ValuesSame()) + releasedHashPath := tfjsonpath.New("version_latest_release").AtMapKey("content_hash") + + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + // Step 1: Create draft v1 with outputs omitted -> outputs computed from inputs + { + Config: base.String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_spec").AtMapKey("outputs"), baseOutputs), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(1, versionStateDraft)), + }, + }, + // Step 2: Release v1 + { + Config: releaseBBDVersion(t, base).String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_spec").AtMapKey("outputs"), baseOutputs), + releasedHashStable.AddStateValue(addr.String(), releasedHashPath), + }, + }, + // Step 3: Re-draft (draft false->true) AND add an input -> new draft v2 with reconciled outputs. + { + Config: redraft.String(), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(addr.String(), plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(2, versionStateDraft)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_spec").AtMapKey("outputs"), redraftOutputs), + releasedHashStable.AddStateValue(addr.String(), releasedHashPath), + }, + }, + }, + }) + }) + + // Regression test for issue #196: rotating a sensitive input's secret on a released (immutable) + // version previously failed with an opaque "Failed to determine content hash ... [plaintext]" + // error, because the planned DTO carries the rotated secret's plaintext and the content hash + // disallows plaintext keys. Released versions are immutable, so the rotation must be rejected with + // a clear, actionable error instead. + t.Run("08_release_secret_rotation_rejected", func(t *testing.T) { + config, _ := testconfig.BBDTerraform(t) + sensitiveInputs := func(version, value string) testconfig.Config { + return config.WithFirstBlock(testconfig.Descend("version_spec", "inputs")(testconfig.SetRawExpr("%s", fmt.Sprintf(`{ + CONNECTOR_SECRET = { + display_name = "Connector Secret" + type = "STRING" + assignment_type = "STATIC" + sensitive = { + argument = { + secret_value = %q + secret_version = %q + } + } + } + }`, value, version)))) + } + base := sensitiveInputs("v1", "plaintext-secret-v1") + rotated := sensitiveInputs("v2", "plaintext-secret-v2") + + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + // Step 1: Create draft v1 with a sensitive input + {Config: base.String()}, + // Step 2: Release v1 (now immutable) + {Config: releaseBBDVersion(t, base).String()}, + // Step 3: Rotate the secret on the released version -> clear error, not an opaque hash failure + { + Config: releaseBBDVersion(t, rotated).String(), + ExpectError: regexp.MustCompile("Updating a version_spec in non-draft state is not allowed"), + }, + }, + }) + }) + + // Regression test for issue #196: the released-version secret-rotation guard must key off an actual + // secret change, not the presence of a JSON key literally named "plaintext". An input named "plaintext" + // (or any STATIC argument whose JSON carries a "plaintext" key) is user data, not a secret. Mutating a + // non-version_spec field (the description) on a released version that contains such an input must NOT be + // rejected as a secret rotation. + t.Run("09_released_plaintext_named_input_is_not_a_secret", func(t *testing.T) { + config, addr := testconfig.BBDTerraform(t) + withPlaintextInput := func(c testconfig.Config) testconfig.Config { + return c.WithFirstBlock(testconfig.Descend("version_spec", "inputs")(testconfig.SetRawExpr("%s", `{ + plaintext = { display_name = "Plaintext", type = "STRING", assignment_type = "STATIC", argument = jsonencode("hello") } + }`))) + } + base := withPlaintextInput(config) + released := releaseBBDVersion(t, base) + + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + // Step 1: Create draft v1 with a non-sensitive input named "plaintext" + {Config: base.String()}, + // Step 2: Release v1 (now immutable) + {Config: released.String()}, + // Step 3: Change only the description on the released version. version_spec is unchanged and + // carries no secret, so this must succeed - the old "plaintext" key match wrongly rejected it. + { + Config: updateBBDDescription(t, released, "updated description, version_spec untouched").String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("spec").AtMapKey("description"), knownvalue.StringExact("updated description, version_spec untouched")), + }, + }, + }, + }) + }) + + // Positive-path companion to issue #196: rotating a secret on a released version is rejected (subtest 08), + // but the documented workaround - flip version_spec.draft false->true AND rotate the secret in the same + // step - must succeed, creating a new draft version that carries the rotated secret while the released + // version stays immutable. Guards the ModifyPlan rejection from over-triggering on the draft-flip path. + t.Run("10_redraft_with_secret_rotation_allowed", func(t *testing.T) { + config, addr := testconfig.BBDTerraform(t) + sensitiveInputs := func(version, value string) testconfig.Config { + return config.WithFirstBlock(testconfig.Descend("version_spec", "inputs")(testconfig.SetRawExpr("%s", fmt.Sprintf(`{ + CONNECTOR_SECRET = { + display_name = "Connector Secret" + type = "STRING" + assignment_type = "STATIC" + sensitive = { + argument = { + secret_value = %q + secret_version = %q + } + } + } + }`, value, version)))) + } + base := sensitiveInputs("v1", "plaintext-secret-v1") + // base defaults to draft=true; after releasing v1 this same config (draft=true) flips back to draft + // while also rotating the secret to v2 -> a new draft version v2. + redraftRotated := sensitiveInputs("v2", "plaintext-secret-v2") + + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + // Step 1: Create draft v1 with a sensitive input + {Config: base.String()}, + // Step 2: Release v1 (now immutable) + {Config: releaseBBDVersion(t, base).String()}, + // Step 3: Flip back to draft AND rotate the secret in one step -> new draft v2, v1 untouched + { + Config: redraftRotated.String(), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(addr.String(), plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(2, versionStateDraft)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("versions"), knownvalue.ListExact([]knownvalue.Check{ + expectedVersion(1, versionStateReleased), + expectedVersion(2, versionStateDraft), + })), + }, + }, + }, + }) + }) + + // Regression test (companion to #131) for the integration reference of CI implementations: github + // (like gitlab/azure) carries an integration_ref besides the implementation. Releasing a github + // version and then re-drafting it (draft false->true) while switching to a different integration must + // create a new draft v2 pointing at the new integration, while the already-released v1 keeps its + // original integration and stays immutable (stable content_hash, no "inconsistent result after + // apply"). The backend keeps this safe by deep-copying the implementation binding when deriving the + // draft, so changing the draft's integration reference does not retroactively repoint the released one. + t.Run("11_github_release_redraft_integration_change", func(t *testing.T) { + config, addr, integrationBAddr := testconfig.BBDGithubTwoIntegrations(t) + + // The released version's content_hash (which includes integration_ref) must stay identical + // across the re-draft, proving the released version was not retroactively repointed. + releasedHashStable := statecheck.CompareValue(compare.ValuesSame()) + releasedHashPath := tfjsonpath.New("version_latest_release").AtMapKey("content_hash") + + // Sanity check that the test actually switches the integration: the draft's integration_ref uuid + // must differ between draft v1 (integration A) and draft v2 (integration B). + integrationSwitched := statecheck.CompareValue(compare.ValuesDiffer()) + integrationUuidPath := tfjsonpath.New("version_spec").AtMapKey("implementation"). + AtMapKey("github_workflows").AtMapKey("integration_ref").AtMapKey("uuid") + + // base defaults to draft=true with integration A; this same config (draft=true) flips back to + // draft while switching to integration B -> a new draft version v2. + redraftWithIntegrationChange := config.WithFirstBlock( + testconfig.Descend("version_spec", "implementation", "github_workflows", "integration_ref")( + testconfig.SetAddr(integrationBAddr, "ref"), + ), + ) + + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{ + // Step 1: Create draft v1 with integration A + { + Config: config.String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(1, versionStateDraft)), + integrationSwitched.AddStateValue(addr.String(), integrationUuidPath), + }, + }, + // Step 2: Release v1 (now immutable) + { + Config: releaseBBDVersion(t, config).String(), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(1, versionStateReleased)), + releasedHashStable.AddStateValue(addr.String(), releasedHashPath), + }, + }, + // Step 3: Flip draft false->true AND switch integration A->B -> new draft v2. + // Released v1 must keep integration A and stay immutable (same content_hash). + { + Config: redraftWithIntegrationChange.String(), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(addr.String(), plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest_release"), expectedVersion(1, versionStateReleased)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("version_latest"), expectedVersion(2, versionStateDraft)), + statecheck.ExpectKnownValue(addr.String(), tfjsonpath.New("versions"), knownvalue.ListExact([]knownvalue.Check{ + expectedVersion(1, versionStateReleased), + expectedVersion(2, versionStateDraft), + })), + releasedHashStable.AddStateValue(addr.String(), releasedHashPath), + integrationSwitched.AddStateValue(addr.String(), integrationUuidPath), + }, + }, + }, + }) + }) } // checkBBDMetadataFull checks metadata for the 01_terraform example (tags with 2 entries). @@ -832,3 +1174,63 @@ resource "meshstack_building_block_definition" "test" { }) } } + +func TestAccBuildingBlockDefinitionManualOutputsValidation(t *testing.T) { + // Output configuration rules for manual building blocks are validated client-side only. + if !IsMockClientTest() { + t.Skip("manual outputs validation is tested with mock client only") + } + + t.Parallel() + + manualConfig := func(outputs string) string { + return fmt.Sprintf(` +resource "meshstack_building_block_definition" "test" { + metadata = { owned_by_workspace = "my-workspace" } + spec = { display_name = "Test", description = "Test" } + version_spec = { + draft = true + inputs = { + tenant = { display_name = "Tenant", type = "STRING", assignment_type = "STATIC", argument = jsonencode("t") } + } + implementation = { manual = {} } + %s + } +}`, outputs) + } + + tests := []struct { + name string + outputs string + expectError *regexp.Regexp + }{ + { + name: "NONE output rejected for manual", + outputs: `outputs = { tenant = { display_name = "Tenant", type = "STRING", assignment_type = "NONE" } }`, + expectError: regexp.MustCompile(`may only assign PLATFORM_TENANT_ID`), + }, + { + // Omitting assignment_type defaults it to NONE, which the backend ignores; it must be + // rejected the same as an explicit NONE rather than silently accepted. + name: "omitted assignment_type rejected for manual", + outputs: `outputs = { tenant = { display_name = "Tenant", type = "STRING" } }`, + expectError: regexp.MustCompile(`may only assign PLATFORM_TENANT_ID`), + }, + { + name: "PLATFORM_TENANT_ID output allowed for manual", + outputs: `outputs = { tenant = { display_name = "Tenant", type = "STRING", assignment_type = "PLATFORM_TENANT_ID" } }`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + step := resource.TestStep{Config: manualConfig(tt.outputs)} + if tt.expectError != nil { + step.ExpectError = tt.expectError + } + ApplyAndTest(t, resource.TestCase{ + Steps: []resource.TestStep{step}, + }) + }) + } +} diff --git a/internal/provider/provider_test.go b/internal/provider/provider_test.go index 9cd4ad28..039cf2cd 100644 --- a/internal/provider/provider_test.go +++ b/internal/provider/provider_test.go @@ -2,7 +2,9 @@ package provider import ( "context" + "fmt" "os" + "path/filepath" "strings" "testing" @@ -32,12 +34,42 @@ func IsMockClientTest() bool { return os.Getenv("TF_ACC") == "" } +// envKeyScratchDump, when non-empty, makes ApplyAndTest dump each step's HCL config to disk +// (as a standalone, re-runnable config) instead of running the test. Set it to "1"/"true" to +// dump into the repo-root scratch/ dir, or to a directory path to dump there. See the +// scratch-config-testing skill. +const envKeyScratchDump = "MESHSTACK_SCRATCH_DUMP" + +// scratchProviderTf is written alongside each dumped main.tf so the config resolves the +// dev-built provider via a dev_overrides CLI config (TF_CLI_CONFIG_FILE). Credentials and +// endpoint come from the MESHSTACK_* environment variables. +const scratchProviderTf = `terraform { + required_providers { + meshstack = { + source = "meshcloud/meshstack" + } + } +} + +provider "meshstack" { + # endpoint/credentials read from MESHSTACK_ENDPOINT, MESHSTACK_API_KEY, MESHSTACK_API_SECRET +} +` + // ApplyAndTest runs a TF test case. When TF_ACC is not set, it uses a mock // client (unit test mode). When TF_ACC is set, it runs against a real meshStack. // All tests using ApplyAndTest run in parallel. +// +// When MESHSTACK_SCRATCH_DUMP is set, it instead dumps each step's config to disk and +// returns without running the test (see dumpStepConfigs). func ApplyAndTest(t *testing.T, testCase resource.TestCase) { t.Helper() + if target := os.Getenv(envKeyScratchDump); target != "" { + dumpStepConfigs(t, target, testCase.Steps) + return + } + if IsMockClientTest() { mockClient := clientmock.NewMock() testCase.IsUnitTest = true @@ -47,6 +79,8 @@ func ApplyAndTest(t *testing.T, testCase resource.TestCase) { } }) } else { + // os.Setenv (not t.Setenv) because t.Setenv is incompatible with the t.Parallel() call below. + require.NoError(t, os.Setenv("MESHSTACK_SKIP_VERSION_CHECK", "true")) //nolint:usetesting // see comment above t.Parallel() testCase.PreCheck = func() { DefaultTestPreCheck(t) } testCase.ProtoV6ProviderFactories = ProviderFactoriesForTest() @@ -63,3 +97,67 @@ func DefaultTestPreCheck(t *testing.T) { require.NotEmptyf(t, os.Getenv(envKeyMeshstackApiKey), "Env %s empty, please set before running", envKeyMeshstackApiKey) require.NotEmptyf(t, os.Getenv(envKeyMeshstackApiSecret), "Env %s empty, please set before running", envKeyMeshstackApiSecret) } + +// dumpStepConfigs writes each test step's HCL config to +// //stepNN/{main.tf,provider.tf} so it can be run standalone +// against a local meshStack via the dev-built provider. target is either "1"/"true" +// (dump into the repo-root scratch/ dir) or a directory path. Steps without a Config +// (e.g. import-only steps) are skipped. +func dumpStepConfigs(t *testing.T, target string, steps []resource.TestStep) { + t.Helper() + + base := target + if base == "1" || base == "true" { + base = "scratch" + } + if !filepath.IsAbs(base) { + base = filepath.Join(moduleRoot(t), base) + } + + testDir := filepath.Join(base, sanitizeTestName(t.Name())) + + written := 0 + for i, step := range steps { + if step.Config == "" { + continue + } + stepDir := filepath.Join(testDir, fmt.Sprintf("step%02d", i+1)) + require.NoError(t, os.MkdirAll(stepDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(stepDir, "main.tf"), []byte(step.Config), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(stepDir, "provider.tf"), []byte(scratchProviderTf), 0o644)) + written++ + } + + t.Logf("dumped %d step config(s) to %s", written, testDir) +} + +// moduleRoot returns the repository root by walking up from the working directory until it +// finds go.mod (go test runs in the package dir, e.g. internal/provider/). +func moduleRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + require.NoError(t, err) + for { + if _, statErr := os.Stat(filepath.Join(dir, "go.mod")); statErr == nil { + return dir + } + parent := filepath.Dir(dir) + require.NotEqualf(t, parent, dir, "could not locate go.mod above %s", dir) + dir = parent + } +} + +// sanitizeTestName turns a *testing.T name into a relative path. Subtest separators ("/") +// are kept so subtests nest into directories; any other unsafe character becomes "_". +func sanitizeTestName(name string) string { + return strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + return r + case r == '/' || r == '.' || r == '_' || r == '-': + return r + default: + return '_' + } + }, name) +} diff --git a/internal/util/hash/hash.go b/internal/util/hash/hash.go index c6850941..fc111e1d 100644 --- a/internal/util/hash/hash.go +++ b/internal/util/hash/hash.go @@ -14,22 +14,14 @@ import ( type Hasher struct { Underlying func() hash.Hash - - // Private fields are modified by Option passed to Hasher.Hash. - disallowedMapKeys []reflect.Value } type Sum []byte -type Option func(*Hasher) - -func (h Hasher) Hash(v any, opts ...Option) (Sum, error) { +func (h Hasher) Hash(v any) (Sum, error) { if h.Underlying == nil { h.Underlying = sha256.New } - for _, opt := range opts { - opt(&h) - } sum, _, err := h.hash(nil, reflect.ValueOf(v)) return sum, err } @@ -38,14 +30,6 @@ func (s Sum) Hex() string { return hex.EncodeToString(s) } -func DisallowMapKeys[T comparable](keys ...T) Option { - return func(h *Hasher) { - for _, key := range keys { - h.disallowedMapKeys = append(h.disallowedMapKeys, reflect.ValueOf(key)) - } - } -} - func (h Hasher) hash(root reflectwalk.WalkPath, v reflect.Value) (Sum, bool, error) { w := writer{hasher: h.Underlying()} visitor := func(path reflectwalk.WalkPath, v reflect.Value) error { @@ -104,9 +88,6 @@ func (h Hasher) hash(root reflectwalk.WalkPath, v reflect.Value) (Sum, bool, err } // as we're stopping WalkMap immediately (as we've walked the map values via h.hash(value) above) // the provided key pointer should never be nil (otherwise reflectwalk has an implementation bug) - if err := h.checkDisallowedMapKeys(path, key.Value); err != nil { - return err - } if keySum, _, err := h.hash(path, key.Value); err != nil { return err } else { @@ -147,15 +128,6 @@ func (h Hasher) hash(root reflectwalk.WalkPath, v reflect.Value) (Sum, bool, err return w.hasher.Sum(nil), w.written, err } -func (h Hasher) checkDisallowedMapKeys(path reflectwalk.WalkPath, key reflect.Value) error { - for _, mapKey := range h.disallowedMapKeys { - if key.Equal(mapKey) { - return fmt.Errorf("key path %s matches one of disallowed keys %s", path, h.disallowedMapKeys) - } - } - return nil -} - func (s *Sum) addIgnoringOrder(other Sum) { if len(*s) < len(other) { *s = append(*s, other[len(*s):]...) diff --git a/internal/util/hash/hash_test.go b/internal/util/hash/hash_test.go index 80b91b5b..9f5cc96e 100644 --- a/internal/util/hash/hash_test.go +++ b/internal/util/hash/hash_test.go @@ -240,12 +240,3 @@ func (c *writtenBytesHash) Size() int { func (c *writtenBytesHash) BlockSize() int { panic("not implemented for test") } - -func TestHasher_Hash_disallowedMapKeys(t *testing.T) { - v := []any{ - 1.0, - &map[string]any{"key1": "value1", "key2": map[string]any{"superillegal": true}}, - } - _, err := Hasher{}.Hash(v, DisallowMapKeys("superillegal")) - require.ErrorContains(t, err, "key path **[1]*[key2][superillegal] matches one of disallowed keys [superillegal]") -}