Add plural IP fields to EndpointSpec #3267
Conversation
tpantelis
requested review from
Oats87,
skitt,
sridhargaddam,
vthapar and
yboaron
as code owners
|
🤖 Created branch: z_pr3267/tpantelis/new_endpoint_fields |
tpantelis
force-pushed
the
new_endpoint_fields
branch
from
8758998 to
50a3b93
Compare
| return fromArrayOrString(ep.PublicIPs, ep.PublicIP) | ||
| } | ||
|
|
||
| func (ep *EndpointSpec) SetPublicIPs(ips ...string) { |
There was a problem hiding this comment.
overall code LGTM,
I have one minor comment, since some of the fields (e.g Public IP) can be discovered at different timing maybe it makes sense to replace (or add new API) SetxxxIPs with UpdatexxIP (Update should call GetxxxIPs and update it if needed ) instead of having this logic in clients side
There was a problem hiding this comment.
Sorry I'm not sure I follow what you mean...
There was a problem hiding this comment.
I'm sorry, I'll try to explain it by example ...
Assuming the following use case for dual-stack case:
A. public IPV4 address successfully discovered on t1
B. public IPV6 address only discovered on t2
I thought it might be useful to have Update API (that will add IP if it doesn't exist, we can use it in step B for example).
But on second thought since there is only single client (gateway) that will call this APIs we can have this logic at the gateway side.
There was a problem hiding this comment.
I see - there may be cases where we need to set IPs atomically or piecemeal. Either way we can add/modify the helper functions as needed. The main point of these helpers is to hide the singular fields.
There was a problem hiding this comment.
This highlights that there’s a difference between the way the data is stored and how it’s used. Externally, the API could be Set…IPv4/Set…IPv6 (with corresponding getters); that would make it explicit that the data structure only stores one IPv4 address and one IPv6 address.
(Incidentally, there’s currently nothing stopping a caller from storing two IPv4 addresses or two IPv6 addresses 😉.)
There was a problem hiding this comment.
OK I changed the getters to retrieve the singular IP for an IP family. The family is passed as a parameter as, after looking at usages, I think that's going to be more natural/easier rather than separate functions for each family. The setters add the address if not already present to ensure there's only one per family.
tpantelis
force-pushed
the
new_endpoint_fields
branch
from
50a3b93 to
8b13fb0
Compare
|
I made |
tpantelis
force-pushed
the
new_endpoint_fields
branch
from
746e81f to
43942b9
Compare
...that mirror the singular HealthCheckIP, PrivateIP and PublicIP fields to support dual-stack addresses. The singular fields are deprecated and remain to support backwards compatibility and migration with prior versions. Going forward only the plural fields will be used. Get*IP/Add*IP methods were added to EndpointSpec that handle the singular fields. On Get for IPv4, if the plural field doesn't contain an IPv4 address then retrieve the singular field. On Set for IPv4 also set the corresponding singular field. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
force-pushed
the
new_endpoint_fields
branch
from
43942b9 to
c0d875a
Compare
|
🤖 Closed branches: [z_pr3267/tpantelis/new_endpoint_fields] |
...that mirror the singular
HealthCheckIP,PrivateIPandPublicIPfields to support dual-stack addresses. The singular fields are deprecated and remain to support backwards compatibility and migration with prior versions. Going forward only the plural fields will be used. Get/Set methods were added toEndpointSpecthat handle the singular fields. On Get, if the plural field is empty then retrieve the singular field. On Set, if one of the specified IPs is IPv4 then set the corresponding singular field.