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

bridge: add support for setting vlans as pvid and untagged #936

Closed

Conversation

bersoare
Copy link

@bersoare bersoare commented Aug 4, 2023

This PR adds support for specifying a PVID and untagged vlans for a trunk. Added new checks for the presence of these attributes in the tests, as well as a new test case that uses the newly introduced values.

@@ -56,6 +56,8 @@ type NetConf struct {
HairpinMode bool `json:"hairpinMode"`
PromiscMode bool `json:"promiscMode"`
Vlan int `json:"vlan"`
PVID int `json:"pvid,omitempty"`
UntaggedIDs []int `json:"untaggedIDs,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

How about to use VlanTrunk to specify range for untagged, to support range as { "minID": 101, "maxID": 110 }?

(but I'm not quite sure that it provides better userbility yet but at least, we provide consistent UI between vlanTrunk and untaggedIDs...)

@@ -391,7 +403,7 @@ func ensureVlanInterface(br *netlink.Bridge, vlanID int, preserveDefaultVlan boo
return nil, fmt.Errorf("faild to find host namespace: %v", err)
}

_, brGatewayIface, err := setupVeth(hostNS, br, name, br.MTU, false, vlanID, nil, preserveDefaultVlan, "")
_, brGatewayIface, err := setupVeth(hostNS, br, name, br.MTU, false, vlanID, trunkConfig{allowedVlans: nil}, preserveDefaultVlan, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

trunkConfig{allowedVlans: nil} can be changed as trunkConfig{}

$ cat test.go 
package main

import "fmt"
type trunkConfig struct {
	pvid         int
	untaggedVids map[int]struct{}
	allowedVlans []int
}

func main() {
	fmt.Printf("%v\n", trunkConfig{}.allowedVlans)
}
$ go run test.go 
[]

This PR adds support for specifying a PVID and untagged vlans for a trunk.
Added new checks for the presence of these attributes in the tests,
as well as a new test case that uses the newly introduced values.

Signed-off-by: Bernardo Soares <bsoares.it@gmail.com>
@bersoare
Copy link
Author

hello @s1061123 ,

thanks for the suggestions, indeed changing it to be a []VlanTrunk makes sense to keep it consistent.
just changed it, all tests passed locally. let me know what you think.

thank you!

@bersoare bersoare closed this Sep 27, 2023
@s1061123
Copy link
Contributor

s1061123 commented Oct 2, 2023

@bersoare sorry but let me ask you why the PR close? I suppose the almost design seems to be reasonable. Currently CNI community focus on new CNI spec hence we don't have time to review PRs yet, but I'd like to push this PR....

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.

2 participants