Skip to content

Commit

Permalink
refactor
Browse files Browse the repository at this point in the history
  • Loading branch information
@Tamas-Biro1
Tamas-Biro1 committed Nov 28, 2023
1 parent 5f875a2 commit d5ec32b
Show file tree
Hide file tree
Showing 12 changed files with 213 additions and 108 deletions.
11 changes: 7 additions & 4 deletions api/v1/installation_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,11 @@ const (
WindowsDataplaneHNS WindowsDataplaneOption = "HNS"
)

// +kubebuilder:validation:Enum=net.ipv4.tcp_keepalive_intvl;net.ipv4.tcp_keepalive_probes;net.ipv4.tcp_keepalive_time
type SysctlTuningKey string
type Sysctl struct {
// +kubebuilder:validation:Enum=net.ipv4.tcp_keepalive_intvl;net.ipv4.tcp_keepalive_probes;net.ipv4.tcp_keepalive_time
Key string `json:"key"`
Value string `json:"value"`
}

// CalicoNetworkSpec specifies configuration options for Calico provided pod networking.
type CalicoNetworkSpec struct {
Expand Down Expand Up @@ -467,9 +470,9 @@ type CalicoNetworkSpec struct {
// +kubebuilder:validation:Enum=Enabled;Disabled
ContainerIPForwarding *ContainerIPForwardingType `json:"containerIPForwarding,omitempty"`

// SysctlTuning configures sysctl parameters for tuning plugin
// Sysctl configures sysctl parameters for tuning plugin
// +optional
SysctlTuning *map[SysctlTuningKey]string `json:"sysctlTuning,omitempty"`
Sysctl []Sysctl `json:"sysctl,omitempty"`
}

// NodeAddressAutodetection provides configuration options for auto-detecting node addresses. At most one option
Expand Down
29 changes: 19 additions & 10 deletions api/v1/zz_generated.deepcopy.go
View file

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

23 changes: 5 additions & 18 deletions pkg/controller/installation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
kubecontrollers "github.com/tigera/operator/pkg/common/validation/kube-controllers"
typha "github.com/tigera/operator/pkg/common/validation/typha"
"github.com/tigera/operator/pkg/controller/k8sapi"
"github.com/tigera/operator/pkg/controller/utils"
"github.com/tigera/operator/pkg/render"
appsv1 "k8s.io/api/apps/v1"

Expand Down Expand Up @@ -337,26 +338,12 @@ func validateCustomResource(instance *operatorv1.Installation) error {
}
}

type TuningSpec struct {
Sysctl *map[operatorv1.SysctlTuningKey]string `json:"sysctl,omitempty"`
Type string `json:"type"`
}

