fix: paginate template lookup for keyfactor_template_role_binding#173
Draft
spbsoluble wants to merge 68 commits into
Draft
fix: paginate template lookup for keyfactor_template_role_binding#173spbsoluble wants to merge 68 commits into
spbsoluble wants to merge 68 commits into
Conversation
…ates The keyfactor-go-client GetTemplates() fix (Keyfactor/keyfactor-go-client#55) makes the client paginate GET /Templates with ?PageReturned&ReturnLimit. The VCR matcher compares the request query string exactly, so every cassette that recorded a no-query GET /Templates had to be re-recorded: - template_role_binding_resource{,_import}: re-recorded (+ params) - certificate_template_data_source / resource / resource_update: re-recorded Adds an opt-in replayable-interactions VCR variant (newVCRProviderFactoriesReplayable) used only by the read-only certificate-template data-source unit test. The SDK refresh cycle reads the same template by id more times in replay than the record run captures; consuming-mode replay (the default) then fails with "requested interaction not found". Replayable mode stays opt-in so stateful/polling cassettes keep consuming ordered interactions. NOTE: requires a keyfactor-go-client release containing Keyfactor/keyfactor-go-client#55 and a corresponding go.mod bump before this is mergeable / CI-green. Refs #172
Pulls in the paginated GetTemplates() fix (Keyfactor/keyfactor-go-client#55), so keyfactor_template_role_binding resolves templates that sort beyond the first 50 on the instance. RC pending the final v3.5.6 release. Refs #172
rc.1 adds the security/compliance audit hardening to GetTemplates (max-page DoS guard, per-iteration body close, audit logging) on top of the pagination fix. Pagination semantics unchanged; provider unit suite re-validated green against rc.1. RC pending the final v3.5.6 release. Refs #172
spbsoluble
force-pushed
the
fix/template-role-binding-pagination
branch
from
June 20, 2026 00:09
98283a9 to
1d91927
Compare
…ling TestIntKeyfactorCertificateResource_SANs and TestIntKeyfactorCertificateAuthorityResourceUpdate fail only due to lab constraints, not provider bugs. Handle each in-test so an unexpected failure still fails: - SANs: add ErrorCheck(skipOnKnownLabConstraint) for the EJBCA "Wrong number of DNSNAME fields in Subject Alternative Name" error. - CA update: the blocked-deletion error surfaces at the SDK's post-test destroy (a defer that bypasses ErrorCheck), so probe the Certificates endpoint up front and skip with a warning when the CA has issued certs (or the probe is inconclusive). - skipOnKnownLabConstraint: normalize whitespace before matching, since Terraform wraps long error messages across indented lines. Verified against the ses2541 lab: both now SKIP (warn), not FAIL. Refs #172
Add v2.9.1 CHANGELOG section (template_role_binding template lookup now pages past the first 50 templates via the paginated GetTemplates in keyfactor-go-client v3.5.6) and set version to 2.9.1-rc.0. Refs #172
… Update (#174) * fix(certificate): guard against nil GetCertificateContext response in Update The Update path called GetCertificateContext and, when it returned (nil, err), only logged a warning via hasAPIErrors before dereferencing certGetResp.ContentBytes — causing a SIGSEGV nil-pointer dereference during terraform apply (observed when flipping certificate_format to PFX on an in-place update). The Read path was hardened with certGetResp != nil guards in v2.9.0, but Update was missed. Update now returns a clean diagnostic error and bails out when the certificate GET fails or returns a nil response, before any dereference. All subsequent certGetResp accesses in Update were already nil-guarded. Adds TestUnitKeyfactorCertificateResource_UpdateNilGetResponse, an httptest-backed regression test that drives Update with a failing certificate GET and asserts a diagnostic error is surfaced instead of a panic. * test(certificate): tighten Update nil-deref regression test + update-scoped diagnostic Add ERR_SUMMARY_CERTIFICATE_RESOURCE_UPDATE and use it in the defensive certGetResp == nil guard in Update, so the update path no longer borrows the read-path diagnostic summary. Correct the regression test's docstring/comments to describe what is actually exercised (GET 500 -> hasAPIErrors early return, not the format-change/parseLeafCert path) and note the certGetResp == nil guard is belt-and-suspenders for a (nil, nil) response not reproducible via the httptest harness. Trim the plan to only the fields load-bearing before the early return.
…d add cert Update fix
…with derefOrEmpty getStringType(...).Value appeared at 7 call sites (helpers.go and 6 in resource_keyfactor_oauth_security_role_claim_association.go) purely to dereference a *string to a plain Go string for a non-nullable request DTO field, discarding the types.String/Null flag getStringType exists to carry. Using a Terraform-state conversion helper for that obscures intent and adds an unnecessary intermediate types.String allocation. Added derefOrEmpty(v *string) string next to getStringType in helpers.go and replaced all 7 call sites. No behavior change: nil still maps to "", non-nil still passes the value through unchanged. Added TestUnitDerefOrEmpty for direct coverage.
certUpdateMockAuthConfig (resource_keyfactor_certificate_unit_test.go), certDeployMockAuthConfig (resource_keyfactor_certificate_deploy_unit_test.go), and oauthRoleClaimAssocMockAuthConfig (resource_keyfactor_oauth_security_role_claim_association_unit_test.go) were three near-identical httptest-backed mock api.AuthConfig implementations, each with its own GetServerConfig/GetHttpClient/ Authenticate/GetCommandVersion bodies. This was already flagged as deferred debt in this repo's own CHANGELOG.md "Pending" section. Extracted one shared mockAuthConfig type into test_helpers_test.go (alongside the existing VCR-backed vcrAuthConfig, which follows the same "implements the AuthConfig interface used by both the v3 Client and the SDK v24 clients" pattern). The only real difference between the three original mocks was Host format: api.Client wants a scheme-prefixed URL plus an explicit APIPath, while the SDK v24 clients' prepareRequest sets the scheme itself and wants a bare "host:port" Host. mockAuthConfig takes an apiPath and a stripScheme flag to cover both, via two constructors (newCertAPIMockAuthConfig, newSDKMockAuthConfig). All three call sites (newCertUpdateMockClient, newCertDeployMockClient, newOAuthRoleClaimAssocMockClient) now build the shared type instead of their own. oauthRoleClaimAssocUnreachableAuthConfig, a different mock in the claim-association test file simulating a transport-level failure, is untouched -- it is not one of the three duplicates this cleanup targets. No behavior change; covered by the full existing test suite for all three files (every test in them exercises the mock client). Also removes the "extract shared httptest mock helper" item from CHANGELOG.md's "Pending" section now that it is done.
Folds the 7 verified code-review findings from this triage pass into the existing v2.9.1 CHANGELOG entry, following this release's established style: the keyfactor_security_identity/keyfactor_security_role bullets are extended in place (same resources, same bug class, same unreleased version) rather than duplicated; keyfactor_template_role_binding and the new OAuth claim warning get their own additions. The three cleanup items (enum-pointer dedup, derefOrEmpty, shared httptest mock) are logged under Chore/Internal. Removes "extract the shared httptest mock helper" from the Pending section now that it has landed, and adds a Pending note flagging keyfactor_certificate_authority's remaining unaudited Optional+Computed pointer fields as the same bug class, since this pass touched that resource's enum converters without auditing the rest of it.
…mmand PUT is full-replace) Command's PUT /Security/Roles is a full-replace endpoint, not a merge patch: omitting the "Permissions" key from the request body entirely (not just sending null) still clears the role's permissions server-side. Confirmed live against a real Command instance while validating PR #179 end-to-end. buildSecurityRoleUpdateArg now resends state.Permissions explicitly whenever the config-declared value is Null/Unknown, instead of omitting the field and relying on the (incorrect) assumption that absence means "leave unchanged". Adds TestUnitKeyfactorSecurityRoleResource_UpdateOmittingPermissionsPreservesThem, a REAL Terraform Core plan/apply/refresh cycle test (resource.UnitTest + ProtoV6ProviderFactories) closing the fix's Core-cycle coverage gap: the prior regression tests for this bug class all called Update()/Read() directly with hand-built tfsdk.Plan/State/Config values, so they could not catch a "Provider produced inconsistent result after apply" error, which only Terraform Core's own plan/apply diffing surfaces. The new test is backed by a hand-built httptest mock server rather than a VCR cassette, since the VCR matcher intentionally ignores request bodies and so could never assert what the provider actually sent on the PUT call. Confirmed this test fails against the pre-fix buildSecurityRoleUpdateArg (3-argument form, no state fallback) with exactly "Provider produced inconsistent result after apply: .permissions: element 0 has vanished", and passes against the fix.
…9 fixes Adds three purpose-built Terraform CLI demo directories (following the existing terraform/oauth_security_demo/ convention) that validate recent resource_keyfactor_security_role.go / resource_keyfactor_security_identity.go fixes using the actual terraform binary against a live lab, one level up from this repo's Go-level resource.UnitTest coverage: - terraform/security_identity_demo/: proves PR179's keyfactor_identity `roles` Optional+Computed fix. `make security-identity-demo-omit-update` applies with roles declared, then again with roles omitted from config (an unrelated Update isn't even needed here -- the roles diff alone triggers Update on this resource, since account_name is RequiresReplace and there's no other user-settable attribute). Requires an AD-backed lab for Create (see the target's NOTE). - terraform/security_role_permissions_demo/: proves PR179's keyfactor_role `permissions` Optional+Computed fix, and the follow-up buildSecurityRoleUpdateArg fix (resend state.Permissions explicitly instead of omitting the field, since Command's PUT /Security/Roles is a full replace). `make security-role-permissions-demo-omit-update` applies with permissions declared, applies again with description changed and permissions omitted, then cross-checks via a direct Command REST call that permissions are still intact server-side afterward. Ran this against int25-4-1.kftestlab.com while validating PR #179: clean apply, no inconsistent-result crash, permissions confirmed preserved server-side. - terraform/security_role_demo/: proves the separate, already-merged PR178 fix (out-of-band permission drift detection on Read) via `make security-role-demo-oob-drift`. This demo predates PR179 but was never committed; adding it now alongside its siblings for completeness. GNUmakefile gains top-level wrappers for all three (security-identity-demo*, security-role-permissions-demo*, security-role-demo*), matching the existing oauth-security-demo* convention.
…sources Extends the real-terraform-CLI lab validation to every resource touched by PR #179, per direct request. Adds three more demo directories alongside the existing security_identity_demo/security_role_permissions_demo: - terraform/pam_provider_type_demo/: full lifecycle (plan, apply, import, reconcile, drift-check, destroy) for keyfactor_pam_provider_type, proving the enumPtrToTfInt64 refactor of pamParameterDataTypePtrToTfInt64 didn't regress the real create/read/import round-trip for parameters[].data_type. Ran clean against int25-4-1.kftestlab.com: 0 drift after import, both data_type values (1, 2) round-tripped correctly. - terraform/template_role_binding_demo/: same lifecycle shape for keyfactor_template_role_binding, proving the Read()-swallowed-error fix. Documents a KNOWN BLOCKER discovered while validating it: `apply` fails against every template in this lab with "'Policies' cannot be empty" -- keyfactor-go-client/v3's UpdateTemplateArg has no Policies field at all, and buildTemplateRoleBindingUpdateArg never sets one. This looks like a genuine, separate SDK/Command-API-version gap unrelated to PR179's Read()-only fix; flagging for separate triage rather than fixing here. - terraform/certificate_authority_demo/: deliberately import-only (not full CRUD) for keyfactor_certificate_authority. There is exactly one real CA in this lab; fabricating a second to freely create/destroy risked confusing its PKI setup for no real benefit, since the enumPtrToTfInt64 refactor (enrollmentTypePtrToTfInt64, keyRetentionPtrToTfInt64, cleanupTimeUnitsPtrToTfInt64) only touches how Read() converts already-fetched values. Imported the lab's real CA (`Sub-CA`, id 1) cleanly; confirmed via `terraform show` that allowed_enrollment_types (0) and key_retention ("Indefinite") round-trip correctly post-import, and time_after_expiration_units correctly reads as null (matching the server's null TimeAfterExpirationUnits) -- exercising all three refactored converters' nil and non-nil paths against real data. Did not run a reconcile `apply` against this shared, non-disposable CA connection. Also re-ran keyfactor_role's demo through the FULL lifecycle this time (previously only the targeted 2-step regression proof had been run): plan → apply → import-all → reconcile → drift-check (no changes) → destroy, all clean. Ditto for keyfactor_oauth_security_role / keyfactor_oauth_security_role_claim_association via the existing oauth_security_demo, including its lab-update step (exercising mapOAuthSecurityClaimsFromRole's real Update() code path): plan → apply → update → import-all → reconcile → drift-check (no changes) → destroy, all clean. Remaining gap: keyfactor_identity's full lifecycle requires an AD-backed lab account (the default lab rejects Create with HTTP 400 for any account_name); not run here pending a decision on which lab/account to use. GNUmakefile gains top-level wrappers for the three new demos, matching the existing convention.
fix: triage code-review findings from PR #177
…covery Backfill three gaps in the v2.9.1 changelog that landed on this branch via PR #178/#179 but weren't yet reflected: the security_role Read drift-detection fix, the security_role full-replace Update fix (Command's PUT /Security/Roles clears permissions when the field is merely omitted, not just explicitly nulled), and the new terraform/*_demo real-CLI lab validation infrastructure. Also add a known-gap entry for template_role_binding's TemplatePolicy full-replace issue (#180, blocked on keyfactor-go-client#57) and update the GA dependency gate to reflect #55/#56 merging and #57 joining the same v3.5.6 GA cut.
The terraform/*_demo/ lab-validation directories added while validating PR178/PR179 (security_identity_demo, security_role_permissions_demo, security_role_demo, pam_provider_type_demo, template_role_binding_demo, certificate_authority_demo) are useful for local validation but not worth the size they add to an already-large PR. Untracked them (files remain on disk, untracked) and reverted the matching GNUmakefile targets. Also reformats the v2.9.1 changelog's audit-findings section to match the rest of the file's concise, user-facing style instead of an internal commit-narrative style, and removes the changelog bullet that documented the now-untracked demo infrastructure as shipped.
…3.5.6 v3.5.6 GA (Keyfactor/keyfactor-go-client#55, #56, #57) includes the GetTemplates pagination fix, the store-type/EntryPassword omitempty fixes, and the TemplatePolicy full-replace fix (issue #180) that previously required an RC pin. Verified vendor-free (rm -rf vendor, go build/test -mod=mod against the real published module): clean build, full TestUnit suite green (0 failures).
keyfactor_template_role_binding attach/detach fails on every template linked to an enrollment pattern with "'Policies' cannot be empty" (#180). Command's UpdateTemplate is a full-replace PUT: the request's TemplatePolicy carries the template's PrimaryKeyAlgorithms / AlternativeKeyAlgorithms, and Command derives its internal "Policies" set from that object. buildTemplateRoleBindingUpdateArg had no way to populate it because api.UpdateTemplateArg had no TemplatePolicy field at all, so every attach/detach silently omitted it and Command rejected the update outright for any enrollment-pattern-linked template. Confirmed live against a Command 25.4.1 instance: the previous request shape reproduces the error on an enrollment-pattern-linked template; round-tripping TemplatePolicy from the prior GetTemplate response succeeds. Templates with no policy configured (nil TemplatePolicy) correctly stay nil rather than fabricating one. Requires the corresponding keyfactor-go-client fix (adds GetTemplateResponse.TemplatePolicy / UpdateTemplateArg.TemplatePolicy) to be published before this builds against a released SDK version. Adds a red/green regression test: TestUnitTemplateRoleBindingUpdateArgPreservesTemplatePolicy.
…it-triage fix: resolve 29 inconsistent-state findings from resource-wide audit
…ner API probes Add api-update-template and api-template-schema-diff (mirroring api-update-ca / api-ca-schema-diff) to verify the Templates PUT wire type for KeyUsage, and api-set-cert-owner / api-clear-cert-owner to verify the Certificates Owner clear payload. Used to live-verify four Command v25.5 API behaviors against the lab ahead of the go-client KeyUsage type fix and CA schedule/owner work.
buildTemplateRoleBindingUpdateArg never carried KeyUsage from GetTemplate onto the UpdateTemplate request, so every role attach/detach silently reset the template's KeyUsage bitmask to 0 on Command even though this resource does not manage KeyUsage at all. Command's UpdateTemplate is a full replacement (PR #180 context), so any omitted field is cleared server-side. Pin keyfactor-go-client/v3 v3.6.0-rc.0 (Keyfactor/keyfactor-go-client#58), which changes UpdateTemplateArg.KeyUsage from *bool to *int to match Command's v25.5 wire format (int bitmask on both GET and PUT; a JSON boolean is rejected with HTTP 400). Use the non-collapsing pointer pattern already established in this function so a KeyUsage of 0 round- trips instead of being dropped by omitempty. TestUnitTemplateRoleBindingUpdatePreservesKeyUsage reproduces the wipe end-to-end against a mock Command server (GET returns KeyUsage 160, capture the PUT body) and fails on the unfixed code with the PUT body carrying no KeyUsage field at all.
Read() rebuilt the roles state unconditionally from the server's canonical role names (identity.Roles), even when the declared role set was semantically identical to the server's -- only spelled differently in case, or declared by numeric role ID instead of name. That rewrote a practitioner's declared casing/ID-form to Command's canonical name on every Read, manufacturing a diff that no apply could ever resolve. Add identityRolesResultForRead, mirroring permissionsResultForUpdate's order-vs-drift invariant (resource_keyfactor_security_role.go): if the prior state's role list bijectively matches the server's role list (numeric role ID, or case-insensitive name), return the prior state verbatim; otherwise return the server's canonical names so real out-of-band role drift still surfaces. Create/Update are unaffected -- they already write verbatim-from-plan. TestUnitSecurityIdentityReadKeepsDeclaredRoleSpelling and TestUnitSecurityIdentityReadNumericIdMatchesServerRole reproduce the bug against a mock Command server and fail on the unfixed code (Read overwrites "administrators"/"5" with the server's "Administrators"). TestUnitSecurityIdentityReadSurfacesRealDrift is the negative-case companion, asserting a genuinely different role set still surfaces as drift. Filed #183 for the separate, pre-existing `[^\w]` regex mangling bug in Create/Update's role name lookup (out of scope here).
Introduce keyfactor/attribute_contract.go, standardizing the "server-managed unless declared" pattern this provider already applies ad hoc in several resources (template_role_binding's KeyUsage, security_role's Permissions, security_identity's Roles) into two reusable primitives: - declaredInConfig(v attr.Value): the single predicate for "did the practitioner declare this attribute", always keyed on request.Config (never request.Plan, since Optional+Computed plan modifiers rewrite Plan to carry forward state on omission). - pairedVariantModifier / pairedWith(sibling): a plan modifier for mutually-exclusive attribute pairs (e.g. an interval-based vs. a daily-time-based schedule variant), replacing UseStateForUnknown on each half of such a pair. Modify order: (1) plan already known -> leave it; (2) sibling declared in Config -> plan Null, so a variant switch produces a truthful diff instead of resurrecting the superseded attribute's prior value; (3) neither declared -> fall back to useStateOrNullModifier semantics (copy state forward, stay Unknown on Create). Generalizes useStateOrNullModifier (resource_keyfactor_certificate_template.go) and formatDependentModifier (resource_keyfactor_certificate.go), and works generically across attr.Value types (types.Int64, types.String, ...) via the attr.Type ValueFromTerraform/TerraformType round-trip rather than a type switch. This lands the shared mechanism ahead of its first real consumer (certificate_authority's preserveCAUpdateFields, next commit) and the mutually-exclusive schedule variant work planned for a follow-on PR.
… plan Rework preserveCAUpdateFields to take the request's Config alongside Plan/State and key each attribute's declared-ness on declaredInConfig(config.X) (attribute_contract.go) instead of plan.X.Null. The scan/threshold schedule intervals and allowed_requesters are Optional+Computed with a UseStateForUnknown plan modifier, so an undeclared attribute's Plan value is normally already resolved to the prior state by the time this function runs -- checking plan.X.Null only catches the narrow case where the incoming plan value is literally Null, not Unknown (a distinct state for types.Int64/types.List), and would miss any future plan modifier (e.g. the mutually-exclusive schedule variant handling planned for a follow-on PR) that resolves an undeclared attribute to something other than a bare Null. Update() now fetches request.Config alongside Plan/State and passes it through, mirroring security_role's Update (resource_keyfactor_security_role.go). TestUnitCAUpdatePreservesScanScheduleWhenPlanIsUnknownNotNull demonstrates the gap the config-keyed check closes: an undeclared-in-config schedule whose incoming plan value is Unknown (not Null) is still correctly preserved from state, where the old plan.Null-keyed check would have let it fall through and clear server-side. Existing preserveCAUpdateFields tests are updated to pass a config argument representing "undeclared" alongside plan/state.
Per live verification against Command v25.5 (V4 in the attribute contract investigation): GET /CertificateAuthority returns both UseAllowedRequesters and AllowedRequesters with real, current values -- this is no longer a write-only field the provider must echo back from plan/state. Remove the AllowedRequesters echo from preserveSecrets. caResponseToState already maps the server's list into state; the echo ran AFTER that mapping and unconditionally overwrote it with the stale prior plan/state value whenever one was known, so Read (and Create/Update's post-response reconciliation) could never detect a role added or removed from the allowed-requester list out-of-band. preserveCAUpdateFields (Update-only, full-replace PUT reconciliation) is unaffected: it still preserves the prior state's list onto the plan when config leaves allowed_requesters undeclared, since Command's CA PUT remains a full replacement that clears an omitted field. With the config-keyed check from the previous commit, an explicitly-declared empty list ([]) is still treated as declared and sent through as a real clear -- confirmed via the SDK's ToMap()-based MarshalJSON, which serializes an explicit empty slice as "AllowedRequesters":[] rather than omitting it (distinct from a nil slice, which is omitted). Update the attribute description to document the v25.5+ read requirement and the omit/declare/clear contract. Updated doc comments on preserveSecrets and preserveCAUpdateFields to match. TestUnitCAReadSurfacesAllowedRequestersDrift reproduces the fixed behavior directly against caResponseToState + preserveSecrets. TestUnitCAUpdateExplicitEmptyAllowedRequestersIsSent covers the clear path. Existing TestUnitCAUpdatePreservesAllowedRequesters and TestUnitCAUpdatePostImportAllowedRequestersOmitted keep their original intent (undeclared preserves prior state) under the config-keyed check.
5 tasks
spbsoluble
commented
Jul 23, 2026
spbsoluble
left a comment
spbsoluble
left a comment
Collaborator
Author
There was a problem hiding this comment.
High-effort review of the audit rollup (same methodology as the PR #182 review: per-resource inventory of the preserve/omitted-vs-cleared changes, schema/Read-path drift survey, and adversarial hunt for the bug classes confirmed on #182). Overall verdict: the audit substantially IMPROVES drift detection (Read paths now write server truth for role permissions, identity roles, template attachments, store-type flags) and most resources already expose explicit-empty clear sentinels. Three correctness findings were confirmed — all three are now fixed by commits on this branch (details inline). Also fixed on this branch as part of the same contract work: allowed_requesters server-truth on Read (1970154, drift now visible + [] clears, verified live on Command v25.5) and declarative schedule clear sentinels landing via the stacked #182.
Pre-existing, out of scope, filed separately: #183 (security_identity role lookup strips non-word characters, breaking role names with spaces).
| allowedEnrollmentTypes := template.AllowedEnrollmentTypes | ||
| rfcEnforcement := template.RFCEnforcement | ||
| requiresApproval := template.RequiresApproval | ||
| keyUsage := template.KeyUsage |
Collaborator
Author
There was a problem hiding this comment.
[CONFIRMED, wipe class — fixed by c6c70e5 + Keyfactor/keyfactor-go-client#58] buildTemplateRoleBindingUpdateArg's doc comment promised a faithful round-trip of every GetTemplate field so the full-replace PUT /Templates wouldn't wipe unmanaged settings, but it omitted KeyUsage — every role attach/detach sent a body with no KeyUsage key.
Root cause was upstream: the go-client modeled UpdateTemplateArg.KeyUsage as *bool while Command's wire format is an int bitmask (verified against the live v25.5 swagger: TemplateUpdateRequest.KeyUsage is integer/int32; a boolean payload is rejected with HTTP 400 "Unexpected character encountered while parsing value: t. Path 'KeyUsage'"). Fixed as *int in keyfactor-go-client v3.6.0-rc.0 (pinned here) and round-tripped non-collapsingly at this line. Regression test: TestUnitTemplateRoleBindingUpdatePreservesKeyUsage (red before the fix: PUT body carried no KeyUsage key).
Lab nuance worth recording: on an EJBCA-connector template the server ignored attempts to change KeyUsage (connector-derived), so omission did not visibly wipe there — the test therefore asserts round-trip faithfulness of the PUT body rather than omission-wipes-to-zero.
| AccountName: types.String{Value: identity.AccountName}, | ||
| IdentityType: types.String{Value: identity.IdentityType}, | ||
| Roles: types.List{Elems: validRoles, ElemType: types.StringType}, | ||
| Roles: identityRolesResultForRead(state.Roles, identity.Roles), |
Collaborator
Author
There was a problem hiding this comment.
[CONFIRMED, perpetual-diff — fixed by 0e1c1be] The audit changed Read to rebuild roles from the server's canonical role.Name (good: real drift becomes visible), but Create/Update still write the user's config strings verbatim. Any case difference — or numeric IDs, which the schema description explicitly invited — produced an unresolvable loop: apply writes ["administrators"], refresh rewrites ["Administrators"], plan shows a diff forever.
Fix mirrors permissionsResultForUpdate's order-preservation contract: identityRolesResultForRead keeps the declared spelling when prior state is semantically equal to the server set (bijective match by numeric ID or case-insensitive name) and writes server-canonical names only on real drift. Regression tests: TestUnitSecurityIdentityReadKeepsDeclaredRoleSpelling, TestUnitSecurityIdentityReadNumericIdMatchesServerRole (both red pre-fix), TestUnitSecurityIdentityReadSurfacesRealDrift.
| // Null and there is nothing to preserve; in that case the field remains | ||
| // omitted from the request, relying on Command to leave an omitted | ||
| // AllowedRequesters unchanged. | ||
| func preserveCAUpdateFields(plan *KeyfactorCertificateAuthority, config, state KeyfactorCertificateAuthority) { |
Collaborator
Author
There was a problem hiding this comment.
[CONFIRMED, contract inconsistency — fixed by e4e1a43] preserveCAUpdateFields keyed "did the user declare this attribute" on the PLAN value being Null, while the equivalent logic in security_role (buildSecurityRoleUpdateArg) and security_identity (identityRolesDeclared) correctly keys on request.Config. Plan-keyed checks are unsound here: plan modifiers rewrite the plan (UseStateForUnknown resurrects prior state as Known, and an Unknown plan value slips past a Null check entirely), so the preserve guard could both fire when it shouldn't and miss when it should — the exact bug class that broke the schedule variant switch on #182.
Now config-keyed via the shared declaredInConfig helper (keyfactor/attribute_contract.go, added on this branch), with Update() reading request.Config. New test TestUnitCAUpdatePreservesScanScheduleWhenPlanIsUnknownNotNull covers the Unknown-plan gap the old check missed. The paired-variant plan modifier that completes this fix for the schedule attribute pairs lands with the stacked #182.
…ontract fixes Adds v2.9.1 entries for the six commits since the #180 rollup: template_role_binding KeyUsage round-trip (go-client v3.6.0-rc.0 KeyUsage *bool->*int fix), security_identity Read preserving declared role spelling (follow-up #183 filed), the shared attribute_contract.go helpers, certificate_authority's config-keyed preserveCAUpdateFields, and allowed_requesters now surfacing server truth on Read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #172. Bundles #174 (cert Update nil-deref fix) and #175 (cert-store container-clearing fix), each merged into this branch. #177 has since merged into this branch too — a resource-wide audit for the same "omitted vs. explicitly-cleared" bug class as #175, plus #178, #179, and #180 (all merged into #177 in turn). See "Provider-wide inconsistent-state audit" below.
Features
None — this release is fixes plus internal/dependency work.
Fixes
keyfactor_template_role_bindingtemplate pagination (keyfactor_template_role_binding fails with "template name not found" on instances with >50 templates #172): no longer fails withError template name not foundwhen a template intemplate_short_namessorts beyond the first 50 templates. The lookup now pages through the full template list instead of reading only Command's default first page of 50. Root fix is upstream — fix(template): paginate GetTemplates to return all templates Keyfactor/keyfactor-go-client#55 (closes chore(deps): bump github.com/hashicorp/terraform-plugin-log from 0.8.0 to 0.9.0 #54).keyfactor_certificateUpdate crash (fix(certificate): guard against nil GetCertificateContext response in Update #174): the Update path dereferenced a nil API response after a failedGET /Certificates/{id}, crashing the provider with a SIGSEGV (Plugin did not respond) mid-apply— observed when flippingcertificate_formatto PFX.Updatenow fails closed with a clear diagnostic instead of panicking. Regression test added (TestUnitKeyfactorCertificateResource_UpdateNilGetResponse).Certificate Stores
keyfactor_certificate_storecontainer/application-clearing bug (Bug: keyfactor_certificate_store update clears container/application assignment when not declared in config #175):Updateno longer silently clears an existing container/application assignment whenapplication_name/container_nameis not declared in config. A resolved container ID of0was previously omitted from the update request entirely, which Command interprets as "unassign" — destroying a real out-of-band assignment before Terraform's own consistency check could even flag it.container_idis now preserved whenever prior state shows a real assignment and the plan gives no explicit name (an explicit empty-string name still clears it, unchanged).lookupContainerNameByIDalso now retries via the list endpoint before falling back to a hint, reducing spuriouscontainer_name/application_namenulling from a single transient/permission-scoped lookup failure. Regression tests added covering both the preserve-on-undeclared and explicit-clear-still-works paths.keyfactor_certificate_storeno longer sends an explicit emptyInventoryScheduleobject when the config doesn't declare one;Updatealways re-resolves the container/application name by ID after a successful update instead of trusting a locally-resolved name;Create/Updateno longer crash the provider with a nil-pointer dereference when the agent identifier lookup returns no matching agentProvider-wide inconsistent-state audit (#177, #178, #179, #180)
A provider-wide audit for the same bug class as #175 — an omitted attribute and an explicitly-cleared attribute being treated as the same thing, which could silently clear real server-side state or crash the provider — found and fixed issues across 12 resources:
keyfactor_certificate_deploy,keyfactor_oauth_security_role_claim_association, and OAuth claim role-mapping, on certain error responses from Commandkeyfactor_applicationno longer silently drops an existing schedule on an unrelated update whenschedule_*attributes are omitted from configkeyfactor_certificate_authorityno longer silently clears scan schedules orallowed_requesterson an unrelated updatekeyfactor_certificate_store_typeexplicitfalse/empty values forlocal_store,server_required,power_shell, andblueprint_allowedare no longer dropped from requests or misread asfalsekeyfactor_certificate_templateexplicit empty lists (template_regexes,template_defaults,enrollment_fields,metadata_fields) now actually clear server-side; 4 Command v25+ attributes no longer drift on refreshkeyfactor_pam_provider_typeexplicit emptydisplay_nameis now preserved instead of falling back to a server default; a nulldata_typeno longer reads as a misleading concrete valuekeyfactor_pam_provideran explicitparam_valuesclear now reaches the update request instead of being silently ignoredkeyfactor_security_identityno longer strips existing role assignments on an unrelated update whenrolesis omitted from config;Readnow detects role changes made outside Terraform instead of showing stale data.rolesis nowOptional+Computed, which also fixes aProvider produced inconsistent result after applyerror on updates to identities that already have roles assignedkeyfactor_security_roleno longer clears a role'spermissionson an unrelated update whenpermissionsis omitted from config — this previously happened two ways: an explicit clear being sent by mistake, and (even after that fix) Command's update endpoint treating a merely-missing field the same as an explicit clear.Readnow detects permission/description changes made outside Terraform instead of showing stale data, andpermissionsis nowOptional+Computed, fixing aProvider produced inconsistent result after applyerror on updates to roles that already have permissions assignedkeyfactor_template_role_bindingrole attach/detach no longer risks clearing unrelated template settings;Readnow detects role changes made outside Terraform and surfaces an error instead of failing silently when template data can't be readkeyfactor_template_role_bindingrole attach/detach no longer fails with'Policies' cannot be emptyon templates linked to an enrollment pattern (keyfactor_template_role_binding: attach/detach fails with "'Policies' cannot be empty" — UpdateTemplateArg has no Policies field #180) — Command's update endpoint is full-replace and was clearing the template's key-algorithm policy whenever it wasn't resentkeyfactor_certificateowner_role_nameno longer causes a spurious ownership change (and drift) when left unset in configChore / Internal
keyfactor-go-client/v3bumped to GAv3.5.6(fix(template): paginate GetTemplates to return all templates Keyfactor/keyfactor-go-client#55, chore(deps): bump github.com/hashicorp/terraform-plugin-framework from 0.10.0 to 1.3.1 #56, chore(deps): bump github.com/hashicorp/terraform-plugin-go from 0.15.0 to 0.16.0 #57) — adds nullable*boolstore-type flags,EntryPasswordomitempty, paginatedGetStoreContainers/GetTemplates, and theTemplatePolicymodel needed to fix keyfactor_template_role_binding: attach/detach fails with "'Policies' cannot be empty" — UpdateTemplateArg has no Policies field #180. No RC dependency remains.GET /Templatescassettes for the paginated request (?PageReturned&ReturnLimit); added an opt-innewVCRProviderFactoriesReplayablevariant used only by the read-only certificate-template data-source unit test. Stateful/polling cassettes keep consuming ordered interactions.TestIntKeyfactorCertificateResource_SANsandTestIntKeyfactorCertificateAuthorityResourceUpdateare now handled in-test (skip with warning) for lab-constraint-only failures, so an unexpected failure still fails.skipOnKnownLabConstraintnormalizes whitespace before matching.*enumType -> types.Int64pointer converters into one genericenumPtrToTfInt64helper; replaced an ad-hocgetStringType(...).Valueidiom (7 call sites) with a plainderefOrEmptyhelper; extracted a sharedhttptest-backed mockAuthConfigtest helper, replacing three near-identical per-file mocks. No behavior change.CHANGELOG.md# v2.9.1section (regrouped Features/Fixes/Chore/Pending) + version2.9.1-rc.1.Verification
application_name/container_nameclear being silently overridden; a preserved nonzerocontainer_idthat could pair with an empty-stringContainerNamein the request) plus 3 cleanup issues — all fixed, with additional regression tests, before this PR was opened. Verified end-to-end against the real ses2541 lab: reproduced the original bug on published provider2.9.0(confirmed the exact reported error and confirmed via API that the assignment was genuinely wiped server-side), then confirmed the fix using a local build of this branch (apply succeeded, assignment survived, unrelated attribute change took effect, no drift on a follow-up plan).keyfactor_security_role/keyfactor_security_identityOptional+Computedfixes and thekeyfactor_security_rolefull-replace-PUT fix were confirmed against a real lab via actualterraform applycycles (not just Go-level tests) — this is what caught the full-replace-PUT bug and, separately, thekeyfactor_template_role_bindingTemplatePolicybug (keyfactor_template_role_binding: attach/detach fails with "'Policies' cannot be empty" — UpdateTemplateArg has no Policies field #180) in the first place.make testunitgreen throughout: 155 pass, 0 fail, 3 pre-existing/unrelated skips. Vendor-free validation against the real published GAv3.5.6— identical pass count to the vendored run.Pending (before GA v2.9.1)
GA dependency gate— resolved.go.modnow pins GAv3.5.6; fix(template): paginate GetTemplates to return all templates Keyfactor/keyfactor-go-client#55, chore(deps): bump github.com/hashicorp/terraform-plugin-framework from 0.10.0 to 1.3.1 #56, and chore(deps): bump github.com/hashicorp/terraform-plugin-go from 0.15.0 to 0.16.0 #57 are all merged and released. This was the only item keeping this PR a draft.keyfactor_certificate_authorityhas many Optional+Computed pointer fields where server-side omission could still flip a value to zero — same bug class as thekeyfactor_security_identity/keyfactor_security_rolefixes above, unaudited.