Loosen requirement for static assignment with IPAddressPool #31
Conversation
|
/lgtm |
| @@ -13,5 +13,5 @@ spec: | |||
| {{ .spec.addresses | toYaml | indent 2}} | |||
| {{ end }} | |||
| #- 3.3.3.0/24 | |||
| autoAssign: true | |||
| autoAssign: false | |||
There was a problem hiding this comment.
the justification is that if IPAddressPool contains multiple IP addresses then static assignments will ensure that CNF-A always has IP address X.X.X.X(unless IPAddressPool is solely dedicated to the CNF-A)
There was a problem hiding this comment.
if that's a requirement, the right way to address it is to use the service / namespace selectors in the ip address pools (assuming each CNF will have its own namespace, but that can be done per service too).
This guarantees each cnf will have the ip the admin means to assign to them, otherwise they can steal the ip from any pool.
There was a problem hiding this comment.
good points @fedepaol. And while without service/namespace selectors IP address from any IPAddressPool could be selected, imho autoAssign set to false is a good safety net from assigning not expected IP addresses.
There was a problem hiding this comment.
yes but here we are advocating for static IPs requested by the CNF always, which feels like the exception to me, not the norm. And bear in mind, if you have a pool of ips with autoAssign = false and no selector, any CNF can get any IP they want from that pool, which is not really a safety net.
If you ask me, the proper way to have a good ux and ensure the right tenant gets the right ip(s), this is with selectors.
There was a problem hiding this comment.
@fedepaol , I added serviceAllocation stanza to the CR, wdyt?
There was a problem hiding this comment.
As always, either approaches have their advantages and disadvantages. This file defines a mandatory CR configuration, and non-compliance will lead to RDS compliance failure. However, I don’t believe we need to be that strict. There are undoubtedly Telco customers using autoAssign: true without any wrongdoing.
I'm happy to continue discussing this.
There was a problem hiding this comment.
@cgoncalves that makes sense, what about usage of serviceAllocation config?
There was a problem hiding this comment.
Same thing. We would still be enforcing the use of serviceAllocation.
I'd like to see a stronger argument in favor of enforcing/requiring it before it is added to RDS.
There was a problem hiding this comment.
Thanks, that's what I thought. Updated the PR
openshift-merge-robot
added
the
needs-rebase
yprokule
force-pushed
the
metallb-static-assignment
branch
from
16eedb9 to
812c0ad
Compare
openshift-merge-robot
removed
the
needs-rebase
| {{ if .spec.serviceAllocation.namespaces }} | ||
| serviceAllocation: | ||
| namespaces: | ||
| {{ .spec.serviceAllocation.namespaces | toYaml | indent 4}} | ||
| {{ end }} |
There was a problem hiding this comment.
@lack could U please check if the syntax is correct here, thanks!
yprokule
force-pushed
the
metallb-static-assignment
branch
from
812c0ad to
51d372f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yprokule The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
yprokule
changed the title
|
/lgtm |
Leave IP address assignment to the operators