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

Add vcd_rde_behavior_invocation data source to invoke behaviors #1117

Merged
merged 29 commits into from
Oct 11, 2023

Conversation

adambarreiro
Copy link
Collaborator

@adambarreiro adambarreiro commented Sep 7, 2023

Overview

This PR adds a new data source to invoke RDE Interface/Type Behaviors.

Detailed description

We onboarded RDE Interface/Type Behaviors in v3.10.0 of the VCD Provider. For simplicity, let's say that a Behavior is an equivalent term for "function", so we're talking about invoking/calling functions, that are specified in RDE Interfaces/Types and invoked on RDEs. One could invoke them programatically using the Go SDK, but users may be in the need of invoking them directly using Terraform.

For that purpose, this PR onboards a new data source vcd_rde_behavior_invocation that behaves similarly to the HTTP data source created by HashiCorp, in the sense that it is an imperative concept (calling a function, calling a HTTP method) introduced in a declarative toolset.

That's why it was created as a data source. While it would make more sense to have it as a resource, as conceptually this would fit more with the term "invocation" (as we know, a function invocation can mutate/write things), because invoking Behaviors can modify RDEs. But the implementation of a resource for such a "imperative" concept is tricky. You can check the section "Caveats of the resource implementation" to know more.

The data source is pretty trivial, it only has a Read operation that invokes the Behavior on every refresh, with the given arguments/metadata (if given), and writes the result of the invocation in a Computed field. This field can be pretty much anything, as the invocation returns the raw result (it could be a JSON, plain text, etc).

Caveats of the resource implementation

As we have Create, Update, Read and Delete, we would need a required argument invoke_on_every_refresh to decide whether we want to invoke the Behavior on every Read, or only during Create/Update. This has also an odd drawback that could be undesired, a Refresh and an Update would invoke the Behavior twice. We could make Update a no-op, but we need to be able to update arguments/metadata for the invocation. If we make Update a no-nop, we need to make everything ForceNew: true, but this would give us the same situation.

Also, Delete would be a no-op in any case.

While resource fits more with the concept of "invoking Behaviors can mutate RDEs", in reality the implementation is tricky and would be more complicated for end users. Also, we have the precedent of the HTTP Provider giving the http data source, despite one can do PUT/POST/DELETE calls with it. Probably they faced the same situation, which gets trivially solved with a data source.

Other things included in this PR

  • Improvement: The field rde_type_id from resource vcd_rde does not have ForceNew: true anymore. This was a bit inconvenient when one wanted to upgrade an RDE from one RDE Type to a newer version of the same, as it forced its deletion and re-creation, which may not suit all users needs. Now the RDE Type can be updated without any previous deletion, allowing smoother RDE Type version upgrades. Note: As stated in the updated documentation, one should only upgrade the RDE Type version. VCD API doesn't allow updating to a different type or downgrading the version.

  • Fixes a bug that made impossible to delete vcd_rde_type_behavior_acl resources when the Access Level is the last one in the Behavior. This fix is automatically done taking the changes in Improve RDE related methods go-vcloud-director#615. The bug happened because the Provider calls SetBehaviorAccessControls with a nil slice of Access Levels, and the method was not considering this case as special.

  • Fixes the resource vcd_rde_type_behavior_acl to avoid race conditions when creating, updating or deleting more than one Access Level. This is fixed by using a mutex whenever SetBehaviorAccessControls method is used. The problem with SetBehaviorAccessControls is that, as the name implies, sets whatever Access Levels are specified in the input, so two calls at the same time can override each other.

#
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro self-assigned this Sep 7, 2023
abarreiro added 8 commits September 7, 2023 17:12
Signed-off-by: abarreiro <[email protected]>
WIP
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro changed the title Add vcd_rde_behavior_invocation resource to invoke behaviors Add vcd_rde_behavior_invocation data source to invoke behaviors Sep 12, 2023
abarreiro added 7 commits September 12, 2023 10:52
Signed-off-by: abarreiro <[email protected]>
fmt
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro marked this pull request as ready for review September 12, 2023 09:57
abarreiro added 2 commits September 12, 2023 12:09
fmt
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
abarreiro added 2 commits September 19, 2023 09:07
Signed-off-by: abarreiro <[email protected]>
vcd/datasource_vcd_rde_behavior_invocation.go Show resolved Hide resolved
vcd/resource_vcd_rde_test.go Show resolved Hide resolved
vcd/resource_vcd_rde_type_behavior_acl.go Outdated Show resolved Hide resolved
website/docs/r/rde.html.markdown Outdated Show resolved Hide resolved
Signed-off-by: abarreiro <[email protected]>
abarreiro added 2 commits September 26, 2023 10:15
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
@adezxc
Copy link

adezxc commented Sep 26, 2023

One thing that comes to mind: could we have the invoke_on_every_refresh as an argument for the datasource instead? I'm not sure what could happen exactly when a behaviour is invoked, but just as a thought so users have more control instead of having to remove/add datasources.

Signed-off-by: abarreiro <[email protected]>
@adambarreiro
Copy link
Collaborator Author

adambarreiro commented Sep 26, 2023

One thing that comes to mind: could we have the invoke_on_every_refresh as an argument for the datasource instead? I'm not sure what could happen exactly when a behaviour is invoked, but just as a thought so users have more control instead of having to remove/add datasources.

It could be a good option indeed, and would be more optimal as one could use a variable to switch it on/off (more CI/CD friendly than adding/removing code). Also the caveat mentioned in the docs could be controlled in a better way (instead of using -refresh=false):

## Known caveats

* Executing a `terraform destroy` will cause a refresh that will invoke the Behavior. To avoid this situation, you can
  remove the `vcd_rde_behavior_invocation` data source from the HCL configuration before running the destroy operation,
  or use the `-refresh=false` flag (this will skip the refresh for all resources).

Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Copy link

@adezxc adezxc left a comment

Choose a reason for hiding this comment

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

Successfully tested on 10.5.0, thanks for the feature!

website/docs/d/rde_behavior_invocation.html.markdown Outdated Show resolved Hide resolved
Signed-off-by: abarreiro <[email protected]>
vcd/resource_vcd_rde.go Show resolved Hide resolved
Signed-off-by: abarreiro <[email protected]>
vcd/datasource_vcd_rde_behavior_invocation.go Outdated Show resolved Hide resolved
vcd/datasource_vcd_rde_behavior_invocation.go Outdated Show resolved Hide resolved
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro merged commit 8152ce6 into vmware:main Oct 11, 2023
3 checks passed
@adambarreiro adambarreiro deleted the add-rde-behavior-invocation branch October 11, 2023 15:10
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.

5 participants