-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency #1510
Conversation
…me being called Signed-off-by: Alexandros Kazantzidis <8426315+akazantzidis@users.noreply.github.com>
…m state. Signed-off-by: Alexandros Kazantzidis <8426315+akazantzidis@users.noreply.github.com>
Signed-off-by: Alexandros Kazantzidis <8426315+akazantzidis@users.noreply.github.com>
d63e2d0
to
1a01a96
Compare
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 @akazantzidis 👋🏼
Thanks for the contribution! I have a question about the code, could you take a look? Thanks!
func RulesRead(ctx context.Context, api firewall.Rule, d *schema.ResourceData) diag.Diagnostics { | ||
var diags diag.Diagnostics | ||
|
||
readRule := func(pos int, ruleMap map[string]interface{}) error { |
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.
Hm.. I didn't quite get this change. The RulesRead
supposed to read rules from PVE API. Now it reads back only from the state 🤔
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, sure let me explain:
rulesCreate
function was resetting the rule state before calling RulesRead
. This was redundant because the rules were already created in specific order in one take within rulesCreate
and represented correctly in d
. However, the correct pos
value for each rule was not being set at that time.
By removing the state reset at the end of rulesCreate
, rules are represented in the correct (in terms of user configuration) order.
Calling RulesRead
we can iterate over the rules and for each rule id increment the pos
value for each rule to set the actual rule pos
.
This sets the pos
attribute, and the in the correct order (according to user specified configuration),without reading them from the API.
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.
Although this proposal does not handle changes outside of Terraform, does stabilizes the terraform plan / apply / delete.
Next step though is the rule comparison in terms of API/state-configuration ,so outside changes to be reverted and to be ensured the terraform configuration.
Co-authored-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Signed-off-by: Alexandros Kazantzidis <8426315+akazantzidis@users.noreply.github.com>
@akazantzidis thank you for working on this. I tested your changes and they do not solve the issue described in bug report (yet?). I managed to get it working for almost all the required attributes, except for "interface", in this commit: The issue with "interface" is that its not enough to send it with an empty value but instead "delete=iface" is required. |
Moving it to in-progress as working in a different approach |
Contributor's Note
/docs
for any user-facing features or additions./fwprovider/tests
for any new or updated resources / data sources.make example
to verify that the change works as expected.Proof of Work
This PR is handling the below 2 issues:
Issue 1
:Rule resources produce the same rule ID:
Example rules:
Provider's Output:
Issue 2
:Rule checking/state logic is inconsistent, which produces the below issues:
... no rule at position X ...
This PR fixes the rule unique id creation and the state/rule consistency and as result no more errors during rule delete/update operations.
FIXES #1504
Community Note