From 238416e6483f11dcabc4b61990f8c83880ad7d22 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Fri, 10 Mar 2023 16:30:40 -0800 Subject: [PATCH] Add InboundCIDRs field to IngressClassParams (#3089) --- .../elbv2/v1beta1/ingressclassparams_types.go | 4 + apis/elbv2/v1beta1/zz_generated.deepcopy.go | 5 + .../elbv2.k8s.aws_ingressclassparams.yaml | 6 + docs/guide/ingress/ingress_class.md | 5 + .../crds/crds.yaml | 6 + pkg/ingress/model_build_listener.go | 17 ++- pkg/ingress/model_builder_test.go | 126 ++++++++++++++++++ .../elbv2/ingressclassparams_validator.go | 42 ++++++ .../ingressclassparams_validator_test.go | 66 +++++++++ 9 files changed, 273 insertions(+), 4 deletions(-) diff --git a/apis/elbv2/v1beta1/ingressclassparams_types.go b/apis/elbv2/v1beta1/ingressclassparams_types.go index e818061b0..b1d9234c2 100644 --- a/apis/elbv2/v1beta1/ingressclassparams_types.go +++ b/apis/elbv2/v1beta1/ingressclassparams_types.go @@ -99,6 +99,10 @@ type IngressClassParamsSpec struct { // +optional Scheme *LoadBalancerScheme `json:"scheme,omitempty"` + // InboundCIDRs specifies the CIDRs that are allowed to access the Ingresses that belong to IngressClass with this IngressClassParams. + // +optional + InboundCIDRs []string `json:"inboundCIDRs,omitempty"` + // SSLPolicy specifies the SSL Policy for all Ingresses that belong to IngressClass with this IngressClassParams. // +optional SSLPolicy string `json:"sslPolicy,omitEmpty"` diff --git a/apis/elbv2/v1beta1/zz_generated.deepcopy.go b/apis/elbv2/v1beta1/zz_generated.deepcopy.go index 2be9f8246..32aeab3fe 100644 --- a/apis/elbv2/v1beta1/zz_generated.deepcopy.go +++ b/apis/elbv2/v1beta1/zz_generated.deepcopy.go @@ -133,6 +133,11 @@ func (in *IngressClassParamsSpec) DeepCopyInto(out *IngressClassParamsSpec) { *out = new(LoadBalancerScheme) **out = **in } + if in.InboundCIDRs != nil { + in, out := &in.InboundCIDRs, &out.InboundCIDRs + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Subnets != nil { in, out := &in.Subnets, &out.Subnets *out = new(SubnetSelector) diff --git a/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml b/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml index 0cc5fb603..051aa25f5 100644 --- a/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml +++ b/config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml @@ -61,6 +61,12 @@ spec: required: - name type: object + inboundCIDRs: + description: InboundCIDRs specifies the CIDRs that are allowed to + access the Ingresses that belong to IngressClass with this IngressClassParams. + items: + type: string + type: array ipAddressType: description: IPAddressType defines the ip address type for all Ingresses that belong to IngressClass with this IngressClassParams. diff --git a/docs/guide/ingress/ingress_class.md b/docs/guide/ingress/ingress_class.md index d14b8df16..42fd2830d 100644 --- a/docs/guide/ingress/ingress_class.md +++ b/docs/guide/ingress/ingress_class.md @@ -135,6 +135,11 @@ Cluster administrators can use the `scheme` field to restrict the scheme for all 1. If `scheme` specified, all Ingresses with this IngressClass will have the specified scheme. 2. If `scheme` un-specified, Ingresses with this IngressClass can continue to use `alb.ingress.kubernetes.io/scheme annotation` to specify scheme. +#### spec.inboundCIDRs + +Cluster administrators can use the optional `inboundCIDRs` field to specify the CIDRs that are allowed to access the load balancers that belong to this IngressClass. +If the field is specified, LBC will ignore the `alb.ingress.kubernetes.io/inbound-cidrs` annotation. + #### spec.sslPolicy Cluster administrators can use the optional `sslPolicy` field to specify the SSL policy for the load balancers that belong to this IngressClass. diff --git a/helm/aws-load-balancer-controller/crds/crds.yaml b/helm/aws-load-balancer-controller/crds/crds.yaml index f8c45bd6b..78c226660 100644 --- a/helm/aws-load-balancer-controller/crds/crds.yaml +++ b/helm/aws-load-balancer-controller/crds/crds.yaml @@ -60,6 +60,12 @@ spec: required: - name type: object + inboundCIDRs: + description: InboundCIDRs specifies the CIDRs that are allowed to + access the Ingresses that belong to IngressClass with this IngressClassParams. + items: + type: string + type: array ipAddressType: description: IPAddressType defines the ip address type for all Ingresses that belong to IngressClass with this IngressClassParams. diff --git a/pkg/ingress/model_build_listener.go b/pkg/ingress/model_build_listener.go index 9d96166c1..1dc6b2451 100644 --- a/pkg/ingress/model_build_listener.go +++ b/pkg/ingress/model_build_listener.go @@ -107,7 +107,7 @@ type listenPortConfig struct { func (t *defaultModelBuildTask) computeIngressListenPortConfigByPort(ctx context.Context, ing *ClassifiedIngress) (map[int64]listenPortConfig, error) { explicitTLSCertARNs := t.computeIngressExplicitTLSCertARNs(ctx, ing.Ing) explicitSSLPolicy := t.computeIngressExplicitSSLPolicy(ctx, ing) - inboundCIDRv4s, inboundCIDRV6s, err := t.computeIngressExplicitInboundCIDRs(ctx, ing.Ing) + inboundCIDRv4s, inboundCIDRV6s, err := t.computeIngressExplicitInboundCIDRs(ctx, ing) if err != nil { return nil, err } @@ -209,15 +209,24 @@ func (t *defaultModelBuildTask) computeIngressListenPorts(_ context.Context, ing return portAndProtocols, nil } -func (t *defaultModelBuildTask) computeIngressExplicitInboundCIDRs(_ context.Context, ing *networking.Ingress) ([]string, []string, error) { +func (t *defaultModelBuildTask) computeIngressExplicitInboundCIDRs(_ context.Context, ing *ClassifiedIngress) ([]string, []string, error) { var rawInboundCIDRs []string - _ = t.annotationParser.ParseStringSliceAnnotation(annotations.IngressSuffixInboundCIDRs, &rawInboundCIDRs, ing.Annotations) + fromIngressClassParams := false + if ing.IngClassConfig.IngClassParams != nil && len(ing.IngClassConfig.IngClassParams.Spec.InboundCIDRs) != 0 { + rawInboundCIDRs = ing.IngClassConfig.IngClassParams.Spec.InboundCIDRs + fromIngressClassParams = true + } else { + _ = t.annotationParser.ParseStringSliceAnnotation(annotations.IngressSuffixInboundCIDRs, &rawInboundCIDRs, ing.Ing.Annotations) + } var inboundCIDRv4s, inboundCIDRv6s []string for _, cidr := range rawInboundCIDRs { _, _, err := net.ParseCIDR(cidr) if err != nil { - return nil, nil, errors.Wrapf(err, "invalid %v settings on Ingress: %v", annotations.IngressSuffixInboundCIDRs, k8s.NamespacedName(ing)) + if fromIngressClassParams { + return nil, nil, fmt.Errorf("invalid CIDR in IngressClassParams InboundCIDR %s: %w", cidr, err) + } + return nil, nil, fmt.Errorf("invalid %v settings on Ingress: %v: %w", annotations.IngressSuffixInboundCIDRs, k8s.NamespacedName(ing.Ing), err) } if strings.Contains(cidr, ":") { inboundCIDRv6s = append(inboundCIDRv6s, cidr) diff --git a/pkg/ingress/model_builder_test.go b/pkg/ingress/model_builder_test.go index 2474841ac..b3f5ae9f1 100644 --- a/pkg/ingress/model_builder_test.go +++ b/pkg/ingress/model_builder_test.go @@ -1474,6 +1474,132 @@ func Test_defaultModelBuilder_Build(t *testing.T) { "ns-1/ing-1-svc-3:https": null } } +}`, + }, + { + name: "Ingress - inboundCIDRs in IngressClassParams", + env: env{ + svcs: []*corev1.Service{ns_1_svc_1, ns_1_svc_2, ns_1_svc_3}, + }, + fields: fields{ + resolveViaDiscoveryCalls: []resolveViaDiscoveryCall{resolveViaDiscoveryCallForInternalLB}, + listLoadBalancersCalls: []listLoadBalancersCall{listLoadBalancerCallForEmptyLB}, + enableBackendSG: true, + }, + args: args{ + ingGroup: Group{ + ID: GroupID{Namespace: "ns-1", Name: "ing-1"}, + Members: []ClassifiedIngress{ + { + IngClassConfig: ClassConfiguration{ + IngClassParams: &v1beta1.IngressClassParams{ + Spec: v1beta1.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "10.0.0.0/8", + "172.16.0.0/12", + }, + }, + }, + }, + Ing: &networking.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "ing-1", + Annotations: map[string]string{ + "alb.ingress.kubernetes.io/inbound-cidrs": "20.0.0.0/8", + }, + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "app-1.example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/svc-1", + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: ns_1_svc_1.Name, + Port: networking.ServiceBackendPort{ + Name: "http", + }, + }, + }, + }, + { + Path: "/svc-2", + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: ns_1_svc_2.Name, + Port: networking.ServiceBackendPort{ + Name: "http", + }, + }, + }, + }, + }, + }, + }, + }, + { + Host: "app-2.example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/svc-3", + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: ns_1_svc_3.Name, + Port: networking.ServiceBackendPort{ + Name: "https", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + wantStackPatch: ` +{ + "resources": { + "AWS::EC2::SecurityGroup": { + "ManagedLBSecurityGroup": { + "spec": { + "ingress": [ + { + "fromPort": 80, + "ipProtocol": "tcp", + "ipRanges": [ + { + "cidrIP": "10.0.0.0/8" + } + ], + "toPort": 80 + }, + { + "fromPort": 80, + "ipProtocol": "tcp", + "ipRanges": [ + { + "cidrIP": "172.16.0.0/12" + } + ], + "toPort": 80 + } + ] + } + } + } + } }`, }, { diff --git a/webhooks/elbv2/ingressclassparams_validator.go b/webhooks/elbv2/ingressclassparams_validator.go index fbddc3724..68e9ce9ba 100644 --- a/webhooks/elbv2/ingressclassparams_validator.go +++ b/webhooks/elbv2/ingressclassparams_validator.go @@ -2,6 +2,9 @@ package elbv2 import ( "context" + "fmt" + "net" + "strings" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -30,6 +33,7 @@ func (v *ingressClassParamsValidator) Prototype(_ admission.Request) (runtime.Ob func (v *ingressClassParamsValidator) ValidateCreate(ctx context.Context, obj runtime.Object) error { icp := obj.(*elbv2api.IngressClassParams) allErrs := field.ErrorList{} + allErrs = append(allErrs, v.checkInboundCIDRs(icp)...) allErrs = append(allErrs, v.checkSubnetSelectors(icp)...) return allErrs.ToAggregate() @@ -38,6 +42,7 @@ func (v *ingressClassParamsValidator) ValidateCreate(ctx context.Context, obj ru func (v *ingressClassParamsValidator) ValidateUpdate(ctx context.Context, obj runtime.Object, oldObj runtime.Object) error { icp := obj.(*elbv2api.IngressClassParams) allErrs := field.ErrorList{} + allErrs = append(allErrs, v.checkInboundCIDRs(icp)...) allErrs = append(allErrs, v.checkSubnetSelectors(icp)...) return allErrs.ToAggregate() @@ -47,6 +52,43 @@ func (v *ingressClassParamsValidator) ValidateDelete(ctx context.Context, obj ru return nil } +// checkInboundCIDRs will check for valid inboundCIDRs. +func (v *ingressClassParamsValidator) checkInboundCIDRs(icp *elbv2api.IngressClassParams) (allErrs field.ErrorList) { + for idx, cidr := range icp.Spec.InboundCIDRs { + fieldPath := field.NewPath("spec", "inboundCIDRs").Index(idx) + allErrs = append(allErrs, validateCIDR(cidr, fieldPath)...) + } + + return allErrs +} + +// validateCIDR will check for a valid CIDR. +func validateCIDR(cidr string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + ip, ipNet, err := net.ParseCIDR(cidr) + if err != nil { + detail := "Could not be parsed as a CIDR" + if !strings.Contains(cidr, "/") { + ip := net.ParseIP(cidr) + if ip != nil { + if ip.To4() != nil && !strings.Contains(cidr, ":") { + detail += fmt.Sprintf(" (did you mean \"%s/32\")", cidr) + } else { + detail += fmt.Sprintf(" (did you mean \"%s/64\")", cidr) + } + } + } + allErrs = append(allErrs, field.Invalid(fieldPath, cidr, detail)) + } else if !ip.Equal(ipNet.IP) { + maskSize, _ := ipNet.Mask.Size() + detail := fmt.Sprintf("Network contains bits outside prefix (did you mean \"%s/%d\")", ipNet.IP, maskSize) + allErrs = append(allErrs, field.Invalid(fieldPath, cidr, detail)) + } + + return allErrs +} + // checkSubnetSelectors will check for valid SubnetSelectors func (v *ingressClassParamsValidator) checkSubnetSelectors(icp *elbv2api.IngressClassParams) (allErrs field.ErrorList) { if icp.Spec.Subnets != nil { diff --git a/webhooks/elbv2/ingressclassparams_validator_test.go b/webhooks/elbv2/ingressclassparams_validator_test.go index db2c2b0f1..6c312cfef 100644 --- a/webhooks/elbv2/ingressclassparams_validator_test.go +++ b/webhooks/elbv2/ingressclassparams_validator_test.go @@ -18,6 +18,72 @@ func Test_ingressClassParamsValidator_ValidateCreate(t *testing.T) { name: "empty", obj: &elbv2api.IngressClassParams{}, }, + { + name: "inboundCIDRs is valid CIDR list", + obj: &elbv2api.IngressClassParams{ + Spec: elbv2api.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "10.0.0.0/8", + "2001:DB8::/32", + }, + }, + }, + }, + { + name: "inboundCIDRs IPv4 no length", + obj: &elbv2api.IngressClassParams{ + Spec: elbv2api.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "192.168.0.1", + }, + }, + }, + wantErr: "spec.inboundCIDRs[0]: Invalid value: \"192.168.0.1\": Could not be parsed as a CIDR (did you mean \"192.168.0.1/32\")", + }, + { + name: "inboundCIDRs IPv6 no length", + obj: &elbv2api.IngressClassParams{ + Spec: elbv2api.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "2001:DB8::", + }, + }, + }, + wantErr: "spec.inboundCIDRs[0]: Invalid value: \"2001:DB8::\": Could not be parsed as a CIDR (did you mean \"2001:DB8::/64\")", + }, + { + name: "inboundCIDRs bits outside prefix", + obj: &elbv2api.IngressClassParams{ + Spec: elbv2api.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "10.128.0.0/8", + }, + }, + }, + wantErr: "spec.inboundCIDRs[0]: Invalid value: \"10.128.0.0/8\": Network contains bits outside prefix (did you mean \"10.0.0.0/8\")", + }, + { + name: "inboundCIDRs empty string", + obj: &elbv2api.IngressClassParams{ + Spec: elbv2api.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "", + }, + }, + }, + wantErr: "spec.inboundCIDRs[0]: Invalid value: \"\": Could not be parsed as a CIDR", + }, + { + name: "inboundCIDRs domain", + obj: &elbv2api.IngressClassParams{ + Spec: elbv2api.IngressClassParamsSpec{ + InboundCIDRs: []string{ + "invalid.example.com", + }, + }, + }, + wantErr: "spec.inboundCIDRs[0]: Invalid value: \"invalid.example.com\": Could not be parsed as a CIDR", + }, { name: "subnet is valid ID list", obj: &elbv2api.IngressClassParams{