Skip to content

Commit 65f3d96

Browse files
jotakjotak
authored
Code cleanup / refactoring to minimize dependencies in api (#2498)
The api package should keep minimal dependencies, as it is typically the kind of package that other integrating tools may want to import The dependency on github.com/netobserv/network-observability-operator/internal/pkg/cluster was problematic because it pulls a lot of things transitively, including kube clients. So it's removed, and cluster.Info is replaced in the webhook with an interface clusterInfo. The remaining other dependencies have more limited impact.
1 parent 288cfa9 commit 65f3d96

File tree

7 files changed

+98
-65
lines changed

7 files changed

+98
-65
lines changed

‎api/flowcollector/v1beta2/flowcollector_validation_webhook.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,25 @@ import (
1414
"k8s.io/apimachinery/pkg/util/intstr"
1515
logf "sigs.k8s.io/controller-runtime/pkg/log"
1616
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
17+
)
18+
19+
type NetworkType string
1720

18-
"github.com/netobserv/network-observability-operator/internal/pkg/cluster"
21+
const (
22+
OpenShiftSDN NetworkType = "OpenShiftSDN"
23+
OVNKubernetes NetworkType = "OVNKubernetes"
1924
)
2025

26+
type clusterInfo interface {
27+
IsOpenShift() bool
28+
IsOpenShiftVersionAtLeast(v string) (bool, string, error)
29+
GetNbNodes() (uint16, error)
30+
GetCNI() (NetworkType, error)
31+
}
32+
2133
var (
2234
log = logf.Log.WithName("flowcollector-resource")
23-
CurrentClusterInfo *cluster.Info
35+
CurrentClusterInfo clusterInfo
2436
needPrivileged = []AgentFeature{UDNMapping, NetworkEvents}
2537
neededOpenShiftVersion = map[AgentFeature]string{
2638
PacketDrop: "4.14.0",
@@ -98,9 +110,9 @@ func (v *validator) validateNetPol() {
98110
cni, err := CurrentClusterInfo.GetCNI()
99111
if err != nil {
100112
v.warnings = append(v.warnings, fmt.Sprintf("Could not detect CNI: %s", err.Error()))
101-
} else if cni == cluster.OpenShiftSDN && v.fc.NetworkPolicy.Enable != nil && *v.fc.NetworkPolicy.Enable {
113+
} else if cni == OpenShiftSDN && v.fc.NetworkPolicy.Enable != nil && *v.fc.NetworkPolicy.Enable {
102114
v.warnings = append(v.warnings, "OpenShiftSDN detected with unsupported setting: spec.networkPolicy.enable; this setting will be ignored; to remove this warning set spec.networkPolicy.enable to false.")
103-
} else if cni != cluster.OVNKubernetes && v.fc.DeployNetworkPolicyOtherCNI() {
115+
} else if cni != OVNKubernetes && v.fc.DeployNetworkPolicyOtherCNI() {
104116
v.warnings = append(v.warnings, "Network policy is enabled via spec.networkPolicy.enable, despite not running OVN-Kubernetes: this configuration has not been tested; to remove this warning set spec.networkPolicy.enable to false.")
105117
}
106118
} else {

‎api/flowcollector/v1beta2/flowcollector_validation_webhook_test.go‎

Lines changed: 49 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ package v1beta2
22

33
import (
44
"context"
5+
"errors"
56
"testing"
67

7-
"github.com/netobserv/network-observability-operator/internal/pkg/cluster"
8+
"github.com/coreos/go-semver/semver"
89
"github.com/stretchr/testify/assert"
910
corev1 "k8s.io/api/core/v1"
1011
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -13,6 +14,40 @@ import (
1314
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
1415
)
1516

17+
type clusterInfoMock struct {
18+
cni NetworkType
19+
version string
20+
}
21+
22+
func (m *clusterInfoMock) IsOpenShift() bool {
23+
return m.version != ""
24+
}
25+
26+
func (m *clusterInfoMock) IsOpenShiftVersionAtLeast(v string) (bool, string, error) {
27+
if m.version == "" {
28+
return false, "", errors.New("unknown OpenShift version, cannot compare versions")
29+
}
30+
openshiftVersion, err := semver.NewVersion(m.version)
31+
if err != nil {
32+
return false, "", err
33+
}
34+
version, err := semver.NewVersion(v)
35+
if err != nil {
36+
return false, "", err
37+
}
38+
// Ignore pre-release block for comparison
39+
openshiftVersion.PreRelease = ""
40+
return !openshiftVersion.LessThan(*version), m.version, nil
41+
}
42+
43+
func (m *clusterInfoMock) GetNbNodes() (uint16, error) {
44+
return 1, nil
45+
}
46+
47+
func (m *clusterInfoMock) GetCNI() (NetworkType, error) {
48+
return m.cni, nil
49+
}
50+
1651
func TestValidateAgent(t *testing.T) {
1752
tests := []struct {
1853
name string
@@ -437,9 +472,8 @@ func TestValidateAgent(t *testing.T) {
437472
},
438473
}
439474

440-
CurrentClusterInfo = &cluster.Info{}
441475
for _, test := range tests {
442-
CurrentClusterInfo.Mock(test.ocpVersion, "")
476+
CurrentClusterInfo = &clusterInfoMock{version: test.ocpVersion}
443477
v := validator{fc: &test.fc.Spec}
444478
v.validateAgent()
445479
if test.expectedError == "" {
@@ -527,8 +561,7 @@ func TestValidateConntrack(t *testing.T) {
527561
}
528562

529563
r := FlowCollector{}
530-
CurrentClusterInfo = &cluster.Info{}
531-
CurrentClusterInfo.Mock("", "")
564+
CurrentClusterInfo = &clusterInfoMock{}
532565
for _, test := range tests {
533566
warnings, err := r.Validate(context.TODO(), test.fc)
534567
if test.expectedError == "" {
@@ -862,10 +895,9 @@ func TestValidateFLP(t *testing.T) {
862895
},
863896
}
864897

865-
CurrentClusterInfo = &cluster.Info{}
866898
r := FlowCollector{}
867899
for _, test := range tests {
868-
CurrentClusterInfo.Mock(test.ocpVersion, "")
900+
CurrentClusterInfo = &clusterInfoMock{version: test.ocpVersion}
869901
warnings, err := r.Validate(context.TODO(), test.fc)
870902
if test.expectedError == "" {
871903
assert.NoError(t, err, test.name)
@@ -1021,10 +1053,9 @@ func TestValidateScheduling(t *testing.T) {
10211053
},
10221054
}
10231055

1024-
CurrentClusterInfo = &cluster.Info{}
10251056
r := FlowCollector{}
10261057
for _, test := range tests {
1027-
CurrentClusterInfo.Mock(test.ocpVersion, "")
1058+
CurrentClusterInfo = &clusterInfoMock{version: test.ocpVersion}
10281059
warnings, err := r.Validate(context.TODO(), test.fc)
10291060
if test.expectedError == "" {
10301061
assert.NoError(t, err, test.name)
@@ -1063,7 +1094,7 @@ func TestValidateNetPol(t *testing.T) {
10631094
tests := []struct {
10641095
name string
10651096
fc *FlowCollector
1066-
cni cluster.NetworkType
1097+
cni NetworkType
10671098
expectedError string
10681099
expectedWarnings admission.Warnings
10691100
}{
@@ -1075,7 +1106,7 @@ func TestValidateNetPol(t *testing.T) {
10751106
},
10761107
Spec: FlowCollectorSpec{},
10771108
},
1078-
cni: cluster.OVNKubernetes,
1109+
cni: OVNKubernetes,
10791110
},
10801111
{
10811112
name: "Empty config is valid for sdn",
@@ -1085,7 +1116,7 @@ func TestValidateNetPol(t *testing.T) {
10851116
},
10861117
Spec: FlowCollectorSpec{},
10871118
},
1088-
cni: cluster.OpenShiftSDN,
1119+
cni: OpenShiftSDN,
10891120
},
10901121
{
10911122
name: "Empty config is valid for unknown",
@@ -1107,7 +1138,7 @@ func TestValidateNetPol(t *testing.T) {
11071138
NetworkPolicy: NetworkPolicy{Enable: ptr.To(true)},
11081139
},
11091140
},
1110-
cni: cluster.OVNKubernetes,
1141+
cni: OVNKubernetes,
11111142
},
11121143
{
11131144
name: "Enabled netpol triggers warning for sdn",
@@ -1119,7 +1150,7 @@ func TestValidateNetPol(t *testing.T) {
11191150
NetworkPolicy: NetworkPolicy{Enable: ptr.To(true)},
11201151
},
11211152
},
1122-
cni: cluster.OpenShiftSDN,
1153+
cni: OpenShiftSDN,
11231154
expectedWarnings: admission.Warnings{"OpenShiftSDN detected with unsupported setting: spec.networkPolicy.enable; this setting will be ignored; to remove this warning set spec.networkPolicy.enable to false."},
11241155
},
11251156
{
@@ -1137,9 +1168,11 @@ func TestValidateNetPol(t *testing.T) {
11371168
},
11381169
}
11391170

1140-
CurrentClusterInfo = &cluster.Info{}
11411171
for _, test := range tests {
1142-
CurrentClusterInfo.Mock("4.20.0", test.cni)
1172+
CurrentClusterInfo = &clusterInfoMock{
1173+
version: "4.20.0",
1174+
cni: test.cni,
1175+
}
11431176
v := validator{fc: &test.fc.Spec}
11441177
v.validateNetPol()
11451178
if test.expectedError == "" {

‎internal/controller/ebpf/agent_controller_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ func TestNetworkEventsOVNMount(t *testing.T) {
247247
assert.Equal(t, "/var/run/openvswitch", ds.Spec.Template.Spec.Volumes[2].HostPath.Path)
248248

249249
// OpenShift OVN
250-
info.ClusterInfo.Mock("4.20.0", cluster.OVNKubernetes)
250+
info.ClusterInfo.Mock("4.20.0", flowslatest.OVNKubernetes)
251251
ds, err = agent.desired(context.Background(), &fc)
252252
assert.NoError(t, err)
253253
assert.NotNil(t, ds)

‎internal/controller/networkpolicy/np_controller_test.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
package networkpolicy
33

44
import (
5-
"time"
6-
75
. "github.com/onsi/ginkgo/v2"
86
. "github.com/onsi/gomega"
97
networkingv1 "k8s.io/api/networking/v1"
@@ -18,11 +16,8 @@ import (
1816
)
1917

2018
const (
21-
timeout = test.Timeout
22-
interval = test.Interval
23-
conntrackEndTimeout = 10 * time.Second
24-
conntrackTerminatingTimeout = 5 * time.Second
25-
conntrackHeartbeatInterval = 30 * time.Second
19+
timeout = test.Timeout
20+
interval = test.Interval
2621
)
2722

2823
var (

‎internal/controller/networkpolicy/np_objects.go‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package networkpolicy
33
import (
44
flowslatest "github.com/netobserv/network-observability-operator/api/flowcollector/v1beta2"
55
"github.com/netobserv/network-observability-operator/internal/controller/constants"
6-
"github.com/netobserv/network-observability-operator/internal/pkg/cluster"
76
"github.com/netobserv/network-observability-operator/internal/pkg/helper"
87
"github.com/netobserv/network-observability-operator/internal/pkg/manager"
98
corev1 "k8s.io/api/core/v1"
@@ -50,14 +49,14 @@ func addAllowedNamespaces(np *networkingv1.NetworkPolicy, in, out []string) {
5049
}
5150
}
5251

53-
func buildMainNetworkPolicy(desired *flowslatest.FlowCollector, mgr *manager.Manager, cni cluster.NetworkType, apiServerIPs []string) (types.NamespacedName, *networkingv1.NetworkPolicy) {
52+
func buildMainNetworkPolicy(desired *flowslatest.FlowCollector, mgr *manager.Manager, cni flowslatest.NetworkType, apiServerIPs []string) (types.NamespacedName, *networkingv1.NetworkPolicy) {
5453
ns := desired.Spec.GetNamespace()
5554

5655
name := types.NamespacedName{Name: netpolName, Namespace: ns}
5756
switch cni {
58-
case cluster.OpenShiftSDN:
57+
case flowslatest.OpenShiftSDN:
5958
return name, nil
60-
case cluster.OVNKubernetes:
59+
case flowslatest.OVNKubernetes:
6160
if !desired.Spec.DeployNetworkPolicyOVN() {
6261
return name, nil
6362
}
@@ -205,15 +204,15 @@ func buildMainNetworkPolicy(desired *flowslatest.FlowCollector, mgr *manager.Man
205204
return name, &np
206205
}
207206

208-
func buildPrivilegedNetworkPolicy(desired *flowslatest.FlowCollector, mgr *manager.Manager, cni cluster.NetworkType) (types.NamespacedName, *networkingv1.NetworkPolicy) {
207+
func buildPrivilegedNetworkPolicy(desired *flowslatest.FlowCollector, mgr *manager.Manager, cni flowslatest.NetworkType) (types.NamespacedName, *networkingv1.NetworkPolicy) {
209208
mainNs := desired.Spec.GetNamespace()
210209
privNs := mainNs + constants.EBPFPrivilegedNSSuffix
211210

212211
name := types.NamespacedName{Name: netpolName, Namespace: privNs}
213212
switch cni {
214-
case cluster.OpenShiftSDN:
213+
case flowslatest.OpenShiftSDN:
215214
return name, nil
216-
case cluster.OVNKubernetes:
215+
case flowslatest.OVNKubernetes:
217216
if !desired.Spec.DeployNetworkPolicyOVN() {
218217
return name, nil
219218
}

‎internal/controller/networkpolicy/np_test.go‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -74,23 +74,23 @@ func TestNpBuilder(t *testing.T) {
7474
mgr := &manager.Manager{ClusterInfo: &cluster.Info{}}
7575

7676
desired.Spec.NetworkPolicy.Enable = nil
77-
name, np := buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, nil)
77+
name, np := buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, nil)
7878
assert.Equal(netpolName, name.Name)
7979
assert.Equal("netobserv", name.Namespace)
8080
assert.NotNil(np)
81-
name, np = buildPrivilegedNetworkPolicy(&desired, mgr, cluster.OVNKubernetes)
81+
name, np = buildPrivilegedNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes)
8282
assert.Equal(netpolName, name.Name)
8383
assert.Equal("netobserv-privileged", name.Namespace)
8484
assert.NotNil(np)
8585

8686
desired.Spec.NetworkPolicy.Enable = ptr.To(false)
87-
_, np = buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, nil)
87+
_, np = buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, nil)
8888
assert.Nil(np)
89-
_, np = buildPrivilegedNetworkPolicy(&desired, mgr, cluster.OVNKubernetes)
89+
_, np = buildPrivilegedNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes)
9090
assert.Nil(np)
9191

9292
desired.Spec.NetworkPolicy.Enable = ptr.To(true)
93-
name, np = buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, nil)
93+
name, np = buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, nil)
9494
assert.NotNil(np)
9595
assert.Equal(np.ObjectMeta.Name, name.Name)
9696
assert.Equal(np.ObjectMeta.Namespace, name.Namespace)
@@ -115,14 +115,14 @@ func TestNpBuilder(t *testing.T) {
115115
}},
116116
}, np.Spec.Egress)
117117

118-
name, np = buildPrivilegedNetworkPolicy(&desired, mgr, cluster.OVNKubernetes)
118+
name, np = buildPrivilegedNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes)
119119
assert.NotNil(np)
120120
assert.Equal(np.ObjectMeta.Name, name.Name)
121121
assert.Equal(np.ObjectMeta.Namespace, name.Namespace)
122122
assert.Equal([]networkingv1.NetworkPolicyIngressRule{}, np.Spec.Ingress)
123123

124124
desired.Spec.NetworkPolicy.AdditionalNamespaces = []string{"foo", "bar"}
125-
name, np = buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, nil)
125+
name, np = buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, nil)
126126
assert.NotNil(np)
127127
assert.Equal(np.ObjectMeta.Name, name.Name)
128128
assert.Equal(np.ObjectMeta.Namespace, name.Namespace)
@@ -156,7 +156,7 @@ func TestNpBuilder(t *testing.T) {
156156
}},
157157
}, np.Spec.Egress)
158158

159-
name, np = buildPrivilegedNetworkPolicy(&desired, mgr, cluster.OVNKubernetes)
159+
name, np = buildPrivilegedNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes)
160160
assert.NotNil(np)
161161
assert.Equal(np.ObjectMeta.Name, name.Name)
162162
assert.Equal(np.ObjectMeta.Namespace, name.Namespace)
@@ -170,15 +170,15 @@ func TestNpBuilderSDN(t *testing.T) {
170170
mgr := &manager.Manager{ClusterInfo: &cluster.Info{}}
171171

172172
desired.Spec.NetworkPolicy.Enable = nil
173-
_, np := buildMainNetworkPolicy(&desired, mgr, cluster.OpenShiftSDN, nil)
173+
_, np := buildMainNetworkPolicy(&desired, mgr, flowslatest.OpenShiftSDN, nil)
174174
assert.Nil(np)
175-
_, np = buildPrivilegedNetworkPolicy(&desired, mgr, cluster.OpenShiftSDN)
175+
_, np = buildPrivilegedNetworkPolicy(&desired, mgr, flowslatest.OpenShiftSDN)
176176
assert.Nil(np)
177177

178178
desired.Spec.NetworkPolicy.Enable = ptr.To(true)
179-
_, np = buildMainNetworkPolicy(&desired, mgr, cluster.OpenShiftSDN, nil)
179+
_, np = buildMainNetworkPolicy(&desired, mgr, flowslatest.OpenShiftSDN, nil)
180180
assert.Nil(np)
181-
_, np = buildPrivilegedNetworkPolicy(&desired, mgr, cluster.OpenShiftSDN)
181+
_, np = buildPrivilegedNetworkPolicy(&desired, mgr, flowslatest.OpenShiftSDN)
182182
assert.Nil(np)
183183
}
184184

@@ -206,15 +206,15 @@ func TestNpBuilderWithAPIServerIPs(t *testing.T) {
206206

207207
desired := getConfig()
208208
clusterInfo := &cluster.Info{}
209-
clusterInfo.Mock("4.14.0", cluster.OVNKubernetes) // Mock as OpenShift 4.14 with OVN
209+
clusterInfo.Mock("4.14.0", flowslatest.OVNKubernetes) // Mock as OpenShift 4.14 with OVN
210210
mgr := &manager.Manager{
211211
ClusterInfo: clusterInfo,
212212
Config: &manager.Config{DownstreamDeployment: false},
213213
}
214214

215215
// Test with specific API server IPs (HyperShift scenario)
216216
apiServerIPs := []string{"172.20.0.1", "10.0.0.5"}
217-
_, np := buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, apiServerIPs)
217+
_, np := buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, apiServerIPs)
218218
assert.NotNil(np)
219219

220220
// Verify that we have a single egress rule with multiple IP peers
@@ -231,7 +231,7 @@ func TestNpBuilderWithAPIServerIPs(t *testing.T) {
231231
assert.True(found, "Expected to find a single egress rule with multiple API server IPs")
232232

233233
// Test without API server IPs - should not create the external API server egress rule
234-
_, npWithoutIPs := buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, nil)
234+
_, npWithoutIPs := buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, nil)
235235
assert.NotNil(npWithoutIPs)
236236

237237
// Verify that we do NOT have an egress rule for external API server when IPs are not provided
@@ -251,7 +251,7 @@ func TestNpBuilderWithAPIServerIPs(t *testing.T) {
251251

252252
// Test with IPv6 addresses
253253
apiServerIPsV6 := []string{"2001:db8::1", "2001:db8::2"}
254-
_, npV6 := buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, apiServerIPsV6)
254+
_, npV6 := buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, apiServerIPsV6)
255255
assert.NotNil(npV6)
256256

257257
// Verify IPv6 addresses get /128 CIDR
@@ -268,7 +268,7 @@ func TestNpBuilderWithAPIServerIPs(t *testing.T) {
268268

269269
// Test with mixed IPv4 and IPv6
270270
apiServerIPsMixed := []string{"192.168.1.1", "2001:db8::1"}
271-
_, npMixed := buildMainNetworkPolicy(&desired, mgr, cluster.OVNKubernetes, apiServerIPsMixed)
271+
_, npMixed := buildMainNetworkPolicy(&desired, mgr, flowslatest.OVNKubernetes, apiServerIPsMixed)
272272
assert.NotNil(npMixed)
273273

274274
foundMixed := false

0 commit comments

Comments
 (0)