feat: Add OAuth 2.0 support to mongodbatlas_metric_integration resource - #4737
Conversation
|
Test failures are unrelated to this implementation. Note that currently new added tests are being skipped in CI, see Further Notes section of the PR description for further details |
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
|
APIx bot: a message has been sent to Docs Slack channel |
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds OAuth 2.0 authentication support to the metric integration resource and data sources.
Changes:
- Adds client-secret and private-key JWT OAuth configuration.
- Regenerates schemas, API models, documentation, and acceptance tests.
- Updates changelog and flattened API specification.
File summaries
| File | Description |
|---|---|
| tools/codegen/models/metric_integration.yaml | Updated as part of this pull request. |
| tools/codegen/config.yml | Updated as part of this pull request. |
| tools/codegen/atlasapispec/multi-version-api-spec.flattened.yml | Updated as part of this pull request. |
| internal/serviceapi/metricintegration/resource_test.go | Updated as part of this pull request. |
| internal/serviceapi/metricintegration/resource_schema.go | Updated as part of this pull request. |
| internal/serviceapi/metricintegration/plural_data_source_schema.go | Updated as part of this pull request. |
| internal/serviceapi/metricintegration/data_source_schema.go | Updated as part of this pull request. |
| docs/resources/metric_integration.md | Updated as part of this pull request. |
| docs/data-sources/metric_integrations.md | Updated as part of this pull request. |
| docs/data-sources/metric_integration.md | Updated as part of this pull request. |
| .changelog/4737.txt | Updated as part of this pull request. |
Review details
Files not reviewed (3)
- internal/serviceapi/metricintegration/data_source_schema.go: Generated file
- internal/serviceapi/metricintegration/plural_data_source_schema.go: Generated file
- internal/serviceapi/metricintegration/resource_schema.go: Generated file
Suppressed comments (2)
tools/codegen/config.yml:1666
- The documentation postprocessor only recognizes the
Required for <discriminator>:prefix (tools/docpostprocess/schema_section.go:19). Because this override saysRequired when, the generated resource docs leaveoauthin the generic Optional section and do not create the correspondingOAUTH2conditional section, unlikeheaders; use the supported prefix here and mirror it in the model description.
description: "OAuth 2.0 client credentials configuration. Required when auth_type is `OAUTH2`. Secrets are never returned."
tools/codegen/models/metric_integration.yaml:201
- Removing
client_secretfrom configuration also results in the field being omitted from the update request. The API contract explicitly distinguishes this from clearing the stored secret (clientSecretomission keeps the stored value), so Terraform can report no secret while Atlas continues authenticating with the old one. Send an explicit null on update viasend_null_as_null_on_updateand test the removal lifecycle.
computed_optional_required: optional
tf_schema_name: client_secret
tf_model_name: ClientSecret
api_name: clientSecret
req_body_usage: all_request_bodies
- Files reviewed: 7/12 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
JuliaMongo
left a comment
There was a problem hiding this comment.
Left a few comments to clarify text strings.
| @@ -0,0 +1,11 @@ | |||
| ```release-note:enhancement | |||
| resource/mongodbatlas_metric_integration: Adds OAuth 2.0 support | |||
There was a problem hiding this comment.
Can this be made longer, or are we keeping it short because that's the style?
such as: (taking from the PR description)
Adds OAuth 2.0 support to the mongodbatlas_metric_integration resource and its data sources, alongside the existing header-based authentication. With auth_type = "OAUTH2", the integration authenticates to an OTel destination through an OAuth 2.0 token endpoint, using either a shared client secret (client_auth_method = "CLIENT_SECRET") or an Atlas-managed signing key (client_auth_method = "PRIVATE_KEY_JWT").
There was a problem hiding this comment.
Short is the repo's changelog style, see see contributing/changelog-process.md, but agreed that the wording could be more explicit. Updated it in 328cf3d
| - `headers_redacted` (Attributes List) HTTP headers for authentication and configuration. Values are redacted and never returned in plaintext. (see [below for nested schema](#nestedatt--headers_redacted)) | ||
| - `integration_type` (String) Type of metric integration. Identifies which protocol will be used for the integration. | ||
| - `metric_selection` (Set of String) Array of metric categories to export. Determines which types of metrics are sent to the integration. | ||
| - `oauth` (Attributes) OAuth 2.0 configuration returned for a metric integration. Secrets are never returned. (see [below for nested schema](#nestedatt--oauth)) |
There was a problem hiding this comment.
should this say: Attributes list, like line 72 does?
Replace "see below for" with "To learn more, see [nested schema] ((#nestedatt--oauth))
| - `client_auth_method` (String) How the client authenticates to the token endpoint. | ||
| - `client_id` (String) OAuth 2.0 client identifier registered with the token endpoint. | ||
| - `scopes` (Set of String) OAuth 2.0 scopes requested on the token. | ||
| - `signing_key_info` (Attributes) Read-only metadata for the Atlas-managed signing key used by `PRIVATE_KEY_JWT`. Present only for that method. Register the jwks_uri with your identity provider. Atlas rotates the underlying key without changing this URL. (see [below for nested schema](#nestedatt--oauth--signing_key_info)) |
There was a problem hiding this comment.
should this be Attributes list?
same comment re: learn more.
Question re:
Register the jwks_uri with your identity provider. -- I don't have context as a reader and would be confused. Might need a bit more explanation in words.
| Read-Only: | ||
|
|
||
| - `algorithm` (String) Signing algorithm of the Atlas-managed key. | ||
| - `created_at` (String) When the currently active signing key was created. This parameter expresses its value in the ISO 8601 timestamp format in UTC. |
There was a problem hiding this comment.
The timestamp for when Atlas created the ....
|
|
||
| - `algorithm` (String) Signing algorithm of the Atlas-managed key. | ||
| - `created_at` (String) When the currently active signing key was created. This parameter expresses its value in the ISO 8601 timestamp format in UTC. | ||
| - `jwks_uri` (String) Public JWKS URL serving this integration's signing keys. Fixed for the lifetime of the integration. |
There was a problem hiding this comment.
what does "Fixed for the lifetime of the integration." mean?
might be worth rewording this.
Also this line 106 contains the definition in words for what is jwks uri -- consider using these same words Public JWKS URL in all other places too where this is discussed.
|
|
||
| - `client_secret` (String, Sensitive) Shared client secret. Required when client_auth_method is `CLIENT_SECRET`, and rejected for `PRIVATE_KEY_JWT`. Encrypted at rest and never returned. | ||
| - `scopes` (Set of String) Optional OAuth 2.0 scopes requested on the token, sent as a space delimited `scope` parameter. Applies to both client authentication methods. | ||
| - `token_request_params` (Map of String) Optional provider-specific parameters added to the token request, for example a resource indicator. Applies to both client authentication methods. |
There was a problem hiding this comment.
same here: name both auth methods to which this applies.
|
|
||
| Read-Only: | ||
|
|
||
| - `signing_key_info` (Attributes) Read-only metadata for the Atlas-managed signing key used by `PRIVATE_KEY_JWT`. Present only for that method. Register the jwks_uri with your identity provider. Atlas rotates the underlying key without changing this URL. (see [below for nested schema](#nestedatt--oauth--signing_key_info)) |
There was a problem hiding this comment.
same comments as above for similar content
| Read-Only: | ||
|
|
||
| - `algorithm` (String) Signing algorithm of the Atlas-managed key. | ||
| - `created_at` (String) When the currently active signing key was created. This parameter expresses its value in the ISO 8601 timestamp format in UTC. |
There was a problem hiding this comment.
same comments as above for similar content.
| description: "Shared client secret. Required when client_auth_method is `CLIENT_SECRET`, and rejected for `PRIVATE_KEY_JWT`. Encrypted at rest and never returned." | ||
| oauth.signing_key_info: | ||
| description: "Read-only metadata for the Atlas-managed signing key used by `PRIVATE_KEY_JWT`. Present only for that method. Register the jwks_uri with your identity provider. Atlas rotates the underlying key without changing this URL." |
There was a problem hiding this comment.
see comments above
Not clear "present only for that method" -- which method. for which method it is not present? what does "not present" mean?
Add a word definition for what is jwks_uri
When should registering happen? Is it before some action, or can it be registered at any point? Not clear.
| project_id: | ||
| description: *project_id_description | ||
| oauth.signing_key_info: | ||
| description: "Read-only metadata for the Atlas-managed signing key used by `PRIVATE_KEY_JWT`. Present only for that method. Register the jwks_uri with your identity provider. Atlas rotates the underlying key without changing this URL." |
There was a problem hiding this comment.
same q here re: registering -- is it a prerequisite?
AgustinBettati
left a comment
There was a problem hiding this comment.
Changes LGTM, want to understand the CI testing and would revise examples
| - `project_id` (String) Unique 24-hexadecimal digit string that identifies your project, also known as `groupId` in the official documentation. | ||
| - `provider_type` (String) The provider type for the metric integration. Identifies the third-party service provider. | ||
|
|
||
| ### Optional |
There was a problem hiding this comment.
q: Any examples worth adjusting to cover OAuth?
There was a problem hiding this comment.
Yes, this will be addressed in CLOUDP-445734
| endpoint = %[2]q | ||
| metric_selection = ["ATLAS_STREAM_PROCESSING"] | ||
|
|
||
| oauth = { |
There was a problem hiding this comment.
Do we have a test checking full removal of oauth attribute? Just want confirm the behaviour, I imagine this will unset the configured value.
There was a problem hiding this comment.
Removing oauth currently omits it from the PUT, so the API retains the stored OAuth configuration; add send_null_as_null_on_update for the block and cover removal
There was a problem hiding this comment.
Removing oauth produces a 400 from the API. oauth is required when auth_type is OAUTH2, that's why there's no test for it: removing oauth while staying OAUTH2 isn't a valid state. The error message is Invalid attribute oauth is required when authType is 'OAUTH2'.
There was a problem hiding this comment.
That covers an invalid OAUTH2 configuration, but not switching an existing integration to HEADER. In that valid configuration, oauth is absent from Terraform configuration and becomes a null object. Because the generated root field has no sendnullasnullonupdate tag, the marshaller omits oauth from the PUT instead of sending "oauth": null.
The update contract says omitted OAuth fields retain their stored values, so Atlas receives the new authType = "HEADER" and headers but no instruction to delete the previous OAuth configuration. Since the API returns oauth, the stale values can re-enter Terraform state after refresh despite the absent configuration block. Generate send_null_as_null_on_update for the root oauth attribute and add an acceptance-test step that changes from OAuth to header authentication.
More details: 🤖
When configuration changes from OAuth to headers, the desired Terraform configuration is conceptually:
auth_type = "HEADER"
headers = [{ name = "Authorization", value = "..." }]
# oauth is absent At that point oauth is a null Terraform object. The generated model at resource_schema.go:163 has no sendnullasnullonupdate tag, so the autogen marshaller omits the oauth key entirely from the update PUT
rather than sending "oauth": null.
The OpenAPI update contract explicitly distinguishes those cases: an omitted OAuth field retains its stored value, while an explicit null clears it. Thus the request updates authType to HEADER and supplies
headers, but does not tell Atlas to delete the previously stored OAuth configuration.
That leaves stale OAuth data on the server. Because oauth is returned by the API and is optional rather than computed on the resource schema, a subsequent read can put that server-side OAuth value back into
Terraform state even though the configuration no longer contains an oauth block. That can cause persistent plan/state drift and means switching authentication modes does not fully apply the declared
configuration.
The fix is to generate send_null_as_null_on_update for the root oauth attribute, then add an acceptance-test step that creates OAuth, changes to HEADER while omitting oauth, and verifies the API/state no
longer contain OAuth fields.
There was a problem hiding this comment.
Oh sorry! I misunderstood the use case, now I get it's related to making the switch from OAUTH2 to HEADERS.
Thanks for the analysis, but I checked and the API clears oauth natively when doing the switch from OAUTH2 to HEADER, so I don't believe the send_null_as_null_on_update is needed. I added a test to cover this scenario and it passed locally (see 3dd8b4b). PR description has been updated to include the passing test.
There was a problem hiding this comment.
Thanks for covering this case.
Just to be clear, given it is a PUT I would expect an omitted oauth property implies it is removed, which I understand is the confirmed behaviour given tests are passing without having send_null_as_null_on_update. Explicit null is relevant for PATCH operations, in which case omission is interpreted as "leave value as is".
| endpoint = %[2]q | ||
| metric_selection = ["ATLAS_STREAM_PROCESSING"] | ||
|
|
||
| oauth = { |
There was a problem hiding this comment.
Removing oauth currently omits it from the PUT, so the API retains the stored OAuth configuration; add send_null_as_null_on_update for the block and cover removal
Co-authored-by: Agustin Bettati <bettatiagustin@gmail.com>
…OTel_Metric_Sink' into CLOUDP-445732_Add_OAuth_Support_Metric_Integration_Resource # Conflicts: # tools/codegen/atlasapispec/multi-version-api-spec.flattened.yml # tools/codegen/atlasapispec/raw-multi-version-api-spec.yml
|
RE: docs comments. Synced offline and clarified that for autogen, we streamline the descriptions from the OpenAPI spec, so these improvements should be done on the API side to avoid extra overriding. Will sync with the team in upcoming meeting to decide on what's the best approach to communicate this. |
EspenAlbert
left a comment
There was a problem hiding this comment.
Thank you for addressing the comments!
| endpoint = %[2]q | ||
| metric_selection = ["ATLAS_STREAM_PROCESSING"] | ||
|
|
||
| oauth = { |
| oauth.scopes: | ||
| request_body_usage: send_null_as_empty_on_update | ||
| oauth.token_request_params: | ||
| request_body_usage: send_null_as_empty_on_update |
There was a problem hiding this comment.
just double checking, without this override do we get non-empty plans when attribute is omitted?
There was a problem hiding this comment.
Correct. To my understanding, auth_type is a discriminator: switching it discards the previous type's block, so oauth is cleared even if omitted. scopes/token_request_params don't get that treatment: within a stable auth_type they're retained when omitted, which is why send_null_as_empty_on_update is needed. I can confirm with the upstream team, but I believe the API is designed as intended unless I'm missing something
36e3ba3
into
dev/CLOUDP-412204_OAuth_Support_OTel_Metric_Sink
Description
Adds OAuth 2.0 support to the
mongodbatlas_metric_integrationresource and its data sources, alongside the existing header-based authentication. Withauth_type = "OAUTH2", the integration authenticates to an OTel destination through an OAuth 2.0 token endpoint, using either a shared client secret (client_auth_method = "CLIENT_SECRET") or an Atlas-managed signing key (client_auth_method = "PRIVATE_KEY_JWT").Adds an
oauthblock with:client_auth_method,token_endpoint,client_idscopes,token_request_params(optional)client_secret(write-only,CLIENT_SECRETonly)signing_key_info(read-only,PRIVATE_KEY_JWTonly):kid,algorithm,jwks_uri,created_atThe
client_secretis write-only and never returned by the API. ForPRIVATE_KEY_JWT, Atlas generates and manages the signing key server-side.Link to any related issue(s): CLOUDP-412204
Type of change:
Required Checklist:
Further comments
Validated locally against cloud-dev on a project with the OAuth feature flag enabled:
The OAuth acceptance tests are temporarily skipped in CI because the OAuth metric integration fields are still behind a feature flag and not available on the CI test project. They run locally against a cloud-dev project with the flag enabled and will be un-gated once the fields are available in prod.
Examples for the new
oauthblock are deferred to CLOUDP-445734 and will be added there.The OpenAPI spec changes in this PR are committed from the
devbranch ofmongodb/openapi, since the OAuth metric integration fields are only available there and not yet in the prod spec. This will be re-synced to prod once the fields are deployed.