Skip to content
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.

Sign up for GitHub

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

fix: Avoids sending database user password in update request if the value has not changed #2005

Merged
merged 4 commits into from
Mar 12, 2024
Merged

fix: Avoids sending database user password in update request if the value has not changed #2005

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2741f70
include warning during creation
AgustinBettati Mar 11, 2024
6a0b414
fix: Avoids sending database user password in update request if the v…
AgustinBettati Mar 11, 2024
b01d5d1
add unit test for case when password attribute is removed and should …
AgustinBettati Mar 11, 2024
a93ffea
include warning message
AgustinBettati Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions internal/service/databaseuser/model_database_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"go.mongodb.org/atlas-sdk/v20231115007/admin"
)

func NewMongoDBDatabaseUser(ctx context.Context, dbUserModel *TfDatabaseUserModel) (*admin.CloudDatabaseUser, diag.Diagnostics) {
func NewMongoDBDatabaseUser(ctx context.Context, statePasswordValue types.String, dbUserModel *TfDatabaseUserModel) (*admin.CloudDatabaseUser, diag.Diagnostics) {
var rolesModel []*TfRoleModel
var labelsModel []*TfLabelModel
var scopesModel []*TfScopeModel
Expand All @@ -31,10 +31,9 @@ func NewMongoDBDatabaseUser(ctx context.Context, dbUserModel *TfDatabaseUserMode
return nil, diags
}

return &admin.CloudDatabaseUser{
result := admin.CloudDatabaseUser{
GroupId: dbUserModel.ProjectID.ValueString(),
Username: dbUserModel.Username.ValueString(),
Password: dbUserModel.Password.ValueStringPointer(),
X509Type: dbUserModel.X509Type.ValueStringPointer(),
AwsIAMType: dbUserModel.AWSIAMType.ValueStringPointer(),
OidcAuthType: dbUserModel.OIDCAuthType.ValueStringPointer(),
Expand All @@ -43,7 +42,13 @@ func NewMongoDBDatabaseUser(ctx context.Context, dbUserModel *TfDatabaseUserMode
Roles: NewMongoDBAtlasRoles(rolesModel),
Labels: NewMongoDBAtlasLabels(labelsModel),
Scopes: NewMongoDBAtlasScopes(scopesModel),
}, nil
}

if statePasswordValue != dbUserModel.Password {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess this also covers if the password attribute is deleted, then we set nil so it's not sent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, added an additional unit test to cover this case as well, thanks

// Password value has been modified or no previous state was present. Password is only updated if changed in the terraform configuration CLOUDP-235738
result.Password = dbUserModel.Password.ValueStringPointer()
}
return &result, nil
}

func NewTfDatabaseUserModel(ctx context.Context, model *TfDatabaseUserModel, dbUser *admin.CloudDatabaseUser) (*TfDatabaseUserModel, diag.Diagnostics) {
Expand Down
55 changes: 45 additions & 10 deletions internal/service/databaseuser/model_database_user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,41 +73,76 @@ var (
Labels: &[]admin.ComponentLabel{sdkLabel},
Scopes: &[]admin.UserScope{sdkScope},
}
cloudDatabaseUserWithoutPassword = &admin.CloudDatabaseUser{
GroupId: projectID,
DatabaseName: authDatabaseName,
Username: username,
X509Type: &x509Type,
OidcAuthType: &oidCAuthType,
LdapAuthType: &ldapAuthType,
AwsIAMType: &awsIAMType,
Roles: &[]admin.DatabaseUserRole{sdkRole},
Labels: &[]admin.ComponentLabel{sdkLabel},
Scopes: &[]admin.UserScope{sdkScope},
}
)

func TestNewMongoDBDatabaseUser(t *testing.T) {
testCases := []struct {
TfDatabaseUserModel databaseuser.TfDatabaseUserModel
tfDatabaseUserModel databaseuser.TfDatabaseUserModel
passwordStateValue types.String
expectedResult *admin.CloudDatabaseUser
name string
expectedError bool
}{
{
name: "Success CloudDatabaseUser",
TfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, scopesSet),
name: "CloudDatabaseUser for create",
tfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, scopesSet, types.StringValue(password)),
passwordStateValue: types.StringNull(),
expectedResult: cloudDatabaseUser,
expectedError: false,
},
{
name: "CloudDatabaseUser with new password in model when password value is modified",
tfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, scopesSet, types.StringValue(password)),
passwordStateValue: types.StringValue("oldPassword"),
expectedResult: cloudDatabaseUser,
expectedError: false,
},
{
name: "CloudDatabaseUser with no password in model when password value remains the same",
tfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, scopesSet, types.StringValue(password)),
passwordStateValue: types.StringValue(password),
expectedResult: cloudDatabaseUserWithoutPassword,
expectedError: false,
},
{
name: "CloudDatabaseUser with no password in model when password value is removed",
tfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, scopesSet, types.StringNull()),
passwordStateValue: types.StringValue(password),
expectedResult: cloudDatabaseUserWithoutPassword,
expectedError: false,
},
{
name: "Roles fail",
TfDatabaseUserModel: *getDatabaseUserModel(wrongRoleSet, labelsSet, scopesSet),
tfDatabaseUserModel: *getDatabaseUserModel(wrongRoleSet, labelsSet, scopesSet, types.StringValue(password)),
expectedError: true,
},
{
name: "Labels fail",
TfDatabaseUserModel: *getDatabaseUserModel(rolesSet, wrongLabelSet, scopesSet),
tfDatabaseUserModel: *getDatabaseUserModel(rolesSet, wrongLabelSet, scopesSet, types.StringValue(password)),
expectedError: true,
},
{
name: "Scopes fail",
TfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, wrongScopeSet),
tfDatabaseUserModel: *getDatabaseUserModel(rolesSet, labelsSet, wrongScopeSet, types.StringValue(password)),
expectedError: true,
},
}

