[FWaaS_V2]: Add fw_policy_v2 #1584

Koodt · 2023-06-20T09:10:45Z

Add data_source_openstack_fw_policy_v2 with tests. Add import_openstack_fw_policy_v2_test.
Add resource_openstack_fw_policy_v2 with tests.
Add policy_v2 docs.
Add policy_v2 to sources maps.

Close 2/3 part of #115

Add data_source_openstack_fw_policy_v2 with tests. Add import_openstack_fw_policy_v2_test. Add resource_openstack_fw_policy_v2 with tests. Add policy_v2 docs. Add policy_v2 to sources maps.

kayrus · 2023-06-20T09:56:16Z

openstack/resource_openstack_fw_policy_v2.go

+		return diag.Errorf("Error creating OpenStack networking client: %s", err)
+	}
+
+	var updateOpts policies.UpdateOpts


I'd make it a pointer and check for nil before the update. Since potentially there could be empty POST requests.

Good point. Thank you!

openstack/data_source_openstack_fw_policy_v2_test.go

openstack/data_source_openstack_fw_policy_v2.go

openstack/resource_openstack_fw_policy_v2.go

Koodt · 2023-06-20T11:47:34Z

@nikParasyr
I read too late your comment about four requests.
May I keep datasource and resource in one PR or split them?

nikParasyr · 2023-06-20T11:51:20Z

@Koodt keep them on this one its ok. split them in the next PR for fw groups

kayrus · 2023-06-20T12:42:05Z

openstack/data_source_openstack_fw_policy_v2.go

+	}
+
+	if v, ok := d.GetOk("shared"); ok {
+		listOpts.Shared = v.(*bool)


this code will panic with panic: interface conversion: interface {} is bool, not *bool. I'd recommend to add an extra functional test to track this. Also please take a look to your other PRs and make sure this issue doesn't exist there as well.

Add test with AdminOnly for all PRs
I don't see panic not in my IDE, not in runtime, not in tests.
Do you speak about possible panic?

No, it's an actual panic, because you're trying to assert a bool to *bool. Terraform schema data types were never a pointer.

Reproduced and understood
Thank you

…estAccFWPolicyV2_shared, TestAccFWRuleV2_shared, fix indents, remove unused value_specs

kayrus · 2023-06-21T08:40:05Z

openstack/resource_openstack_fw_policy_v2.go

+		return diag.Errorf("Error creating OpenStack networking client: %s", err)
+	}
+
+	audited := d.Get("audited").(bool)