if instance.Spec.CalicoNetwork.SysctlTuning != nil {
if instance.Spec.CalicoNetwork.Sysctl != nil {
// CNI tuning plugin
pluginData := instance.Spec.CalicoNetwork.SysctlTuning

// sysctl settings
allowedKeys := map[operatorv1.SysctlTuningKey]struct{}{
"net.ipv4.tcp_keepalive_intvl": {},
"net.ipv4.tcp_keepalive_probes": {},
"net.ipv4.tcp_keepalive_time": {},
}
pluginData := instance.Spec.CalicoNetwork.Sysctl

for key, value := range *pluginData {
if _, allowed := allowedKeys[key]; !allowed {
return fmt.Errorf("value %s not allowed in spec.calicoNetwork.sysctlTuning", value)
}
if err := utils.VerifySysctl(pluginData); err != nil {
return err
}

}
Expand Down
27 changes: 23 additions & 4 deletions pkg/controller/installation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -411,11 +411,30 @@ var _ = Describe("Installation validation tests", func() {
Expect(err).To(HaveOccurred())
})

It("should pass on allowed CNI sysctl tuning plugin config", func() {
instance.Spec.CalicoNetwork.Sysctl = []operator.Sysctl{
{
Key: "net.ipv4.tcp_keepalive_intvl",
Value: "15",
}, {
Key: "net.ipv4.tcp_keepalive_probes",
Value: "6",
},
{
Key: "net.ipv4.tcp_keepalive_time",
Value: "40",
},
}
err := validateCustomResource(instance)
Expect(err).ShouldNot(HaveOccurred())
})

It("should error on not-allowed CNI sysctl tuning plugin config", func() {
instance.Spec.CalicoNetwork.SysctlTuning = &map[operator.SysctlTuningKey]string{
"net.ipv4.not_allowed_setting": "15",
"net.ipv4.tcp_keepalive_probes": "6",
"net.ipv4.tcp_keepalive_time": "40",
instance.Spec.CalicoNetwork.Sysctl = []operator.Sysctl{
{
Key: "net.ipv4.not_allowed_parameter",
Value: "1",
},
}
err := validateCustomResource(instance)
Expect(err).To(HaveOccurred())
Expand Down
29 changes: 17 additions & 12 deletions pkg/controller/migration/convert/network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
operatorv1 "github.com/tigera/operator/api/v1"
v1 "github.com/tigera/operator/api/v1"
"github.com/tigera/operator/pkg/controller/migration/cni"
"github.com/tigera/operator/pkg/controller/utils"
)

const (
Expand Down Expand Up @@ -200,8 +201,8 @@ func handleCalicoCNI(c *components, install *operatorv1.Installation) error {
}

type TuningSpec struct {
Sysctl *map[operatorv1.SysctlTuningKey]string `json:"sysctl,omitempty"`
Type string `json:"type"`
Sysctl []operatorv1.Sysctl `json:"sysctl,omitempty"`
Type string `json:"type"`
}

// CNI tuning plugin
Expand All @@ -217,21 +218,25 @@ func handleCalicoCNI(c *components, install *operatorv1.Installation) error {
}
}

// sysctl settings
allowedKeys := map[operatorv1.SysctlTuningKey]struct{}{
"net.ipv4.tcp_keepalive_intvl": {},
"net.ipv4.tcp_keepalive_probes": {},
"net.ipv4.tcp_keepalive_time": {},
sysctlTuning := []operatorv1.Sysctl{}
for _, setting := range tuningSpecData.Sysctl {
sysctl := operatorv1.Sysctl{
Key: setting.Key,
Value: setting.Value,
}
sysctlTuning = append(sysctlTuning, sysctl)
}
sysctlTuning := make(map[operatorv1.SysctlTuningKey]string)
for key, value := range *tuningSpecData.Sysctl {
if _, allowed := allowedKeys[key]; allowed {
sysctlTuning[key] = value

if err = utils.VerifySysctl(sysctlTuning); err != nil {
return ErrIncompatibleCluster{
err: err.Error(),
component: ComponentCNIConfig,
fix: "remove unsupported Tuning parameter from CNI config",
}
}

if len(sysctlTuning) > 0 {
install.Spec.CalicoNetwork.SysctlTuning = &sysctlTuning
install.Spec.CalicoNetwork.Sysctl = sysctlTuning
}
}

Expand Down
59 changes: 39 additions & 20 deletions pkg/controller/migration/convert/network_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2022 Tigera, Inc. All rights reserved.
// Copyright (c) 2023 Tigera, Inc. All rights reserved.

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -782,11 +782,20 @@ var _ = Describe("Convert network tests", func() {
},
{
"type": "tuning",
"sysctl": {
"net.ipv4.tcp_keepalive_intvl": "15",
"net.ipv4.tcp_keepalive_probes": "6",
"net.ipv4.tcp_keepalive_time": "40"
}
"sysctl": [
{
"key": "net.ipv4.tcp_keepalive_intvl",
"value": "15"
},
{
"key": "net.ipv4.tcp_keepalive_probes",
"value": "6"
},
{
"key": "net.ipv4.tcp_keepalive_time",
"value": "40"
}
]
}
]
}`,
Expand All @@ -795,15 +804,23 @@ var _ = Describe("Convert network tests", func() {
cfg, err := Convert(ctx, c)
Expect(err).ToNot(HaveOccurred())
Expect(cfg).ToNot(BeNil())
Expect(cfg.Spec.CalicoNetwork.SysctlTuning).ToNot(BeNil())
Expect(*cfg.Spec.CalicoNetwork.SysctlTuning).To(Equal(map[string]string{
"net.ipv4.tcp_keepalive_time": "40",
"net.ipv4.tcp_keepalive_intvl": "15",
"net.ipv4.tcp_keepalive_probes": "6",
Expect(cfg.Spec.CalicoNetwork.Sysctl).ToNot(BeNil())
Expect(cfg.Spec.CalicoNetwork.Sysctl).To(Equal([]operatorv1.Sysctl{
{
Key: "net.ipv4.tcp_keepalive_intvl",
Value: "15",
}, {
Key: "net.ipv4.tcp_keepalive_probes",
Value: "6",
},
{
Key: "net.ipv4.tcp_keepalive_time",
Value: "40",
},
}))
})

It("no sysctl tuning in config, cfg must be nil", func() {
It("not allowed sysctl tuning in config", func() {
ds := emptyNodeSpec()
ds.Spec.Template.Spec.InitContainers[0].Env = []corev1.EnvVar{{
Name: "CNI_NETWORK_CONFIG",
Expand All @@ -829,21 +846,23 @@ var _ = Describe("Convert network tests", func() {
},
{
"type": "tuning",
"sysctl": {
"net.ipv4.not_allowed": "40"
}
"sysctl": [
{
"key": "net.ipv4.not_allowed",
"value": "40"
}
]
}
]
}`,
}}
c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(ds, emptyKubeControllerSpec(), v4pool, emptyFelixConfig()).Build()
cfg, err := Convert(ctx, c)
Expect(err).ToNot(HaveOccurred())
Expect(cfg).ToNot(BeNil())
Expect(cfg.Spec.CalicoNetwork.SysctlTuning).To(BeNil())
Expect(err).To(HaveOccurred())
Expect(cfg).To(BeNil())
})

It("not allowed sysctl tuning in config", func() {
It("no sysctl tuning in config, cfg must be nil", func() {
ds := emptyNodeSpec()
ds.Spec.Template.Spec.InitContainers[0].Env = []corev1.EnvVar{{
Name: "CNI_NETWORK_CONFIG",
Expand Down Expand Up @@ -874,7 +893,7 @@ var _ = Describe("Convert network tests", func() {
cfg, err := Convert(ctx, c)
Expect(err).ToNot(HaveOccurred())
Expect(cfg).ToNot(BeNil())
Expect(cfg.Spec.CalicoNetwork.SysctlTuning).To(BeNil())
Expect(cfg.Spec.CalicoNetwork.Sysctl).To(BeNil())
})
})

Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/utils/merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,9 @@ func mergeCalicoNetwork(cfg, override *operatorv1.CalicoNetworkSpec) *operatorv1
out.ContainerIPForwarding = override.ContainerIPForwarding
}

switch compareFields(out.SysctlTuning, override.SysctlTuning) {
switch compareFields(out.Sysctl, override.Sysctl) {
case BOnlySet, Different:
out.SysctlTuning = override.SysctlTuning
out.Sysctl = override.Sysctl
}
return out
}
Expand Down
34 changes: 21 additions & 13 deletions pkg/controller/utils/merge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,33 +479,41 @@ var _ = Describe("Installation merge tests", func() {
}),
)

_sysctlTuningA := map[opv1.SysctlTuningKey]string{
"net.ipv4.tcp_keepalive_time": "40",
"net.ipv4.tcp_keepalive_intvl": "15",
"net.ipv4.tcp_keepalive_probes": "6",
_sysctlTuningA := []opv1.Sysctl{
{
Key: "net.ipv4.tcp_keepalive_intvl",
Value: "15",
}, {
Key: "net.ipv4.tcp_keepalive_probes",
Value: "6",
},
{
Key: "net.ipv4.tcp_keepalive_time",
Value: "40",
},
}
_sysctlTuningB := map[opv1.SysctlTuningKey]string{}
DescribeTable("merge CNI Tuning", func(main, second, expect *map[opv1.SysctlTuningKey]string) {
_sysctlTuningB := []opv1.Sysctl{}
DescribeTable("merge CNI Tuning", func(main, second, expect []opv1.Sysctl) {
m := opv1.InstallationSpec{}
s := opv1.InstallationSpec{}
if main != nil {
m.CalicoNetwork = &opv1.CalicoNetworkSpec{SysctlTuning: main}
m.CalicoNetwork = &opv1.CalicoNetworkSpec{Sysctl: main}
}
if second != nil {
s.CalicoNetwork = &opv1.CalicoNetworkSpec{SysctlTuning: second}
s.CalicoNetwork = &opv1.CalicoNetworkSpec{Sysctl: second}
}
inst := OverrideInstallationSpec(m, s)
if expect == nil {
Expect(inst.CalicoNetwork).To(BeNil())
} else {
Expect(*inst.CalicoNetwork.SysctlTuning).To(Equal(*expect))
Expect(inst.CalicoNetwork.Sysctl).To(Equal(expect))
}
},
Entry("Both unset", nil, nil, nil),
Entry("Main only set", &_sysctlTuningA, nil, &_sysctlTuningA),
Entry("Second only set", nil, &_sysctlTuningB, &_sysctlTuningB),
Entry("Both set equal", &_sysctlTuningA, &_sysctlTuningA, &_sysctlTuningA),
Entry("Both set not matching", &_sysctlTuningA, &_sysctlTuningB, &_sysctlTuningB),
Entry("Main only set", _sysctlTuningA, nil, _sysctlTuningA),
Entry("Second only set", nil, _sysctlTuningB, _sysctlTuningB),
Entry("Both set equal", _sysctlTuningA, _sysctlTuningA, _sysctlTuningA),
Entry("Both set not matching", _sysctlTuningA, _sysctlTuningB, _sysctlTuningB),
)

})
Expand Down
15 changes: 15 additions & 0 deletions pkg/controller/utils/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ var (
// most scenarios. Retries should be used sparingly, and only in extraordinary
// circumstances. Use this as a default when retries are needed.
StandardRetry = 30 * time.Second

// AllowedSysctlKeys controls the allowed Sysctl keys can be set in Tuning plugin
AllowedSysctlKeys = map[string]bool{
"net.ipv4.tcp_keepalive_intvl": true,
"net.ipv4.tcp_keepalive_probes": true,
"net.ipv4.tcp_keepalive_time": true,
}
)

// ContextLoggerForResource provides a logger instance with context set for the provided object.
Expand Down Expand Up @@ -844,3 +851,11 @@ func IsDexDisabled(authentication *operatorv1.Authentication) bool {
}
return disableDex
}
func VerifySysctl(pluginData []operatorv1.Sysctl) error {
for _, setting := range pluginData {
if _, ok := AllowedSysctlKeys[setting.Key]; !ok {
return fmt.Errorf("key %s is not allowed in spec.calicoNetwork.sysctl", setting.Key)
}
}
return nil
}
Loading

0 comments on commit d5ec32b

Please sign in to comment.