Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes Ingress Load Balancer ACL #1893

Merged
merged 1 commit into from Dec 6, 2020
Merged

Conversation

trozet
Copy link
Contributor

@trozet trozet commented Dec 5, 2020

The ACL was accidentally being configured on the cluster load balancer
instead of the gateway load balancer.

Signed-off-by: Tim Rozet trozet@redhat.com

The ACL was accidentally being configured on the cluster load balancer
instead of the gateway load balancer.

Signed-off-by: Tim Rozet <trozet@redhat.com>
@trozet
Copy link
Contributor Author

trozet commented Dec 5, 2020

@dcbw @danwinship

@@ -371,6 +371,10 @@ func (ovn *Controller) createService(service *kapi.Service) error {
break
}
if svcQualifiesForReject(service) {
gateways, _, err := ovn.getOvnGateways()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note, looking over the createService function there is a lot of duplication of code that and some of this stuff doesn't make much sense anymore. I'll look at cleaning up this function in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trozet I 've tested it only adding the reject ACL on the portgroup and it works for clusterIP and NodePorts for me, but maybe I 'm missing something, what do we need to add reject on the switches if it will be always rejected by the portgroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think about shared gateway mode and a packet coming into OVN GR destined for the host IP (node Port). It will end up hitting the IP of the GR and the GR will consume the packet. That's why we have load balancer there. If you put the ACL for node port on the portgroup only, the ACL would never block external access to the node port, because the DNAT would have already happened in the GR. For ingress load balancer, the GR may forward that to cluster router, but then cluster router woudln't know what do with it and probably route it back out the DGP and definitely not to the worker switch, so that would never hit the port group ACL either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, the ACL would never block external access to the node port, because the DNAT would have already happened in the GR

if there are no endpoints ... what DNAT is performed?

this is working for me if (in local mode) if I add a reject to the VIP fo the (nodePortIP:NodePortPort) and add it to the PortGroup
my question is, which ports belong to the clusterPortGroup?

I didn't tried in shared gw though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just confirmed that services without endpoints reject correctly in local gateway mode using an ACL for portgroups only, because the NodePort adds iptables rules to the ClusterIP

I need to test in shared gw with ACL for portgroups only, but there are more things failing with shared gw and nodeports

@trozet
Copy link
Contributor Author

trozet commented Dec 5, 2020

running this downstream to make sure:
openshift/ovn-kubernetes#370

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 55.751% when pulling 412f0c1 on trozet:fix_reject_acl_ingress into 303a248 on ovn-org:master.

@dcbw
Copy link
Contributor

dcbw commented Dec 6, 2020

/lgtm
we really need to clean this up. It's confusing that there's a loadbalancer variable at the top of the block, but we aren't using it anywhere except the first if _, hasEps := ovn.getServiceLBInfo(loadBalancer, vip); hasEps { bit, and then we shadow that variable later.

But regardless, it passes our downstream CI, so it's clearly more correct than what was there before.

@dcbw dcbw merged commit 020dce6 into ovn-org:master Dec 6, 2020
kyrtapz pushed a commit to kyrtapz/ovn-kubernetes that referenced this pull request Sep 25, 2023
…lus-fix

OCPBUGS-19449: Do not return error if pod IP cannot be retrieved for `podSelectorPodNeedsDelete` and perf improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants