Deprecate Whitelist for IBM-cloud-databases. Introduce allowlisting #3852
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -459,6 +459,29 @@ func ResourceIBMDatabaseInstance() *schema.Resource { | |
| }, | ||
| }, | ||
| }, | ||
| Deprecated: "Whitelist is deprecated please use allowlist", | ||
| ConflictsWith: []string{"allowlist"}, | ||
| }, | ||
| "allowlist": { | ||
| Type: schema.TypeSet, | ||
| Optional: true, | ||
| Elem: &schema.Resource{ | ||
| Schema: map[string]*schema.Schema{ | ||
| "address": { | ||
| Description: "Allowlist IP address in CIDR notation", | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ValidateFunc: validate.ValidateCIDR, | ||
| }, | ||
| "description": { | ||
| Description: "Unique allow list description", | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ValidateFunc: validation.StringLenBetween(1, 32), | ||
| }, | ||
| }, | ||
| }, | ||
| ConflictsWith: []string{"whitelist"}, | ||
| }, | ||
| "group": { | ||
| Type: schema.TypeSet, | ||
|
|
@@ -1476,6 +1499,36 @@ func resourceIBMDatabaseInstanceCreate(context context.Context, d *schema.Resour | |
| "[ERROR] Error waiting for update of database (%s) whitelist task to complete: %s", icdId, err)) | ||
| } | ||
| } | ||
| } else if al, ok := d.GetOk("allowlist"); ok { | ||
| cloudDatabasesClient, err := meta.(conns.ClientSession).CloudDatabasesV5() | ||
|
|
||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf("[ERROR] Error getting database client settings: %s", err)) | ||
| } | ||
|
|
||
| add := flex.ExpandAllowlist(al.(*schema.Set)) | ||
| for _, entry := range add { | ||
| holdEntry := &clouddatabasesv5.AllowlistEntry{ | ||
| Address: core.StringPtr(*entry.Address), | ||
| Description: core.StringPtr(*entry.Description), | ||
| } | ||
| alEntry := &clouddatabasesv5.AddAllowlistEntryOptions{ | ||
| ID: &instanceID, | ||
| IPAddress: holdEntry, | ||
| } | ||
| addAllowListResponse, _, err := cloudDatabasesClient.AddAllowlistEntry(alEntry) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf( | ||
| "[ERROR] Error updating database allowlist entry: (%s)", err)) | ||
| } | ||
|
|
||
| taskID := *addAllowListResponse.Task.ID | ||
| _, err = waitForDatabaseTaskComplete(taskID, d, meta, d.Timeout(schema.TimeoutUpdate)) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf( | ||
| "[ERROR] Error waiting for update of database (%s) allowlist task to complete: %s", instanceID, err)) | ||
| } | ||
| } | ||
| } | ||
| if cpuRecord, ok := d.GetOk("auto_scaling.0.cpu"); ok { | ||
| params := icdv4.AutoscalingSetGroup{} | ||
|
|
@@ -1710,11 +1763,25 @@ func resourceIBMDatabaseInstanceRead(context context.Context, d *schema.Resource | |
| } | ||
| d.Set("auto_scaling", flattenICDAutoScalingGroup(autoSclaingGroup)) | ||
|
|
||
| if _, ok := d.GetOk("whitelist"); ok { | ||
|
There was a problem hiding this comment. In Resource Read also don't use d.Get operations because the import functionality doesn't work There was a problem hiding this comment. 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: Here's my explanation as to why
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! There was a problem hiding this comment. Thanks for explanation There was a problem hiding this comment. @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 |
||
| whitelist, err := icdClient.Whitelists().GetWhitelist(icdId) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf("[ERROR] Error getting database whitelist: %s", err)) | ||
| } | ||
| d.Set("whitelist", flex.FlattenWhitelist(whitelist)) | ||
| } else if _, ok := d.GetOk("allowlist"); ok { | ||
| cloudDatabasesClient, err := meta.(conns.ClientSession).CloudDatabasesV5() | ||
| alEntry := &clouddatabasesv5.GetAllowlistOptions{ | ||
| ID: &instanceID, | ||
| } | ||
|
|
||
| allowlist, _, err := cloudDatabasesClient.GetAllowlist(alEntry) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf("[ERROR] Error getting database allowlist: %s", err)) | ||
| } | ||
|
|
||
| d.Set("allowlist", flex.FlattenGetAllowlist(*allowlist)) | ||
| } | ||
|
|
||
| var connectionStrings []flex.CsEntry | ||
| //ICD does not implement a GetUsers API. Users populated from tf configuration. | ||
|
|
@@ -2103,6 +2170,80 @@ func resourceIBMDatabaseInstanceUpdate(context context.Context, d *schema.Resour | |
| "[ERROR] Error waiting for database (%s) whitelist delete task to complete for ipAddress %s : %s", icdId, ipAddress, err)) | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } else if d.HasChange("allowlist") { | ||
| cloudDatabasesClient, err := meta.(conns.ClientSession).CloudDatabasesV5() | ||
|
|
||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf("[ERROR] Error getting database client settings: %s", err)) | ||
| } | ||
|
|
||
| oldList, newList := d.GetChange("allowlist") | ||
| if oldList == nil { | ||
| oldList = new(schema.Set) | ||
| } | ||
| if newList == nil { | ||
| newList = new(schema.Set) | ||
| } | ||
| os := oldList.(*schema.Set) | ||
| ns := newList.(*schema.Set) | ||
| remove := os.Difference(ns).List() | ||
| add := ns.Difference(os).List() | ||
|
|
||
| if len(add) > 0 { | ||
| for _, entry := range add { | ||
| newEntry := entry.(map[string]interface{}) | ||
| holdEntry := &clouddatabasesv5.AllowlistEntry{ | ||
| Address: core.StringPtr(newEntry["address"].(string)), | ||
| Description: core.StringPtr(newEntry["description"].(string)), | ||
| } | ||
| alEntry := &clouddatabasesv5.AddAllowlistEntryOptions{ | ||
| ID: &instanceID, | ||
| IPAddress: holdEntry, | ||
| } | ||
| addAllowListResponse, response, err := cloudDatabasesClient.AddAllowlistEntry(alEntry) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf( | ||
| "[ERROR] DeleteAllowlistEntry (%s) failed %s\n%s", *addAllowListResponse.Task.Description, err, response)) | ||
| } | ||
|
|
||
| taskID := *addAllowListResponse.Task.ID | ||
| _, err = waitForDatabaseTaskComplete(taskID, d, meta, d.Timeout(schema.TimeoutUpdate)) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf( | ||
| "[ERROR] Error waiting for database (%s) allowlist delete task to complete for ipAddress %s : %s", instanceID, *addAllowListResponse.Task.Description, err)) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| if len(remove) > 0 { | ||
| for _, entry := range remove { | ||
| newEntry := entry.(map[string]interface{}) | ||
| holdEntry := &clouddatabasesv5.AllowlistEntry{ | ||
| Address: core.StringPtr(newEntry["address"].(string)), | ||
| Description: core.StringPtr(newEntry["description"].(string)), | ||
| } | ||
| alEntry := &clouddatabasesv5.DeleteAllowlistEntryOptions{ | ||
| ID: &instanceID, | ||
| Ipaddress: holdEntry.Address, | ||
| } | ||
|
|
||
| deleteAllowListResponse, response, err := cloudDatabasesClient.DeleteAllowlistEntry(alEntry) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf( | ||
| "[ERROR] DeleteAllowlistEntry (%s) failed %s\n%s", *deleteAllowListResponse.Task.Description, err, response)) | ||
| } | ||
|
|
||
| taskID := *deleteAllowListResponse.Task.ID | ||
| _, err = waitForDatabaseTaskComplete(taskID, d, meta, d.Timeout(schema.TimeoutUpdate)) | ||
| if err != nil { | ||
| return diag.FromErr(fmt.Errorf( | ||
| "[ERROR] Error waiting for database (%s) allowlist delete task to complete for ipAddress %s : %s", instanceID, *deleteAllowListResponse.Task.Description, err)) | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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,
AllowlistEntryis written as follows: https://github.com/IBM/cloud-databases-go-sdk/blob/de91b81e136fd4c53c0000e87932aaf150658a33/clouddatabasesv5/cloud_databases_v5.go#L2046While
WhitelistEntryis written as follows https://github.com/IBM-Cloud/bluemix-go/blob/d538cb4fd9bece50f7b012496df63b3f62d58662/api/icd/icdv4/whitelist.go#L13My 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I also tested the instances using import and it works :)