Skip to content

Commit 0e39ce9

Browse files
zohar7chzohar7ch
authored
Fixed a bug where auto-allow webhook network policy didn't allow communication to the service target port (#597)
1 parent c0ebfc4 commit 0e39ce9

File tree

3 files changed

+122
-53
lines changed

3 files changed

+122
-53
lines changed

‎src/operator/controllers/webhook_traffic/network_policy_handler.go‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ func (n *NetworkPolicyHandler) reduceWebhooksNetpols(ctx context.Context, webhoo
168168
// At this point we want to create the network policy, because the configuration is either set to "always" or
169169
// that it is set to "if blocked by otterize" and the service is blocked by otterize
170170
// TODO: do we also need to create netpol for Otterize?
171-
netpol, err := n.buildNetworkPolicy(ctx, webhookClientConfig.webhookName, webhookClientConfig.clientConfiguration.Service, service)
171+
netpol, err := n.buildNetworkPolicy(ctx, webhookClientConfig.webhookName, service)
172172
if err != nil {
173173
return make(NetworkPolicyWithMetaByName), errors.Wrap(err)
174174
}
@@ -307,7 +307,7 @@ func (n *NetworkPolicyHandler) getWebhookService(ctx context.Context, webhookSer
307307
return service, true, nil
308308
}
309309

310-
func (n *NetworkPolicyHandler) buildNetworkPolicy(ctx context.Context, webhookName string, webhookService *admissionv1.ServiceReference, service *corev1.Service) (v1.NetworkPolicy, error) {
310+
func (n *NetworkPolicyHandler) buildNetworkPolicy(ctx context.Context, webhookName string, service *corev1.Service) (v1.NetworkPolicy, error) {
311311
policyName := fmt.Sprintf("webhook-%s-access-to-%s", strings.ToLower(webhookName), strings.ToLower(service.Name))
312312
rule := v1.NetworkPolicyIngressRule{}
313313

@@ -328,12 +328,17 @@ func (n *NetworkPolicyHandler) buildNetworkPolicy(ctx context.Context, webhookNa
328328
rule.From = append(rule.From, fromControlPlaneIPs...)
329329
}
330330

331+
labelValue := webhookName
332+
if len(labelValue) > 63 {
333+
labelValue = labelValue[:63]
334+
}
335+
331336
newPolicy := v1.NetworkPolicy{
332337
ObjectMeta: metav1.ObjectMeta{
333338
Name: policyName,
334339
Namespace: service.Namespace,
335340
Labels: map[string]string{
336-
v2alpha1.OtterizeNetworkPolicyWebhooks: webhookName,
341+
v2alpha1.OtterizeNetworkPolicyWebhooks: labelValue,
337342
},
338343
},
339344
Spec: v1.NetworkPolicySpec{
@@ -347,13 +352,27 @@ func (n *NetworkPolicyHandler) buildNetworkPolicy(ctx context.Context, webhookNa
347352
},
348353
}
349354

350-
if webhookService.Port != nil {
351-
newPolicy.Spec.Ingress[0].Ports = append(newPolicy.Spec.Ingress[0].Ports, v1.NetworkPolicyPort{
352-
Port: lo.ToPtr(intstr.IntOrString{IntVal: *webhookService.Port, Type: intstr.Int}),
353-
Protocol: lo.ToPtr(corev1.ProtocolTCP),
354-
})
355+
netpolPorts := make([]v1.NetworkPolicyPort, 0, 2*len(service.Spec.Ports))
356+
357+
for _, servicePort := range service.Spec.Ports {
358+
// Add the port
359+
netpolPorts = append(netpolPorts,
360+
v1.NetworkPolicyPort{
361+
Port: lo.ToPtr(intstr.IntOrString{IntVal: servicePort.Port, Type: intstr.Int}),
362+
Protocol: lo.ToPtr(servicePort.Protocol),
363+
})
364+
// Add the target port
365+
if servicePort.TargetPort.IntVal != 0 || servicePort.TargetPort.StrVal != "" {
366+
netpolPorts = append(netpolPorts,
367+
v1.NetworkPolicyPort{
368+
Port: lo.ToPtr(servicePort.TargetPort),
369+
Protocol: lo.ToPtr(servicePort.Protocol),
370+
})
371+
}
355372
}
356373

374+
newPolicy.Spec.Ingress[0].Ports = netpolPorts
375+
357376
return newPolicy, nil
358377
}
359378

‎src/operator/controllers/webhook_traffic/network_policy_handler_test.go‎

Lines changed: 72 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1616
"k8s.io/apimachinery/pkg/runtime"
1717
"k8s.io/apimachinery/pkg/types"
18+
"k8s.io/apimachinery/pkg/util/intstr"
1819
"sigs.k8s.io/controller-runtime/pkg/client"
1920
"testing"
2021
)
@@ -117,6 +118,12 @@ func (s *NetworkPolicyHandlerTestSuite) SetupTest() {
117118
Selector: map[string]string{
118119
"Taylor": "Swift",
119120
},
121+
Ports: []corev1.ServicePort{
122+
{
123+
Port: TestServicePort,
124+
Protocol: corev1.ProtocolTCP,
125+
},
126+
},
120127
},
121128
}
122129

@@ -154,7 +161,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleIfBlocked
154161
s.mockGetControlPlaneIPs()
155162
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
156163

157-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, false)
164+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, false, nil)
158165
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
159166
err := s.handler.HandleAll(context.Background())
160167
s.Require().NoError(err)
@@ -189,43 +196,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleIfBlocked
189196

