use labels and env vars to set EIP metro or facility, else error #222
Conversation
deitch
force-pushed
the
remove-metadata
branch
3 times, most recently
from
307237d
to
0346b5e
Compare
deitch
force-pushed
the
remove-metadata
branch
from
0346b5e
to
1326e90
Compare
|
Done. It now determines where to place the EIP as follows:
The annotation follows the pattern of all other annotations we use: Ready for another full review @displague and @detiber |
|
I'm in favor of merging this to get the agreed changes in the project. We have not updated the Helm chart in this PR (https://github.com/equinix/cloud-provider-equinix-metal/tree/master/deploy/chart). Do you think the new location options should be required? I worry about deploying this change in a release with "Step 5 Fail" because anyone upgrading to this image, via Perhaps we can follow up this PR with one that adds a default behavior to check EM metadata (or API data). This could then be disabled in use-cases where it does not make sense. Upgrades would not break and the Helm chart would not need to require a location parameter. What do you think? |
Yes, but it can afford a separate PR. We should check all of the deployment stuff.
Agreed, but that is what semver is for. Someone who uses
Location parameter only is required if you do not have it on the service annotations. I am comfortable with it being this way |
| // 1. if metro is set globally, use it; else | ||
| // 2. if facility is set globally, use it; else | ||
| // 3. if Service.Metadata.Labels["topology.kubernetes.io/region"] is set, use it; else | ||
| // 4. if Service.Metadata.Labels["topology.kubernetes.io/zone"] is set, use it; else |
There was a problem hiding this comment.
A user is able to offer conflicting annotations vs other settings.
The rules spell out how this will be processed so I can't complain about how conflicts are handled.
When both Region and Zone are set, especially when the zone resides in the region, I think it's reasonable to assume the more granular, Zone, will be used. This is not the case today. Based on how EM EIPs work, I think users supplying both settings will be better served with the Regional EIP (Metro based), so I'm fine with this staying as-is.
There was a problem hiding this comment.
I agree with you on all of the above. There can be conflicts, so we spell out exactly what the behaviour is.
When both Region and Zone are set, especially when the zone resides in the region, I think it's reasonable to assume the more granular, Zone, will be used. This is not the case today. Based on how EM EIPs work, I think users supplying both settings will be better served with the Regional EIP (Metro based), so I'm fine with this staying as-is.
I agree here as well. The right behaviour should be (everywhere, not just here), if you supplied a more granular request for anything, I will respect that.
The code we actually use shows:
// create a request
req := packngo.IPReservationRequest{
Type: "public_ipv4",
Quantity: 1,
Description: ccmIPDescription,
Tags: []string{
emTag,
svcTag,
clsTag,
},
FailOnApprovalRequired: true,
}
if metro != "" {
req.Metro = &metro
}
if facility != "" {
req.Facility = &facility
}So we basically add both if set, and let the API deal with it.
| @@ -388,6 +393,12 @@ func (l *loadBalancers) addService(ctx context.Context, svc *v1.Service, ips []p | |||
| // if we did not find an IP reserved, create a request | |||
| klog.V(2).Infof("no IP assignment found for %s, requesting", svcName) | |||
| // create a request | |||
| // our logic as to where to create the IP: | |||
| // 1. if metro is set globally, use it; else | |||
There was a problem hiding this comment.
Perhaps the more granular setting, the service annotation, should be consulted first with the "default" coming from the global settings?
There was a problem hiding this comment.
Administrators are more likely to have access to the CPEM runtime parameters.
Users are more likely to have access to the service annotation.
There was a problem hiding this comment.
If the annotated locations need to be restricted, we could introduce another global setting to define the permitted values.
There was a problem hiding this comment.
You mean to use the env var / CLI flag as a fallback rather than override? I can see the use case, but that is not how it has been used in the past. In any case, the only ones ever likely to use env var on the actual CPEM will be administrators. If they set it, they really mean to override.
I don't mind considering having another env var for fallback, but this one historically has been the explicit override; I would like to leave it as is.
There was a problem hiding this comment.
The flags/env vars really are just meant as controlling overrides for development time. They don't have a good production purpose.
Signed-off-by: Avi Deitcher <avi@deitcher.net>
deitch
force-pushed
the
remove-metadata
branch
from
1326e90
to
06414e3
Compare
|
Rebased, letting CI run |
|
Needs a new approval after rebase @displague |
Fixes #183
Completely eliminates the usage of EQXM metadata in CPEM. Instead, follows similar logic suggested in this comment by @detiber:
Service.Spec.LoadBalancerIPis set, we use it, nothing more to do; if not...Service.Metadata.Annotations["topology.kubernetes.io/region"]is set, get an EIP for the given metro; if not...Service.Metadata.Annotations["topology.kubernetes.io/zone"]is set, get an EIP for the given facility; if not...Does not address the "global EIP" option; we can add it later.
Updated README as well, of course