oci_database_autonomous_database use_latest_available_backup_time_stamp argument failed to check condition

luckeyca commented 1 year ago

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version and Provider Version

Terraform v1.4.2 on windows_amd64

provider registry.terraform.io/hashicorp/random v3.4.3
provider registry.terraform.io/oracle/oci v4.113.0

Affected Resource(s)

use_latest_available_backup_time_stamp

Terraform Configuration Files

# Copy-paste your Terraform configurations here - for large Terraform configs,
# please use a service like Dropbox and share a link to the ZIP file. 
# Please remove any sensitive information from configuration files before sharing them. 

resource "oci_database_autonomous_database" "this" {
......

  use_latest_available_backup_time_stamp = var.primary_adb_source == "BACKUP_FROM_TIMESTAMP" && length(var.primary_adb_source_adb_backup_timestamp) == 0 ? var.primary_adb_use_latest_available_adb_backup_time_stamp : null
  timestamp                              = var.primary_adb_source == "BACKUP_FROM_TIMESTAMP" && !var.primary_adb_use_latest_available_adb_backup_time_stamp ? var.primary_adb_source_adb_backup_timestamp : null

}

Expected Behavior

With proper conditions set, the use_latest_available_backup_time_stamp and timestamp argument should not conflict given they cannot be used at the same time.

As shown in the code snippet above, use_latest_available_backup_time_stamp should only be true when timestamp is not set. At the same time, timestamp value should only be used when use_latest_available_backup_time_stamp is not set to true.

Actual Behavior

with the code above, even the use_latest_available_backup_time_stamp is hardcoded to false or null, during terraform plan with NO error, terraform apply will fail with the following error.

│ Error: 400-InvalidParameter, Operation failed. Creating a clone is supported with either the timestamp or the latest present, but not both. │ Suggestion: Please update the parameter(s) in the Terraform config as per error message Operation failed. Creating a clone is supported with either the timestamp or the latest present, but not both.

The only way for timestamp to work is to completely remove the use_latest_available_backup_time_stamp from the code which doesn't make any sense whatsoever.

Steps to Reproduce

use the code snippet above to have both use_latest_available_backup_time_stamp and timestamp arguments, then set the use_latest_available_backup_time_stamp to either false or null, along with timestamp argument with a valid value.
terraform plan will succeed.
terraform apply will failed with the error mentioned above.

Important Factoids

This falls under another big problem I reported which is this terraform resource does NOT have application level input validation and dependency checks which makes is NOT production-worthy.

Details here: https://github.com/oracle/terraform-provider-oci/issues/1791

References

ravinitp commented 1 year ago

Thank you for reporting the issue. We have raised an internal ticket to track this. Our service engineers will get back to you.

xiaoditao1 commented 1 year ago

If timeStamp is null and useLatestAvailableBackUpTS is true, it won't throw this error. If timeStamp is not null and useLatestAvailableBackUpTS is false, it won't throw this error. In what case did you see the error? Please provide opc-request-id.

luckeyca commented 1 year ago

Hi @xiaoditao1, I think you may misunderstand the problem I am reporting.

what you said above is true and works, but NOT the problem I am reporting. To do what you said, it's basically a manual judgement on the user side when doing the deployment to manually ensure one or the other parameter is put in.

However, from the code snippet I put, as you can see, I tried to create a root module with both arguments in the resource, but with conditions to make sure one of them is null. However, that is NOT working.

The problem is I cannot put both use_latest_available_backup_time_stamp and timestamp arguments in the resource at the same time even though they both have conditions attached to them to make sure only one of them is to be used, the other will be set to null. This should work. However, it looks like your code only checks the presents of the two arguments, but not the actual values.

what your code should do is to check the condition when both arguments are set in the resource, one of them must be null, not just the fact that both arguments are there which I believe it's what the code is currently doing

luckeyca commented 1 year ago

Hi @xiaoditao1, I think I found what the bug is. It seems that the current code doesn't respect terraform preserved keyword "null"(I have seen this in quite a few different places), instead your code is using it arbitrarily as variable value. Basically from your response, I updated my code to be a workaround which worked. see below.

NOT WORKING:

