Deprecate Whitelist for IBM-cloud-databases. Introduce allowlisting #3852

omaraibrahim · 2022-06-17T19:06:33Z

In the api version 5 whitelist was deprecated and allowlist was introduced. In our strive for feature parity, in the following PR I deprecated whitelist and introduced allowlist.

This involved:

Deprecating whitelist in our data_source_ibm_database and introducing allowlist
Deprecating whitelist in our database resource and introducing allowlist
Updating tests

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Output from acceptance testing:

$ make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseDataSource_basic'
--- PASS: TestAccIBMDatabaseDataSource_basic (597.39s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	598.703s
...

$ make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMEDBDatabaseInstanceBasic'
--- PASS: TestAccIBMEDBDatabaseInstanceBasic (1836.72s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	1837.595s

$ make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseInstancePostgresBasic'
--- PASS: TestAccIBMDatabaseInstancePostgresBasic (1493.88s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database	1494.758s

$ make testacc TEST=./ibm/service/database TESTARGS='-run=TestAccIBMDatabaseInstancePostgresAllowlistMigration'
--- PASS: TestAccIBMDatabaseInstancePostgresAllowlistMigration (665.06s)
PASS
ok      github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/database        668.257s

…form-provider-ibm into icd-allowlist

hkantare · 2022-06-21T05:42:29Z

ibm/service/database/data_source_ibm_database.go

-	whitelist, err := icdClient.Whitelists().GetWhitelist(icdId)
-	if err != nil {
-		return fmt.Errorf("[ERROR] Error getting database whitelist: %s", err)
+	if _, ok := d.GetOk("whitelist"); ok {


whitelist and allowlist are computed variables you can't do d.GetOk...

can we use cloud v5 API and flatten the result back to both whitelist and allowlist

@hkantare unfortunately we can't. Whitelist does not exist in v5 and was replaced by allowlist. So whitelist is using v4 while allowlist uses v5.
v5 API: https://cloud.ibm.com/apidocs/cloud-databases-api/cloud-databases-api-v5#getallowlist
v4 API: https://cloud.ibm.com/apidocs/cloud-databases-api/cloud-databases-api-v4#getwhitelist-permissions

Thank you for catching the previous mistake though! I really appreciate it!

hkantare · 2022-06-22T08:25:26Z

ibm/service/database/resource_ibm_database.go

+				Deprecated:    "Whitelist is deprecated please use allowlist",
+				ConflictsWith: []string{"allowlist"},
+			},
+			"allowlist": {


Can't we set the v5 API response of allowlist to whitelist (because the schema looks same for both) it just flatten and setting the same

Hi @hkantare!

They're two different apis endpoints, and despite them being very similar, I think it's safer to keep them separate. Whitelist is depreciated and will be removed soon as well. Hence, I think a separation of concerns is better. Here are the api docs for both. #3852 (comment)

Thanks!
Omar

Yes ...v4/v5 API are two different endpoint with same model structure and functionality ...cant we just handle in the backend (i mean resource/datasource) to migrate to V5 AP's without any change to schema.

@hkantare so the responses are the same yes :D But the supporting functionality is not. Meaning, the autogenerated allowlist functions utilize allowlist structs the use pointers. For example, https://github.com/IBM/cloud-databases-go-sdk/blob/de91b81e136fd4c53c0000e87932aaf150658a33/clouddatabasesv5/cloud_databases_v5.go#L1848

In contrast the whitelist functions utilizes a whitelist struct that does not use pointers. An example of the functions that consume structs that are shaped differently can be found here: https://github.com/IBM-Cloud/bluemix-go/blob/d538cb4fd9bece50f7b012496df63b3f62d58662/api/icd/icdv4/whitelist.go#L38

So despite achieving the same result, the code is structured very differently and the structs that allowlist relies on are different than whitelist. For example, AllowlistEntry is written as follows: https://github.com/IBM/cloud-databases-go-sdk/blob/de91b81e136fd4c53c0000e87932aaf150658a33/clouddatabasesv5/cloud_databases_v5.go#L2046
While WhitelistEntry is written as follows https://github.com/IBM-Cloud/bluemix-go/blob/d538cb4fd9bece50f7b012496df63b3f62d58662/api/icd/icdv4/whitelist.go#L13

My concern of just setting the v5 response of allowlist to whitelist is we'll have to do lots of downstream changes that will break things or cause unexpected bugs.

Oh I also tested the instances using import and it works :)

hkantare · 2022-06-22T08:26:09Z

ibm/service/database/resource_ibm_database.go

-	whitelist, err := icdClient.Whitelists().GetWhitelist(icdId)
-	if err != nil {
-		return diag.FromErr(fmt.Errorf("[ERROR] Error getting database whitelist: %s", err))
+	if _, ok := d.GetOk("whitelist"); ok {


In Resource Read also don't use d.Get operations because the import functionality doesn't work

Hi @hkantare!

Thanks for taking a look at the PR. I really appreciate it! After spending a few days trying to incorporate this requested change, I am not convinced that it is a good idea. Removing the operation actually causes bugs as seen below:

test_working_directory=/var/folders/zl/1pk63bj95xx56m3vvm0yn35m0000gn/T/plugintest824757667 error= | After applying this test step and performing a `terraform refresh`, the plan was not empty. | stdout | | | Terraform used the selected providers to generate the following execution | plan. Resource actions are indicated with the following symbols: | ~ update in-place | | Terraform will perform the following actions: | | # ibm_database.tf-edb-58 will be updated in-place | ~ resource "ibm_database" "tf-edb-58" { | id = "crn:v1:bluemix:public:databases-for-enterprisedb:eu-gb:a/40ddc34a953a8c02f10987b59085b60e:c1ff0800-8682-4b6b-a8b7-5fa8df912a31::" | name = "tf-edb-58" | tags = [ | "one:two", | ] | # (25 unchanged attributes hidden) | | | | | | - whitelist { | - address = "172.168.1.2/32" -> null | - description = "desc1" -> null | } | # (4 unchanged blocks hidden) | } | | Plan: 0 to add, 1 to change, 0 to destroy. resource_ibm_database_edb_test.go:25: Step 1/4 error: After applying this test step and performing a `terraform refresh`, the plan was not empty. stdout Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place Terraform will perform the following actions: # ibm_database.tf-edb-58 will be updated in-place ~ resource "ibm_database" "tf-edb-58" { id = "crn:v1:bluemix:public:databases-for-enterprisedb:eu-gb:a/40ddc34a953a8c02f10987b59085b60e:c1ff0800-8682-4b6b-a8b7-5fa8df912a31::" name = "tf-edb-58" tags = [ "one:two", ] # (25 unchanged attributes hidden) - whitelist { - address = "172.168.1.2/32" -> null - description = "desc1" -> null } # (4 unchanged blocks hidden) } Plan: 0 to add, 1 to change, 0 to destroy.

Here's my explanation as to why d.GetOK is actually vital:

getOK does work in resource read (I've confirmed this). In general we use many operations associated with the resourceDataStructure throughout the function and this isn't any different really.

By taking it away, we end up setting whitelist to a value. The reason begin is the api commands for allowlist and whitelist update the same source. We do not want to set our whitelist to anything if allowlist is used, and vice versa.

By not doing the above we end up setting whitelist to some value. This value then gets updated to null down the line. This is bad and can cause unintended bugs like overwriting data.

Also note, whitelist and allowlist are special cases where both apis do the same exact thing. This is highly unusual, but it is what it is.

Hence, the best was to approach this is the original way which is check if allowlist or whitelist is used. Then proceed to do the api calls depending on which one is used. Both can never be set at the same time due to the conflict code, and we’ve established that d.getOK does work perfectly fine.

Thanks!
Omar

Thanks for explanation
getOK does work in resource read but it fails during the import scenario. Let say you have created a database outside Terraform and want to import and manage in future using Terraform. We need to import the resource. (https://learn.hashicorp.com/tutorials/terraform/state-import?in=terraform/state&utm_source=WEBSITE&utm_medium=WEB_IO&utm_offer=ARTICLE_PAGE&utm_content=DOCS)
For import we first create a empty resource block
resource "ibm_database" "mydb" {
}
Run terraform import ibm_database.mydb
Import also calls the same resource read since the block is empty d.GetOk fails for both whitelist and allowlist

@hkantare thank you very very much for the explanation :D I've gone ahead and update the code so that if whitelist isn't there we always use allowlist. I follow the terraform docs here: https://www.terraform.io/plugin/sdkv2/best-practices/deprecations#renaming-an-optional-attribute

So if d.getOK("whitelist") fails we'll always use allowlist. lmk what you think :D

…der-ibm into icd-allowlist

hkantare · 2022-09-12T09:18:21Z

@omaraibrahim Can you add a migration testcase from whitelist to allowlist

…der-ibm into icd-allowlist

omaraibrahim · 2022-11-02T17:53:38Z

@hkantare I've gone ahead and added migration tests. I've also modified the code to address some concerns and ensure a seemless migration to allowlist - while retaining full functionality for whitelist.

Migration tests can be found here:

terraform-provider-ibm/ibm/service/database/resource_ibm_database_postgresql_test.go

Line 105 in 9845af3

func TestAccIBMDatabaseInstancePostgresGroupMigration(t *testing.T) {

…3852) * works with local vendor changes. * working using cloud databases v5 now * updated deprecation message * data_source_ibm_database and some tests * modified the rest of the resource tests * fixed calling d.getOK for computed variable * potential import issue fix * added migration test * fixed migration bug * fixed allowlist removal bug * setting import state verify to false due to terraform bug * removed extra logs * updated docs

omaraibrahim and others added 7 commits June 8, 2022 14:54

works with local vendor changes.

740b229

working using cloud databases v5 now

5c2a8a1

Merge branch 'IBM-Cloud:master' into icd-allowlist

53c40e1

updated deprecation message

992c917

data_source_ibm_database and some tests

975774a

Merge branch 'icd-allowlist' of https://github.com/omaraibrahim/terra…

8dc4474

…form-provider-ibm into icd-allowlist

modified the rest of the resource tests

ef0a37f

hkantare reviewed Jun 21, 2022

View reviewed changes

fixed calling d.getOK for computed variable

f635561

hkantare reviewed Jun 22, 2022

View reviewed changes

Merge branch 'master' of https://github.com/IBM-Cloud/terraform-provi…

a241f8d

…der-ibm into icd-allowlist

hkantare force-pushed the master branch 2 times, most recently from e07c2f7 to 17b24e9 Compare July 21, 2022 06:34

omaraibrahim added 3 commits August 30, 2022 14:05

Merge branch 'master' of https://github.com/IBM-Cloud/terraform-provi…

a75bb41

…der-ibm into icd-allowlist

potential import issue fix

fb40772

Merge branch 'master' of https://github.com/IBM-Cloud/terraform-provi…

d84700d

…der-ibm into icd-allowlist

omaraibrahim closed this Sep 9, 2022

omaraibrahim reopened this Sep 9, 2022

omaraibrahim added 8 commits September 12, 2022 15:06

Merge branch 'master' of https://github.com/IBM-Cloud/terraform-provi…

cb2a144

…der-ibm into icd-allowlist

added migration test

22a4573

Merge branch 'master' of https://github.com/IBM-Cloud/terraform-provi…

26fe2bc

…der-ibm into icd-allowlist

fixed migration bug

470cee1

fixed allowlist removal bug

05da7d3

setting import state verify to false due to terraform bug

44017d4

removed extra logs

b7773ac

updated docs

9845af3

hkantare merged commit ccf0ae0 into IBM-Cloud:master Nov 3, 2022

omaraibrahim mentioned this pull request Jul 26, 2023

Removed whitelist #4714

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate Whitelist for IBM-cloud-databases. Introduce allowlisting #3852

Deprecate Whitelist for IBM-cloud-databases. Introduce allowlisting #3852

omaraibrahim commented Jun 17, 2022 •

edited

Loading

hkantare Jun 21, 2022

hkantare Jun 21, 2022

omaraibrahim Jun 21, 2022

hkantare Jun 22, 2022

omaraibrahim Jul 27, 2022

hkantare Aug 5, 2022

omaraibrahim Aug 31, 2022

omaraibrahim Sep 1, 2022 •

edited

Loading

hkantare Jun 22, 2022

omaraibrahim Jul 27, 2022

hkantare Aug 5, 2022

omaraibrahim Aug 31, 2022

hkantare commented Sep 12, 2022

omaraibrahim commented Nov 2, 2022 •

edited

Loading

Deprecate Whitelist for IBM-cloud-databases. Introduce allowlisting #3852

Deprecate Whitelist for IBM-cloud-databases. Introduce allowlisting #3852

Conversation

omaraibrahim commented Jun 17, 2022 • edited Loading

Community Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omaraibrahim Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkantare commented Sep 12, 2022

omaraibrahim commented Nov 2, 2022 • edited Loading

omaraibrahim commented Jun 17, 2022 •

edited

Loading

omaraibrahim Sep 1, 2022 •

edited

Loading

omaraibrahim commented Nov 2, 2022 •

edited

Loading