opiproject / opi-evpn-bridge

OPI gRPC to EVPN GW FRR bridge
Apache License 2.0
14 stars 13 forks source link

Port: review the netlink commands #92

Open glimchb opened 1 year ago

glimchb commented 1 year ago

see https://github.com/opiproject/opi-evpn-bridge/blob/6b0df33130c7a7f658491510c08d2b44607d7ebb/pkg/evpn/port.go#L54-L117 need another look and review

glimchb commented 1 year ago

from @mardim91

In the below code I would have done it a bit differently. I would have first checked for the type of port and the do a for loop for the logical bridges. That is because you would have always one type of BridgePort for all the logical Birdges in the list

for _, bridgeRefName := range in.BridgePort.Spec.LogicalBridges {
        fmt.Printf("add iface to logical bridge %s", bridgeRefName)
        // get object from DB
        bridgeObject, ok := s.Bridges[bridgeRefName]
        if !ok {
            err := status.Errorf(codes.NotFound, "unable to find key %s", bridgeRefName)
            log.Printf("error: %v", err)
            return nil, err
        }
        vid := uint16(bridgeObject.Spec.VlanId)
        switch in.BridgePort.Spec.Ptype {
        case pb.BridgePortType_ACCESS:
            // Example: bridge vlan add dev eth2 vid 20 pvid untagged
            if err := netlink.BridgeVlanAdd(iface, vid, true, true, false, false); err != nil {
                fmt.Printf("Failed to add vlan to bridge: %v", err)
                return nil, err
            }
        case pb.BridgePortType_TRUNK:
            // Example: bridge vlan add dev eth2 vid 20
            if err := netlink.BridgeVlanAdd(iface, vid, false, false, false, false); err != nil {
                fmt.Printf("Failed to add vlan to bridge: %v", err)
                return nil, err
            }
        default:
            msg := fmt.Sprintf("Only ACCESS or TRUNK supported and not (%d)", in.BridgePort.Spec.Ptype)
            log.Print(msg)
            return nil, status.Errorf(codes.InvalidArgument, msg)
        }
    }
mardim91 commented 1 year ago

The only comment that I have is the above one. The rest looks ok