for i, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
resultModel, err := databaseuser.NewMongoDBDatabaseUser(context.Background(), &testCases[i].TfDatabaseUserModel)
resultModel, err := databaseuser.NewMongoDBDatabaseUser(context.Background(), tc.passwordStateValue, &testCases[i].tfDatabaseUserModel)

if (err != nil) != tc.expectedError {
t.Errorf("Case %s: Received unexpected error: %v", tc.name, err)
Expand All @@ -129,7 +164,7 @@ func TestNewTfDatabaseUserModel(t *testing.T) {
name: "Success TfDatabaseUserModel",
sdkDatabaseUser: cloudDatabaseUser,
currentModel: databaseuser.TfDatabaseUserModel{Password: types.StringValue(password)},
expectedResult: getDatabaseUserModel(rolesSet, labelsSet, scopesSet),
expectedResult: getDatabaseUserModel(rolesSet, labelsSet, scopesSet, types.StringValue(password)),
expectedError: false,
},
}
Expand Down Expand Up @@ -308,7 +343,7 @@ func TestNewTFRolesModel(t *testing.T) {
}
}

func getDatabaseUserModel(roles, labels, scopes basetypes.SetValue) *databaseuser.TfDatabaseUserModel {
func getDatabaseUserModel(roles, labels, scopes basetypes.SetValue, password types.String) *databaseuser.TfDatabaseUserModel {
encodedID := conversion.EncodeStateID(map[string]string{
"project_id": projectID,
"username": username,
Expand All @@ -319,7 +354,7 @@ func getDatabaseUserModel(roles, labels, scopes basetypes.SetValue) *databaseuse
ProjectID: types.StringValue(projectID),
AuthDatabaseName: types.StringValue(authDatabaseName),
Username: types.StringValue(username),
Password: types.StringValue(password),
Password: password,
X509Type: types.StringValue(x509Type),
OIDCAuthType: types.StringValue(oidCAuthType),
LDAPAuthType: types.StringValue(ldapAuthType),
Expand Down
11 changes: 7 additions & 4 deletions internal/service/databaseuser/resource_database_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ func (r *databaseUserRS) Create(ctx context.Context, req resource.CreateRequest,
return
}

dbUserReq, d := NewMongoDBDatabaseUser(ctx, databaseUserPlan)
dbUserReq, d := NewMongoDBDatabaseUser(ctx, types.StringNull(), databaseUserPlan)
resp.Diagnostics.Append(d...)
if resp.Diagnostics.HasError() {
return
Expand All @@ -231,6 +231,8 @@ func (r *databaseUserRS) Create(ctx context.Context, req resource.CreateRequest,
return
}

resp.Diagnostics.AddWarning("If the password value will be managed externally it is advised to remove the attribute", "More details can be found in resource documentation under the 'password' attribute")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this warning shown always or it has some condition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will always be shown, not really any particular condition we can use because password will always be defined for create operation (fails in API otherwise)


resp.Diagnostics.Append(resp.State.Set(ctx, &dbUserModel)...)
if resp.Diagnostics.HasError() {
return
Expand Down Expand Up @@ -286,14 +288,15 @@ func (r *databaseUserRS) Read(ctx context.Context, req resource.ReadRequest, res

func (r *databaseUserRS) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
var databaseUserPlan *TfDatabaseUserModel
var databaseUserState *TfDatabaseUserModel

diags := req.Plan.Get(ctx, &databaseUserPlan)
resp.Diagnostics.Append(diags...)
resp.Diagnostics.Append(req.Plan.Get(ctx, &databaseUserPlan)...)
resp.Diagnostics.Append(req.State.Get(ctx, &databaseUserState)...)
if resp.Diagnostics.HasError() {
return
}

dbUserReq, d := NewMongoDBDatabaseUser(ctx, databaseUserPlan)
dbUserReq, d := NewMongoDBDatabaseUser(ctx, databaseUserState.Password, databaseUserPlan)
resp.Diagnostics.Append(d...)
if resp.Diagnostics.HasError() {
return
Expand Down
2 changes: 1 addition & 1 deletion website/docs/r/database_user.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ Accepted values include:
* `project_id` - (Required) The unique ID for the project to create the database user.
* `roles` - (Required) List of user’s roles and the databases / collections on which the roles apply. A role allows the user to perform particular actions on the specified database. A role on the admin database can include privileges that apply to the other databases as well. See [Roles](#roles) below for more details.
* `username` - (Required) Username for authenticating to MongoDB. USER_ARN or ROLE_ARN if `aws_iam_type` is USER or ROLE.
* `password` - (Required) User's initial password. A value is required to create the database user, however the argument but may be removed from your Terraform configuration after user creation without impacting the user, password or Terraform management. IMPORTANT --- Passwords may show up in Terraform related logs and it will be stored in the Terraform state file as plain-text. Password can be changed after creation using your preferred method, e.g. via the MongoDB Atlas UI, to ensure security. If you do change management of the password to outside of Terraform be sure to remove the argument from the Terraform configuration so it is not inadvertently updated to the original password.
* `password` - (Required) User's initial password. A value is required to create the database user, however the argument may be removed from your Terraform configuration after user creation without impacting the user, password or Terraform management. If you do change management of the password to outside of Terraform it is advised to remove the argument from the Terraform configuration. IMPORTANT --- Passwords may show up in Terraform related logs and it will be stored in the Terraform state file as plain-text. Password can be changed after creation using your preferred method, e.g. via the MongoDB Atlas UI, to ensure security.
Comment on lines 134 to +135
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a warning at the top of this page to make sure that this warning message is not missed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, included.


* `x509_type` - (Optional) X.509 method by which the provided username is authenticated. If no value is given, Atlas uses the default value of NONE. The accepted types are:
* `NONE` - The user does not use X.509 authentication.
Expand Down