190197
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
191198

192-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
193-
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
194-
err := s.handler.HandleAll(context.Background())
195-
s.Require().NoError(err)
196-
s.ExpectEvent(ReasonCreatingWebhookTrafficNetpol)
197-
s.ExpectEvent(ReasonCreatingWebhookTrafficNetpolSuccess)
198-
}
199-
200-
func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleIfBlockedByOtterize_ServiceIsBlockedByOtterize_TwoWebhooksToSameServiceDifferentPorts_CreatingOneWebhookPolicy() {
201-
secondPort := int32(1432)
202-
s.validatingWebhook.Webhooks = append(s.validatingWebhook.Webhooks,
203-
admissionv1.ValidatingWebhook{
204-
Name: "Second",
205-
ClientConfig: admissionv1.WebhookClientConfig{
206-
Service: &admissionv1.ServiceReference{
207-
Name: TestServiceName,
208-
Namespace: TestNamespace,
209-
Port: lo.ToPtr(secondPort),
210-
},
211-
},
212-
})
213-
214-
s.mockForReturningValidatingWebhook()
215-
216-
// Called once for "First" webhook
217-
s.mockReturningWebhookService()
218-
s.mockServiceIsBlockedByOtterize(OtterizeIngressNetpols)
219-
s.mockGetControlPlaneIPs()
220-
221-
// Called second time for "Second"" webhook
222-
s.mockReturningWebhookService()
223-
s.mockServiceIsBlockedByOtterize(OtterizeIngressNetpols)
224-
s.mockGetControlPlaneIPs()
225-
226-
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
227-
228-
netpolMatcher := NewNetworkPolicyMatcher([]int32{secondPort, TestServicePort}, s.handler.allowAllIncomingTraffic)
199+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
229200
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
230201
err := s.handler.HandleAll(context.Background())
231202
s.Require().NoError(err)
@@ -272,7 +243,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleIfBlocked
272243
WithFromIPBlock(s.handler.allowAllIncomingTraffic).
273244
Build())
274245

275-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
246+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
276247
s.Client.EXPECT().Patch(gomock.Any(), gomock.All(netpolMatcher), gomock.Any()).Return(nil)
277248
err := s.handler.HandleAll(context.Background())
278249
s.Require().NoError(err)
@@ -325,7 +296,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleOff_Servi
325296
WithFromIPBlock(s.handler.allowAllIncomingTraffic).
326297
Build()})
327298

328-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
299+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
329300
s.Client.EXPECT().Delete(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
330301
err := s.handler.HandleAll(context.Background())
331302
s.Require().NoError(err)
@@ -343,7 +314,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleAlways_Se
343314
s.mockGetControlPlaneIPs()
344315
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
345316

346-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
317+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
347318
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
348319
err := s.handler.HandleAll(context.Background())
349320
s.Require().NoError(err)
@@ -358,7 +329,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_MutatingWebhook
358329
s.mockGetControlPlaneIPs()
359330
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
360331

361-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
332+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
362333
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
363334
err := s.handler.HandleAll(context.Background())
364335
s.Require().NoError(err)
@@ -373,7 +344,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_CRDsWebhooks_Ha
373344
s.mockGetControlPlaneIPs()
374345
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
375346

376-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
347+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
377348
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
378349
err := s.handler.HandleAll(context.Background())
379350
s.Require().NoError(err)
@@ -391,7 +362,64 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleAlways_Al
391362
//s.mockGetControlPlaneIPs()
392363
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
393364

394-
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic)
365+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
366+
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
367+
err := s.handler.HandleAll(context.Background())
368+
s.Require().NoError(err)
369+
s.ExpectEvent(ReasonCreatingWebhookTrafficNetpol)
370+
s.ExpectEvent(ReasonCreatingWebhookTrafficNetpolSuccess)
371+
}
372+
373+
func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleAlways_ServiceHasDifferentTargetPortThanPort_CreatePolicy() {
374+
s.handler = NewNetworkPolicyHandler(s.Client, &runtime.Scheme{}, automate_third_party_network_policy.Always, 32, false)
375+
s.handler.InjectRecorder(s.Recorder)
376+
targetPort := int32(1820)
377+
s.webhookService = &corev1.Service{
378+
ObjectMeta: metav1.ObjectMeta{
379+
Name: TestServiceName,
380+
Namespace: TestNamespace,
381+
},
382+
Spec: corev1.ServiceSpec{
383+
Selector: map[string]string{
384+
"Taylor": "Swift",
385+
},
386+
Ports: []corev1.ServicePort{
387+
{
388+
Port: TestServicePort,
389+
TargetPort: intstr.FromInt32(targetPort),
390+
Protocol: corev1.ProtocolTCP,
391+
},
392+
},
393+
},
394+
}
395+
396+
s.mockForReturningValidatingWebhook()
397+
s.mockReturningWebhookService()
398+
//s.mockServiceIsBlockedByOtterize(make([]v1.NetworkPolicy, 0))
399+
s.mockGetControlPlaneIPs()
400+
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
401+
402+
netpolMatcher := NewNetworkPolicyMatcher([]int32{targetPort, TestServicePort}, s.handler.allowAllIncomingTraffic, nil)
403+
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
404+
err := s.handler.HandleAll(context.Background())
405+
s.Require().NoError(err)
406+
s.ExpectEvent(ReasonCreatingWebhookTrafficNetpol)
407+
s.ExpectEvent(ReasonCreatingWebhookTrafficNetpolSuccess)
408+
}
409+
410+
func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleAlways_WebhookNameTooLong_CreatePolicy() {
411+
s.handler = NewNetworkPolicyHandler(s.Client, &runtime.Scheme{}, automate_third_party_network_policy.Always, 32, false)
412+
s.handler.InjectRecorder(s.Recorder)
413+
s.validatingWebhook = ValidatingWebhookConfiguration.DeepCopy()
414+
s.validatingWebhook.Name = "A-lantern-lit-her-journey-through-the-meadow-A-breeze-whispered-shadows-danced-and-the-forest-echoed-with-silent-wonder"
415+
416+
s.mockForReturningValidatingWebhook()
417+
s.mockReturningWebhookService()
418+
//s.mockServiceIsBlockedByOtterize(make([]v1.NetworkPolicy, 0))
419+
s.mockGetControlPlaneIPs()
420+
s.mockGetExistingOtterizeWebhooksNetpols([]v1.NetworkPolicy{})
421+
422+
netpolMatcher := NewNetworkPolicyMatcher([]int32{TestServicePort}, false, lo.ToPtr("A-lantern-lit-her-journey-through-the-meadow-A-breeze-whispered-shadows-danced-and-the-forest-echoed-with-silent-wonder"))
395423
s.Client.EXPECT().Create(gomock.Any(), gomock.All(netpolMatcher)).Return(nil)
396424
err := s.handler.HandleAll(context.Background())
397425
s.Require().NoError(err)

‎src/operator/controllers/webhook_traffic/network_policy_handler_utils_test.go‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
corev1 "k8s.io/api/core/v1"
88
v1 "k8s.io/api/networking/v1"
99
"k8s.io/apimachinery/pkg/util/intstr"
10+
"strings"
1011

1112
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1213
"reflect"
@@ -39,12 +40,14 @@ type NetworkPolicyBuilder struct {
3940
type NetworkPolicyMatcher struct {
4041
ports []int32
4142
allowAllIncomingTraffic bool
43+
webhookName *string
4244
}
4345

44-
func NewNetworkPolicyMatcher(ports []int32, allowAllIncomingTraffic bool) *NetworkPolicyMatcher {
46+
func NewNetworkPolicyMatcher(ports []int32, allowAllIncomingTraffic bool, webhookName *string) *NetworkPolicyMatcher {
4547
return &NetworkPolicyMatcher{
4648
ports: ports,
4749
allowAllIncomingTraffic: allowAllIncomingTraffic,
50+
webhookName: webhookName,
4851
}
4952
}
5053

@@ -61,6 +64,7 @@ func (m *NetworkPolicyMatcher) Matches(other interface{}) bool {
6164
expectedNetpol := NewNetworkPolicyBuilder(ExpectedNetpol).
6265
WithPorts(m.ports).
6366
WithFromIPBlock(m.allowAllIncomingTraffic).
67+
WithWebhookName(m.webhookName).
6468
Build()
6569

6670
return otherAsNetpol.Namespace == TestNamespace &&
@@ -99,6 +103,24 @@ func (b *NetworkPolicyBuilder) WithFromIPBlock(allowAll bool) *NetworkPolicyBuil
99103
return b
100104
}
101105

106+
func (b *NetworkPolicyBuilder) WithWebhookName(webhookName *string) *NetworkPolicyBuilder {
107+
if webhookName == nil {
108+
return b
109+
}
110+
111+
webhookShortName := *webhookName
112+
if len(webhookShortName) > 63 {
113+
webhookShortName = webhookShortName[:63]
114+
}
115+
b.policy.Labels = map[string]string{
116+
v2alpha1.OtterizeNetworkPolicyWebhooks: webhookShortName,
117+
}
118+
119+
b.policy.Name = fmt.Sprintf("webhook-%s-access-to-test-service", strings.ToLower(*webhookName))
120+
121+
return b
122+
}
123+
102124
func (b *NetworkPolicyBuilder) Build() v1.NetworkPolicy {
103125
return *b.policy
104126
}

0 commit comments

Comments
 (0)