Skip to content

Commit 7b88acb

Browse files
hown3dhown3d
committed
remove panics
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
1 parent dabeba3 commit 7b88acb

File tree

4 files changed

+109
-62
lines changed

4 files changed

+109
-62
lines changed

‎pkg/controller/actuator.go‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -309,9 +309,12 @@ func (a *actuator) createSeedResources(
309309
return err
310310
}
311311

312-
vpnEnvoyFilterSpec := envoyfilters.BuildVPNEnvoyFilterSpecForHelmChart(
312+
vpnEnvoyFilterSpec, err := envoyfilters.BuildVPNEnvoyFilterSpecForHelmChart(
313313
cluster, spec.Rule, alwaysAllowedCIDRs, istioLabels,
314314
)
315+
if err != nil {
316+
return err
317+
}
315318

316319
cfg := map[string]interface{}{
317320
"shootName": cluster.Shoot.Status.TechnicalID,
@@ -323,13 +326,17 @@ func (a *actuator) createSeedResources(
323326
defaultLabels, err := a.findDefaultIstioLabels(ctx)
324327
if client.IgnoreNotFound(err) != nil {
325328
return err
326-
} else if err == nil {
327-
// The `nginx-ingress-controller` Gateway object only exists in g/g@v1.89, (introduced with
328-
// https://github.com/gardener/gardener/pull/9038).
329-
// If it doesn't exist yet, we can't apply ACLs to shoot ingresses.
330-
ingressEnvoyFilterSpec := envoyfilters.BuildIngressEnvoyFilterSpecForHelmChart(
331-
cluster, spec.Rule, alwaysAllowedCIDRs, defaultLabels)
329+
}
330+
// The `nginx-ingress-controller` Gateway object only exists in g/g@v1.89, (introduced with
331+
// https://github.com/gardener/gardener/pull/9038).
332+
// If it doesn't exist yet, we can't apply ACLs to shoot ingresses.
333+
ingressEnvoyFilterSpec, err := envoyfilters.BuildIngressEnvoyFilterSpecForHelmChart(
334+
cluster, spec.Rule, alwaysAllowedCIDRs, defaultLabels)
335+
if err != nil {
336+
return err
337+
}
332338

339+
if ingressEnvoyFilterSpec != nil {
333340
cfg["ingressEnvoyFilterSpec"] = ingressEnvoyFilterSpec
334341
}
335342

‎pkg/envoyfilters/envoyfilters.go‎

Lines changed: 76 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,14 @@ type ACLRule struct {
3838
Type string `json:"type"`
3939
}
4040

41-
func (r *ACLRule) actionProto() envoy_rbacv3.RBAC_Action {
41+
func (r *ACLRule) actionProto() (envoy_rbacv3.RBAC_Action, error) {
4242
switch r.Action {
4343
case "DENY":
44-
return envoy_rbacv3.RBAC_DENY
44+
return envoy_rbacv3.RBAC_DENY, nil
4545
case "ALLOW":
46-
return envoy_rbacv3.RBAC_ALLOW
46+
return envoy_rbacv3.RBAC_ALLOW, nil
4747
default:
48-
panic("unknown action")
48+
return -1, fmt.Errorf("unknown action %s", r.Action)
4949
}
5050
}
5151

@@ -57,21 +57,24 @@ type FilterPatch struct {
5757
}
5858

5959
// asStructPB returns FilterPatch represented as a structpb.Struct
60-
func (f *FilterPatch) asStructPB() *structpb.Struct {
60+
func (f *FilterPatch) asStructPB() (*structpb.Struct, error) {
6161
pb, err := structpb.NewStruct(map[string]any{
6262
"name": f.Name,
6363
"typed_config": f.TypedConfig.AsMap(),
6464
})
6565
if err != nil {
66-
// This state is not valid and should not be propergated
67-
panic(err)
66+
return nil, err
6867
}
69-
return pb
68+
return pb, nil
7069
}
7170

7271
// AsMap returns FilterPatch represented as a map[string]interface{}
73-
func (f *FilterPatch) AsMap() map[string]interface{} {
74-
return f.asStructPB().AsMap()
72+
func (f *FilterPatch) AsMap() (map[string]interface{}, error) {
73+
s, err := f.asStructPB()
74+
if err != nil {
75+
return nil, err
76+
}
77+
return s.AsMap(), nil
7578
}
7679

7780
// BuildAPIEnvoyFilterSpecForHelmChart assembles EnvoyFilter patches for API server
@@ -97,28 +100,30 @@ func BuildAPIEnvoyFilterSpecForHelmChart(
97100
// endpoints using the seed ingress domain.
98101
func BuildIngressEnvoyFilterSpecForHelmChart(
99102
cluster *controller.Cluster, rule *ACLRule, alwaysAllowedCIDRs []string, istioLabels map[string]string,
100-
) *istio_networkingv1alpha3.EnvoyFilter {
103+
) (*istio_networkingv1alpha3.EnvoyFilter, error) {
101104
seedIngressDomain := helper.GetSeedIngressDomain(cluster.Seed)
102-
if seedIngressDomain != "" {
103-
shootID := helper.ComputeShortShootID(cluster.Shoot)
105+
if seedIngressDomain == "" {
106+
return nil, nil
107+
}
108+
shootID := helper.ComputeShortShootID(cluster.Shoot)
104109

105-
return &istio_networkingv1alpha3.EnvoyFilter{
106-
WorkloadSelector: &istio_networkingv1alpha3.WorkloadSelector{
107-
Labels: istioLabels,
108-
},
109-
ConfigPatches: []*istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{
110-
ingressConfigPatchFromRule(rule, seedIngressDomain, shootID, alwaysAllowedCIDRs),
111-
},
112-
}
110+
configPatch, err := ingressConfigPatchFromRule(rule, seedIngressDomain, shootID, alwaysAllowedCIDRs)
111+
if err != nil {
112+
return nil, err
113113
}
114-
return nil
114+
return &istio_networkingv1alpha3.EnvoyFilter{
115+
WorkloadSelector: &istio_networkingv1alpha3.WorkloadSelector{
116+
Labels: istioLabels,
117+
},
118+
ConfigPatches: []*istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{configPatch},
119+
}, nil
115120
}
116121

117122
// ingressConfigPatchFromRule creates a network filter patch that can be
118123
// applied to the `GATEWAY` network filter chain matching the wildcard ingress domain.
119124
func ingressConfigPatchFromRule(
120125
rule *ACLRule, seedIngressDomain, shootID string, alwaysAllowedCIDRs []string,
121-
) *istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch {
126+
) (*istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch, error) {
122127
rbacName := "acl-ingress"
123128
ingressSuffix := "-" + shootID + "." + seedIngressDomain
124129

@@ -173,13 +178,16 @@ func ingressConfigPatchFromRule(
173178
}
174179
typedConfig, err := protoMessageToTypedConfig(rbacFilter)
175180
if err != nil {
176-
// this is a error in the code itself, don't return to caller
177-
panic(err)
181+
return nil, err
178182
}
179183
filter := &FilterPatch{
180184
Name: rbacName,
181185
TypedConfig: typedConfig,
182186
}
187+
pb, err := filter.asStructPB()
188+
if err != nil {
189+
return nil, err
190+
}
183191
return &istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{
184192
ApplyTo: istio_networkingv1alpha3.EnvoyFilter_NETWORK_FILTER,
185193
Match: &istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectMatch{
@@ -194,30 +202,32 @@ func ingressConfigPatchFromRule(
194202
},
195203
Patch: &istio_networkingv1alpha3.EnvoyFilter_Patch{
196204
Operation: istio_networkingv1alpha3.EnvoyFilter_Patch_INSERT_FIRST,
197-
Value: filter.asStructPB(),
205+
Value: pb,
198206
},
199-
}
207+
}, nil
200208
}
201209

202210
// BuildVPNEnvoyFilterSpecForHelmChart assembles EnvoyFilter patches for VPN.
203211
func BuildVPNEnvoyFilterSpecForHelmChart(
204212
cluster *controller.Cluster, rule *ACLRule, alwaysAllowedCIDRs []string, istioLabels map[string]string,
205-
) *istio_networkingv1alpha3.EnvoyFilter {
213+
) (*istio_networkingv1alpha3.EnvoyFilter, error) {
214+
patch, err := vpnConfigPatchFromRule(rule, helper.ComputeShortShootID(cluster.Shoot), cluster.Shoot.Status.TechnicalID, alwaysAllowedCIDRs)
215+
if err != nil {
216+
return nil, err
217+
}
206218
return &istio_networkingv1alpha3.EnvoyFilter{
207219
WorkloadSelector: &istio_networkingv1alpha3.WorkloadSelector{
208220
Labels: istioLabels,
209221
},
210-
ConfigPatches: []*istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{
211-
vpnConfigPatchFromRule(rule, helper.ComputeShortShootID(cluster.Shoot), cluster.Shoot.Status.TechnicalID, alwaysAllowedCIDRs),
212-
},
213-
}
222+
ConfigPatches: []*istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{patch},
223+
}, nil
214224
}
215225

216226
// vpnConfigPatchFromRule creates an HTTP filter patch that can be applied to the
217227
// `GATEWAY` HTTP filter chain for the VPN.
218228
func vpnConfigPatchFromRule(rule *ACLRule,
219229
shortShootID, technicalShootID string, alwaysAllowedCIDRs []string,
220-
) *istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch {
230+
) (*istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch, error) {
221231
rbacName := "acl-vpn"
222232
headerMatcher := envoy_routev3.HeaderMatcher{
223233
Name: "reversed-vpn",
@@ -280,13 +290,17 @@ func vpnConfigPatchFromRule(rule *ACLRule,
280290
}
281291
typedConfig, err := protoMessageToTypedConfig(rbacFilter)
282292
if err != nil {
283-
// this is a error in the code itself, don't return to caller
284-
panic(err)
293+
return nil, err
285294
}
286295
filterPatch := &FilterPatch{
287296
Name: rbacName,
288297
TypedConfig: typedConfig,
289298
}
299+
pb, err := filterPatch.asStructPB()
300+
if err != nil {
301+
return nil, err
302+
}
303+
290304
return &istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{
291305
ApplyTo: istio_networkingv1alpha3.EnvoyFilter_HTTP_FILTER,
292306
Match: &istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectMatch{
@@ -299,9 +313,9 @@ func vpnConfigPatchFromRule(rule *ACLRule,
299313
},
300314
Patch: &istio_networkingv1alpha3.EnvoyFilter_Patch{
301315
Operation: istio_networkingv1alpha3.EnvoyFilter_Patch_INSERT_FIRST,
302-
Value: filterPatch.asStructPB(),
316+
Value: pb,
303317
},
304-
}
318+
}, nil
305319
}
306320

307321
// CreateAPIConfigPatchFromRule combines an ACLRule, the first entry of the
@@ -315,7 +329,14 @@ func CreateAPIConfigPatchFromRule(
315329
}
316330
rbacName := "acl-api"
317331
principals := ruleCIDRsToPrincipal(rule, alwaysAllowedCIDRs)
318-
332+
action, err := rule.actionProto()
333+
if err != nil {
334+
return nil, err
335+
}
336+
patch, err := principalsToPatch(rbacName, action, principals)
337+
if err != nil {
338+
return nil, err
339+
}
319340
return &istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectPatch{
320341
ApplyTo: istio_networkingv1alpha3.EnvoyFilter_NETWORK_FILTER,
321342
Match: &istio_networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectMatch{
@@ -328,27 +349,30 @@ func CreateAPIConfigPatchFromRule(
328349
},
329350
},
330351
},
331-
Patch: principalsToPatch(rbacName, rule.actionProto(), principals),
352+
Patch: patch,
332353
}, nil
333354
}
334355

335356
func principalsToPatch(
336357
rbacName string, ruleAction envoy_rbacv3.RBAC_Action, principals []*envoy_rbacv3.Principal,
337-
) *istio_networkingv1alpha3.EnvoyFilter_Patch {
358+
) (*istio_networkingv1alpha3.EnvoyFilter_Patch, error) {
338359
rbacFilter := newRBACFilter(rbacName, ruleAction, principals)
339360
typedConfig, err := protoMessageToTypedConfig(rbacFilter)
340361
if err != nil {
341-
// this is a error in the code itself, don't return to caller
342-
panic(err)
362+
return nil, err
343363
}
344364
filter := &FilterPatch{
345365
Name: rbacName,
346366
TypedConfig: typedConfig,
347367
}
368+
pb, err := filter.asStructPB()
369+
if err != nil {
370+
return nil, err
371+
}
348372
return &istio_networkingv1alpha3.EnvoyFilter_Patch{
349373
Operation: istio_networkingv1alpha3.EnvoyFilter_Patch_INSERT_FIRST,
350-
Value: filter.asStructPB(),
351-
}
374+
Value: pb,
375+
}, nil
352376
}
353377

354378
// CreateInternalFilterPatchFromRule combines an ACLRule, the
@@ -357,20 +381,23 @@ func CreateInternalFilterPatchFromRule(
357381
rule *ACLRule,
358382
alwaysAllowedCIDRs []string,
359383
shootSpecificCIDRs []string,
360-
) *FilterPatch {
384+
) (*FilterPatch, error) {
361385
rbacName := "acl-internal"
362386
principals := ruleCIDRsToPrincipal(rule, append(alwaysAllowedCIDRs, shootSpecificCIDRs...))
363-
rbacFilter := newRBACFilter(rbacName, rule.actionProto(), principals)
387+
action, err := rule.actionProto()
388+
if err != nil {
389+
return nil, err
390+
}
391+
rbacFilter := newRBACFilter(rbacName, action, principals)
364392
typedConfig, err := protoMessageToTypedConfig(rbacFilter)
365393
if err != nil {
366-
// this is a error in the code itself, don't return to caller
367-
panic(err)
394+
return nil, err
368395
}
369396

370397
return &FilterPatch{
371398
Name: rbacName + "-" + strings.ToLower(rule.Type),
372399
TypedConfig: typedConfig,
373-
}
400+
}, nil
374401
}
375402

376403
// ruleCIDRsToPrincipal translates a list of strings in the form "0.0.0.0/0"

‎pkg/envoyfilters/envoyfilters_test.go‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,8 @@ var _ = Describe("EnvoyFilter Unit Tests", func() {
6868
"app": "istio-ingressgateway",
6969
"istio": "ingressgateway",
7070
}
71-
ingressEnvoyFilterSpec := BuildIngressEnvoyFilterSpecForHelmChart(cluster, rule, alwaysAllowedCIDRs, labels)
71+
ingressEnvoyFilterSpec, err := BuildIngressEnvoyFilterSpecForHelmChart(cluster, rule, alwaysAllowedCIDRs, labels)
72+
Expect(err).NotTo(HaveOccurred())
7273

7374
checkIfFilterEquals(ingressEnvoyFilterSpec, "ingressEnvoyFilterSpecWithOneAllowRule.yaml")
7475
})
@@ -79,7 +80,9 @@ var _ = Describe("EnvoyFilter Unit Tests", func() {
7980
"app": "istio-ingressgateway",
8081
"istio": "ingressgateway",
8182
}
82-
ingressEnvoyFilterSpec := BuildIngressEnvoyFilterSpecForHelmChart(cluster, rule, alwaysAllowedCIDRs, labels)
83+
ingressEnvoyFilterSpec, err := BuildIngressEnvoyFilterSpecForHelmChart(cluster, rule, alwaysAllowedCIDRs, labels)
84+
85+
Expect(err).NotTo(HaveOccurred())
8386
Expect(ingressEnvoyFilterSpec).To(BeNil())
8487
})
8588
})
@@ -93,7 +96,9 @@ var _ = Describe("EnvoyFilter Unit Tests", func() {
9396
"app": "istio-ingressgateway",
9497
"istio": "ingressgateway",
9598
}
96-
result := BuildVPNEnvoyFilterSpecForHelmChart(cluster, rule, alwaysAllowedCIDRs, labels)
99+
result, err := BuildVPNEnvoyFilterSpecForHelmChart(cluster, rule, alwaysAllowedCIDRs, labels)
100+
101+
Expect(err).NotTo(HaveOccurred())
97102
checkIfFilterEquals(result, "vpnEnvoyFilterSpecWithOneAllowRule.yaml")
98103
})
99104
})
@@ -104,7 +109,8 @@ var _ = Describe("EnvoyFilter Unit Tests", func() {
104109
It("Should create a filter spec matching the expected one, including the always allowed CIDRs", func() {
105110
rule := createRule("ALLOW", "remote_ip", "0.0.0.0/0")
106111

107-
result := CreateInternalFilterPatchFromRule(rule, alwaysAllowedCIDRs, []string{})
112+
result, err := CreateInternalFilterPatchFromRule(rule, alwaysAllowedCIDRs, []string{})
113+
Expect(err).NotTo(HaveOccurred())
108114
checkIfFilterEquals(result, "singleFiltersAllowEntry.yaml")
109115
})
110116
})

