Longsight commented on issue #115:
URL:
https://github.com/apache/cloudstack-terraform-provider/issues/115#issuecomment-2757522110
@baltazorbest I'm not convinced that swapping the logic of that conditional
is the fix we're looking for.
I was hitting a similar issue with Network ACL rules, and after some hours
of debugging found that it was occurring _there_ because I wasn't supplying a
project ID to `cloudstack_network_acl_rule`, which meant that
`resourceCloudStackNetworkACLRuleRead` was returning early:
```go
_, count, err := cs.NetworkACL.GetNetworkACLListByID(
d.Id(),
cloudstack.WithProject(d.Get("project").(string)),
)
if err != nil {
if count == 0 {
log.Printf(
"[DEBUG] Network ACL list %s does no longer
exist", d.Id())
d.SetId("")
return nil
}
return err
}
```
```go
func (s *NetworkACLService) GetNetworkACLListByID(id string, opts
...OptionFunc) (*NetworkACLList, int, error) {
p := &ListNetworkACLListsParams{}
p.p = make(map[string]interface{})
p.p["id"] = id
for _, fn := range append(s.cs.options, opts...) {
if err := fn(s.cs, p); err != nil {
return nil, -1, err
}
}
l, err := s.ListNetworkACLLists(p)
if err != nil {
if strings.Contains(err.Error(), fmt.Sprintf(
"Invalid parameter id value=%s due to incorrect long
value format, "+
"or entity does not exist", id)) {
return nil, 0, fmt.Errorf("No match found for %s: %+v",
id, l)
}
return nil, -1, err
}
// -
// - This is where a zero-count set results in an error, which is
cascaded to the calling function
// -
if l.Count == 0 {
return nil, l.Count, fmt.Errorf("No match found for %s: %+v",
id, l)
}
if l.Count == 1 {
return l.NetworkACLLists[0], l.Count, nil
}
return nil, l.Count, fmt.Errorf("There is more then one result for
NetworkACLList UUID: %s!", id)
}
```
`GetNetworkACLListByID` _always_ returned an empty set and an error _if_ the
parent `NetworkACLList` belonged to a project, and the `project` parameter was
omitted from the resource. This is by design - project-scoped resources are not
returned by `List*` API calls _unless_ a `project` (called `projectid` in the
API) parameter is supplied, even if `listall` is `true`.
When I included the `project` parameter in the Terraform resource, the rules
were matched and created correctly.
---
In the case of `cloudstack_firewall`, the relevant read logic is:
```go
// Get all the rules from the running environment
p := cs.Firewall.NewListFirewallRulesParams()
p.SetIpaddressid(d.Id())
p.SetListall(true)
l, err := cs.Firewall.ListFirewallRules(p)
if err != nil {
return err
}
```
```go
func (s *FirewallService) ListFirewallRules(p *ListFirewallRulesParams)
(*ListFirewallRulesResponse, error) {
resp, err := s.cs.newRequest("listFirewallRules", p.toURLValues())
if err != nil {
return nil, err
}
resp, err = convertFirewallServiceResponse(resp)
if err != nil {
return nil, err
}
var r ListFirewallRulesResponse
if err := json.Unmarshal(resp, &r); err != nil {
return nil, err
}
return &r, nil
}
```
In this case, if the firewall is scoped to a project, this code will _never_
find any rules, as the `project` is never included in the API call. However,
unlike `GetNetworkACLListByID `, `ListFirewallRules` does _not_ throw an error
in the case of an empty set, so `resourceCloudStackFirewallRead` continues:
```go
// -
// ruleMap is supposed to be a map of existing rules fetched from
the API
// BUT:
// if the firewall belongs to a project, it will always be empty
// This is incorrect behaviour
// -
ruleMap := make(map[string]*cloudstack.FirewallRule, l.Count)
for _, r := range l.FirewallRules {
ruleMap[r.Id] = r
}
// Create an empty schema.Set to hold all rules
rules :=
resourceCloudStackFirewall().Schema["rule"].ZeroValue().(*schema.Set)
// Read all rules that are configured
if rs := d.Get("rule").(*schema.Set); rs.Len() > 0 {
for _, rule := range rs.List() {
// ...rule matching logic
```
Under normal circumstances, `ruleMap` is a map of the rules fetched from the
API, `rs` is a set of the rules passed by the Terraform resource, and `rules`
is the resulting intersection of the two, representing rules that exist in
Cloudstack _and_ are known to Terraform. At this point in the code, when
reading an existing rule, there are two valid scenarios:
- `managed` is `false`, and `rules` has 1 or more elements
- `managed` is `true`, and `rules` has 0 or more elements
This logic exists because a _managed_ `cloudstack_firewall` with zero rules
is a valid means of ensuring that no rules are added to the firewall by other
means until you choose to add them with Terraform.
If the firewall is scoped to a project, and `rulesMap` is empty, then the
rule matching logic will ensure that `rules` is also empty. This _should not
happen_, and the block you've identified is the code that handles this:
```go
// If we've matched any of the existing Cloudstack firewall rules to
the rules in the resource,
// include them in the resulting ResourceData:
if rules.Len() > 0 {
d.Set("rule", rules)
// If we haven't, AND this resource is NOT marked as `managed`,
unset the resource ID
// of the newly-created `cloudstack_firewall` resource, as this
should only happen IF
// the resource does not actually exist during Refresh, and should
be removed from Terraform state.
} else if !managed {
d.SetId("")
}
```
The logic here is correct - during a normal resource read, if `rules.Len()
== 0` and `!managed` then this suggests that a firewall resource that exists in
Terraform state has been destroyed in infrastructure, and should be marked as
destroyed in state.
However, since `resourceCloudStackFirewallRead` is _also_ called from
`resourceCloudStackFirewallCreate` after initial resource creation (so that
unknown values like UUID can be fetched from Cloudstack), this logic causes the
`apply` operation to fail if the firewall rule belongs to a project, as
`Create` operations should never return a resource with an empty `Id`:
```go
// ...
// The SetId method must be called with a non-empty value for the
managed
// resource instance to be properly saved into the Terraform state and
// avoid a "inconsistent result after apply" error.
// ...
Create CreateFunc
```
This is why your fix appears to work: if you change `else if !managed` to
`else if managed`, then the resource ID is no longer unset on apply even though
the Read operation found an empty ruleset, so the actual error is masked by
what looks like a successful Create that just happened to create zero rules.
What you will probably find, however, is that duplicate rules are created on
every apply, as existing rules will never be seen by the provider, so it will
think that they're always missing. The more correct fix is to include the
`projectid` parameter in the `ListFirewallRules ` API call in the first place,
so that the newly-created firewall resource is returned by Cloudstack and
recognised by the provider.
---
Since `cloudstack_firewall` is mapped to a given `ip_address_id` rather than
the `id` of the actual firewall, we cannot duplicate the `project` parameter
logic used in other resources, as we can't call
`cs.Firewall.GetFirewallRuleByID` without the firewall ID. What we can do,
however, is ensure that the `project` is included in the `ListFirewallRules`
call, by making use of `cloudstack.WithProject` directly:
```diff
--- a/cloudstack/resource_cloudstack_firewall.go
+++ b/cloudstack/resource_cloudstack_firewall.go
@@ -95,6 +95,12 @@ func resourceCloudStackFirewall() *schema.Resource {
},
},
+ "project": {
+ Type: schema.TypeString,
+ Optional: true,
+ ForceNew: true,
+ },
+
"parallelism": {
Type: schema.TypeInt,
Optional: true,
@@ -256,6 +262,12 @@ func resourceCloudStackFirewallRead(d
*schema.ResourceData, meta interface{}) er
p.SetIpaddressid(d.Id())
p.SetListall(true)
+ // Use WithProject(d.Get("project").(string)) to set the `projectid`
parameter on the API call
+ // if we've passed a `project` parameter to the resource
+ if err := cloudstack.WithProject(d.Get("project").(string))(cs, p);
err != nil {
+ return err
+ }
+
l, err := cs.Firewall.ListFirewallRules(p)
if err != nil {
return err
```
I'm not in a position to test this fix myself right now as I'm not currently
using firewall rules in my deployment, but I'm fairly confident it should work.
Side note: we have to supply the `project` resource parameter ourselves
rather than look it up from the `PublicIpAddress` referenced in
`ip_address_id` because, even though `cs.Address.GetPublicIpAddressByID`
exists, we hit the same problem of scoping - unless we already know the
`projectid` of the resource in Cloudstack, the underlying `List*` call will
fail to return the resource we're looking for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