resource "oci_database_autonomous_database" "this" {
......

  use_latest_available_backup_time_stamp = var.primary_adb_source == "BACKUP_FROM_TIMESTAMP" && length(var.primary_adb_source_adb_backup_timestamp) == 0 ? var.primary_adb_use_latest_available_adb_backup_time_stamp : null
  timestamp                              = var.primary_adb_source == "BACKUP_FROM_TIMESTAMP" && !var.primary_adb_use_latest_available_adb_backup_time_stamp ? var.primary_adb_source_adb_backup_timestamp : null

}

WORKAROUND:

resource "oci_database_autonomous_database" "this" {
......

  use_latest_available_backup_time_stamp = var.primary_adb_source == "BACKUP_FROM_TIMESTAMP" && length(var.primary_adb_source_adb_backup_timestamp) == 0 ? var.primary_adb_use_latest_available_adb_backup_time_stamp : false
  timestamp                              = var.primary_adb_source == "BACKUP_FROM_TIMESTAMP" && !var.primary_adb_use_latest_available_adb_backup_time_stamp ? var.primary_adb_source_adb_backup_timestamp : null

}

By changing the "null" to "false", in the use_latest_available_backup_time_stamp conditional logical NO value, the code works now without error. NULL in terraform has special meaning. When setting an argument to null, it means to omit/skip that argument completely. However, in quite a few different places(not just in this resource), I have seen error messages indicating the code is interpreting the null as the actual value because from time to time I would see errors like "null value is not allowed for this argument/variable..." which is wrong in terraform. please see the hashicorp official documentation on terraform null value: https://developer.hashicorp.com/terraform/language/expressions/types

null: a value that represents absence or omission. If you set an argument of a resource to null, Terraform behaves as though you had completely omitted it — it will use the argument's default value if it has one, or raise an error if the argument is mandatory. null is most useful in conditional expressions, so you can dynamically omit an argument if a condition isn't met.

xiaoditao1 commented 1 year ago

Thanks @luckeyca , we will see how to improve it.

sspeddi commented 1 year ago

@luckeyca , This issue is now fixed. Please validate.

luckeyca commented 1 year ago

Hi @sspeddi Thanks. Is it in the latest 5.3.0? I didn't see the change mentioned in the changelog, also running "terraform init" only downloaded 5.2.1 right now. maybe it's not yet published to the public registry. I will test it tomorrow again and report back.

Also a feedback in general, I found the autonomous database resource has quite a few duplicate arguments, likely due to programming logic is a direct "translation" of the backend API, rather than following the proper terraform declarative programming logic which does require additional coding compared to direct "translation".

for example, in this case, between use_latest_available_backup_time_stamp and timestamp arguments, the normal terraform resource logic should be "if source=BACKUP_FROM_TIMESTAMP AND timestamp is NOT defined, use latest timestamp". In this way, one less argument to handle, also less chance for user error.

Same for source_id vs autonomous_database_id which are both source autonomous database ocids, but used in different source conditions. This should also be handled in your source code to use just one argument, like source_autonomous_database_id, and then depending on the source= condition, use the source_autonomous_database_id accordingly with other required argement values.

I had a similar in person meeting with data safe team and hope to have the same conversation with you guys as I know terraform is being handled by individual product teams within oracle. Thanks.

luckeyca commented 1 year ago

Hi @sspeddi, looks like this was fixed. Where did you fix it as I didn't see any related changes in the provider code, not sure if I missed something. Thanks.

sspeddi commented 1 year ago

Hey @luckeyca, This is fixed as part of code validation in the API. There's no change in the provider code for this.

luckeyca commented 1 year ago

Hi @sspeddi, thanks for the fix and info. Just a thought. I know given your org structure, you guys are handling terraform, ansible and api in the same team and thus it may seem to make sense to handle validation and error handling at the api level. However, given different programming languages work differently, especially in the case of terraform being a declarative vs imperative/procedural, it's better to handling those in the specific language level. As is right now, many terraform declarative functions are not working the way should be, most likely due to this approach. Please discuss within your team on this. Thanks. This ticket can be closed.

luckeyca commented 1 year ago

Hi @sspeddi, As I was saying, "fixing" this problem at API is not really fix the problem, but masking it. I just reported another issue with the same thing here: https://github.com/oracle/terraform-provider-oci/issues/1909

oracle / terraform-provider-oci