From e52750ccf3b22b6756c4bc3d8014f539ca23e452 Mon Sep 17 00:00:00 2001 From: samirgandhi19 <17574913+samir-gandhi@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:47:32 -0600 Subject: [PATCH 1/3] resource/pingone_davinci_variable: Export secret variables with variable-backed empty values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Secret DaVinci variables (data_type = "secret") were exported as resources and import blocks but without their value block and without a generated Terraform input variable, because skip_conditions suppressed the whole type_discriminated_block for secret data types. They now export like other variable types: conditional_defaults re-emits the suppressed block with the provider-required secret_string key, and the variable extractor backs it with a generated Terraform variable marked sensitive = true whose tfvars entry stays empty (the API returns no readable value). The mutable conditional default no longer fires for secrets, so the API-provided mutable value passes through. Mechanism is declarative — no platform Go code: - variable.yaml: conditional_defaults emits value = {secret_string: ...} for data_type = secret (declared before the mutable rule so secrets keep their API mutable); the eligible_attributes rule gains conditional is_secret_when_all / sensitive_when_all flags on data_type. - schema: VariableRule grows conditional is_secret_when_all / sensitive_when_all lists; the DefaultCondition evaluator moves to schema.EvaluateConditions so the processor and extractor share semantics. - variables/extractor: evaluates the conditional rule flags; absent condition lists never fire (an empty list must not be vacuously true). Secret values are never emitted: the provider requires a non-empty secret_string (LengthAtLeast(1)), so Terraform plan fails until the user fills the value in — the same manual-fill contract as connector clientSecret, which has no length validator. Co-Authored-By: Claude Code --- .changelog/pr-160.txt | 3 + contributing/ARCHITECTURE.md | 15 ++- definitions/pingone/davinci/variable.yaml | 35 +++++- internal/core/processor.go | 47 +------- internal/core/processor_test.go | 82 +++++++++++++ internal/formatters/hcl/formatter_test.go | 52 ++++++++ internal/formatters/tfjson/formatter_test.go | 33 +++++ internal/schema/conditions.go | 50 ++++++++ internal/schema/integration_test.go | 22 +++- internal/schema/types.go | 12 ++ internal/variables/extractor.go | 17 +++ internal/variables/extractor_test.go | 106 ++++++++++++++++ .../secret_variable_integration_test.go | 113 ++++++++++++++++++ 13 files changed, 535 insertions(+), 52 deletions(-) create mode 100644 .changelog/pr-160.txt create mode 100644 internal/schema/conditions.go create mode 100644 internal/variables/secret_variable_integration_test.go diff --git a/.changelog/pr-160.txt b/.changelog/pr-160.txt new file mode 100644 index 0000000..64d2dc1 --- /dev/null +++ b/.changelog/pr-160.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +`resource/pingone_davinci_variable`: Secret variables (`data_type = "secret"`) now export with a `value` block referencing a generated Terraform variable (`sensitive = true`) instead of being exported without a value. The generated variable's tfvars entry is empty — set it manually before applying; Terraform plan fails on an empty secret by design (the provider requires a non-empty `secret_string`), matching connector `clientSecret` handling. Non-secret variables are unchanged. Re-export your configuration to pick up the new blocks. +``` diff --git a/contributing/ARCHITECTURE.md b/contributing/ARCHITECTURE.md index 32e6806..1bfa985 100644 --- a/contributing/ARCHITECTURE.md +++ b/contributing/ARCHITECTURE.md @@ -292,6 +292,8 @@ Some Terraform provider attributes expect a block with exactly one type-specific The processor reads the runtime type of the extracted value, looks up the block key in `type_key_map`, and emits the appropriate single-key map. Values for keys listed in `json_encode_keys` are `json.Marshal`'d and emitted as `RawHCLValue`. +`skip_conditions` suppresses the whole block when matched (OR'd). A suppressed block can be re-emitted by a `conditional_defaults` rule — used by the DaVinci variable definition, where the API returns no readable value for `data_type = "secret"`, so the definition suppresses the block and re-emits it with the provider-required `secret_string` key holding a placeholder that the variable extractor swaps for a `var.X` reference. + #### Conditional Defaults Post-processing attribute overrides based on the state of other processed attributes: @@ -307,7 +309,7 @@ conditional_defaults: value: false ``` -Evaluated after all attributes are processed and before formatting. +Evaluated after all attributes are processed and before formatting. Rules apply in declaration order — a rule that emits an attribute (e.g. the secret `value` block) must be declared before rules whose conditions reference that attribute's emptiness. ### Schema Registry @@ -895,6 +897,7 @@ The `VariableExtractor` (`internal/variables/extractor.go`) evaluates schema-dri - Applies `variables.eligible_attributes` rules for naming and descriptions - Handles `type_discriminated_block` value unwrapping for tfvars output - Custom transforms can also produce variables via `TransformResultWithVariables` +- Applies rule-level `is_secret` / `sensitive` flags and their conditional forms (`is_secret_when_all`, `sensitive_when_all`) — a secret variable renders with `sensitive = true` and its tfvars entry stays empty - **Variable names are sanitized via `utils.SanitizeVariableName()` to ensure valid Terraform identifiers** ### HCL Template Character Escaping in tfvars @@ -1027,6 +1030,16 @@ variables: - attribute_path: value variable_prefix: davinci_variable_ is_secret: false + # Conditional flags — secretness/sensitivity that depends on resource + # data. Uses the same condition vocabulary as conditional_defaults. + sensitive_when_all: + - attribute_equals: + name: data_type + value: secret + is_secret_when_all: + - attribute_equals: + name: data_type + value: secret description: "Value for DaVinci variable {name}" ``` diff --git a/definitions/pingone/davinci/variable.yaml b/definitions/pingone/davinci/variable.yaml index 9e449f9..4969c1e 100644 --- a/definitions/pingone/davinci/variable.yaml +++ b/definitions/pingone/davinci/variable.yaml @@ -96,7 +96,10 @@ attributes: # Value - optional, variable-eligible # Uses type_discriminated_block to map Go runtime type to Terraform block key. - # Skip conditions suppress the block for secret data types. + # Skip conditions suppress the block for secret data types (the API returns + # no readable value); conditional_defaults below re-emits it with the + # provider-required secret_string key so secret variables export like other + # types, backed by an empty sensitive Terraform variable. - name: Value terraform_name: value type: type_discriminated_block @@ -130,9 +133,24 @@ dependencies: import_id_format: "{env_id}/{resource_id}" # Post-processing attribute overrides -# When no value will be written and mutable is false, the Terraform provider -# requires mutable = true. This replaces the Go handleVariableMutableOverride handler. +# Secret variables: the API never returns a readable value (suppressed by the +# skip condition above), so emit the block with the provider-required +# secret_string key. The string placeholder is replaced by the variable +# extractor with a var.X reference (see variables.eligible_attributes below). +# This rule must precede the mutable rule so secret variables keep their +# API-provided mutable value instead of being forced to true. conditional_defaults: + - target_attribute: value + set_value: + secret_string: "REPLACED_BY_VARIABLE_EXTRACTOR" + when_all: + - attribute_empty: value + - attribute_equals: + name: data_type + value: secret + + # When no value will be written and mutable is false, the Terraform provider + # requires mutable = true. This replaces the Go handleVariableMutableOverride handler. - target_attribute: mutable set_value: true when_all: @@ -146,4 +164,15 @@ variables: - attribute_path: value variable_prefix: davinci_variable_ is_secret: false + # Secret variables: value becomes sensitive, and its tfvars entry stays + # empty (the API value is not exportable). Conditional flags — applies + # only when data_type is secret, leaving other variable types untouched. + sensitive_when_all: + - attribute_equals: + name: data_type + value: secret + is_secret_when_all: + - attribute_equals: + name: data_type + value: secret description: "Value for DaVinci variable {name}" diff --git a/internal/core/processor.go b/internal/core/processor.go index 4fb0331..78f4bfd 100644 --- a/internal/core/processor.go +++ b/internal/core/processor.go @@ -1069,57 +1069,12 @@ func unwrapUnionStruct(val reflect.Value) (interface{}, bool) { // for the override to apply. func (p *Processor) evaluateConditionalDefaults(attrs map[string]interface{}, defaults []schema.ConditionalDefault) { for _, cd := range defaults { - if p.allConditionsMet(attrs, cd.WhenAll) { + if schema.EvaluateConditions(attrs, cd.WhenAll) { attrs[cd.TargetAttribute] = cd.SetValue } } } -// allConditionsMet checks whether all conditions in a ConditionalDefault rule -// are satisfied by the current attribute map. -func (p *Processor) allConditionsMet(attrs map[string]interface{}, conditions []schema.DefaultCondition) bool { - for _, cond := range conditions { - if cond.AttributeEmpty != "" { - val, exists := attrs[cond.AttributeEmpty] - if exists && val != nil { - if s, ok := val.(string); ok && s == "" { - // empty string counts as empty - } else { - return false - } - } - // nil or not-present counts as empty — condition passes - } - - if cond.AttributeEquals != nil { - val, exists := attrs[cond.AttributeEquals.Name] - if !exists { - return false - } - if !valuesEqual(val, cond.AttributeEquals.Value) { - return false - } - } - } - return true -} - -// valuesEqual compares two values for equality, handling type coercion between -// bool and interface{} from YAML unmarshaling. -func valuesEqual(a, b interface{}) bool { - if a == b { - return true - } - // YAML unmarshals booleans as bool, but processor may store them differently. - if ab, ok := a.(bool); ok { - if bb, ok := b.(bool); ok { - return ab == bb - } - } - // Compare as strings for mixed-type edge cases. - return fmt.Sprintf("%v", a) == fmt.Sprintf("%v", b) -} - // ProcessResourceList processes a list of API resources func (p *Processor) ProcessResourceList(resourceType string, apiDataList interface{}) ([]*ResourceData, error) { val := reflect.ValueOf(apiDataList) diff --git a/internal/core/processor_test.go b/internal/core/processor_test.go index d38d3e3..3cd4717 100644 --- a/internal/core/processor_test.go +++ b/internal/core/processor_test.go @@ -657,6 +657,88 @@ func TestProcessorConditionalDefaults_NoOverrideWhenMutableTrue(t *testing.T) { assert.Equal(t, true, result.Attributes["mutable"]) } +// secretVariableDef builds a DaVinci-variable-shaped definition: a +// type_discriminated_block value with the secret skip condition, a mutable +// bool, a data_type string, and the two conditional_defaults rules from +// definitions/pingone/davinci/variable.yaml in the same order (secret value +// emission must precede the mutable override). +func secretVariableDef() *schema.ResourceDefinition { + def := tdbDef([]schema.SkipCondition{ + {SourceField: "DataType", Equals: "secret"}, + }) + def.Attributes = append(def.Attributes, + schema.AttributeDefinition{Name: "DataType", TerraformName: "data_type", Type: "string", SourcePath: "DataType"}, + ) + def.ConditionalDefaults = []schema.ConditionalDefault{ + { + TargetAttribute: "value", + SetValue: map[string]interface{}{"secret_string": "REPLACED_BY_VARIABLE_EXTRACTOR"}, + WhenAll: []schema.DefaultCondition{ + {AttributeEmpty: "value"}, + {AttributeEquals: &schema.AttributeValueCondition{Name: "data_type", Value: "secret"}}, + }, + }, + { + TargetAttribute: "mutable", + SetValue: true, + WhenAll: []schema.DefaultCondition{ + {AttributeEmpty: "value"}, + {AttributeEquals: &schema.AttributeValueCondition{Name: "mutable", Value: false}}, + }, + }, + } + return def +} + +func TestProcessorConditionalDefaults_SecretValueBlockEmitted(t *testing.T) { + registry := schema.NewRegistry() + require.NoError(t, registry.Register(secretVariableDef())) + p := core.NewProcessor(registry) + + // Secret variable: API returns no readable value (nil) — skip condition + // suppresses the tdb block, then the conditional default re-emits it with + // the provider-required secret_string key. + mock := &MockTDB{ID: "1", Name: "v", DataType: "secret", Mutable: true, Value: nil} + result, err := p.ProcessResource("test_tdb", mock) + require.NoError(t, err) + + m, ok := result.Attributes["value"].(map[string]interface{}) + require.True(t, ok, "secret variable must have a value block") + assert.Equal(t, "REPLACED_BY_VARIABLE_EXTRACTOR", m["secret_string"]) +} + +func TestProcessorConditionalDefaults_SecretMutablePassesThrough(t *testing.T) { + registry := schema.NewRegistry() + require.NoError(t, registry.Register(secretVariableDef())) + p := core.NewProcessor(registry) + + // With the value block emitted for secrets, the mutable override must not + // fire — the API-provided mutable value passes through. Rule ordering in + // the YAML (secret rule before mutable rule) is what makes this hold. + mock := &MockTDB{ID: "1", Name: "v", DataType: "secret", Mutable: false, Value: nil} + result, err := p.ProcessResource("test_tdb", mock) + require.NoError(t, err) + assert.Equal(t, false, result.Attributes["mutable"], "secret variable keeps API-provided mutable") + assert.Contains(t, result.Attributes, "value") +} + +func TestProcessorConditionalDefaults_SecretRuleNotFiredForStringType(t *testing.T) { + registry := schema.NewRegistry() + require.NoError(t, registry.Register(secretVariableDef())) + p := core.NewProcessor(registry) + + // Plain string variable: tdb emits {"string": ...}, the secret rule does + // not fire (data_type mismatch), and the mutable rule still applies when + // the value is absent. + mock := &MockTDB{ID: "1", Name: "v", DataType: "string", Mutable: false, Value: "hello"} + result, err := p.ProcessResource("test_tdb", mock) + require.NoError(t, err) + + m := result.Attributes["value"].(map[string]interface{}) + assert.Equal(t, "hello", m["string"]) + assert.Nil(t, m["secret_string"], "secret_string key must not leak into non-secret variables") +} + // ── Choice wrapper unwrapping tests ────────────────────────────── // MockChoiceWrapper mimics PingOne SDK choice types like diff --git a/internal/formatters/hcl/formatter_test.go b/internal/formatters/hcl/formatter_test.go index 608e0fe..c8c641e 100644 --- a/internal/formatters/hcl/formatter_test.go +++ b/internal/formatters/hcl/formatter_test.go @@ -969,6 +969,58 @@ func TestFormatter_Format_TypeDiscriminatedBlockVariableReference(t *testing.T) "variable references inside type_discriminated_block must render as var.X") } +// Secret DaVinci variables export a value block keyed secret_string (the +// provider-required key) whose value is a sensitive Terraform variable +// reference. See definitions/pingone/davinci/variable.yaml conditional_defaults. +func TestFormatter_Format_TypeDiscriminatedBlock_SecretStringKey(t *testing.T) { + f := hclformatter.NewFormatter() + + def := &schema.ResourceDefinition{ + Metadata: schema.ResourceMetadata{ + ResourceType: "pingone_davinci_variable", + }, + Attributes: []schema.AttributeDefinition{ + {Name: "ID", TerraformName: "id", Type: "string", Computed: true}, + {Name: "Name", TerraformName: "name", Type: "string"}, + {Name: "DataType", TerraformName: "data_type", Type: "string"}, + { + Name: "Value", + TerraformName: "value", + Type: "type_discriminated_block", + TypeDiscriminatedBlock: &schema.TypeDiscriminatedBlockConfig{ + TypeKeyMap: map[string]string{ + "string": "string", + }, + }, + }, + }, + } + + data := &core.ResourceData{ + Name: "secret-a", + ID: "var-123", + Attributes: map[string]interface{}{ + "id": "var-123", + "name": "secret-a", + "data_type": "secret", + "value": map[string]interface{}{ + "secret_string": core.ResolvedReference{ + IsVariable: true, + VariableName: "davinci_variable_secret_a_value", + }, + }, + }, + } + + output, err := f.Format(data, def, hclformatter.FormatOptions{}) + require.NoError(t, err) + + assert.Contains(t, output, "secret_string = var.davinci_variable_secret_a_value", + "secret_string must render as an unquoted var.X reference") + assert.NotContains(t, output, `"secret_string"`, + "secret_string must not be rendered as a quoted JSON-style key") +} + // ── nil_value: keep_empty on collection types formatter tests ──── // TestFormatter_Format_SetWithNestedAttributesKeepEmpty verifies that an empty diff --git a/internal/formatters/tfjson/formatter_test.go b/internal/formatters/tfjson/formatter_test.go index 32b47c0..4697449 100644 --- a/internal/formatters/tfjson/formatter_test.go +++ b/internal/formatters/tfjson/formatter_test.go @@ -946,3 +946,36 @@ func TestFormat_TypeDiscriminatedBlock_ResolvedVariableReference(t *testing.T) { assert.Equal(t, "${var.davinci_variable_test_value}", value["string"], "variable references inside type_discriminated_block must render as ${var.X}") } + +// Secret DaVinci variables export a value block keyed secret_string (the +// provider-required key) whose value is a sensitive Terraform variable +// reference. See definitions/pingone/davinci/variable.yaml conditional_defaults. +func TestFormat_TypeDiscriminatedBlock_SecretStringKey(t *testing.T) { + f := NewFormatter() + def := baseDef(schema.AttributeDefinition{ + Name: "Value", + TerraformName: "value", + Type: "type_discriminated_block", + TypeDiscriminatedBlock: &schema.TypeDiscriminatedBlockConfig{ + TypeKeyMap: map[string]string{ + "string": "string", + }, + }, + }) + data := baseData("res1", "id1", map[string]interface{}{ + "value": map[string]interface{}{ + "secret_string": core.ResolvedReference{ + IsVariable: true, + VariableName: "davinci_variable_secret_a_value", + }, + }, + }) + out, err := f.Format(data, def, FormatOptions{}) + require.NoError(t, err) + + attrs := unmarshalResource(t, out, "test_resource", "pingcli__res1") + value, ok := attrs["value"].(map[string]interface{}) + require.True(t, ok, "value should be a map object") + assert.Equal(t, "${var.davinci_variable_secret_a_value}", value["secret_string"], + "secret_string must render as ${var.X} in tfjson output") +} diff --git a/internal/schema/conditions.go b/internal/schema/conditions.go new file mode 100644 index 0000000..b8dfb35 --- /dev/null +++ b/internal/schema/conditions.go @@ -0,0 +1,50 @@ +package schema + +import "fmt" + +// EvaluateConditions reports whether all conditions hold against the given +// processed-attribute map. Shared by conditional_defaults evaluation +// (processor) and conditional variable rule flags (extractor) so both use the +// same condition semantics. +func EvaluateConditions(attrs map[string]interface{}, conditions []DefaultCondition) bool { + for _, cond := range conditions { + if cond.AttributeEmpty != "" { + val, exists := attrs[cond.AttributeEmpty] + if exists && val != nil { + if s, ok := val.(string); ok && s == "" { + // empty string counts as empty + } else { + return false + } + } + // nil or not-present counts as empty — condition passes + } + + if cond.AttributeEquals != nil { + val, exists := attrs[cond.AttributeEquals.Name] + if !exists { + return false + } + if !conditionValuesEqual(val, cond.AttributeEquals.Value) { + return false + } + } + } + return true +} + +// conditionValuesEqual compares two values for equality, handling type +// coercion between bool and interface{} from YAML unmarshaling. +func conditionValuesEqual(a, b interface{}) bool { + if a == b { + return true + } + // YAML unmarshals booleans as bool, but processor may store them differently. + if ab, ok := a.(bool); ok { + if bb, ok := b.(bool); ok { + return ab == bb + } + } + // Compare as strings for mixed-type edge cases. + return fmt.Sprintf("%v", a) == fmt.Sprintf("%v", b) +} diff --git a/internal/schema/integration_test.go b/internal/schema/integration_test.go index e3b5282..b4eaf91 100644 --- a/internal/schema/integration_test.go +++ b/internal/schema/integration_test.go @@ -62,8 +62,26 @@ func TestLoadVariableDefinition(t *testing.T) { // Verify variable extraction rules assert.Len(t, def.Variables.EligibleAttributes, 1) - assert.Equal(t, "value", def.Variables.EligibleAttributes[0].AttributePath) - assert.Equal(t, "davinci_variable_", def.Variables.EligibleAttributes[0].VariablePrefix) + valueRule := def.Variables.EligibleAttributes[0] + assert.Equal(t, "value", valueRule.AttributePath) + assert.Equal(t, "davinci_variable_", valueRule.VariablePrefix) + // Secret variables: secretness/sensitivity are conditional on data_type, + // never unconditional (plain variables keep their exportable values). + assert.False(t, valueRule.IsSecret, "secretness must be conditional, not blanket") + assert.Len(t, valueRule.IsSecretWhenAll, 1) + assert.Equal(t, "data_type", valueRule.IsSecretWhenAll[0].AttributeEquals.Name) + assert.Equal(t, "secret", valueRule.IsSecretWhenAll[0].AttributeEquals.Value) + assert.Len(t, valueRule.SensitiveWhenAll, 1) + + // Verify conditional_defaults: the secret value emission rule must + // precede the mutable override so secret variables keep their + // API-provided mutable value. + require.Len(t, def.ConditionalDefaults, 2) + assert.Equal(t, "value", def.ConditionalDefaults[0].TargetAttribute, + "secret value rule must be first") + assert.Equal(t, map[string]interface{}{"secret_string": "REPLACED_BY_VARIABLE_EXTRACTOR"}, + def.ConditionalDefaults[0].SetValue) + assert.Equal(t, "mutable", def.ConditionalDefaults[1].TargetAttribute) } func TestRegistryLoadPlatform(t *testing.T) { diff --git a/internal/schema/types.go b/internal/schema/types.go index 4700e4d..21a1910 100644 --- a/internal/schema/types.go +++ b/internal/schema/types.go @@ -125,6 +125,18 @@ type VariableRule struct { IsSecret bool `yaml:"is_secret"` DefaultValue interface{} `yaml:"default_value,omitempty"` Description string `yaml:"description,omitempty"` + + // IsSecretWhenAll marks the variable as secret (empty tfvars value) only + // when all conditions hold against the processed attribute map. Evaluated + // in addition to the unconditional IsSecret flag — use it when secretness + // depends on resource data, e.g. DaVinci variables whose value is secret + // only for data_type == "secret". + IsSecretWhenAll []DefaultCondition `yaml:"is_secret_when_all,omitempty"` + + // SensitiveWhenAll marks the variable as sensitive (sensitive = true in + // variables.tf) only when all conditions hold against the processed + // attribute map. + SensitiveWhenAll []DefaultCondition `yaml:"sensitive_when_all,omitempty"` } // CustomHandlerDefinition defines optional custom processing diff --git a/internal/variables/extractor.go b/internal/variables/extractor.go index 7c1ea62..03a4b7f 100644 --- a/internal/variables/extractor.go +++ b/internal/variables/extractor.go @@ -92,6 +92,23 @@ func (e *VariableExtractor) Extract( // Apply rule overrides. if hasRule { ev.IsSecret = rule.IsSecret + if rule.IsSecret { + ev.Sensitive = true + } + // Conditional rule flags: secretness/sensitivity that depends on + // resource data (e.g. DaVinci variable values, secret only for + // data_type == "secret"). Evaluated against the processed + // attribute map with the same semantics as conditional_defaults. + // Absent condition lists never fire — unlike conditional_defaults, + // an empty condition set here means "no conditional flag", not + // "always apply" (EvaluateConditions alone is vacuously true). + if len(rule.IsSecretWhenAll) > 0 && schema.EvaluateConditions(data, rule.IsSecretWhenAll) { + ev.IsSecret = true + ev.Sensitive = true + } + if len(rule.SensitiveWhenAll) > 0 && schema.EvaluateConditions(data, rule.SensitiveWhenAll) { + ev.Sensitive = true + } if rule.Description != "" { ev.Description = expandDescription(rule.Description, resourceName) } diff --git a/internal/variables/extractor_test.go b/internal/variables/extractor_test.go index 374557b..d99a5ed 100644 --- a/internal/variables/extractor_test.go +++ b/internal/variables/extractor_test.go @@ -3,6 +3,7 @@ package variables import ( "testing" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/core" "github.com/pingidentity/pingcli-plugin-terraformer/internal/schema" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -135,6 +136,111 @@ func TestExtractUnknownResourceType(t *testing.T) { require.Error(t, err) } +// secretVariableRuleDef mirrors the live variables.eligible_attributes rule +// from definitions/pingone/davinci/variable.yaml: the value attribute is +// eligible, and secretness/sensitivity are conditional on data_type == secret. +func secretVariableRuleDef() *schema.ResourceDefinition { + return &schema.ResourceDefinition{ + Metadata: schema.ResourceMetadata{ + Platform: "pingone", + ResourceType: "pingone_davinci_variable", + }, + Attributes: []schema.AttributeDefinition{ + {Name: "value", TerraformName: "value", Type: "type_discriminated_block", VariableEligible: true}, + }, + Variables: schema.VariableDefinition{ + EligibleAttributes: []schema.VariableRule{ + { + AttributePath: "value", + VariablePrefix: "davinci_variable_", + IsSecret: false, + SensitiveWhenAll: []schema.DefaultCondition{ + {AttributeEquals: &schema.AttributeValueCondition{Name: "data_type", Value: "secret"}}, + }, + IsSecretWhenAll: []schema.DefaultCondition{ + {AttributeEquals: &schema.AttributeValueCondition{Name: "data_type", Value: "secret"}}, + }, + Description: "Value for DaVinci variable {name}", + }, + }, + }, + } +} + +func TestExtract_ConditionalSecretRule(t *testing.T) { + reg := testRegistry(secretVariableRuleDef()) + ext := NewVariableExtractor(reg) + + // Secret variable: processor-emitted placeholder inside the tdb map. + data := map[string]interface{}{ + "data_type": "secret", + "value": map[string]interface{}{"secret_string": "REPLACED_BY_VARIABLE_EXTRACTOR"}, + } + vars, err := ext.Extract("pingone_davinci_variable", data, "secret-a") + require.NoError(t, err) + require.Len(t, vars, 1) + + v := vars[0] + assert.True(t, v.IsSecret, "data_type=secret must mark the variable secret") + assert.True(t, v.Sensitive, "secret variables must be sensitive") + assert.Equal(t, "davinci_variable_secret_a_value", v.VariableName) + assert.Equal(t, "Value for DaVinci variable secret-a", v.Description) + assert.Equal(t, "REPLACED_BY_VARIABLE_EXTRACTOR", v.CurrentValue) +} + +func TestExtract_ConditionalSecretRule_NotSecretForStringType(t *testing.T) { + reg := testRegistry(secretVariableRuleDef()) + ext := NewVariableExtractor(reg) + + // Plain string variable: same rule, but data_type != secret → the + // conditional flags must not fire and the exported value keeps its + // tfvars value. + data := map[string]interface{}{ + "data_type": "string", + "value": map[string]interface{}{"string": "abcd"}, + } + vars, err := ext.Extract("pingone_davinci_variable", data, "string-a") + require.NoError(t, err) + require.Len(t, vars, 1) + + v := vars[0] + assert.False(t, v.IsSecret, "non-secret data_type must not be marked secret") + assert.False(t, v.Sensitive, "non-secret data_type must not be sensitive") +} + +func TestExtract_ConditionalRule_NoConditionsNeverFires(t *testing.T) { + // A rule with no conditional flags must stay non-secret/non-sensitive + // even when other rules in the same definition carry them — guards the + // vacuous-true pitfall of evaluating an empty condition list. + def := secretVariableRuleDef() + def.Attributes = append(def.Attributes, + schema.AttributeDefinition{Name: "name", TerraformName: "name", Type: "string", VariableEligible: true}, + ) + def.Variables.EligibleAttributes = append(def.Variables.EligibleAttributes, + schema.VariableRule{AttributePath: "name", VariablePrefix: "davinci_variable_"}, + ) + + reg := testRegistry(def) + ext := NewVariableExtractor(reg) + + data := map[string]interface{}{ + "data_type": "secret", + "value": map[string]interface{}{"secret_string": "REPLACED_BY_VARIABLE_EXTRACTOR"}, + "name": "secret-a", + } + vars, err := ext.Extract("pingone_davinci_variable", data, "secret-a") + require.NoError(t, err) + require.Len(t, vars, 2) + + byPath := make(map[string]*core.ExtractedVariable) + for _, v := range vars { + byPath[v.AttributePath] = v + } + assert.True(t, byPath["value"].IsSecret, "value rule with matching conditions fires") + assert.False(t, byPath["name"].IsSecret, "rule without conditional flags must never fire") + assert.False(t, byPath["name"].Sensitive) +} + func TestInferTerraformType(t *testing.T) { tests := []struct { in string diff --git a/internal/variables/secret_variable_integration_test.go b/internal/variables/secret_variable_integration_test.go new file mode 100644 index 0000000..623fa8e --- /dev/null +++ b/internal/variables/secret_variable_integration_test.go @@ -0,0 +1,113 @@ +package variables_test + +import ( + "testing" + + "github.com/pingidentity/pingcli-plugin-terraformer/definitions" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/core" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/platform/pingone" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/schema" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/variables" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockDaVinciVariable mirrors the SDK's DaVinciVariableResponse for the fields +// the variable.yaml definition reads. Value mirrors the SDK union struct — +// nil for secret variables, a *string for string-typed values. +type mockDaVinciVariable struct { + ID string + Name string + Context *string + DataType string + Mutable *bool + Min *int + Max *int + FlowID *string + Value *string +} + +// TestSecretVariablePipeline is the end-to-end check for secret DaVinci +// variable export: the real variable.yaml definition drives the processor and +// extractor, and a data_type=secret resource must come out with +// value = { secret_string = var.davinci_variable__value } while a plain +// string variable keeps its exportable value. +func TestSecretVariablePipeline(t *testing.T) { + registry := schema.NewRegistry() + require.NoError(t, registry.LoadFromFS(definitions.FS, "pingone")) + customReg := core.NewCustomHandlerRegistry() + pingone.RegisterCustomHandlers(customReg) + proc := core.NewProcessor(registry, core.WithCustomHandlers(customReg)) + + mutable := true + flowCtx := "company" + mock := &mockDaVinciVariable{ + ID: "37b88458-74c3-4c4d-aa42-295e29c29f2b", + Name: "secret-a", + Context: &flowCtx, + DataType: "secret", + Mutable: &mutable, + } + + result, err := proc.ProcessResource("pingone_davinci_variable", mock) + require.NoError(t, err) + assert.Equal(t, "secret", result.Attributes["data_type"]) + + // The conditional default re-emitted the suppressed block with the + // provider-required secret_string key. + valueMap, ok := result.Attributes["value"].(map[string]interface{}) + require.True(t, ok, "secret variable must have a value block") + assert.Equal(t, "REPLACED_BY_VARIABLE_EXTRACTOR", valueMap["secret_string"]) + + // Extractor: the value variable is secret + sensitive, and the injection + // contract (single-key tdb map) holds for cmd/export.go. + ext := variables.NewVariableExtractor(registry) + vars, err := ext.Extract("pingone_davinci_variable", result.Attributes, result.Name) + require.NoError(t, err) + require.Len(t, vars, 1) + + v := vars[0] + assert.True(t, v.IsSecret) + assert.True(t, v.Sensitive) + assert.Equal(t, "value", v.AttributePath) + assert.Equal(t, "davinci_variable_secret_a_value", v.VariableName) + assert.Equal(t, "REPLACED_BY_VARIABLE_EXTRACTOR", v.CurrentValue) +} + +func TestSecretVariablePipeline_PlainStringUnchanged(t *testing.T) { + registry := schema.NewRegistry() + require.NoError(t, registry.LoadFromFS(definitions.FS, "pingone")) + customReg := core.NewCustomHandlerRegistry() + pingone.RegisterCustomHandlers(customReg) + proc := core.NewProcessor(registry, core.WithCustomHandlers(customReg)) + + mutable := true + flowCtx := "company" + value := "abcd" + mock := &mockDaVinciVariable{ + ID: "11111111-2222-4333-8444-555555555555", + Name: "string-a", + Context: &flowCtx, + DataType: "string", + Mutable: &mutable, + Value: &value, + } + + result, err := proc.ProcessResource("pingone_davinci_variable", mock) + require.NoError(t, err) + + valueMap, ok := result.Attributes["value"].(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "abcd", valueMap["string"], "plain variables keep their runtime-typed key") + assert.Nil(t, valueMap["secret_string"]) + + ext := variables.NewVariableExtractor(registry) + vars, err := ext.Extract("pingone_davinci_variable", result.Attributes, result.Name) + require.NoError(t, err) + require.Len(t, vars, 1) + + v := vars[0] + assert.False(t, v.IsSecret, "plain variable values keep their tfvars value") + assert.False(t, v.Sensitive) + assert.Equal(t, "abcd", v.CurrentValue) +} From f27ba3b9e880bf0a0dea1ea1dcd6804799e0e770 Mon Sep 17 00:00:00 2001 From: samirgandhi19 <17574913+samir-gandhi@users.noreply.github.com> Date: Tue, 13 Oct 2026 17:36:33 -0600 Subject: [PATCH 2/3] core: Deep-copy conditional_defaults SetValue to stop cross-resource aliasing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parsed YAML ConditionalDefault.SetValue is a single shared value across every resource of a type. Assigning it directly into each resource's Attributes map aliased one underlying map — for the DaVinci secret-variable rule (value = {secret_string: ...}), the variable extractor's subsequent ResolvedReference injection into that single-key block was visible from every resource, so two secret variables both rendered the LAST resource's var.X reference in the exported .tf (secret-a pointed at davinci_variable_secret_b_value). evaluateConditionalDefaults now deep-copies map and slice SetValue values per resource; scalars are unaffected. Regression test TestSecretVariablePipeline_TwoSecretsNoCrossWire processes two secret variables through the real YAML definition and asserts each resource references its own variable. Found in live export verification against the org-admin environment; unit tests using single resources could not observe it. Co-Authored-By: Claude Code --- internal/core/processor.go | 30 +++++++- .../secret_variable_crosstalk_test.go | 77 +++++++++++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 internal/variables/secret_variable_crosstalk_test.go diff --git a/internal/core/processor.go b/internal/core/processor.go index 78f4bfd..4d02b15 100644 --- a/internal/core/processor.go +++ b/internal/core/processor.go @@ -1067,14 +1067,42 @@ func unwrapUnionStruct(val reflect.Value) (interface{}, bool) { // evaluateConditionalDefaults applies declarative post-processing overrides // to the processed attributes. Each rule's WhenAll conditions must all be true // for the override to apply. +// +// SetValue values that are maps are deep-copied per resource: the parsed YAML +// rule is shared across every resource of a type, and assigning the map +// directly would alias one underlying map into every Attributes map — later +// per-resource mutations (e.g. the variable extractor's ResolvedReference +// injection into a single-key block) would then leak across resources. func (p *Processor) evaluateConditionalDefaults(attrs map[string]interface{}, defaults []schema.ConditionalDefault) { for _, cd := range defaults { if schema.EvaluateConditions(attrs, cd.WhenAll) { - attrs[cd.TargetAttribute] = cd.SetValue + attrs[cd.TargetAttribute] = deepCopyValue(cd.SetValue) } } } +// deepCopyValue recursively copies maps and slices so a shared schema-derived +// value can be safely mutated per resource. Scalars and type-asserted special +// values are returned unchanged. +func deepCopyValue(v interface{}) interface{} { + switch typed := v.(type) { + case map[string]interface{}: + copied := make(map[string]interface{}, len(typed)) + for k, val := range typed { + copied[k] = deepCopyValue(val) + } + return copied + case []interface{}: + copied := make([]interface{}, len(typed)) + for i, val := range typed { + copied[i] = deepCopyValue(val) + } + return copied + default: + return v + } +} + // ProcessResourceList processes a list of API resources func (p *Processor) ProcessResourceList(resourceType string, apiDataList interface{}) ([]*ResourceData, error) { val := reflect.ValueOf(apiDataList) diff --git a/internal/variables/secret_variable_crosstalk_test.go b/internal/variables/secret_variable_crosstalk_test.go new file mode 100644 index 0000000..5af06e1 --- /dev/null +++ b/internal/variables/secret_variable_crosstalk_test.go @@ -0,0 +1,77 @@ +package variables_test + +import ( + "testing" + + "github.com/pingidentity/pingcli-plugin-terraformer/definitions" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/core" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/platform/pingone" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/schema" + "github.com/pingidentity/pingcli-plugin-terraformer/internal/variables" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSecretVariablePipeline_TwoSecretsNoCrossWire reproduces the live-export +// bug where two secret variables both rendered var.davinci_variable_secret_b_value: +// the conditional_defaults SetValue map is a single shared map[string]interface{} +// value from the parsed YAML. Assigning it into each resource's Attributes map +// aliases the same underlying map across all resources — the last resource to +// have its ResolvedReference injected wins for every resource of the type. +func TestSecretVariablePipeline_TwoSecretsNoCrossWire(t *testing.T) { + registry := schema.NewRegistry() + require.NoError(t, registry.LoadFromFS(definitions.FS, "pingone")) + customReg := core.NewCustomHandlerRegistry() + pingone.RegisterCustomHandlers(customReg) + proc := core.NewProcessor(registry, core.WithCustomHandlers(customReg)) + + mutable := true + company := "company" + + // Process a LIST of two secret variables, like the live orchestrator does. + list := []*mockDaVinciVariable{ + {ID: "id-a", Name: "secret-a", Context: &company, DataType: "secret", Mutable: &mutable}, + {ID: "id-b", Name: "secret-b", Context: &company, DataType: "secret", Mutable: &mutable}, + } + + results, err := proc.ProcessResourceList("pingone_davinci_variable", list) + require.NoError(t, err) + require.Len(t, results, 2) + + // Simulate cmd/export.go's injection loop on each resource. + ext := variables.NewVariableExtractor(registry) + names := make(map[string]string) // resource name -> referenced variable name + for _, rd := range results { + extracted, extErr := ext.Extract("pingone_davinci_variable", rd.Attributes, rd.Name) + require.NoError(t, extErr) + require.Len(t, extracted, 1) + + ev := extracted[0] + names[rd.Name] = ev.VariableName + + ref := core.ResolvedReference{IsVariable: true, VariableName: ev.VariableName} + if ev.CurrentValue != nil { + if s, ok := ev.CurrentValue.(string); ok { + ref.OriginalValue = s + } + } + if m, ok := rd.Attributes[ev.AttributePath].(map[string]interface{}); ok && len(m) == 1 { + for k := range m { + m[k] = ref + } + } + } + + // Each resource must reference its OWN variable. + assert.Equal(t, "davinci_variable_secret_a_value", names["secret-a"]) + assert.Equal(t, "davinci_variable_secret_b_value", names["secret-b"]) + + // And the in-attribute reference must match the per-resource name — the + // aliasing bug made both resources carry the LAST injected reference. + for _, rd := range results { + m := rd.Attributes["value"].(map[string]interface{}) + ref := m["secret_string"].(core.ResolvedReference) + assert.Equal(t, names[rd.Name], ref.VariableName, + "resource %s must reference its own variable", rd.Name) + } +} From 76d13f110103b14756e8c3a5a13182788c068f17 Mon Sep 17 00:00:00 2001 From: samirgandhi19 <17574913+samir-gandhi@users.noreply.github.com> Date: Fri, 16 Oct 2026 15:07:12 -0600 Subject: [PATCH 3/3] chore: Rename changelog file to PR number 161 The PR opened as #161, not the predicted #160. Renames the changelog entry file and updates the PR references in the generated v0.5.2 release notes to match. Co-Authored-By: Claude Code # Conflicts: # release-notes/v0.5.2/GITHUB_RELEASE.md # release-notes/v0.5.2/RELEASE_NOTES.adoc --- .changelog/{pr-160.txt => pr-161.txt} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{pr-160.txt => pr-161.txt} (100%) diff --git a/.changelog/pr-160.txt b/.changelog/pr-161.txt similarity index 100% rename from .changelog/pr-160.txt rename to .changelog/pr-161.txt