Skip to content

Commit

Permalink
ovn: add empty Service reject ACLs to cluster port group, not switches
Browse files Browse the repository at this point in the history 
Adding an ACL-per-service-per-node switch doesn't scale. OVN team instead
suggested adding the ACL-per-service to a Port Group which all logical
switch ports are members of.

Note that this does now deny multicast traffic across the management port,
but I can't think of a good reason why that should be allowed or even work
today anyway.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1855408
Signed-off-by: Dan Williams <dcbw@redhat.com>
  • Loading branch information
@dcbw
dcbw committed Aug 11, 2020
1 parent b3e3c05 commit e0ba10a
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 85 deletions.
9 changes: 6 additions & 3 deletions go-controller/pkg/ovn/endpoints_test.go
View file
Expand Up @@ -54,10 +54,13 @@ func (e endpoints) addCmds(fexec *ovntest.FakeExec, service v1.Service, endpoint
func (e endpoints) delCmds(fexec *ovntest.FakeExec, service v1.Service, endpoint v1.Endpoints) {
for _, sPort := range service.Spec.Ports {
if sPort.Protocol == v1.ProtocolTCP {
// When all endpoints have been deleted, the reject ACL is added
aclName := generateACLName(k8sTCPLoadBalancerIP, service.Spec.ClusterIP, sPort.Port)
match := fmt.Sprintf("ip4.dst==%s && tcp && tcp.dst==%d", service.Spec.ClusterIP, sPort.Port)
fexec.AddFakeCmdsNoOutputNoError([]string{
fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid find logical_switch load_balancer{>=}%s", k8sTCPLoadBalancerIP),
fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=name find logical_router load_balancer{>=}%s", k8sTCPLoadBalancerIP),
fmt.Sprintf("ovn-nbctl --timeout=15 set load_balancer %s vips:\"172.124.0.2:8032\"=\"\"", k8sTCPLoadBalancerIP),
fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid find acl name=%s", aclName),
fmt.Sprintf("ovn-nbctl --timeout=15 --id=@acl create acl direction=from-lport priority=1000 match=\"%s\" action=reject name=%s -- add port_group acls @acl", match, aclName),
fmt.Sprintf("ovn-nbctl --timeout=15 set load_balancer %s vips:\"%s:%v\"=\"\"", k8sTCPLoadBalancerIP, service.Spec.ClusterIP, sPort.Port),
})
} else if sPort.Protocol == v1.ProtocolUDP {
fexec.AddFakeCmdsNoOutputNoError([]string{
Expand Down
130 changes: 50 additions & 80 deletions go-controller/pkg/ovn/loadbalancer.go
View file
Expand Up @@ -156,103 +156,87 @@ func (ovn *Controller) createLoadBalancerVIPs(lb string,
return nil
}

func (ovn *Controller) getLogicalSwitchesForLoadBalancer(lb string) ([]string, error) {
out, _, err := util.RunOVNNbctl("--data=bare", "--no-heading",
"--columns=_uuid", "find",
"logical_switch", fmt.Sprintf("load_balancer{>=}%s", lb))
if err != nil {
return nil, err
}
if len(strings.Fields(out)) > 0 {
return strings.Fields(out), nil
}
// if load balancer was not on a switch, then it may be on a router
out, _, err = util.RunOVNNbctl("--data=bare", "--no-heading",
"--columns=name", "find",
"logical_router", fmt.Sprintf("load_balancer{>=}%s", lb))
if err != nil {
return nil, err
}
if len(out) == 0 {
return nil, nil
}
// if this is a GR we know the corresponding join and external switches, otherwise this is an unhandled
// case
if strings.HasPrefix(out, gwRouterPrefix) {
routerName := strings.TrimPrefix(out, gwRouterPrefix)
return []string{joinSwitchPrefix + routerName, externalSwitchPrefix + routerName}, nil
// TODO: Add unittest for function.
func generateACLName(lb string, sourceIP string, sourcePort int32) string {
aclName := fmt.Sprintf("%s-%s:%d", lb, sourceIP, sourcePort)
aclName = strings.ReplaceAll(aclName, ":", "\\:")
// ACL names are limited to 63 characters
if len(aclName) > 63 {
var ipPortLen int
srcPortStr := fmt.Sprintf("%d", sourcePort)
if utilnet.IsIPv6String(sourceIP) {
// Add the length of the IP (max 39 with colons),
// plus 14 for '\\' for each ':' in IP,
// plus length of sourcePort (max 5 char),
// plus 3 for additional '\\:' to separate,
// plus 1 for '-' between lb and IP.
// With full IPv6 address and 5 char port, max ipPortLen is 62.
ipPortLen = len(sourceIP) + 14 + len(srcPortStr) + 3 + 1
} else {
// Add the length of the IP (max 15 with periods),
// plus length of sourcePort (max 5 char),
// plus 3 for additional '\\:' to separate,
// plus 1 for '-' between lb and IP.
// With full IPv4 address and 5 char port, max ipPortLen is 24.
ipPortLen = len(sourceIP) + len(srcPortStr) + 3 + 1
}
lbTrim := 63 - ipPortLen
// Shorten the Load Balancer name to allow full IP:port
tmpLb := lb[:lbTrim]
klog.Infof("Limiting ACL Name from %s to %s-%s:%d to keep under 63 characters", aclName, tmpLb, sourceIP, sourcePort)
aclName = fmt.Sprintf("%s-%s:%d", tmpLb, sourceIP, sourcePort)
aclName = strings.ReplaceAll(aclName, ":", "\\:")
}
return nil, fmt.Errorf("router detected with load balancer that is not a GR")
return aclName
}

func (ovn *Controller) createLoadBalancerRejectACL(lb string, sourceIP string, sourcePort int32, proto kapi.Protocol) (string, error) {
ovn.serviceLBLock.Lock()
defer ovn.serviceLBLock.Unlock()
switches, err := ovn.getLogicalSwitchesForLoadBalancer(lb)
if err != nil {
return "", fmt.Errorf("error finding logical switch that contains load balancer %s: %v", lb, err)
}

if len(switches) == 0 {
klog.V(5).Infof("Ignoring creating reject ACL for load balancer %s. It has no logical switches", lb)
return "", nil
}

func (ovn *Controller) createLoadBalancerRejectACL(lb, sourceIP string, sourcePort int32, proto kapi.Protocol) (string, error) {
ip := net.ParseIP(sourceIP)
if ip == nil {
return "", fmt.Errorf("cannot create reject ACL, invalid source IP: %s", sourceIP)
}
var aclMatch string
var l3Prefix string
var l3Prefix, aclMatch string
if utilnet.IsIPv6(ip) {
l3Prefix = "ip6"
} else {
l3Prefix = "ip4"
}
vip := util.JoinHostPortInt32(sourceIP, sourcePort)
// NOTE: doesn't use vip, to avoid having brackets in the name with IPv6
aclName := fmt.Sprintf("%s-%s:%d", lb, sourceIP, sourcePort)
aclName := generateACLName(lb, sourceIP, sourcePort)
// If ovn-k8s was restarted, we lost the cache, and an ACL may already exist in OVN. In that case we need to check
// using ACL name
aclUUID, stderr, err := util.RunOVNNbctl("--data=bare", "--no-heading", "--columns=_uuid", "find", "acl",
fmt.Sprintf("name=%s", strings.ReplaceAll(aclName, ":", "\\:")))
fmt.Sprintf("name=%s", aclName))
if err != nil {
klog.Errorf("Error while querying ACLs by name: %s, %v", stderr, err)
} else if len(aclUUID) > 0 {
klog.Infof("Existing Service Reject ACL found: %s for %s", aclUUID, aclName)
// If we found the ACL exists we need to ensure it applies to all logical switches
cmd := []string{}
for _, ls := range switches {
cmd = append(cmd, "--", "add", "logical_switch", ls, "acls", aclUUID)
}
if len(cmd) > 0 {
_, _, err = util.RunOVNNbctl(cmd...)
if err != nil {
klog.Warningf("Unable to add reject ACL: %s for switches: %s", aclUUID, switches)
}
_, stderr, err = util.RunOVNNbctl("--", "add", "port_group", ovn.clusterPortGroupUUID, "acls", aclUUID)
if err != nil {
klog.Errorf("Failed to add LB %s ACL %s %q to cluster port group: %q (%v)",
lb, aclUUID, aclName, stderr, err)
return "", err
}
ovn.setServiceACLToLB(lb, vip, aclUUID)
return aclUUID, nil
}

aclMatch = fmt.Sprintf("match=\"%s.dst==%s && %s && %s.dst==%d\"", l3Prefix, sourceIP,
strings.ToLower(string(proto)), strings.ToLower(string(proto)), sourcePort)

cmd := []string{"--id=@acl", "create", "acl", "direction=from-lport", "priority=1000", aclMatch, "action=reject",
fmt.Sprintf("name=%s", strings.ReplaceAll(aclName, ":", "\\:"))}
for _, ls := range switches {
cmd = append(cmd, "--", "add", "logical_switch", ls, "acls", "@acl")
}
fmt.Sprintf("name=%s", aclName)}
cmd = append(cmd, "--", "add", "port_group", ovn.clusterPortGroupUUID, "acls", "@acl")

aclUUID, stderr, err = util.RunOVNNbctl(cmd...)
if err != nil {
return "", fmt.Errorf("error creating ACL reject rule: %s for load balancer %s: %s, %s", cmd, lb, stderr,
err)
} else {
// Associate ACL UUID with load balancer and ip+port so we can remove this ACL if
// backends are re-added.
ovn.setServiceACLToLB(lb, vip, aclUUID)
klog.Errorf("Failed to create LB %s ACL %q and add to cluster port group: %q (%v)",
lb, aclName, stderr, err)
return "", err
}
// Associate ACL UUID with load balancer and ip+port so we can remove this ACL if
// backends are re-added.
ovn.setServiceACLToLB(lb, vip, aclUUID)
return aclUUID, nil
}

Expand All @@ -263,25 +247,11 @@ func (ovn *Controller) deleteLoadBalancerRejectACL(lb, vip string) {
return
}

switches, err := ovn.getLogicalSwitchesForLoadBalancer(lb)
_, stderr, err := util.RunOVNNbctl("remove", "port_group", ovn.clusterPortGroupUUID, "acls", acl)
if err != nil {
klog.Errorf("Could not retrieve logical switches associated with load balancer %s", lb)
klog.Errorf("Failed to remove reject ACL for %s from LB %s: stderr: %q, error: %v", vip, lb, stderr, err)
return
}

args := []string{}
for _, ls := range switches {
args = append(args, "--", "--if-exists", "remove", "logical_switch", ls, "acl", acl)
}

if len(args) > 0 {
_, _, err = util.RunOVNNbctl(args...)
if err != nil {
klog.Errorf("Error while removing ACL: %s, from switches, error: %v", acl, err)
} else {
klog.V(5).Infof("ACL: %s, removed from switches: %s", acl, switches)
}
}

ovn.removeServiceACL(lb, vip)
}
7 changes: 5 additions & 2 deletions go-controller/pkg/ovn/service_test.go
View file
Expand Up @@ -109,9 +109,12 @@ func (s service) baseCmds(fexec *ovntest.FakeExec, service v1.Service) {
fexec.AddFakeCmdsNoOutputNoError([]string{
"ovn-nbctl --timeout=15 --if-exists remove load_balancer sctp_load_balancer_id_1 vips \"172.30.0.10:53\"",
})
// When all endpoints have been deleted, the reject ACL is added
aclName := generateACLName(k8sTCPLoadBalancerIP, service.Spec.ClusterIP, service.Spec.Ports[0].Port)
match := fmt.Sprintf("ip4.dst==%s && tcp && tcp.dst==%d", service.Spec.ClusterIP, service.Spec.Ports[0].Port)
fexec.AddFakeCmdsNoOutputNoError([]string{
fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid find logical_switch load_balancer{>=}k8s_tcp_load_balancer"),
fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=name find logical_router load_balancer{>=}k8s_tcp_load_balancer"),
fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid find acl name=%s", aclName),
fmt.Sprintf("ovn-nbctl --timeout=15 --id=@acl create acl direction=from-lport priority=1000 match=\"%s\" action=reject name=%s -- add port_group acls @acl", match, aclName),
})
}

Expand Down

0 comments on commit e0ba10a

Please sign in to comment.