feat: Adds without_initial_secret to mongodbatlas_service_account - #4727
EspenAlbert wants to merge 14 commits into
Conversation
EspenAlbert
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Outstanding acceptance-test and generated-documentation issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds without_initial_secret support for MongoDB Atlas Service Accounts, enabling Terraform-managed secret creation.
Changes:
- Adds the new create-only schema attribute and updates code generation.
- Adds acceptance tests and dedicated example flows.
- Updates documentation and changelog entries.
File summaries
| File | Summary |
|---|---|
tools/codegen/models/service_account.yaml |
Updates Service Account model metadata. |
tools/codegen/config.yml |
Configures schema overrides and descriptions. |
tools/codegen/codespec/config.go |
Supports create-only computed booleans with defaults. |
tools/codegen/codespec/config_test.go |
Tests code-generation transformations. |
templates/resources/service_account.md.tmpl |
Updates resource documentation examples. |
templates/data-sources/service_accounts.md.tmpl |
Updates plural data-source documentation. |
templates/data-sources/service_account.md.tmpl |
Updates singular data-source documentation. |
internal/serviceapi/serviceaccount/resource_test.go |
Adds acceptance coverage and test configuration updates. |
internal/serviceapi/serviceaccount/resource_schema.go |
Adds the new resource attribute. |
examples/mongodbatlas_service_account/without_initial_secret/versions.tf |
Defines Terraform and provider requirements. |
examples/mongodbatlas_service_account/without_initial_secret/variables.tf |
Defines the organization variable. |
examples/mongodbatlas_service_account/without_initial_secret/README.md |
Documents secret-free Service Account creation. |
examples/mongodbatlas_service_account/without_initial_secret/providers.tf |
Configures the provider example. |
examples/mongodbatlas_service_account/without_initial_secret/main.tf |
Demonstrates explicit secret creation. |
examples/mongodbatlas_service_account/variables.tf |
Removes obsolete root variables. |
examples/mongodbatlas_service_account/README.md |
Indexes the available example flows. |
examples/mongodbatlas_service_account/provider.tf |
Removes obsolete root provider configuration. |
examples/mongodbatlas_service_account/bootstrap_secret/versions.tf |
Defines Terraform and provider requirements. |
examples/mongodbatlas_service_account/bootstrap_secret/variables.tf |
Defines the organization variable. |
examples/mongodbatlas_service_account/bootstrap_secret/README.md |
Documents bootstrap-secret usage. |
examples/mongodbatlas_service_account/bootstrap_secret/providers.tf |
Configures the provider example. |
examples/mongodbatlas_service_account/bootstrap_secret/main.tf |
Provides the bootstrap-secret example. |
docs/resources/service_account.md |
Documents the new attribute and example flows. |
.changelog/4721.txt |
Adds the enhancement release note. |
Review details
Files not reviewed (1)
- internal/serviceapi/serviceaccount/resource_schema.go: Generated file
Suppressed comments (5)
.changelog/4721.txt:1
- This changelog is named
.changelog/4721.txt, but this pull request is #4727. Changelog tooling associates the release-note filename with the PR number, so the entry may be skipped or attributed to another change; please rename it to.changelog/4727.txt.
```release-note:enhancement
docs/resources/service_account.md:54
- The newly added example directories are not present at the pinned
v2.17.0tag, so these links in the generated resource documentation currently return 404. Point the new-example links at a ref that contains this change (for examplemaster, or update them to the release tag when the feature is released).
- [Bootstrap secret](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/v2.17.0/examples/mongodbatlas_service_account/bootstrap_secret)
- [Without initial secret](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/v2.17.0/examples/mongodbatlas_service_account/without_initial_secret)
internal/serviceapi/serviceaccount/resource_schema.go:95
- This
Optional + Computedcreate-only attribute has state-retention behavior that is not covered by the acceptance tests: they only settrueand import. Add lifecycle coverage for omitting it on create (including a subsequent empty plan) and for removing it from a configuration after it was set, asserting whether the value should be retained or rejected; otherwise the custom default/state handling can regress without detection.
internal/serviceapi/serviceaccount/resource_test.go:124 - Please add an acceptance step that removes
without_initial_secretafter creating it astrue. Because this attribute is Optional+Computed and create-only, omission is expected to preserve the state value without an update; that lifecycle behavior is not covered by the current create and false-to-true rejection steps.
Config: configBasic(orgID, name, "Without initial secret", []string{"ORG_READ_ONLY"}, nil, "without_initial_secret = true"),
Check: resource.ComposeAggregateTestCheckFunc(
checkExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "secrets.#", "0"),
resource.TestCheckResourceAttr(resourceName, "without_initial_secret", "true"),
templates/resources/service_account.md.tmpl:19
- The newly added example directories are not present at the pinned
v2.17.0tag, so both links rendered from this template currently return 404. Point these new-example links at a ref that contains this change (for examplemaster, or update them to the release tag when the feature is released).
- [Bootstrap secret](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/v2.17.0/examples/mongodbatlas_service_account/bootstrap_secret)
- [Without initial secret](https://github.com/mongodb/terraform-provider-mongodbatlas/tree/v2.17.0/examples/mongodbatlas_service_account/without_initial_secret)
- Files reviewed: 21/24 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
EspenAlbert
changed the title
without_initial_secret to mongodbatlas_service_account
|
APIx bot: a message has been sent to Docs Slack channel |
🤖 Augment PR SummarySummary: Adds support for creating organization Service Accounts without an Atlas-generated bootstrap secret. Changes:
Technical Notes: Request-only values are retained from Terraform's plan/state because Atlas does not return them on reads; the examples create the first secret through 🤖 Was this summary useful? React with 👍 or 👎 |
manupedrozo
left a comment
manupedrozo
left a comment
There was a problem hiding this comment.
Nice, left comments.
- Planning to cover
project_service_accountchanges and the rotation guide in a separate PR? - Ideally we'd also update other examples/guides to use the
without_initial_secretattribute. A relevant one is the PAK to SA migration guide.
| // A plain CreateOnly plan modifier is not applied to computed attributes: their value may be | ||
| // unknown at plan time, which would falsely fail the update validation. The exception is a | ||
| // computed optional bool with a default that the API never returns, where | ||
| // CreateOnlyBoolWithDefault sets the default and keeps the create-only behavior. | ||
| if attr.ComputedOptionalRequired == Computed { | ||
| return | ||
| } | ||
| if attr.ComputedOptionalRequired == ComputedOptional && !isCreateOnlyBoolWithDefault(attr) { |
There was a problem hiding this comment.
If we leave the attribute optional only, could we avoid the override + this change?
Seems like we are forcing computed to an attribute that given the api spec is not and then we are fixing it back here, I might be missing something!
If we do want this behavior, I think we should add this particular createOnly plan modifier through config/custom hook instead of changing the codegen for a case we are forcing.
There was a problem hiding this comment.
Got the same doubt, would be clear on API behaviour and work backwards from there.
There was a problem hiding this comment.
Implemented the optional-only design: the provider clears the API default and uses the normal create-only modifier, so it does not write an unconfigured value to state. This removes the computed override and prior specialized generator path. The PR description still needs the corresponding Optional-only/ClearDefault update.
JuliaMongo
left a comment
JuliaMongo
left a comment
There was a problem hiding this comment.
LGTM for copy review with a few suggestions to use active voice in a few places (all optional).
| // A plain CreateOnly plan modifier is not applied to computed attributes: their value may be | ||
| // unknown at plan time, which would falsely fail the update validation. The exception is a | ||
| // computed optional bool with a default that the API never returns, where | ||
| // CreateOnlyBoolWithDefault sets the default and keeps the create-only behavior. | ||
| if attr.ComputedOptionalRequired == Computed { | ||
| return | ||
| } | ||
| if attr.ComputedOptionalRequired == ComputedOptional && !isCreateOnlyBoolWithDefault(attr) { |
There was a problem hiding this comment.
Got the same doubt, would be clear on API behaviour and work backwards from there.
Thanks for the comments, will do a tech sync item to make the decision, seems tempting to simplify this for users and use without_initial_secret when the secret_expires is not set. |
| // A plain CreateOnly plan modifier is not applied to computed attributes: their value may be | ||
| // unknown at plan time, which would falsely fail the update validation. The exception is a | ||
| // computed optional bool with a default that the API never returns, where | ||
| // CreateOnlyBoolWithDefault sets the default and keeps the create-only behavior. | ||
| if attr.ComputedOptionalRequired == Computed { | ||
| return | ||
| } | ||
| if attr.ComputedOptionalRequired == ComputedOptional && !isCreateOnlyBoolWithDefault(attr) { |
There was a problem hiding this comment.
Implemented the optional-only design: the provider clears the API default and uses the normal create-only modifier, so it does not write an unconfigured value to state. This removes the computed override and prior specialized generator path. The PR description still needs the corresponding Optional-only/ClearDefault update.
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| "without_initial_secret": schema.BoolAttribute{ | ||
| Optional: true, | ||
| MarkdownDescription: "When true, creates the Service Account without generating an initial secret. If you set this field to true, do not set `secret_expires_after_hours`.", | ||
| PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnly()}, |
There was a problem hiding this comment.
CreateOnly() permits a null state to change to true, which is the normal state after creating a resource with this optional attribute omitted. Terraform will therefore run an update that omits this field from the PATCH but persists true from the plan, leaving an Atlas account with its bootstrap secret while state claims it was created without one (as the new acceptance test explicitly permits).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
AgustinBettati
left a comment
AgustinBettati
left a comment
There was a problem hiding this comment.
Thanks for followups! 2 smaller questions on my side
| @@ -0,0 +1,3 @@ | |||
| ```release-note:enhancement | |||
| resource/mongodbatlas_service_account: Adds `without_initial_secret` attribute to create a Service Account without generating an initial secret. `secret_expires_after_hours` is now optional when `without_initial_secret` is `true` | |||
There was a problem hiding this comment.
Shouldn't project_service_account have the same change? Or was this not added on the API side?
| // clearStaticDefault drops a static default copied from the API spec. Code generation otherwise emits | ||
| // a schema Default, which forces computed_optional and is rejected by terraform-plugin-framework on a |
There was a problem hiding this comment.
generation otherwise emits a schema Default
Is this statement accurate? From what I recall we used API Spec default to determine if an attribute was Optional + Computed, or only Optional (no default value actually reflected in schema). With this in mind, curious of config override of defining it is optional only is sufficient.
Description
Adds a
without_initial_secretattribute tomongodbatlas_service_account. When set totrue, the provider sendswithoutInitialSecret: trueto the Atlas API, so Atlas creates the Service Account without generating a bootstrap secret. The resource then has an emptysecretslist, and the configuration creates its own secrets withmongodbatlas_service_account_secret.secret_expires_after_hoursis no longer required on create. It staysoptionaland create-only. The API contract is:without_initial_secret = truerequiressecret_expires_after_hoursto be unset.ARGUMENT_NOT_PROVIDED).MUTUALLY_EXCLUSIVE_ARGUMENTS).The provider relies on the API error for these two validation cases rather than duplicating the check at plan time.
Link to any related issue(s): CLOUDP-444252, CLOUDP-375930
Type of change:
Required Checklist:
Further comments
Schema:
without_initial_secretis Optional + Computedwithout_initial_secretisOptionalandComputed, and usescustomplanmodifier.CreateOnlyBoolWithDefault(false). The plan modifier injects thefalsedefault into the plan on create so the resource Create handler needs no custom logic, and it rejects a change on update. Terraform Plugin Framework requires a plan-time default to sit on an Optional + Computed attribute, so the attribute is Computed as well.trueis only meaningful at create time. On update, the API omitswithoutInitialSecretfrom responses, so the state value stays whatever the user set at create (defaultfalse), and the attribute is not re-read.Codegen: create-only plan modifier for computed optional bools with a default
setCreateOnlyValueintools/codegen/codespec/config.goskipped every computed attribute when deciding whether to emit a create-only plan modifier. That guard exists because a plainCreateOnly()modifier can see an unknown value at plan time on a computed attribute and fail the update validation.This PR narrows the guard: a called-out
ComputedOptionalbool that has a default and that the API never returns keeps the create-only behavior, becauseCreateOnlyBoolWithDefaulthandles the unknown value and the default. All other computed attributes keep the previous behavior.TestApplyTransformationsToResource_CreateOnlyTransformationcovers the new cases.Regenerating all resources with
make autogen-generate-resources step=code-genchanges onlywithout_initial_secret.Generated files
The tracked OpenAPI spec in
tools/codegen/atlasapispec/is stale for bothwithoutInitialSecretandsystemManaged. I did not commit a refreshed spec: the upstreamdevspec is used as a local input only. Both the raw and flattened spec files are left atHEAD, and no spec diff is part of this PR.The flow used to update the model:
make autogen-update-api-spec spec_source=/tmp/upstream-v2-dev.yamlwrites the raw and flattened spec locally.make autogen-generate-resources resource_name=service_account step=model-genregeneratestools/codegen/models/service_account.yamlfrom that local spec.make autogen-generate-resources resource_name=service_account step=code-genregeneratesinternal/serviceapi/serviceaccount/resource_schema.go.git checkout -- tools/codegen/atlasapispec/reverts the spec files so they stay out of the PR.The upstream
devspec now carries thesystemManagedfield onOrgServiceAccount(this is what the hand-vendoredsystem_managedattribute came from) andwithoutInitialSecretonOrgServiceAccountRequest, and it dropssecretExpiresAfterHoursfrom the request's required list. Regeneration therefore reproducessystem_managedinstead of dropping it, so the model no longer needs a hand edit.CI's
check-autogen-resourcesworkflow runsautogen-generate-resources step=code-genagainst the committed models, so the committed model is the source of truth for generated code and the local spec is not needed to reproduce it.