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

Properly handle uuid and entry-style list resources #126

Merged
merged 15 commits into from
Aug 14, 2024

Conversation

kklimonda-cl
Copy link
Contributor

@kklimonda-cl kklimonda-cl commented Aug 6, 2024

This PR adds support for resource lists, both uuid-style lists (panos_security_policy, pan_security_policy_rules) and entry-style lists (panos_address_objects).

Current status:

  • structures are generated
  • code to copy between sdk and terraform is generated
  • we handle CRUD operations for all lists

What's missing:

  • position attribute is missing for uuid-style non-exhaustive resources like panos_security_policy_rules

Improvements:
I think core logic of create, update, read and delete can be moved into a generic structure - doing this would make it possible to properly test it with a mock client and specifier. Usage from generated code would be a bit like that:

var stateEntries, planEntries []*security.Entry
var client pangoutil.Client
svc := security.NewService(o.client)
specifier, _, err := security.Versioning(r.client.Versioning())

sdkManager := manager.NewSDKManager[security.Entry, security.Location](specifier, client, svc)
updatedEntries := manager.UpdateEntries(stateEntries, planEntries)

Bugs:

  • update function tries really hard to limit number of modifications and properly handle renames - this could still have some edge cases, but is hard to test
  • There is something wrong between provider and SDK when calling svc.MoveGroup() - the first call seems to be passing []Entry list in an expected order, but the result on PAN-OS is not correct (but changed). After update state has the expected order of the entries, next Read() logs in Debug that list from the server has different order, and a new plan is generated. The new plan calls svc.MoveGroup() with the same []Entry list as before, and the order after this call is correct.

@kklimonda-cl kklimonda-cl changed the base branch from main to feat-render-resource-second-part August 6, 2024 15:43
@kklimonda-cl kklimonda-cl force-pushed the feat-security-group-rules-plural branch 3 times, most recently from 9fda4e7 to 9cd495e Compare August 8, 2024 06:04
@kklimonda-cl kklimonda-cl marked this pull request as draft August 8, 2024 06:05
@migara migara force-pushed the feat-render-resource-second-part branch from e84eeeb to aea8772 Compare August 8, 2024 12:53
Base automatically changed from feat-render-resource-second-part to main August 8, 2024 12:53
@kklimonda-cl kklimonda-cl force-pushed the feat-security-group-rules-plural branch from c0695f5 to dede5f1 Compare August 8, 2024 15:02
@kklimonda-cl kklimonda-cl force-pushed the feat-security-group-rules-plural branch 2 times, most recently from 0725782 to 6ab060d Compare August 8, 2024 16:50
@kklimonda-cl kklimonda-cl force-pushed the feat-security-group-rules-plural branch from 90c9c52 to a086742 Compare August 9, 2024 09:34
@kklimonda-cl kklimonda-cl force-pushed the feat-security-group-rules-plural branch from a086742 to ba836d2 Compare August 9, 2024 09:37
@kklimonda-cl kklimonda-cl changed the title Manage UUID-style resources as lists in terraform Properly handle uuid and entry-style list resources Aug 9, 2024
@kklimonda-cl kklimonda-cl force-pushed the feat-security-group-rules-plural branch from c086fdb to 3c92641 Compare August 9, 2024 13:40
@kklimonda-cl kklimonda-cl marked this pull request as ready for review August 14, 2024 10:17
@kklimonda-cl kklimonda-cl merged commit 7d728a7 into main Aug 14, 2024
5 of 6 checks passed
@kklimonda-cl kklimonda-cl deleted the feat-security-group-rules-plural branch August 14, 2024 10:17
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