Skip to content

Commit

Permalink
Fixing iface deletion bug when netIds don't match
Browse files Browse the repository at this point in the history
The project assumes that a DanmNet's NetworkID and ObjectMeta.Name parameters always match.
This is even outlined inthe schema descriptor.
However, recently we have introduced some changes which broke this assumption.
First, with the "default" feature, and then lately with the variable static delegate configuration.
The user might not even perceive it, but as a result of these changes interfaces weren't properly cleaned-up during CNI DEL invocations.
Instead of re-writing the whole codebase, we save the real name of the network as well into the interface's DanmEp, and use this parameter when we read the network descriptor during the deletion of the interface.
Meaning of NetworkID parameter left untouched in all the other places, but eventually, we need to revise when to sue which parameter.

Commit contains one more smal,, but important change: the first interface of a Pod will be always named "eth0" from now on.
This change will make it impossible for users to accidentally screw-up network naming, and thus kill their own Pod.
  • Loading branch information
Levovar committed Apr 23, 2019
1 parent 54c4925 commit d035b15
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 11 deletions.
3 changes: 1 addition & 2 deletions crd/apis/danm/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,14 @@ type DanmEp struct {

type DanmEpSpec struct {
NetworkID string `json:"NetworkID"`
NetworkName string `json:"NetworkName"`
NetworkType string `json:"NetworkType"`
EndpointID string `json:"EndpointID"`
Iface DanmEpIface `json:"Interface"`
Host string `json:"Host,omitempty"`
Pod string `json:"Pod"`
CID string `json:"CID,omitempty"`
Netns string `json:"netns,omitempty"`
Creator string `json:"Creator,omitempty"`
Expires string `json:"Expires,omitempty"`
}

type DanmEpIface struct {
Expand Down
4 changes: 4 additions & 0 deletions pkg/cnidel/cnidel.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,10 @@ func GetEnv(key, fallback string) string {
// If a name is explicitly set in the related DanmNet API object, the NIC will be named accordingly.
// If a name is not explicitly set, then DANM will name the interface ethX where X=sequence number of the interface
func CalculateIfaceName(chosenName, defaultName string, sequenceId int) string {
//Kubelet expects the first interface to be literally named "eth0", so...
if sequenceId == 0 {
return "eth0"
}
if chosenName != "" {
return chosenName + strconv.Itoa(sequenceId)
}
Expand Down
18 changes: 10 additions & 8 deletions pkg/metacni/metacni.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ func createDelegatedInterface(syncher *syncher.Syncher, danmClient danmclientset
Proutes6: iface.Proutes6,
DeviceID: iface.Device,
}
ep, err := createDanmEp(epIfaceSpec, netInfo.Spec.NetworkID, netInfo.Spec.NetworkType, args)
ep, err := createDanmEp(epIfaceSpec, netInfo, args)
if err != nil {
syncher.PushResult(iface.Network, errors.New("DanmEp object could not be created due to error:" + err.Error()), nil)
return
Expand Down Expand Up @@ -333,8 +333,7 @@ func createDanmInterface(syncher *syncher.Syncher, danmClient danmclientset.Inte
Proutes: iface.Proutes,
Proutes6: iface.Proutes6,
}
networkType := "ipvlan"
ep, err := createDanmEp(epSpec, netId, networkType, args)
ep, err := createDanmEp(epSpec, netInfo, args)
if err != nil {
ipam.GarbageCollectIps(danmClient, netInfo, ip4, ip6)
syncher.PushResult(iface.Network, errors.New("DanmEp object could not be created due to error:" + err.Error()), nil)
Expand Down Expand Up @@ -371,7 +370,7 @@ func createDanmInterface(syncher *syncher.Syncher, danmClient danmclientset.Inte
syncher.PushResult(iface.Network, nil, danmResult)
}

func createDanmEp(epInput danmtypes.DanmEpIface, netId string, neType string, args *cniArgs) (danmtypes.DanmEp, error) {
func createDanmEp(epInput danmtypes.DanmEpIface, netInfo *danmtypes.DanmNet, args *cniArgs) (danmtypes.DanmEp, error) {
epidInt, err := uuid.NewV4()
if err != nil {
return danmtypes.DanmEp{}, errors.New("uuid.NewV4 returned error during EP creation:" + err.Error())
Expand All @@ -381,16 +380,19 @@ func createDanmEp(epInput danmtypes.DanmEpIface, netId string, neType string, ar
if err != nil {
return danmtypes.DanmEp{}, errors.New("OS.Hostname returned error during EP creation:" + err.Error())
}
if netInfo.Spec.NetworkType == "" {
netInfo.Spec.NetworkType = "ipvlan"
}
epSpec := danmtypes.DanmEpSpec {
NetworkID: netId,
NetworkType: neType,
NetworkID: netInfo.Spec.NetworkID,
NetworkName: netInfo.ObjectMeta.Name,
NetworkType: netInfo.Spec.NetworkType,
EndpointID: epid,
Iface: epInput,
Host: host,
Pod: args.podId,
CID: args.containerId,
Netns: args.netns,
Creator: "danm",
}
meta := meta_v1.ObjectMeta {
Name: epid,
Expand Down Expand Up @@ -476,7 +478,7 @@ func deleteInterface(args *cniArgs, syncher *syncher.Syncher, ep danmtypes.DanmE
syncher.PushResult(ep.Spec.NetworkID, errors.New("failed to create danmClient:" + err.Error()), nil)
return
}
netInfo, err := danmClient.DanmV1().DanmNets(args.nameSpace).Get(ep.Spec.NetworkID, meta_v1.GetOptions{})
netInfo, err := danmClient.DanmV1().DanmNets(args.nameSpace).Get(ep.Spec.NetworkName, meta_v1.GetOptions{})
if err != nil {
syncher.PushResult(ep.Spec.NetworkID, errors.New("failed to get DanmNet:"+ err.Error()), nil)
return
Expand Down
7 changes: 6 additions & 1 deletion test/uts/cnidel_test/cnidel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,15 +257,20 @@ func TestCalculateIfaceName(t *testing.T) {
testDefaultName := "notthechosenone"
testSequenceId := 4
expChosenName := testChosenName+strconv.Itoa(testSequenceId)
expDefName := testDefaultName+strconv.Itoa(testSequenceId)
ifaceName := cnidel.CalculateIfaceName(testChosenName, testDefaultName, testSequenceId)
if ifaceName != expChosenName {
t.Errorf("Received value for explicitly set interface name: %s does not match with expected: %s", ifaceName, testChosenName)
}
expDefName := testDefaultName+strconv.Itoa(testSequenceId)
defIfaceName := cnidel.CalculateIfaceName("", testDefaultName, testSequenceId)
if defIfaceName != expDefName {
t.Errorf("Received value for default interface name: %s does not match with expected: %s", defIfaceName, testChosenName)
}
expFirstNicName := "eth0"
firstIfaceName := cnidel.CalculateIfaceName(testChosenName, testDefaultName, 0)
if firstIfaceName != expFirstNicName {
t.Errorf("The first interface shall always be named eth0, regardless what the user wants")
}
}

func TestDelegateInterfaceSetup(t *testing.T) {
Expand Down

0 comments on commit d035b15

Please sign in to comment.