Skip to content

Commit 2ba762d

Browse files
johngmyersjohngmyers
committed
Validate securityGroups field of IngressClassParams
1 parent c472a5f commit 2ba762d

File tree

2 files changed

+228
-6
lines changed

2 files changed

+228
-6
lines changed

‎webhooks/elbv2/ingressclassparams_validator.go

Lines changed: 66 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strings"
88

99
"k8s.io/apimachinery/pkg/runtime"
10+
"k8s.io/apimachinery/pkg/util/sets"
1011
"k8s.io/apimachinery/pkg/util/validation/field"
1112
elbv2api "sigs.k8s.io/aws-load-balancer-controller/apis/elbv2/v1beta1"
1213
"sigs.k8s.io/aws-load-balancer-controller/pkg/webhook"
@@ -34,6 +35,7 @@ func (v *ingressClassParamsValidator) ValidateCreate(ctx context.Context, obj ru
3435
icp := obj.(*elbv2api.IngressClassParams)
3536
allErrs := field.ErrorList{}
3637
allErrs = append(allErrs, v.checkInboundCIDRs(icp)...)
38+
allErrs = append(allErrs, v.checkSecurityGroupsSelectors(icp)...)
3739
allErrs = append(allErrs, v.checkSubnetSelectors(icp)...)
3840

3941
return allErrs.ToAggregate()
@@ -43,6 +45,7 @@ func (v *ingressClassParamsValidator) ValidateUpdate(ctx context.Context, obj ru
4345
icp := obj.(*elbv2api.IngressClassParams)
4446
allErrs := field.ErrorList{}
4547
allErrs = append(allErrs, v.checkInboundCIDRs(icp)...)
48+
allErrs = append(allErrs, v.checkSecurityGroupsSelectors(icp)...)
4649
allErrs = append(allErrs, v.checkSubnetSelectors(icp)...)
4750

4851
return allErrs.ToAggregate()
@@ -89,6 +92,63 @@ func validateCIDR(cidr string, fieldPath *field.Path) field.ErrorList {
8992
return allErrs
9093
}
9194

95+
// checkSecurityGroupsSelectors will check for valid SubnetSelectors
96+
func (v *ingressClassParamsValidator) checkSecurityGroupsSelectors(icp *elbv2api.IngressClassParams) (allErrs field.ErrorList) {
97+
if icp.Spec.SecurityGroups != nil {
98+
securityGroups := icp.Spec.SecurityGroups
99+
fieldPath := field.NewPath("spec", "securityGroups")
100+
if securityGroups.IDs == nil && !securityGroups.ManagedInbound && securityGroups.Tags == nil {
101+
allErrs = append(allErrs, field.Required(fieldPath, "must have `ids`, `managed`, or `tags`"))
102+
return allErrs
103+
}
104+
if securityGroups.IDs != nil {
105+
if len(icp.Spec.InboundCIDRs) > 0 {
106+
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "inboundCIDRs"), "May not have both `inboundCIDRs` and `securityGroups.ids`"))
107+
}
108+
if securityGroups.ManagedInbound {
109+
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("managedInbound"), "may not have both `ids` and `managedInbound` set"))
110+
return allErrs
111+
}
112+
if securityGroups.Tags != nil {
113+
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("tags"), "may not have both `ids` and `tags` set"))
114+
return allErrs
115+
}
116+
fieldPath := fieldPath.Child("ids")
117+
seen := sets.New[elbv2api.SecurityGroupID]()
118+
for i, securityGroupID := range securityGroups.IDs {
119+
if seen.Has(securityGroupID) {
120+
allErrs = append(allErrs, field.Duplicate(fieldPath.Index(i), securityGroupID))
121+
}
122+
seen.Insert(securityGroupID)
123+
}
124+
} else if securityGroups.ManagedInbound {
125+
if securityGroups.Tags != nil {
126+
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("tags"), "may not have both `managedInbound` and `tags` set"))
127+
}
128+
} else {
129+
fieldPath := fieldPath.Child("tags")
130+
if len(icp.Spec.InboundCIDRs) > 0 {
131+
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "inboundCIDRs"), "May not have both `inboundCIDRs` and `securityGroups.tags`"))
132+
}
133+
if len(securityGroups.Tags) == 0 {
134+
allErrs = append(allErrs, field.Required(fieldPath, "must have at least one tag key"))
135+
}
136+
for tagKey, tagValues := range securityGroups.Tags {
137+
fieldPath := fieldPath.Key(tagKey)
138+
valueSeen := sets.New[string]()
139+
for i, value := range tagValues {
140+
if valueSeen.Has(value) {
141+
allErrs = append(allErrs, field.Duplicate(fieldPath.Index(i), value))
142+
}
143+
valueSeen.Insert(value)
144+
}
145+
}
146+
}
147+
}
148+
149+
return allErrs
150+
}
151+
92152
// checkSubnetSelectors will check for valid SubnetSelectors
93153
func (v *ingressClassParamsValidator) checkSubnetSelectors(icp *elbv2api.IngressClassParams) (allErrs field.ErrorList) {
94154
if icp.Spec.Subnets != nil {
@@ -104,12 +164,12 @@ func (v *ingressClassParamsValidator) checkSubnetSelectors(icp *elbv2api.Ingress
104164
return allErrs
105165
}
106166
fieldPath := fieldPath.Child("ids")
107-
seen := map[elbv2api.SubnetID]bool{}
167+
seen := sets.New[elbv2api.SubnetID]()
108168
for i, subnetID := range subnets.IDs {
109-
if seen[subnetID] {
169+
if seen.Has(subnetID) {
110170
allErrs = append(allErrs, field.Duplicate(fieldPath.Index(i), subnetID))
111171
}
112-
seen[subnetID] = true
172+
seen.Insert(subnetID)
113173
}
114174
} else {
115175
fieldPath := fieldPath.Child("tags")
@@ -118,12 +178,12 @@ func (v *ingressClassParamsValidator) checkSubnetSelectors(icp *elbv2api.Ingress
118178
}
119179
for tagKey, tagValues := range subnets.Tags {
120180
fieldPath := fieldPath.Key(tagKey)
121-
valueSeen := map[string]bool{}
181+
valueSeen := sets.New[string]()
122182
for i, value := range tagValues {
123-
if valueSeen[value] {
183+
if valueSeen.Has(value) {
124184
allErrs = append(allErrs, field.Duplicate(fieldPath.Index(i), value))
125185
}
126-
valueSeen[value] = true
186+
valueSeen.Insert(value)
127187
}
128188
}
129189
}

‎webhooks/elbv2/ingressclassparams_validator_test.go

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,168 @@ func Test_ingressClassParamsValidator_ValidateCreate(t *testing.T) {
8484
},
8585
wantErr: "spec.inboundCIDRs[0]: Invalid value: \"invalid.example.com\": Could not be parsed as a CIDR",
8686
},
87+
{
88+
name: "securityGroups is valid ID list",
89+
obj: &elbv2api.IngressClassParams{
90+
Spec: elbv2api.IngressClassParamsSpec{
91+
SecurityGroups: &elbv2api.SecurityGroupSelector{
92+
IDs: []elbv2api.SecurityGroupID{"sg-1", "sg-2"},
93+
},
94+
},
95+
},
96+
},
97+
{
98+
name: "securityGroups is valid managed",
99+
obj: &elbv2api.IngressClassParams{
100+
Spec: elbv2api.IngressClassParamsSpec{
101+
SecurityGroups: &elbv2api.SecurityGroupSelector{
102+
ManagedInbound: true,
103+
},
104+
},
105+
},
106+
},
107+
{
108+
name: "securityGroups is managedInbound with inboundCIDRs",
109+
obj: &elbv2api.IngressClassParams{
110+
Spec: elbv2api.IngressClassParamsSpec{
111+
InboundCIDRs: []string{
112+
"10.0.0.0/8",
113+
"2001:DB8::/32",
114+
},
115+
SecurityGroups: &elbv2api.SecurityGroupSelector{
116+
ManagedInbound: true,
117+
},
118+
},
119+
},
120+
},
121+
{
122+
name: "securityGroups is valid tag list",
123+
obj: &elbv2api.IngressClassParams{
124+
Spec: elbv2api.IngressClassParamsSpec{
125+
SecurityGroups: &elbv2api.SecurityGroupSelector{
126+
Tags: map[string][]string{
127+
"key": {"value1", "value2"},
128+
},
129+
},
130+
},
131+
},
132+
},
133+
{
134+
name: "securityGroups selector empty",
135+
obj: &elbv2api.IngressClassParams{
136+
Spec: elbv2api.IngressClassParamsSpec{
137+
SecurityGroups: &elbv2api.SecurityGroupSelector{},
138+
},
139+
},
140+
wantErr: "spec.securityGroups: Required value: must have `ids`, `managed`, or `tags`",
141+
},
142+
{
143+
name: "securityGroups selector with both id and managedInbound",
144+
obj: &elbv2api.IngressClassParams{
145+
Spec: elbv2api.IngressClassParamsSpec{
146+
SecurityGroups: &elbv2api.SecurityGroupSelector{
147+
IDs: []elbv2api.SecurityGroupID{"sg-1", "sg-2"},
148+
ManagedInbound: true,
149+
},
150+
},
151+
},
152+
wantErr: "spec.securityGroups.managedInbound: Forbidden: may not have both `ids` and `managedInbound` set",
153+
},
154+
{
155+
name: "securityGroups selector with both id and tag",
156+
obj: &elbv2api.IngressClassParams{
157+
Spec: elbv2api.IngressClassParamsSpec{
158+
SecurityGroups: &elbv2api.SecurityGroupSelector{
159+
IDs: []elbv2api.SecurityGroupID{"sg-1", "sg-2"},
160+
Tags: map[string][]string{
161+
"Name": {"named-subnet"},
162+
},
163+
},
164+
},
165+
},
166+
wantErr: "spec.securityGroups.tags: Forbidden: may not have both `ids` and `tags` set",
167+
},
168+
{
169+
name: "securityGroups selector with both managedInbound and tag",
170+
obj: &elbv2api.IngressClassParams{
171+
Spec: elbv2api.IngressClassParamsSpec{
172+
SecurityGroups: &elbv2api.SecurityGroupSelector{
173+
ManagedInbound: true,
174+
Tags: map[string][]string{
175+
"Name": {"named-subnet"},
176+
},
177+
},
178+
},
179+
},
180+
wantErr: "spec.securityGroups.tags: Forbidden: may not have both `managedInbound` and `tags` set",
181+
},
182+
{
183+
name: "securityGroups id with inboundCIDRs",
184+
obj: &elbv2api.IngressClassParams{
185+
Spec: elbv2api.IngressClassParamsSpec{
186+
InboundCIDRs: []string{
187+
"10.0.0.0/8",
188+
},
189+
SecurityGroups: &elbv2api.SecurityGroupSelector{
190+
IDs: []elbv2api.SecurityGroupID{"sg-1"},
191+
},
192+
},
193+
},
194+
wantErr: "spec.inboundCIDRs: Forbidden: May not have both `inboundCIDRs` and `securityGroups.ids`",
195+
},
196+
{
197+
name: "securityGroups duplicate id",
198+
obj: &elbv2api.IngressClassParams{
199+
Spec: elbv2api.IngressClassParamsSpec{
200+
SecurityGroups: &elbv2api.SecurityGroupSelector{
201+
IDs: []elbv2api.SecurityGroupID{"sg-1", "sg-2", "sg-1"},
202+
},
203+
},
204+
},
205+
wantErr: "spec.securityGroups.ids[2]: Duplicate value: \"sg-1\"",
206+
},
207+
{
208+
name: "securityGroups tag with inboundCIDRs",
209+
obj: &elbv2api.IngressClassParams{
210+
Spec: elbv2api.IngressClassParamsSpec{
211+
InboundCIDRs: []string{
212+
"10.0.0.0/8",
213+
},
214+
SecurityGroups: &elbv2api.SecurityGroupSelector{
215+
Tags: map[string][]string{
216+
"Name": {"name1"},
217+
"Other": {"other1"},
218+
},
219+
},
220+
},
221+
},
222+
wantErr: "spec.inboundCIDRs: Forbidden: May not have both `inboundCIDRs` and `securityGroups.tags`",
223+
},
224+
{
225+
name: "securityGroups duplicate tag value",
226+
obj: &elbv2api.IngressClassParams{
227+
Spec: elbv2api.IngressClassParamsSpec{
228+
SecurityGroups: &elbv2api.SecurityGroupSelector{
229+
Tags: map[string][]string{
230+
"Name": {"name1"},
231+
"Other": {"other1", "other2", "other1"},
232+
},
233+
},
234+
},
235+
},
236+
wantErr: "spec.securityGroups.tags[Other][2]: Duplicate value: \"other1\"",
237+
},
238+
{
239+
name: "securityGroups empty tags map",
240+
obj: &elbv2api.IngressClassParams{
241+
Spec: elbv2api.IngressClassParamsSpec{
242+
SecurityGroups: &elbv2api.SecurityGroupSelector{
243+
Tags: map[string][]string{},
244+
},
245+
},
246+
},
247+
wantErr: "spec.securityGroups.tags: Required value: must have at least one tag key",
248+
},
87249
{
88250
name: "subnet is valid ID list",
89251
obj: &elbv2api.IngressClassParams{

0 commit comments

Comments
 (0)