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

Remove unexpected AltName after rename interface #6321

Merged
merged 1 commit into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions pkg/agent/util/net_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,15 @@ func RenameInterface(from, to string) error {
if pollErr != nil {
return fmt.Errorf("failed to rename host interface name %s to %s", from, to)
}
// Fix for the issue https://github.com/antrea-io/antrea/issues/6301.
// In some new Linux versions which support AltName, if the only valid altname of the interface is the same as the
// interface name, it would be left empty when the name is occupied by the interface name; after we rename the
// interface name to another value, the altname of the interface would be set to the original interface name by the
// system.
// This altname must be removed as we need to reserve the name for an OVS internal port.
if err := removeInterfaceAltName(to, from); err != nil {
return fmt.Errorf("failed to remove AltName %s on interface %s: %w", from, to, err)
}
return nil
}

Expand Down Expand Up @@ -378,6 +387,20 @@ func renameHostInterface(oriName string, newName string) error {
return nil
}

// removeInterfaceAltName removes altName on interface with provided name. altName not found will return nil.
func removeInterfaceAltName(name string, altName string) error {
gran-vmv marked this conversation as resolved.
Show resolved Hide resolved
link, err := netlinkUtil.LinkByName(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to add check first, if altname exists, and equals to the original name, we shall delete it, otherwise, ignore it.

Copy link
Contributor

@wenyingd wenyingd May 13, 2024

Choose a reason for hiding this comment

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

Is it possible that time delays exist between adding the altname and our deletion on the target altname, e.g., the sequence is like this,

  1. rename uplink
  2. try to delete altname (introduced in this change)
  3. system adds the altname.

If so, the change may not work as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible that time delays exist between adding the altname and our deletion on the target altname, e.g., the sequence is like this,

  1. rename uplink
  2. try to delete altname (introduced in this change)
  3. system adds the altname.

If so, the change may not work as expected.

AltName will be set very quickly.
If this process get delayed, antrea-agent will create internal port and occupy this name first, and this is not an issue.

if err != nil {
return err
}
for _, existAltName := range link.Attrs().AltNames {
if existAltName == altName {
return netlinkUtil.LinkDelAltName(link, altName)
}
}
return nil
}

// PrepareHostInterfaceConnection prepares host interface connection to the OVS bridge client by:
// 1. Renaming the host interface (a bridged suffix will be added to it).
// 2. Creating an internal port (original name of the host interface will be used here).
Expand Down
27 changes: 27 additions & 0 deletions pkg/agent/util/net_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type mockLink struct {
name string
masterIndex int
hardwareAddr net.HardwareAddr
altNames []string
}

func (l mockLink) Attrs() *netlink.LinkAttrs {
Expand All @@ -44,6 +45,7 @@ func (l mockLink) Attrs() *netlink.LinkAttrs {
Name: l.name,
MasterIndex: l.masterIndex,
HardwareAddr: l.hardwareAddr,
AltNames: l.altNames,
}
}

Expand Down Expand Up @@ -557,6 +559,7 @@ func TestGetInterfaceConfig(t *testing.T) {

func TestRenameInterface(t *testing.T) {
renameFailErr := fmt.Errorf("failed to rename host interface name test1 to test2")
removeAltNameFailErr := fmt.Errorf("failed to remove AltName test1 on interface test2: %w", testInvalidErr)
tests := []struct {
name string
expectedCalls func(mockNetlink *netlinktest.MockInterfaceMockRecorder)
Expand All @@ -569,6 +572,18 @@ func TestRenameInterface(t *testing.T) {
mockNetlink.LinkSetDown(mockLink{name: "test1"}).Return(nil)
mockNetlink.LinkSetName(mockLink{name: "test1"}, "test2").Return(nil)
mockNetlink.LinkSetUp(mockLink{name: "test1"}).Return(nil)
mockNetlink.LinkByName("test2").Return(mockLink{name: "test2", altNames: []string{}}, nil)
},
},
{
name: "Rename Interface Success with AltName",
expectedCalls: func(mockNetlink *netlinktest.MockInterfaceMockRecorder) {
mockNetlink.LinkByName("test1").Return(mockLink{name: "test1"}, nil)
mockNetlink.LinkSetDown(mockLink{name: "test1"}).Return(nil)
mockNetlink.LinkSetName(mockLink{name: "test1"}, "test2").Return(nil)
mockNetlink.LinkSetUp(mockLink{name: "test1"}).Return(nil)
mockNetlink.LinkByName("test2").Return(mockLink{name: "test2", altNames: []string{"test1"}}, nil)
mockNetlink.LinkDelAltName(mockLink{name: "test2", altNames: []string{"test1"}}, "test1").Return(nil)
},
},
{
Expand Down Expand Up @@ -605,6 +620,18 @@ func TestRenameInterface(t *testing.T) {
},
wantErr: renameFailErr,
},
{
name: "Remove AltName Err",
expectedCalls: func(mockNetlink *netlinktest.MockInterfaceMockRecorder) {
mockNetlink.LinkByName("test1").Return(mockLink{name: "test1"}, nil)
mockNetlink.LinkSetDown(mockLink{name: "test1"}).Return(nil)
mockNetlink.LinkSetName(mockLink{name: "test1"}, "test2").Return(nil)
mockNetlink.LinkSetUp(mockLink{name: "test1"}).Return(nil)
mockNetlink.LinkByName("test2").Return(mockLink{name: "test2", altNames: []string{"test1"}}, nil)
mockNetlink.LinkDelAltName(mockLink{name: "test2", altNames: []string{"test1"}}, "test1").Return(testInvalidErr)
},
wantErr: removeAltNameFailErr,
},
}

for _, tc := range tests {
Expand Down
4 changes: 4 additions & 0 deletions pkg/agent/util/netlink/netlink_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ type Interface interface {

LinkSetName(link netlink.Link, name string) error

LinkAddAltName(link netlink.Link, name string) error

LinkDelAltName(link netlink.Link, name string) error

LinkSetUp(link netlink.Link) error

ConntrackDeleteFilter(table netlink.ConntrackTableType, family netlink.InetFamily, filter netlink.CustomConntrackFilter) (uint, error)
Expand Down
30 changes: 29 additions & 1 deletion pkg/agent/util/netlink/testing/mock_netlink_linux.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading