-
Notifications
You must be signed in to change notification settings - Fork 207
feat: Support OIDC configs in mongodbatlas_stream_connection #3680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
APIx bot: a message has been sent to Docs Slack channel |
* `username` - Username of the account to connect to the Kafka cluster. | ||
* `password` - Password of the account to connect to the Kafka cluster. | ||
* `token_endpoint_url` - OAUTH issuer token endpoint HTTP(S) URI used to retrieve the token. | ||
* `client_id` - Public identifier for the kafka client. It must be unique across all clients that the authorization server handles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `client_id` - Public identifier for the kafka client. It must be unique across all clients that the authorization server handles. | |
* `client_id` - Public identifier for the Kafka client. It must be unique across all clients that the authorization server handles. |
what is the consequence of this not being unique? Guessing a token wouldn't be issued, but it's not clear why this must be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uniqueness
is applied to the authorization server(IdP provider). If I remove the second sentence, does it make sense to you?
* `client_secret` - Secret known only to the kafka client and the authorization server. | ||
* `scope` - Kafka clients use this to specify the scope of the access request to the broker. | ||
* `sasl_oauthbearer_extensions` - Additional information to be provided to the kafka broker. | ||
* `https_ca_pem` - The CA certificates as a PEM string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it would be helpful to note these should be concatenated? Any specific certs in the chain that are not required? is a specific order required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "be concatenated"? No specific order is required. We don't want to mark optional
on the fields because the OpenID protocol is very flexible. We can't guarantee whether field is a must or not. For backward compatibility, we don't indicate whether it is mandatory or not.
* `username` - Username of the account to connect to the Kafka cluster. | ||
* `password` - Password of the account to connect to the Kafka cluster. | ||
* `token_endpoint_url` - OAUTH issuer token endpoint HTTP(S) URI used to retrieve the token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it would help to clarify that this is usually an IdP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok?
* `token_endpoint_url` - OAUTH issuer token endpoint HTTP(S) URI used to retrieve the token. | |
* `token_endpoint_url` - OAUTH issuer(IdP provider) token endpoint HTTP(S) URI used to retrieve the token. |
as discussed offline please don't merge until 2.0.0 is released |
@@ -0,0 +1,3 @@ | |||
```release-note:enhancement | |||
resource/mongodbatlas_stream_connection: Add new authentication mechanism(OIDC) to the Kafka connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resource/mongodbatlas_stream_connection: Add new authentication mechanism(OIDC) to the Kafka connection | |
resource/mongodbatlas_stream_connection: Adds new authentication mechanism(OIDC) to the Kafka connection |
3rd person for changelog messages
@@ -0,0 +1,3 @@ | |||
```release-note:enhancement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog entries for the 2 datasources are missing
* `username` - Username of the account to connect to the Kafka cluster. | ||
* `password` - Password of the account to connect to the Kafka cluster. | ||
* `token_endpoint_url` - OAUTH issuer(IdP provider) token endpoint HTTP(S) URI used to retrieve the token. | ||
* `client_id` - Public identifier for the Kafka client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do all new attributes apply to Kafka? in that case consider having them inside a kafka_config
nested attribute, although it's probably better to follow as it's in the Atlas API
@@ -55,6 +55,38 @@ resource "mongodbatlas_stream_connection" "test" { | |||
} | |||
``` | |||
|
|||
### Example Kafka SASL OAuthbearer Connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new attributes description is missing in resource doc page
Optional: true, | ||
Sensitive: true, | ||
}, | ||
"scope": schema.StringAttribute{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: does Atlas return some default value if not provided by the client? in that case the atribute might need to be Optional & Computed, although better to keep it as Optional if possible
resource.ParallelTest(t, *testCase) | ||
} | ||
|
||
func testCaseKafkaOAuthBearer(t *testing.T, nameSuffix string) *resource.TestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not inline testCaseKafkaOAuthBearer in TestAccStreamRSStreamConnection_kafkaOAuthBearer ?
Description
This ticket added one authentication mechanism to the Kafka connection.
Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-338207
Type of change:
Required Checklist:
Further comments