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

CIS - Delete filter on deletion of Firewall Rules #3963

Merged

Conversation

arpit-srivastava-ibm
Copy link
Contributor

@arpit-srivastava-ibm arpit-srivastava-ibm commented Aug 11, 2022

Problem statement -

  • On deletion of Firewall Rules, the filter is not removed automatically.

Issue - https://github.ibm.com/NetworkTribe/CIS_Support/issues/2515

@@ -238,6 +238,21 @@ func ResourceIBMCISFirewallrulesDelete(context context.Context, d *schema.Resour
return diag.FromErr(fmt.Errorf("[ERROR] Error deleting the custom resolver %s:%s", err, response))
}

if id, ok := d.GetOk(cisFilterID); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use same filterID forother rules. If we delete the filterID will not be other firewall rules not effected?
If user create a filter using Terraform is the destroy not handled to remove filter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we can not use same filter for other rules. It throws an error on creating a new rule with an already mapped filterID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, destroy handles filter deletion but it does not cover old filters.

@hkantare
Copy link
Collaborator

please share the testcase execution results

@arpit-srivastava-ibm
Copy link
Contributor Author

Testcase results -

arpit-mac@Arpits-MacBook-Pro terraform-provider-ibm % make testacc TEST=./ibm/service/cis TESTARGS='-run=TestAccIBMCisFirewallrules_Basic'
=== RUN   TestAccIBMCisFirewallrules_Basic
--- PASS: TestAccIBMCisFirewallrules_Basic (92.94s)
PASS
ok  	github.com/IBM-Cloud/terraform-provider-ibm/ibm/service/cis	94.358s


filter_id := id.(string)
filterOpt := cisFilterClient.NewDeleteFiltersOptions(xAuthtoken, crn, zoneID, filter_id)
_, _, err = cisFilterClient.DeleteFilters(filterOpt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if a template deletes filter as part of destroy and when a firewall rule makes again same delete call willnot that fail with 404?
should we handle this case?
Can you write tempalte
which creates filter and use that filter in rule and then run destroy

@@ -71,10 +71,12 @@ func ResourceIBMCISDomain() *schema.Resource {
cisDomainVerificationKey: {
Type: schema.TypeString,
Computed: true,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this marked as optional we are just setting the value in Read method

},
cisDomainCnameSuffix: {
Type: schema.TypeString,
Computed: true,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

@hkantare hkantare merged commit f16f2fe into IBM-Cloud:master Aug 17, 2022
SunithaGudisagarIBM pushed a commit to ibm-vpc/terraform-provider-ibm that referenced this pull request Sep 14, 2022
* delete filter along with firewall rule

* added partial check

* removed optional

Co-authored-by: Arpit Srivastava <arpit-mac@Arpits-MacBook-Pro.local>
SunithaGudisagarIBM pushed a commit to ibm-vpc/terraform-provider-ibm that referenced this pull request Sep 14, 2022
* delete filter along with firewall rule

* added partial check

* removed optional

Co-authored-by: Arpit Srivastava <arpit-mac@Arpits-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants