LoadBalancer Rules support #1016
Conversation
|
Hi, @tpolich I found a strange behavior with 'session_affinity_ttl' - when I don't set it at all, it should be '<nil>' but It converts to "0" somehow and I always have one more Override rule with 'session_affinity_ttl' = "0". From apply logs I see: (yes, I built your change with version v2.19.2 for testing purpose) Also, plan logs shows me nothing: but |
|
Ahh thank you @Nmishin yeah that is not the correct behavior let me see about fixing that |
tpolich
force-pushed
the
lb-rules-terraform-support
branch
from
4734196 to
52da873
Compare
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Computed: true, | ||
| Default: nil, |
There was a problem hiding this comment.
are these Defaults all necessary? this one in particular might be problematic as the zero value for int is 0, notnil.
| "condition": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Default: "", |
There was a problem hiding this comment.
we don't need to be explicit about empty strings here; it will also throw off d.GetOkExists checks if we use them as we're setting a value.
| Default: "", |
| Default: false, | ||
| }, | ||
|
|
||
| "overrides_tf": { |
There was a problem hiding this comment.
any reason for naming this differently to the API response? not having a 1:1 mapping with the API will make more work in the cf-terraforming tool.
| "steering_policy": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Default: nil, |
There was a problem hiding this comment.
again, you probably don't want a nil here given that you're expecting a string and you already have a validator defined.
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Default: nil, | ||
| ValidateFunc: validation.StringLenBetween(1, 32), |
There was a problem hiding this comment.
i'm not sure i'd bother with validating this is an ID but if you do, you would want it to be validation.StringLenBetween(32, 32) otherwise "abcd1234" will be valid anyway.
| if err != nil { | ||
| return fmt.Errorf("failed to flatten rules: %s", err) | ||
| } | ||
| if err := d.Set("rules", fr); err != nil { |
There was a problem hiding this comment.
✅ nice check! ensuring the complex type can't fail to write out
| bb, err := json.Marshal(rules) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var rout []interface{} | ||
| if err := json.Unmarshal(bb, &rout); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
i don't quite understand why we are marshaling and unmarshaling here; can't we just iterate over the rules and add them to the return value? similar to something like https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/cloudflare/resource_cloudflare_api_token.go#L179 or another existing resource?
| @@ -434,6 +611,158 @@ func resourceCloudflareLoadBalancerImport(d *schema.ResourceData, meta interface | |||
| return []*schema.ResourceData{d}, nil | |||
| } | |||
|
|
|||
| func flattenRules(rules []*cloudflare.LoadBalancerRule) (interface{}, error) { | |||
|
|
|||
| return nil, err | ||
| } | ||
|
|
||
| // terraform doesn't support maps of maps so lets |
There was a problem hiding this comment.
generally, if you want a map of maps, you probably really want a list of maps instead 😃
| } | ||
|
|
||
| func expandRules(rdata interface{}) ([]*cloudflare.LoadBalancerRule, error) { | ||
|
|
There was a problem hiding this comment.
Thanks for putting this together @tpolich 🙇♂️ It's appreciated!
I have some inline comments mainly targetting the schema as there are some conditions (such as Default and Computed) which I think we may be using unnecessarily and will cause problems later down the track with things like d.GetOkExists and checking whether or not a value has been set by the operator.
The other concern I raised was around the schema attribute naming. Ideally, this is a 1:1 mapping to the API response to avoid any manual mapping in tools like cf-terraforming however we have at least one that violates this and I can't quite see why we'd need to be doing that from what we have. Perhaps I'm missing some context?
I didn't run the tests yet as I'll save those for after we discuss any changes just in case something changes following those 😃
tpolich
force-pushed
the
lb-rules-terraform-support
branch
2 times, most recently
from
20d8dc1 to
8261ce5
Compare
tpolich
force-pushed
the
lb-rules-terraform-support
branch
from
8261ce5 to
d4f5a96
Compare
|
integration CI is also happy |
|
@tpolich, @jacobbednarz guys, one more question - for rule only one field "name" marked as "Required: true" but this is not true, we must have overrides or terminate as well: Do we want to add logic for checking this scenario? |
|
Not really. The resource is for load balancers in general and rules is
an extension of the base support. Trying to bake in validation for
conditional fields makes Create/Update/Read operations overly complex.
Instead, we have been relying on documentation and the service
validation to avoid as many issues as possible.
|
|
@tpolich you are a legend, thank you! @jacobbednarz do you have an idea of when this can be released, please? |
Awesome lets merge it then! Thank you for fixing the doc issue for me too you've been a really big help. |
|
congrats! this looks great ⭐ |
Support for new LoadBalancing Rules feature (currently in beta)