proxy: filter/delegate EndpointSlices as well as Endpoints #296
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
| } else { | ||
| ref.activePolicy = nil | ||
| if len(ns.firewalls) == 1 { | ||
| for uid := range ns.firewalls { |
There was a problem hiding this comment.
if len == 1 it must be ns.firewalls[0], no?
There was a problem hiding this comment.
ns.firewalls is a map; if there is only 1 value, we want the key of that value.
Yeah, this code is really weird, but it was already like that before; this code is just being moved around. But yeah, it should probably be rewritten to be less weird.
danwinship
force-pushed
the
endpointslices
branch
3 times, most recently
from
e9b0711
to
67cc9ec
Compare
danwinship
force-pushed
the
endpointslices
branch
3 times, most recently
from
9a6f6e0
to
87eba6c
Compare
|
/retest |
danwinship
force-pushed
the
endpointslices
branch
from
87eba6c
to
8e5c1db
Compare
|
/retest |
|
/assign @aojea |
| } | ||
| ns.firewalls[policy.UID] = firewall | ||
| } else { | ||
| delete(ns.firewalls, policy.UID) |
There was a problem hiding this comment.
While you're at it, do you want to move the key from UID to NamespacedName? If an object is deleted-and-recreated with the same name, we could leak objects.
There was a problem hiding this comment.
I agree this code is weird - if you don't want to touch it, that's OK.
| // sent to both proxies (so the unidling proxy socket can redirect its connection to | ||
| // the correct place). An Unidling Service becomes Not Idled if its Endpoints are | ||
| // deleted, or else the next time it receives an Endpoints event more than 1 minute | ||
| // after becoming Unidling. (Alternatively it could also become Idled again.) |
There was a problem hiding this comment.
This timeline is different from the previous behavior - why did you do this? Wouldn't a service transition from Unidling to Not Idled iff it has non-empty endpoints? I'm assuming it's because a service can't possible unidle for more than a minute, but does that actually help?
Also, why would deleting Endpoints make a service Not Idled? Just because it means the service is going away?
There was a problem hiding this comment.
The old behavior was that we always sent all Endpoints events to the unidling proxy. The new behavior is that we're trying to not do that, because we have to synthesize fake Endpoints objects from EndpointSlices, and we don't want to have to do that for every EndpointSlice and double our memory usage (especially since almost all of the unidling proxy Endpoints updates are pointless; the unidling proxy has no need to ever know the Endpoints of Services that never get idled).
So the goal is "figure out if the unidling proxy actually cares about the endpoints for this service or not, and don't bother synthesizing an Endpoints from the EndpointSlice if it doesn't actually need to know".
When a Service has just un-idled and the UnidlerSocket still has pending connections to proxy, we need to send it Endpoints updates so it can know where to proxy the connections to. Once it has connected all of its pending connections, then it doesn't need to know about any further Endpoints events. It's likely this will happen within a few seconds of it receiving the first non-empty Endpoints update.
Wouldn't a service transition from Unidling to Not Idled iff it has non-empty endpoints?
If the Service is backed by a Deployment/ReplicaSet/etc, then when it un-idles, we will first get an Endpoints update saying it has 1 endpoint, quickly followed by another Endpoints update saying it has 2 endpoints, etc. We could decide we were only going to send the unidling proxy the first update (so that all connections captured by the unidling proxy would get proxied to the first endpoint to come up) but it seemed better to send it the others too?
There was a problem hiding this comment.
Also, why would deleting Endpoints make a service Not Idled? Just because it means the service is going away?
If it has no Endpoints then it can't be Unidling (because the unidling proxy has nowhere to redirect pending connections to) and it's not Idled (because that is defined to have an empty Endpoints), so therefore it's Not Idled.
Also, yes, it's going away.
When an Endpoints is modified and becomes blocked-by-EgressNetworkPolicy as a result, we send an OnEndpointsDelete to the base proxy. But we were previously passing the _updated_ Endpoints object in the delete event, when it logically makes more sense to pass the _original_ Endpoints object, since that's what you're deleting from the base proxy.
proxy_test.go still tests egress endpoint filtering and hybridproxier_test.go still tests idling/unidling, but both now use an OsdnProxy wrapping a HybridProxier wrapping two testProxies.
Take advantage of NamespacedName rather than manually generating namespaced names everywhere. Don't include "hybrid proxy:" in log messages since they'll already include "hybridproxier.go:" Log at V(1) rather than V(6) when services are idled or unidled, since that's actually interesting.
Remove an unused struct field, and move another one around to keep the "passed in at construct time" stuff separate from the "current state" stuff.
We don't need to cache every Endpoints; it's only the ones that have endpoints outside the cluster network that need to be kept track of in case they get blocked by an EgressNetworkPolicy later.
Proxies have to be able to deal with receiving Endpoints and Service events out of order, which means they have to be able to handle keeping track of Endpoints for Services that they don't know about. So rather than having complicated rules about when we should and shouldn't pass Endpoints to the main proxy based on whether the Service is idled or not, just always pass all Endpoints events to it.
Rather than keeping two maps-of-bool with separate mutexes that must be carefully coordinated, keep a single map-of-struct and a single mutex.
Current versions of "oc" annotation both the Endpoints and the Service when a service is idled. Track the annotation on the Service instead of the Endpoints (though we still need to track whether the Endpoints are empty as well).
The unidling proxy only needs to know a Service's Endpoints when the Service is in the process of unidling (so that the unidlersocket knows where to redirect the connection to). There's no point in making it keep track of the Endpoints of non-idled Services.
Make OsdnProxy and HybridProxier work with either Endpoints or EndpointSlices. When using EndpointSlices, create fake Endpoints objects to pass to the unidling proxier when needed.
danwinship
force-pushed
the
endpointslices
branch
from
8e5c1db
to
8c452cf
Compare
|
/lgtm |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, squeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Updates
OsdnProxy/HybridProxierto handle both Endpoints and EndpointSlices, also adding unit tests and doing a bunch of refactoring along the way.The sdn-level code is compatible with
EndpointSliceProxyingbeing either enabled or disabled, and also compatible with EndpointSlice support being added to the userspace proxy in the future. (In fact, this PR alone doesn't actually make openshift-sdn use EndpointSlices, because CNO is currently always passing--feature-gates=EndpointSliceProxying=false. But once this merges that can be reverted.(for 4.9)
/cc @aojea