Skip to content

Commit e43f546

Browse files
kishorjTimothy-Dougherty
kishorj
authored and
Timothy-Dougherty
committed
fix restricted sg rules for named target port (kubernetes-sigs#2327)
1 parent 081b46e commit e43f546

File tree

6 files changed

+294
-18
lines changed

6 files changed

+294
-18
lines changed

‎helm/aws-load-balancer-controller/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,4 +212,4 @@ The default values set by the application itself can be confirmed [here](https:/
212212
| `enableEndpointSlices` | If enabled, controller uses k8s EndpointSlices instead of Endpoints for IP targets | `false` |
213213
| `enableBackendSecurityGroup` | If enabled, controller uses shared security group for backend traffic | `true` |
214214
| `backendSecurityGroup` | Backend security group to use instead of auto created one if the feature is enabled | `` |
215-
| `disableRestrictedSecurityGroupRules` | If disabled, controller will not specify port range restriction in the backend security group rules | `true` |
215+
| `disableRestrictedSecurityGroupRules` | If disabled, controller will not specify port range restriction in the backend security group rules | `false` |

‎helm/aws-load-balancer-controller/values.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,4 +224,4 @@ enableBackendSecurityGroup:
224224
backendSecurityGroup:
225225

226226
# disableRestrictedSecurityGroupRules specifies whether to disable creating port-range restricted security group rules for traffic
227-
disableRestrictedSecurityGroupRules: true
227+
disableRestrictedSecurityGroupRules:

‎pkg/ingress/model_build_target_group.go

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,11 @@ func (t *defaultModelBuildTask) buildTargetGroup(ctx context.Context,
3030
if tg, exists := t.tgByResID[tgResID]; exists {
3131
return tg, nil
3232
}
33-
34-
tgSpec, err := t.buildTargetGroupSpec(ctx, ing, svc, port)
33+
svcPort, err := k8s.LookupServicePort(svc, port)
34+
if err != nil {
35+
return nil, err
36+
}
37+
tgSpec, err := t.buildTargetGroupSpec(ctx, ing, svc, port, svcPort)
3538
if err != nil {
3639
return nil, err
3740
}
@@ -41,19 +44,23 @@ func (t *defaultModelBuildTask) buildTargetGroup(ctx context.Context,
4144
}
4245
tg := elbv2model.NewTargetGroup(t.stack, tgResID, tgSpec)
4346
t.tgByResID[tgResID] = tg
44-
_ = t.buildTargetGroupBinding(ctx, tg, svc, port, nodeSelector)
47+
_ = t.buildTargetGroupBinding(ctx, tg, svc, port, svcPort, nodeSelector)
4548
return tg, nil
4649
}
4750

48-
func (t *defaultModelBuildTask) buildTargetGroupBinding(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, nodeSelector *metav1.LabelSelector) *elbv2model.TargetGroupBindingResource {
49-
tgbSpec := t.buildTargetGroupBindingSpec(ctx, tg, svc, port, nodeSelector)
51+
func (t *defaultModelBuildTask) buildTargetGroupBinding(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, svcPort corev1.ServicePort, nodeSelector *metav1.LabelSelector) *elbv2model.TargetGroupBindingResource {
52+
tgbSpec := t.buildTargetGroupBindingSpec(ctx, tg, svc, port, svcPort, nodeSelector)
5053
tgb := elbv2model.NewTargetGroupBindingResource(t.stack, tg.ID(), tgbSpec)
5154
return tgb
5255
}
5356

54-
func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, nodeSelector *metav1.LabelSelector) elbv2model.TargetGroupBindingResourceSpec {
57+
func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, svcPort corev1.ServicePort, nodeSelector *metav1.LabelSelector) elbv2model.TargetGroupBindingResourceSpec {
5558
targetType := elbv2api.TargetType(tg.Spec.TargetType)
56-
tgbNetworking := t.buildTargetGroupBindingNetworking(ctx, tg.Spec.Port, *tg.Spec.HealthCheckConfig.Port)
59+
targetPort := svcPort.TargetPort
60+
if targetType == elbv2api.TargetTypeInstance {
61+
targetPort = intstr.FromInt(int(svcPort.NodePort))
62+
}
63+
tgbNetworking := t.buildTargetGroupBindingNetworking(ctx, targetPort, *tg.Spec.HealthCheckConfig.Port)
5764
return elbv2model.TargetGroupBindingResourceSpec{
5865
Template: elbv2model.TargetGroupBindingTemplate{
5966
ObjectMeta: metav1.ObjectMeta{
@@ -75,7 +82,7 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context,
7582
}
7683
}
7784

78-
func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context, targetGroupPort int64, healthCheckPort intstr.IntOrString) *elbv2model.TargetGroupBindingNetworking {
85+
func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context, targetPort intstr.IntOrString, healthCheckPort intstr.IntOrString) *elbv2model.TargetGroupBindingNetworking {
7986
if t.backendSGIDToken == nil {
8087
return nil
8188
}
@@ -103,10 +110,9 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Co
103110
}
104111
var networkingPorts []elbv2api.NetworkingPort
105112
var networkingRules []elbv2model.NetworkingIngressRule
106-
tgPort := intstr.FromInt(int(targetGroupPort))
107113
networkingPorts = append(networkingPorts, elbv2api.NetworkingPort{
108114
Protocol: &protocolTCP,
109-
Port: &tgPort,
115+
Port: &targetPort,
110116
})
111117
if healthCheckPort.String() != healthCheckPortTrafficPort {
112118
networkingPorts = append(networkingPorts, elbv2api.NetworkingPort{
@@ -132,7 +138,7 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Co
132138
}
133139

134140
func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
135-
ing ClassifiedIngress, svc *corev1.Service, port intstr.IntOrString) (elbv2model.TargetGroupSpec, error) {
141+
ing ClassifiedIngress, svc *corev1.Service, port intstr.IntOrString, svcPort corev1.ServicePort) (elbv2model.TargetGroupSpec, error) {
136142
svcAndIngAnnotations := algorithm.MergeStringMap(svc.Annotations, ing.Ing.Annotations)
137143
targetType, err := t.buildTargetGroupTargetType(ctx, svcAndIngAnnotations)
138144
if err != nil {
@@ -158,10 +164,6 @@ func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
158164
if err != nil {
159165
return elbv2model.TargetGroupSpec{}, err
160166
}
161-
svcPort, err := k8s.LookupServicePort(svc, port)
162-
if err != nil {
163-
return elbv2model.TargetGroupSpec{}, err
164-
}
165167
ipAddressType, err := t.buildTargetGroupIPAddressType(ctx, svc)
166168
if err != nil {
167169
return elbv2model.TargetGroupSpec{}, err

‎pkg/ingress/model_builder_test.go

Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,23 @@ func Test_defaultModelBuilder_Build(t *testing.T) {
147147
},
148148
}
149149

150+
svcWithNamedTargetPort := &corev1.Service{
151+
ObjectMeta: metav1.ObjectMeta{
152+
Namespace: "ns-1",
153+
Name: "svc-named-targetport",
154+
},
155+
Spec: corev1.ServiceSpec{
156+
Ports: []corev1.ServicePort{
157+
{
158+
Name: "https",
159+
Port: 443,
160+
TargetPort: intstr.FromString("target-port"),
161+
NodePort: 32768,
162+
},
163+
},
164+
},
165+
}
166+
150167
resolveViaDiscoveryCallForInternalLB := resolveViaDiscoveryCall{
151168
subnets: []*ec2sdk.Subnet{
152169
{
@@ -3507,6 +3524,224 @@ func Test_defaultModelBuilder_Build(t *testing.T) {
35073524
},
35083525
wantErr: errors.New("ingress: ns-1/ing-1: unsupported IPv6 configuration, lb not dual-stack"),
35093526
},
3527+
{
3528+
name: "target type IP with named target port",
3529+
env: env{
3530+
svcs: []*corev1.Service{svcWithNamedTargetPort},
3531+
},
3532+
fields: fields{
3533+
resolveViaDiscoveryCalls: []resolveViaDiscoveryCall{resolveViaDiscoveryCallForInternalLB},
3534+
listLoadBalancersCalls: []listLoadBalancersCall{listLoadBalancerCallForEmptyLB},
3535+
enableBackendSG: true,
3536+
},
3537+
args: args{
3538+
ingGroup: Group{
3539+
ID: GroupID{Namespace: "ns-1", Name: "ing-1"},
3540+
Members: []ClassifiedIngress{
3541+
{
3542+
Ing: &networking.Ingress{ObjectMeta: metav1.ObjectMeta{
3543+
Namespace: "ns-1",
3544+
Name: "ing-1",
3545+
Annotations: map[string]string{
3546+
"alb.ingress.kubernetes.io/target-type": "ip",
3547+
},
3548+
},
3549+
Spec: networking.IngressSpec{
3550+
Rules: []networking.IngressRule{
3551+
{
3552+
IngressRuleValue: networking.IngressRuleValue{
3553+
HTTP: &networking.HTTPIngressRuleValue{
3554+
Paths: []networking.HTTPIngressPath{
3555+
{
3556+
Path: "/",
3557+
Backend: networking.IngressBackend{
3558+
ServiceName: svcWithNamedTargetPort.Name,
3559+
ServicePort: intstr.FromString("https"),
3560+
},
3561+
},
3562+
},
3563+
},
3564+
},
3565+
},
3566+
},
3567+
},
3568+
},
3569+
},
3570+
},
3571+
},
3572+
},
3573+
wantStackJSON: `
3574+
{
3575+
"id":"ns-1/ing-1",
3576+
"resources":{
3577+
"AWS::EC2::SecurityGroup":{
3578+
"ManagedLBSecurityGroup":{
3579+
"spec":{
3580+
"groupName":"k8s-ns1-ing1-bd83176788",
3581+
"description":"[k8s] Managed SecurityGroup for LoadBalancer",
3582+
"ingress":[
3583+
{
3584+
"ipProtocol":"tcp",
3585+
"fromPort":80,
3586+
"toPort":80,
3587+
"ipRanges":[
3588+
{
3589+
"cidrIP":"0.0.0.0/0"
3590+
}
3591+
]
3592+
}
3593+
]
3594+
}
3595+
}
3596+
},
3597+
"AWS::ElasticLoadBalancingV2::Listener":{
3598+
"80":{
3599+
"spec":{
3600+
"loadBalancerARN":{
3601+
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::LoadBalancer/LoadBalancer/status/loadBalancerARN"
3602+
},
3603+
"port":80,
3604+
"protocol":"HTTP",
3605+
"defaultActions":[
3606+
{
3607+
"type":"fixed-response",
3608+
"fixedResponseConfig":{
3609+
"contentType":"text/plain",
3610+
"statusCode":"404"
3611+
}
3612+
}
3613+
]
3614+
}
3615+
}
3616+
},
3617+
"AWS::ElasticLoadBalancingV2::ListenerRule":{
3618+
"80:1":{
3619+
"spec":{
3620+
"listenerARN":{
3621+
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::Listener/80/status/listenerARN"
3622+
},
3623+
"priority":1,
3624+
"actions":[
3625+
{
3626+
"type":"forward",
3627+
"forwardConfig":{
3628+
"targetGroups":[
3629+
{
3630+
"targetGroupARN":{
3631+
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/ns-1/ing-1-svc-named-targetport:https/status/targetGroupARN"
3632+
}
3633+
}
3634+
]
3635+
}
3636+
}
3637+
],
3638+
"conditions":[
3639+
{
3640+
"field":"path-pattern",
3641+
"pathPatternConfig":{
3642+
"values":[
3643+
"/"
3644+
]
3645+
}
3646+
}
3647+
]
3648+
}
3649+
}
3650+
},
3651+
"AWS::ElasticLoadBalancingV2::LoadBalancer":{
3652+
"LoadBalancer":{
3653+
"spec":{
3654+
"name":"k8s-ns1-ing1-b7e914000d",
3655+
"type":"application",
3656+
"scheme":"internal",
3657+
"ipAddressType":"ipv4",
3658+
"subnetMapping":[
3659+
{
3660+
"subnetID":"subnet-a"
3661+
},
3662+
{
3663+
"subnetID":"subnet-b"
3664+
}
3665+
],
3666+
"securityGroups":[
3667+
{
3668+
"$ref":"#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID"
3669+
},
3670+
"sg-auto"
3671+
]
3672+
}
3673+
}
3674+
},
3675+
"AWS::ElasticLoadBalancingV2::TargetGroup":{
3676+
"ns-1/ing-1-svc-named-targetport:https":{
3677+
"spec":{
3678+
"name":"k8s-ns1-svcnamed-3430e53ee8",
3679+
"targetType":"ip",
3680+
"ipAddressType":"ipv4",
3681+
"port":1,
3682+
"protocol":"HTTP",
3683+
"protocolVersion":"HTTP1",
3684+
"healthCheckConfig":{
3685+
"port":"traffic-port",
3686+
"protocol":"HTTP",
3687+
"path":"/",
3688+
"matcher":{
3689+
"httpCode":"200"
3690+
},
3691+
"intervalSeconds":15,
3692+
"timeoutSeconds":5,
3693+
"healthyThresholdCount":2,
3694+
"unhealthyThresholdCount":2
3695+
}
3696+
}
3697+
}
3698+
},
3699+
"K8S::ElasticLoadBalancingV2::TargetGroupBinding":{
3700+
"ns-1/ing-1-svc-named-targetport:https":{
3701+
"spec":{
3702+
"template":{
3703+
"metadata":{
3704+
"name":"k8s-ns1-svcnamed-3430e53ee8",
3705+
"namespace":"ns-1",
3706+
"creationTimestamp":null
3707+
},
3708+
"spec":{
3709+
"targetGroupARN":{
3710+
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/ns-1/ing-1-svc-named-targetport:https/status/targetGroupARN"
3711+
},
3712+
"targetType":"ip",
3713+
"ipAddressType":"ipv4",
3714+
"serviceRef":{
3715+
"name":"svc-named-targetport",
3716+
"port":"https"
3717+
},
3718+
"networking":{
3719+
"ingress":[
3720+
{
3721+
"from":[
3722+
{
3723+
"securityGroup":{
3724+
"groupID": "sg-auto"
3725+
}
3726+
}
3727+
],
3728+
"ports":[
3729+
{
3730+
"port": "target-port",
3731+
"protocol":"TCP"
3732+
}
3733+
]
3734+
}
3735+
]
3736+
}
3737+
}
3738+
}
3739+
}
3740+
}
3741+
}
3742+
}
3743+
}`,
3744+
},
35103745
}
35113746
for _, tt := range tests {
35123747
t.Run(tt.name, func(t *testing.T) {

‎test/e2e/ingress/vanilla_ingress_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,34 @@ var _ = Describe("vanilla ingress tests", func() {
219219
})
220220
})
221221

222+
Context("with ALB IP targets and named target port", func() {
223+
It("with 'alb.ingress.kubernetes.io/target-type' annotation explicitly specified, one ALB shall be created and functional", func() {
224+
appBuilder := manifest.NewFixedResponseServiceBuilder().WithTargetPortName("e2e-targetport")
225+
ingBuilder := manifest.NewIngressBuilder()
226+
dp, svc := appBuilder.Build(sandboxNS.Name, "app")
227+
ingBackend := networking.IngressBackend{ServiceName: svc.Name, ServicePort: intstr.FromInt(80)}
228+
ing := ingBuilder.
229+
AddHTTPRoute("", networking.HTTPIngressPath{Path: "/path", Backend: ingBackend}).
230+
WithAnnotations(map[string]string{
231+
"kubernetes.io/ingress.class": "alb",
232+
"alb.ingress.kubernetes.io/scheme": "internet-facing",
233+
"alb.ingress.kubernetes.io/target-type": "ip",
234+
}).Build(sandboxNS.Name, "ing")
235+
resStack := fixture.NewK8SResourceStack(tf, dp, svc, ing)
236+
resStack.Setup(ctx)
237+
defer resStack.TearDown(ctx)
238+
239+
lbARN, lbDNS := ExpectOneLBProvisionedForIngress(ctx, tf, ing)
240+
241+
// test traffic
242+
ExpectLBDNSBeAvailable(ctx, tf, lbARN, lbDNS)
243+
httpExp := httpexpect.New(tf.Logger, fmt.Sprintf("http://%v", lbDNS))
244+
httpExp.GET("/path").Expect().
245+
Status(http.StatusOK).
246+
Body().Equal("Hello World!")
247+
})
248+
})
249+
222250
Context("with `alb.ingress.kubernetes.io/actions.${action-name}` variant settings", func() {
223251
It("with annotation based actions, one ALB shall be created and functional", func() {
224252
appBuilder := manifest.NewFixedResponseServiceBuilder()

0 commit comments

Comments
 (0)