‎pkg/webhook/webhook.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,17 @@ func (e *EnvoyFilterWebhook) createAdmissionResponse(
121121
if originalFilter == nil {
122122
return admission.Errored(http.StatusBadRequest, fmt.Errorf("filter with name \"envoy.filters.network.tcp_proxy\" not present in EnvoyFilter %s/%s", filter.Namespace, filter.Name))
123123
}
124-
filterPatch := envoyfilters.CreateInternalFilterPatchFromRule(extSpec.Rule, alwaysAllowedCIDRs, shootSpecificCIRDs)
124+
filterPatch, err := envoyfilters.CreateInternalFilterPatchFromRule(extSpec.Rule, alwaysAllowedCIDRs, shootSpecificCIRDs)
125+
if err != nil {
126+
return admission.Errored(http.StatusInternalServerError, err)
127+
}
125128

129+
filterPatchMap, err := filterPatch.AsMap()
130+
if err != nil {
131+
return admission.Errored(http.StatusInternalServerError, err)
132+
}
126133
// make sure the original filter is the last
127-
filterPatches := []map[string]interface{}{filterPatch.AsMap(), originalFilter.AsMap()}
134+
filterPatches := []map[string]interface{}{filterPatchMap, originalFilter.AsMap()}
128135

129136
return buildAdmissionResponseWithFilterPatches(filterPatches)
130137
}

0 commit comments

Comments
 (0)