Skip to content
Open
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
6 changes: 6 additions & 0 deletions pkg/utils/nad.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,12 @@ func (nc *NetConf) IsKubeOVNCNI() bool {
return nc.Type == CNITypeKubeOVN
}

// IsHarvesterManagedCNI returns true for the CNI types whose NADs Harvester builds and manages
// (bridge and kube-ovn). Other types (ipvlan, macvlan, host-device, sriov, ...) are left to the user.
func (nc *NetConf) IsHarvesterManagedCNI() bool {
return nc.IsBridgeCNI() || nc.IsKubeOVNCNI()
}

func (nc *NetConf) GetVlanID() int {
if nc.IsVlanAccessMode() {
return nc.Vlan
Expand Down
8 changes: 7 additions & 1 deletion pkg/webhook/nad/mutator.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ func (m *Mutator) Update(_ *admission.Request, oldObj, newObj runtime.Object) (a
return nil, fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}

// cluster network labels and route annotations only apply to Harvester-managed NADs
if !newNetconf.IsHarvesterManagedCNI() {
return nil, nil
}

patch, err := m.ensureLabels(newNad, oldNetconf, newNetconf)
if err != nil {
return nil, fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
Expand Down Expand Up @@ -228,7 +233,8 @@ func (m *Mutator) patchMTU(nad *cniv1.NetworkAttachmentDefinition) (admission.Pa
return nil, err
}

if netConf.IsKubeOVNCNI() {
// the MTU comes from the bridge's cluster network, which only bridge NADs have
if !netConf.IsBridgeCNI() {
return nil, nil
}

Expand Down
110 changes: 110 additions & 0 deletions pkg/webhook/nad/non_bridge_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package nad

import (
"testing"

"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

cniv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"

"github.com/harvester/harvester-network-controller/pkg/generated/clientset/versioned/fake"
"github.com/harvester/harvester-network-controller/pkg/utils"
"github.com/harvester/harvester-network-controller/pkg/utils/fakeclients"
)

const (
testNadConfigIpvlan = `{"cniVersion":"0.3.1","type":"ipvlan","master":"eth1","mode":"l2","ipam":{"type":"whereabouts","range":"10.99.0.0/24"}}`
testNadConfigIpvlanChanged = `{"cniVersion":"0.3.1","type":"ipvlan","master":"eth1","mode":"l2","ipam":{"type":"whereabouts","range":"10.98.0.0/24"}}`
testNadConfigMacvlan = `{"cniVersion":"0.3.1","type":"macvlan","master":"eth1","mode":"bridge","ipam":{}}`
testNadConfigHostDevice = `{"cniVersion":"0.3.1","type":"host-device","device":"eth2","ipam":{}}`
)

func newTestNad(config string, annotations map[string]string) *cniv1.NetworkAttachmentDefinition {
return &cniv1.NetworkAttachmentDefinition{
ObjectMeta: metav1.ObjectMeta{
Name: testNadName,
Namespace: testNamespace,
Annotations: annotations,
},
Spec: cniv1.NetworkAttachmentDefinitionSpec{Config: config},
}
}

func newTestValidator() *Validator {
c := fake.NewSimpleClientset()
return NewNadValidator(
fakeclients.VirtualMachineCache(c.KubevirtV1().VirtualMachines),
fakeclients.VirtualMachineInstanceCache(c.KubevirtV1().VirtualMachineInstances),
fakeclients.ClusterNetworkCache(c.NetworkV1beta1().ClusterNetworks),
fakeclients.VlanConfigCache(c.NetworkV1beta1().VlanConfigs),
fakeclients.SubnetCache(c.KubeovnV1().Subnets),
true,
fakeclients.HostNetworkConfigCache(c.NetworkV1beta1().HostNetworkConfigs),
fakeclients.NetworkAttachmentDefinitionCache(c.K8sCniCncfIoV1().NetworkAttachmentDefinitions),
)
}

func newTestMutator() *Mutator {
c := fake.NewSimpleClientset()
return NewNadMutator(
fakeclients.ClusterNetworkCache(c.NetworkV1beta1().ClusterNetworks),
fakeclients.VlanConfigCache(c.NetworkV1beta1().VlanConfigs),
fakeclients.NodeCache(c.CoreV1().Nodes),
)
}

func TestIsHarvesterManagedCNI(t *testing.T) {
tests := []struct {
config string
want bool
}{
{testNadConfigVlan300, true},
{`{"cniVersion":"0.3.1","type":"kube-ovn","provider":"nad.default.ovn"}`, true},
{testNadConfigIpvlan, false},
{testNadConfigMacvlan, false},
{testNadConfigHostDevice, false},
}
for _, tc := range tests {
conf, err := utils.DecodeNadConfigToNetConf(newTestNad(tc.config, nil))
assert.NoError(t, err)
assert.Equal(t, tc.want, conf.IsHarvesterManagedCNI(), tc.config)
}
}

func TestValidatorNonBridgeNAD(t *testing.T) {
v := newTestValidator()

for _, config := range []string{testNadConfigIpvlan, testNadConfigMacvlan, testNadConfigHostDevice} {
assert.NoError(t, v.Create(nil, newTestNad(config, nil)), "create %s", config)
}

// the route annotation is still validated
err := v.Create(nil, newTestNad(testNadConfigIpvlan, map[string]string{utils.KeyNetworkRoute: testNadConfigRouteInvalid}))
assert.Error(t, err)

// a non-bridge NAD can be updated
assert.NoError(t, v.Update(nil, newTestNad(testNadConfigIpvlan, nil), newTestNad(testNadConfigIpvlanChanged, nil)))

// but its CNI type still cannot change
err = v.Update(nil, newTestNad(testNadConfigIpvlan, nil), newTestNad(testNadConfigMacvlan, nil))
assert.ErrorContains(t, err, "cannot be updated from network type")
}

func TestValidatorBridgeNADStillChecked(t *testing.T) {
// no cluster network exists, so a bridge NAD must still be rejected
err := newTestValidator().Create(nil, newTestNad(testNadConfigVlan300, nil))
assert.Error(t, err)
}

func TestMutatorNonBridgeNAD(t *testing.T) {
m := newTestMutator()

patch, err := m.Create(nil, newTestNad(testNadConfigIpvlan, nil))
assert.NoError(t, err)
assert.Empty(t, patch)

patch, err = m.Update(nil, newTestNad(testNadConfigIpvlan, nil), newTestNad(testNadConfigIpvlanChanged, nil))
assert.NoError(t, err)
assert.Empty(t, patch)
}
29 changes: 20 additions & 9 deletions pkg/webhook/nad/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ func (v *Validator) Create(_ *admission.Request, newObj runtime.Object) error {
return fmt.Errorf(createErr, nad.Namespace, nad.Name, err)
}

// bridge/VLAN/cluster network checks only apply to Harvester-managed NADs
if !conf.IsHarvesterManagedCNI() {
if err := v.checkRoute(nad.Annotations[utils.KeyNetworkRoute]); err != nil {
return fmt.Errorf(createErr, nad.Namespace, nad.Name, err)
}
return nil
}

if err := v.checkVlanOverlapWithSN(nad, conf); err != nil {
return fmt.Errorf(createErr, nad.Namespace, nad.Name, err)
}
Expand Down Expand Up @@ -108,20 +116,23 @@ func (v *Validator) Update(_ *admission.Request, oldObj, newObj runtime.Object)
return nil
}

if err := v.checkVlanOverlapWithSN(newNad, newConf); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}

if err := v.checkNadTypes(oldConf, newConf); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}

if err := v.checkNadConfigBridgeName(oldConf, newConf); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}
// bridge/VLAN/cluster network checks only apply to Harvester-managed NADs
if newConf.IsHarvesterManagedCNI() {
if err := v.checkVlanOverlapWithSN(newNad, newConf); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}

if err := v.checkNadConfig(newConf, newNad); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
if err := v.checkNadConfigBridgeName(oldConf, newConf); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}

if err := v.checkNadConfig(newConf, newNad); err != nil {
return fmt.Errorf(updateErr, newNad.Namespace, newNad.Name, err)
}
}

if err := v.checkVmi(newNad); err != nil {
Expand Down
Loading