added new attribite load_balancer_security_group #1009
Conversation
…mples/resources/stackit_loadbalancer/resource.tf
|
Gonna add the acceptance tests as well |
bm-stackit
force-pushed
the
bm-terraform-provider-stackit
branch
from
794612c to
83d2356
Compare
bm-stackit
force-pushed
the
bm-terraform-provider-stackit
branch
from
83d2356 to
81ec910
Compare
| Description: descriptions["security_group_id"], | ||
| Computed: true, | ||
| }, | ||
| "load_balancer_security_group_id": schema.StringAttribute{ |
There was a problem hiding this comment.
So you came here 2 weeks ago and introduced the security_group field to this resource (#986). Now you want to introduce a second one.
I don't quite get it yet. What's the other security group id for now?
There was a problem hiding this comment.
Since I took this one over from a colleague that is no longer in our team we realized a bit too late, that he missed adding the other crucial security group attribute that is on the loadbalancer vm itself which is needed to do the actual routing. The Load_balancer_security_group_id is the security group of the LB VM and the other security_group_id is the one that we create but do not assign. Load_balancer_security_group_id is being put into the remote_security_group_id of the backend security group which in return allows communication of the LB and the Backend target.
There was a problem hiding this comment.
You were so speeding with your last PR, now we have the mess. Anyways, lets break it down:
We have the new load_balancer_security_group_id field now which relates to this field in the API docs right?
And then there is the "old" security_group_id attribute which relates to this field in the API docs, right?
There was a problem hiding this comment.
Your observations are correct. The Load_balancer_security_group_id(loadBalanerSecurityGroup) is important for loadbalancers across different Projects in 1 SNA. And the security_group_id (targetSecurityGroup) is useful for load balancers with targets in the same project but different networks.
There was a problem hiding this comment.
So I would say the security_group_id field be named target_security_group_id field instead. Which isn't possible now that easily because we have a deprecation time of 6 months...
There was a problem hiding this comment.
That is not a problem, leave it as is.
There was a problem hiding this comment.
That is not a problem, leave it as is.
Maybe for you but I care about our users. IMO the security_group_id field must be deprecated and a new field target_security_group_id should be added.
|
Any updates on this? |
Description
The attribute of the actual loadbalancer security group wasnt in the provider yet and is now added.
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)