does it make sense to align the audited := d.Get("audited").(bool) and the if r, ok := d.GetOk("shared"); ok { approaches to a single one?

website/docs/r/fw_policy_v2.html.markdown

kayrus · 2023-06-21T08:43:27Z

openstack/fw_policy_v2.go

+
+func fwPolicyV2DeleteFunc(networkingClient *gophercloud.ServiceClient, id string) resource.StateRefreshFunc {
+	return func() (interface{}, string, error) {
+		err := policies.Delete(networkingClient, id).Err


you need to add an ErrDefault404 check here as well. There is a chance that the policy can be deleted manually between checks and this will cause TF to hang.

openstack/resource_openstack_fw_policy_v2.go

openstack/resource_openstack_fw_policy_v2_test.go

Koodt · 2023-06-21T15:01:41Z

@nikParasyr @kayrus I will be revert policies.RemoveRule in resourceFWRuleV2Delete
It was the crutch, like in fwaas_v1.
The rule is deleted first, next deleted association with policy and this will cause TF to exception.

kayrus

Looks much much better now, thank you very much for your patience. See my recent comments and can you also align the d.Get("shared").(bool) bool styles in the openstack_fw_rule_v2 resource?

kayrus · 2023-06-22T08:16:38Z

openstack/data_source_openstack_fw_policy_v2.go

+
+	pages, err := policies.List(networkingClient, listOpts).AllPages()
+	if err != nil {
+		return diag.FromErr(err)


Suggested change

return diag.FromErr(err)

return diag.Errorf("Unable to list openstack_fw_policy_v2 policies: %s", err)

Add this change in data_source_openstack_fw_rule_v2 too

kayrus · 2023-06-22T08:20:09Z

openstack/resource_openstack_fw_policy_v2.go

+			"audited": {
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  false,


I don't think this does make sense :
The same note for openstack_fw_rule_v2 shared argument.

kayrus · 2023-06-22T08:26:59Z

openstack/resource_openstack_fw_rule_v2.go

+		return diag.FromErr(CheckDeleted(d, err, "Error retrieving openstack_fw_rule_v2"))
+	}
+
+	if len(rule.FirewallPolicyID) > 0 {


I don't think it's a good idea to delete rules, which were already attached to a policy. If the resource is controlled by terraform, terraform dependency manager must delete resources according to a dependency, if not, it should fail after a timeout. I'd implement force_destroy or force_delete argument for this case. @nikParasyr we already have force_destroy for containers and force_delete for compute nodes. Which one would you think is better? :)

in addition to

err = rules.Delete(networkingClient, d.Id()).ExtractErr()

Don't you need to introduce a similar approach of calling the fwPolicyV2DeleteFunc function but for rules? And also check for the err.(gophercloud.ErrDefault404) :)

I think It's a bug. TF ignore rule/policy dependencies.
When we wants to detach and delete attached rules, TF for first time delete rules, and after delete rule associations
TestAccFWPolicyV2_deleteRules crashes if we not delete rule from policy prevously.

First step

resource "openstack_fw_rule_v2" "tcp_allow" { protocol = "tcp" action = "allow" } resource "openstack_fw_rule_v2" "udp_deny" { protocol = "udp" action = "deny" } resource "openstack_fw_policy_v2" "policy_1" { name = "policy_1" description = "terraform acceptance test" audited = true rules = [ "${openstack_fw_rule_v2.udp_deny.id}", "${openstack_fw_rule_v2.tcp_allow.id}" ] }

Second Step

resource "openstack_fw_rule_v2" "udp_deny" { protocol = "udp" action = "deny" } resource "openstack_fw_policy_v2" "policy_1" { name = "policy_1" description = "terraform acceptance test" rules = [ "${openstack_fw_rule_v2.udp_deny.id}" ] }

Result

{"NeutronError": {"type": "FirewallRuleInUse", "message": "Firewall rule a8829851-68db-42c9-ba41-d2982a537fd4 is being used.", "detail": ""}}

Since this is an async operation, this may happen. Have you tried with the fwPolicyV2DeleteFunc like approach?

I'd implement force_destroy or force_delete argument for this case.

agreed. force_delete i'd say

Then I think it makes sense to create a separate resource, which will associate the rule with a policy. Blindly removing all the rule associations doesn't look like a good approach.

I have no idea how we will manage the order of the rules in the presence of such a function.
This is not blindly removing. We do it only with deletion of the rule and we'll remove all associations for the rule, not for the policy.

Also, I have no idea how we will manage the list of rules for the policy.
We will need to create a separate resource, which will associate the strictly ordered rule's list with a policy.
How will we import a policy with already created rule's list?

It seems to me that this will be a more complex and confusing flow than deleting an association for a rule, which we will delete anyway

a separate resource, which will associate the rule with a policy

This probably introduces a lot of issues as you need to order the associations.

I think TF dependency graph works fine when both rule & policy are to be deleted. The problem as @Koodt mentioned occurs when in the same apply a rule is removed and the policy it is attached to is updated, because TF will run the delete rule first (and at that point of time the association is still in place) and then update the policy (which would remove the association).

AFAIK, and this only based on docs as i dont have a dev environment with fw_v2, a rule is associated to a single policy, so we are not removing all associations, only one. So from my side this approach is good.

@kayrus if you prefer, and as we discussed above, we could introduce a force_delete flag on the resource, and only when it is set to true the delete function will also remove the association.

Let's keep it as is for now since we have clarified most of the corner cases. If this issue is raised again, we will consider what actions we can take to address it.

kayrus

LGTM. Thanks for your patience.

Koodt · 2023-06-26T13:28:25Z

@kayrus what about close of the conversations?
Can you resolve them? All of them are outdated and was done.
Thank you!

kayrus · 2023-06-26T18:04:59Z

To be honest I'd prefer to keep them open :) It's useful especially during the search on the page, otherwise it's hard to find the needed word or sentence.
@nikParasyr please update your review verdict and feel free to merge if you're ok.

nikParasyr

LGTM. @Koodt thanks for you work and patience. you can proceed with the fw_v2 group resource and data source PRs (seperate ones)

[FWaaS_V2]: Add policies

11a7022

Add data_source_openstack_fw_policy_v2 with tests. Add import_openstack_fw_policy_v2_test. Add resource_openstack_fw_policy_v2 with tests. Add policy_v2 docs. Add policy_v2 to sources maps.

kayrus reviewed Jun 20, 2023

View reviewed changes

nikParasyr requested changes Jun 20, 2023

View reviewed changes

kayrus reviewed Jun 20, 2023

View reviewed changes

Koodt added 2 commits June 21, 2023 11:21

Add testAccPreCheckFW in datasources, TestAccFWPolicyV2_rulesOrder, T…

721747e

…estAccFWPolicyV2_shared, TestAccFWRuleV2_shared, fix indents, remove unused value_specs

Add missing update opts

32d338e

kayrus reviewed Jun 21, 2023

View reviewed changes

nikParasyr reviewed Jun 21, 2023

View reviewed changes

website/docs/r/fw_policy_v2.html.markdown Outdated Show resolved Hide resolved

kayrus reviewed Jun 21, 2023

View reviewed changes

Add ErrDefault404 to policy deletion, fix datasource panic, review

b8b5a12

Koodt marked this pull request as ready for review June 21, 2023 09:56

Add missing step to policy datasource test

dad8d85

kayrus reviewed Jun 21, 2023

View reviewed changes

openstack/resource_openstack_fw_policy_v2.go Show resolved Hide resolved

kayrus reviewed Jun 21, 2023

View reviewed changes

openstack/resource_openstack_fw_policy_v2_test.go Outdated Show resolved Hide resolved

Review policy tests, revert policies.RemoveRule to rule deletion

554328a

kayrus reviewed Jun 22, 2023

View reviewed changes

Small review

7a2917c

kayrus approved these changes Jun 26, 2023

View reviewed changes

nikParasyr approved these changes Jun 27, 2023

View reviewed changes

nikParasyr merged commit 33459b9 into terraform-provider-openstack:main Jun 27, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FWaaS_V2]: Add fw_policy_v2 #1584

[FWaaS_V2]: Add fw_policy_v2 #1584

Koodt commented Jun 20, 2023

kayrus Jun 20, 2023

Koodt Jun 20, 2023

Koodt Jun 21, 2023

Koodt commented Jun 20, 2023

nikParasyr commented Jun 20, 2023

kayrus Jun 20, 2023 •

edited

Koodt Jun 21, 2023

kayrus Jun 21, 2023 •

edited

Koodt Jun 21, 2023

Koodt Jun 21, 2023

kayrus Jun 21, 2023

Koodt Jun 21, 2023 •

edited

kayrus Jun 21, 2023 •

edited

Koodt Jun 21, 2023 •

edited

Koodt commented Jun 21, 2023 •

edited

kayrus left a comment

kayrus Jun 22, 2023

Koodt Jun 22, 2023

kayrus Jun 22, 2023

kayrus Jun 22, 2023 •

edited

kayrus Jun 22, 2023 •

edited

Koodt Jun 22, 2023

kayrus Jun 22, 2023

nikParasyr Jun 22, 2023

kayrus Jun 26, 2023

Koodt Jun 26, 2023 •

edited

Koodt Jun 26, 2023

nikParasyr Jun 26, 2023

kayrus Jun 26, 2023

kayrus left a comment

Koodt commented Jun 26, 2023 •

edited

kayrus commented Jun 26, 2023

nikParasyr left a comment

	return diag.FromErr(err)
	return diag.Errorf("Unable to list openstack_fw_policy_v2 policies: %s", err)

[FWaaS_V2]: Add fw_policy_v2 #1584

[FWaaS_V2]: Add fw_policy_v2 #1584

Conversation

Koodt commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koodt commented Jun 20, 2023

nikParasyr commented Jun 20, 2023

kayrus Jun 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayrus Jun 21, 2023 • edited

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

Koodt Jun 21, 2023 • edited

Choose a reason for hiding this comment

kayrus Jun 21, 2023 • edited

Choose a reason for hiding this comment

Koodt Jun 21, 2023 • edited

Choose a reason for hiding this comment

Koodt commented Jun 21, 2023 • edited

kayrus left a 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

kayrus Jun 22, 2023 • edited

Choose a reason for hiding this comment

kayrus Jun 22, 2023 • edited

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

Koodt Jun 26, 2023 • edited

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

kayrus left a comment

Choose a reason for hiding this comment

Koodt commented Jun 26, 2023 • edited

kayrus commented Jun 26, 2023

nikParasyr left a comment

Choose a reason for hiding this comment

kayrus Jun 20, 2023 •

edited

kayrus Jun 21, 2023 •

edited

Koodt Jun 21, 2023 •

edited

kayrus Jun 21, 2023 •

edited

Koodt Jun 21, 2023 •

edited

Koodt commented Jun 21, 2023 •

edited

kayrus Jun 22, 2023 •

edited

kayrus Jun 22, 2023 •

edited

Koodt Jun 26, 2023 •

edited

Koodt commented Jun 26, 2023 •

edited