allow special hostsubnets to force a vnid on egress packets #11817
Conversation
|
I'm assuming, there are no issues:
|
| @@ -220,7 +221,15 @@ func (master *OsdnMaster) watchSubnets() { | |||
| log.Errorf("Error in deleting annotated subnet from master, name: %s, ip %s: %v", name, hostIP, err) | |||
| return nil | |||
| } | |||
| err = master.addNode(name, hostIP) | |||
| var hsAnnotations map[string]string | |||
| if vnid, ok := hs.Annotations[osapi.FixedVnidHost]; ok { | |||
There was a problem hiding this comment.
This check is only valid for multi-tenant sdn plugin, should ignore for subnet plugin (osapi.IsOpenShiftMultitenantNetworkPlugin)
If we know that F5 with multi-tenant sdn plugin needs this hack, may be enforce osapi.FixedVnidHost to be present when osapi.AssignHostSubnetAnnotation is passed?
There was a problem hiding this comment.
I'd say it's fine to do this in both multitenant and single-tenant. Especially if we only support 0 for the fixed vnid.
There was a problem hiding this comment.
It's probably useful for single tenant also because the tunnel id at the f5 side is configured by the admin separately. This code will allow us to comply to that setting.
| vnidInt, err := strconv.Atoi(vnid) | ||
| if err == nil { | ||
| hsAnnotations = make(map[string]string) | ||
| hsAnnotations[osapi.FixedVnidHost] = strconv.Itoa(vnidInt) |
There was a problem hiding this comment.
Check hard-coded vnid is in the valid range (0 to 2^24-1)
| vnidInt, err := strconv.Atoi(vnid) | ||
| if err == nil { | ||
| hsAnnotations = make(map[string]string) | ||
| hsAnnotations[osapi.FixedVnidHost] = strconv.Itoa(vnidInt) |
There was a problem hiding this comment.
Is there any reason to allow arbitrary VNIDs? Without corresponding translation on the incoming side, this isn't very useful unless vnid==0.
There was a problem hiding this comment.
There may be a choice to have the f5 for a particular project only. Then we will have the facility to change the value per deployment.
| @@ -220,7 +221,15 @@ func (master *OsdnMaster) watchSubnets() { | |||
| log.Errorf("Error in deleting annotated subnet from master, name: %s, ip %s: %v", name, hostIP, err) | |||
| return nil | |||
| } | |||
| err = master.addNode(name, hostIP) | |||
| var hsAnnotations map[string]string | |||
| if vnid, ok := hs.Annotations[osapi.FixedVnidHost]; ok { | |||
There was a problem hiding this comment.
I'd say it's fine to do this in both multitenant and single-tenant. Especially if we only support 0 for the fixed vnid.
|
[testextended][extended:core(idling)] |
|
Evaluated for origin test up to 91a2536 |
|
Evaluated for origin testextended up to 91a2536 |
| var hsAnnotations map[string]string | ||
| if vnid, ok := hs.Annotations[osapi.FixedVnidHost]; ok { | ||
| vnidInt, err := strconv.Atoi(vnid) | ||
| if err == nil && vnidInt >= 0 && uint32(vnidInt) <= osapi.MaxVNID { |
There was a problem hiding this comment.
err == nil && uint32(vnidInt) >= osapi.MinVNID && uint32(vnidInt) <= osapi.MaxVNID ?
There was a problem hiding this comment.
MinVnid will not catch 0
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11283/) (Image: devenv-rhel7_5339) |
|
Evaluated for origin merge up to 91a2536 |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/767/) (Base Commit: 67e9c3f) (Extended Tests: core(idling)) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11283/) (Base Commit: 67e9c3f) |
Fixes the multi-tenant networking for F5 vxlan. The return packets are forced to have a vnid that is preselected for that host (based on an annotation). This is required for F5 configuration to be able to communicate with pods outside of the default vnid in a multitenant sdn installation.
https://bugzilla.redhat.com/show_bug.cgi?id=1389706#c13
@danwinship Please review.
cc @openshift/networking