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.

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: Fixes nil pointer dereference in mongodbatlas_alert_configuration #2463

Merged
merged 7 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 3 additions & 0 deletions .changelog/2463.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/mongodbatlas_alert_configuration: Fixes an issue where the `terraform apply` command crashes if you attempt to edit an existing `mongodbatlas_alert_configuration` by adding a value to `threshold_config`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resource/mongodbatlas_alert_configuration: Fixes an issue where the `terraform apply` command crashes if you attempt to edit an existing `mongodbatlas_alert_configuration` by adding a value to `threshold_config`.
resource/mongodbatlas_alert_configuration: Fixes an issue where the `terraform apply` command crashes if you attempt to edit an existing `mongodbatlas_alert_configuration` by adding a value to `threshold_config`

don't finish in period, e.g.: https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/CHANGELOG.md

same for #2462 in case you can also change its changelog period at the end

Copy link
Collaborator Author

@oarbusi oarbusi Jul 29, 2024

Choose a reason for hiding this comment

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

Okay, removing this period and other unreleased entries periods for consistency. CHANGELOG.md entries will be updated once this is merged. FYI @JuliaMongo

```
28 changes: 14 additions & 14 deletions internal/service/alertconfiguration/model_alert_configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,28 +200,28 @@ func NewTFMetricThresholdConfigModel(t *admin.ServerlessMetricThreshold, currSta
return []TfMetricThresholdConfigModel{
{
MetricName: conversion.StringNullIfEmpty(t.MetricName),
Operator: conversion.StringNullIfEmpty(*t.Operator),
Threshold: types.Float64Value(*t.Threshold),
Units: conversion.StringNullIfEmpty(*t.Units),
Mode: conversion.StringNullIfEmpty(*t.Mode),
Operator: conversion.StringNullIfEmpty(t.GetOperator()),
Threshold: types.Float64Value(t.GetThreshold()),
Units: conversion.StringNullIfEmpty(t.GetUnits()),
Mode: conversion.StringNullIfEmpty(t.GetMode()),
Comment on lines -203 to +206
Copy link
Member

Choose a reason for hiding this comment

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

just to fully understand, how where you able to have the API respond with a MetricThreshold object that only had some fields defined? Which one was not being returned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue was for threshold_config but I also did changes on metric_threshold_config just to be safe.
The issue can be reproduced following this steps (more details here):

  • Create an alert_notification without threshold_config
  • Through the Atlas UI, edit the alert configuration created in terraform and set a threshold_config
  • terraform apply
  • nil pointer dereference happens and provider crashes

Copy link
Member

@lantoli lantoli Jul 29, 2024

Choose a reason for hiding this comment

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

in these cases it would be great to create a test that fails before the fix and passes after the fix is applied

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have not been able to create a test for this because to reproduce this issue the resource has to be modified from outside TF. I don't think we currently are able to reproduce this cases in our tests. Open to suggestions though :)

},
}
}
currState := currStateSlice[0]
newState := TfMetricThresholdConfigModel{
Threshold: types.Float64Value(*t.Threshold),
Threshold: types.Float64Value(t.GetThreshold()),
}
if !currState.MetricName.IsNull() {
newState.MetricName = conversion.StringNullIfEmpty(t.MetricName)
}
if !currState.Operator.IsNull() {
newState.Operator = conversion.StringNullIfEmpty(*t.Operator)
newState.Operator = conversion.StringNullIfEmpty(t.GetOperator())
}
if !currState.Units.IsNull() {
newState.Units = conversion.StringNullIfEmpty(*t.Units)
newState.Units = conversion.StringNullIfEmpty(t.GetUnits())
}
if !currState.Mode.IsNull() {
newState.Mode = conversion.StringNullIfEmpty(*t.Mode)
newState.Mode = conversion.StringNullIfEmpty(t.GetMode())
}
return []TfMetricThresholdConfigModel{newState}
}
Expand All @@ -234,21 +234,21 @@ func NewTFThresholdConfigModel(t *admin.GreaterThanRawThreshold, currStateSlice
if len(currStateSlice) == 0 { // threshold was created elsewhere from terraform, or import statement is being called
return []TfThresholdConfigModel{
{
Operator: conversion.StringNullIfEmpty(*t.Operator),
Threshold: types.Float64Value(float64(*t.Threshold)), // int in new SDK but keeping float64 for backward compatibility
Units: conversion.StringNullIfEmpty(*t.Units),
Operator: conversion.StringNullIfEmpty(t.GetOperator()),
Threshold: types.Float64Value(float64(t.GetThreshold())), // int in new SDK but keeping float64 for backward compatibility
Units: conversion.StringNullIfEmpty(t.GetUnits()),
},
}
}
currState := currStateSlice[0]
newState := TfThresholdConfigModel{}
if !currState.Operator.IsNull() {
newState.Operator = conversion.StringNullIfEmpty(*t.Operator)
newState.Operator = conversion.StringNullIfEmpty(t.GetOperator())
}
if !currState.Units.IsNull() {
newState.Units = conversion.StringNullIfEmpty(*t.Units)
newState.Units = conversion.StringNullIfEmpty(t.GetUnits())
}
newState.Threshold = types.Float64Value(float64(*t.Threshold))
newState.Threshold = types.Float64Value(float64(t.GetThreshold()))

return []TfThresholdConfigModel{newState}
}
Expand Down