There are a number of overlapping places where the ports are compared between s.Endpoint and s.Spec.Endpoint, and possibly updated:
- If a service needs allocation,
reconcilePortConfigs returns a list of port configs, then serviceAllocatePorts updates the endpoint.
- If the service did not need allocation,
portsAllocatedInHostPublishMode checks if host-published ports need to be updated.
- If
portsAllocatedInHostPublishMode returned true, then updatePortsInHostPublishMode has similar logic to update these host-published ports. It doesn't reuse any of the code or results from portsAllocatedInHostPublishMode.
I find these different functions confusing, and we've had bugs before when they behaved inconsistently or weren't correct. It's especially problematic because the host-published-port-specific functions have to avoid interfering with ingress ports.
I think there should be a single function that reconciles s.Endpoint.Ports with s.Endpoint.Spec.Ports. If any ingress ports change, these should be deallocated/allocated as necessary. If nothing at all changes, we should skip updating the service in the store.
I think this will be easier to maintain than separate paths for general port allocation and only updating host-published ports.
cc @aboch @yongtang @abhinandanpb
There are a number of overlapping places where the ports are compared between
s.Endpointands.Spec.Endpoint, and possibly updated:reconcilePortConfigsreturns a list of port configs, thenserviceAllocatePortsupdates the endpoint.portsAllocatedInHostPublishModechecks if host-published ports need to be updated.portsAllocatedInHostPublishModereturned true, thenupdatePortsInHostPublishModehas similar logic to update these host-published ports. It doesn't reuse any of the code or results fromportsAllocatedInHostPublishMode.I find these different functions confusing, and we've had bugs before when they behaved inconsistently or weren't correct. It's especially problematic because the host-published-port-specific functions have to avoid interfering with ingress ports.
I think there should be a single function that reconciles
s.Endpoint.Portswiths.Endpoint.Spec.Ports. If any ingress ports change, these should be deallocated/allocated as necessary. If nothing at all changes, we should skip updating the service in the store.I think this will be easier to maintain than separate paths for general port allocation and only updating host-published ports.
cc @aboch @yongtang @abhinandanpb