SumoLogic / terraform-provider-sumologic

Terraform provider for Sumo Logic
https://www.terraform.io/docs/providers/sumologic/
Mozilla Public License 2.0
36 stars 52 forks source link

Fix use_versioned_api default value discrepancy #650

Open vsinghal13 opened 2 months ago

vsinghal13 commented 2 months ago

The current TF provider version has a bug where any new AWS sources (except S3 Source) when created, are using use_versioned_api as false despite having a default value of true.

Root cause:

This PR fixes that by:

  1. Removing the custom diffSuppressFunc altogether and allowing users to specify false if required. The default value is still set to true and will be correctly set when the parameter is not specified.

Post the TF provider upgrade, all the sources that are using incorrect values of the use_versioned_api parameter can be updated and set to true if the user accepts the tf plan and applies the changes. Otherwise, they can modify their tf code to add the parameter use_versioned_api = false to maintain the existing configuration.

Changing the value from false to true might trigger some duplicate ingestion.

Newly created resources with no external specification of this parameter will be created with the default true value.

Testing Performed:

maimaisie commented 2 months ago

what caused the bug exactly? not sure if I'm getting that information from the PR description

vsinghal13 commented 2 months ago

what caused the bug exactly? not sure if I'm getting that information from the PR description

not sure exactly but seems related to tf not being able to identify correctly when the parameter is not present and setting it to false.

maimaisie commented 2 months ago

let's spend some more time on the real root cause before putting in bandage fixes

vsinghal13 commented 1 month ago

@dagould found an old issue that shows that the default value is ignored when used with DiffSuppressFunc https://github.com/hashicorp/terraform-plugin-sdk/issues/70

maimaisie commented 1 month ago

so what's the proper fix?

maimaisie commented 1 month ago

From standup discussion it sounds like there is no proper fix since this is still an issue present in TF today, and we can only work around it. It would be nice if that was made more clear in your previous comment :)

vsinghal13 commented 1 month ago

I think we should update this doc if we are now restricting the value from being set to false for content types other than AwsS3Bucket https://registry.terraform.io/providers/SumoLogic/sumologic/latest/docs/resources/s3_source#use_versioned_api But on the other hand, we don't have this restriction on the backend, so I'm wondering if it's really a good idea to restrict it only in TF

I don't think we should update the docs as its only mentioned in s3 source documentation and not others and for s3 source we have no change in behavior.

maimaisie commented 1 month ago

we also don't mention it for other aws sources in sumologic documentation, but we don't block users from creating the source with this field in the backend if user sets it in the API

yyyttthan99 commented 1 month ago

Probably add another test to see if there is any unexpected diff after reapplying the plan. I feel like the DiffSuppressFunc is buggy when I am doing testing for azure log source.

vsinghal13 commented 1 month ago

Updated the description with respect to the latest commit.