Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix restricted SG rules for named target port #2327

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Fix restricted SG rules for named target port #2327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion helm/aws-load-balancer-controller/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,4 +212,4 @@ The default values set by the application itself can be confirmed [here](https:/
| `enableEndpointSlices` | If enabled, controller uses k8s EndpointSlices instead of Endpoints for IP targets | `false` |
| `enableBackendSecurityGroup` | If enabled, controller uses shared security group for backend traffic | `true` |
| `backendSecurityGroup` | Backend security group to use instead of auto created one if the feature is enabled | `` |
| `disableRestrictedSecurityGroupRules` | If disabled, controller will not specify port range restriction in the backend security group rules | `true` |
| `disableRestrictedSecurityGroupRules` | If disabled, controller will not specify port range restriction in the backend security group rules | `false` |
2 changes: 1 addition & 1 deletion helm/aws-load-balancer-controller/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,4 @@ enableBackendSecurityGroup:
backendSecurityGroup:

# disableRestrictedSecurityGroupRules specifies whether to disable creating port-range restricted security group rules for traffic
disableRestrictedSecurityGroupRules: true
disableRestrictedSecurityGroupRules:
kishorj marked this conversation as resolved.
Show resolved Hide resolved
32 changes: 17 additions & 15 deletions pkg/ingress/model_build_target_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@ func (t *defaultModelBuildTask) buildTargetGroup(ctx context.Context,
if tg, exists := t.tgByResID[tgResID]; exists {
return tg, nil
}

tgSpec, err := t.buildTargetGroupSpec(ctx, ing, svc, port)
svcPort, err := k8s.LookupServicePort(svc, port)
if err != nil {
return nil, err
}
tgSpec, err := t.buildTargetGroupSpec(ctx, ing, svc, port, svcPort)
if err != nil {
return nil, err
}
Expand All @@ -41,19 +44,23 @@ func (t *defaultModelBuildTask) buildTargetGroup(ctx context.Context,
}
tg := elbv2model.NewTargetGroup(t.stack, tgResID, tgSpec)
t.tgByResID[tgResID] = tg
_ = t.buildTargetGroupBinding(ctx, tg, svc, port, nodeSelector)
_ = t.buildTargetGroupBinding(ctx, tg, svc, port, svcPort, nodeSelector)
return tg, nil
}

func (t *defaultModelBuildTask) buildTargetGroupBinding(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, nodeSelector *metav1.LabelSelector) *elbv2model.TargetGroupBindingResource {
tgbSpec := t.buildTargetGroupBindingSpec(ctx, tg, svc, port, nodeSelector)
func (t *defaultModelBuildTask) buildTargetGroupBinding(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, svcPort corev1.ServicePort, nodeSelector *metav1.LabelSelector) *elbv2model.TargetGroupBindingResource {
tgbSpec := t.buildTargetGroupBindingSpec(ctx, tg, svc, port, svcPort, nodeSelector)
tgb := elbv2model.NewTargetGroupBindingResource(t.stack, tg.ID(), tgbSpec)
return tgb
}

func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, nodeSelector *metav1.LabelSelector) elbv2model.TargetGroupBindingResourceSpec {
func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context, tg *elbv2model.TargetGroup, svc *corev1.Service, port intstr.IntOrString, svcPort corev1.ServicePort, nodeSelector *metav1.LabelSelector) elbv2model.TargetGroupBindingResourceSpec {
targetType := elbv2api.TargetType(tg.Spec.TargetType)
tgbNetworking := t.buildTargetGroupBindingNetworking(ctx, tg.Spec.Port, *tg.Spec.HealthCheckConfig.Port)
targetPort := svcPort.TargetPort
if targetType == elbv2api.TargetTypeInstance {
targetPort = intstr.FromInt(int(svcPort.NodePort))
}
tgbNetworking := t.buildTargetGroupBindingNetworking(ctx, targetPort, *tg.Spec.HealthCheckConfig.Port)
return elbv2model.TargetGroupBindingResourceSpec{
Template: elbv2model.TargetGroupBindingTemplate{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -75,7 +82,7 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingSpec(ctx context.Context,
}
}

func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context, targetGroupPort int64, healthCheckPort intstr.IntOrString) *elbv2model.TargetGroupBindingNetworking {
func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Context, targetPort intstr.IntOrString, healthCheckPort intstr.IntOrString) *elbv2model.TargetGroupBindingNetworking {
if t.backendSGIDToken == nil {
return nil
}
Expand Down Expand Up @@ -103,10 +110,9 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Co
}
var networkingPorts []elbv2api.NetworkingPort
var networkingRules []elbv2model.NetworkingIngressRule
tgPort := intstr.FromInt(int(targetGroupPort))
networkingPorts = append(networkingPorts, elbv2api.NetworkingPort{
Protocol: &protocolTCP,
Port: &tgPort,
Port: &targetPort,
})
if healthCheckPort.String() != healthCheckPortTrafficPort {
networkingPorts = append(networkingPorts, elbv2api.NetworkingPort{
Expand All @@ -132,7 +138,7 @@ func (t *defaultModelBuildTask) buildTargetGroupBindingNetworking(ctx context.Co
}

func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
ing ClassifiedIngress, svc *corev1.Service, port intstr.IntOrString) (elbv2model.TargetGroupSpec, error) {
ing ClassifiedIngress, svc *corev1.Service, port intstr.IntOrString, svcPort corev1.ServicePort) (elbv2model.TargetGroupSpec, error) {
svcAndIngAnnotations := algorithm.MergeStringMap(svc.Annotations, ing.Ing.Annotations)
targetType, err := t.buildTargetGroupTargetType(ctx, svcAndIngAnnotations)
if err != nil {
Expand All @@ -158,10 +164,6 @@ func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
if err != nil {
return elbv2model.TargetGroupSpec{}, err
}
svcPort, err := k8s.LookupServicePort(svc, port)
if err != nil {
return elbv2model.TargetGroupSpec{}, err
}
ipAddressType, err := t.buildTargetGroupIPAddressType(ctx, svc)
if err != nil {
return elbv2model.TargetGroupSpec{}, err
Expand Down
235 changes: 235 additions & 0 deletions pkg/ingress/model_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,23 @@ func Test_defaultModelBuilder_Build(t *testing.T) {
},
}

svcWithNamedTargetPort := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "svc-named-targetport",
},
Spec: corev1.ServiceSpec{
Ports: []corev1.ServicePort{
{
Name: "https",
Port: 443,
TargetPort: intstr.FromString("target-port"),
NodePort: 32768,
},
},
},
}

resolveViaDiscoveryCallForInternalLB := resolveViaDiscoveryCall{
subnets: []*ec2sdk.Subnet{
{
Expand Down Expand Up @@ -3507,6 +3524,224 @@ func Test_defaultModelBuilder_Build(t *testing.T) {
},
wantErr: errors.New("ingress: ns-1/ing-1: unsupported IPv6 configuration, lb not dual-stack"),
},
{
name: "target type IP with named target port",
env: env{
svcs: []*corev1.Service{svcWithNamedTargetPort},
},
fields: fields{
resolveViaDiscoveryCalls: []resolveViaDiscoveryCall{resolveViaDiscoveryCallForInternalLB},
listLoadBalancersCalls: []listLoadBalancersCall{listLoadBalancerCallForEmptyLB},
enableBackendSG: true,
},
args: args{
ingGroup: Group{
ID: GroupID{Namespace: "ns-1", Name: "ing-1"},
Members: []ClassifiedIngress{
{
Ing: &networking.Ingress{ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "ing-1",
Annotations: map[string]string{
"alb.ingress.kubernetes.io/target-type": "ip",
},
},
Spec: networking.IngressSpec{
Rules: []networking.IngressRule{
{
IngressRuleValue: networking.IngressRuleValue{
HTTP: &networking.HTTPIngressRuleValue{
Paths: []networking.HTTPIngressPath{
{
Path: "/",
Backend: networking.IngressBackend{
ServiceName: svcWithNamedTargetPort.Name,
ServicePort: intstr.FromString("https"),
},
},
},
},
},
},
},
},
},
},
},
},
},
wantStackJSON: `
{
"id":"ns-1/ing-1",
"resources":{
"AWS::EC2::SecurityGroup":{
"ManagedLBSecurityGroup":{
"spec":{
"groupName":"k8s-ns1-ing1-bd83176788",
"description":"[k8s] Managed SecurityGroup for LoadBalancer",
"ingress":[
{
"ipProtocol":"tcp",
"fromPort":80,
"toPort":80,
"ipRanges":[
{
"cidrIP":"0.0.0.0/0"
}
]
}
]
}
}
},
"AWS::ElasticLoadBalancingV2::Listener":{
"80":{
"spec":{
"loadBalancerARN":{
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::LoadBalancer/LoadBalancer/status/loadBalancerARN"
},
"port":80,
"protocol":"HTTP",
"defaultActions":[
{
"type":"fixed-response",
"fixedResponseConfig":{
"contentType":"text/plain",
"statusCode":"404"
}
}
]
}
}
},
"AWS::ElasticLoadBalancingV2::ListenerRule":{
"80:1":{
"spec":{
"listenerARN":{
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::Listener/80/status/listenerARN"
},
"priority":1,
"actions":[
{
"type":"forward",
"forwardConfig":{
"targetGroups":[
{
"targetGroupARN":{
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/ns-1/ing-1-svc-named-targetport:https/status/targetGroupARN"
}
}
]
}
}
],
"conditions":[
{
"field":"path-pattern",
"pathPatternConfig":{
"values":[
"/"
]
}
}
]
}
}
},
"AWS::ElasticLoadBalancingV2::LoadBalancer":{
"LoadBalancer":{
"spec":{
"name":"k8s-ns1-ing1-b7e914000d",
"type":"application",
"scheme":"internal",
"ipAddressType":"ipv4",
"subnetMapping":[
{
"subnetID":"subnet-a"
},
{
"subnetID":"subnet-b"
}
],
"securityGroups":[
{
"$ref":"#/resources/AWS::EC2::SecurityGroup/ManagedLBSecurityGroup/status/groupID"
},
"sg-auto"
]
}
}
},
"AWS::ElasticLoadBalancingV2::TargetGroup":{
"ns-1/ing-1-svc-named-targetport:https":{
"spec":{
"name":"k8s-ns1-svcnamed-3430e53ee8",
"targetType":"ip",
"ipAddressType":"ipv4",
"port":1,
"protocol":"HTTP",
"protocolVersion":"HTTP1",
"healthCheckConfig":{
"port":"traffic-port",
"protocol":"HTTP",
"path":"/",
"matcher":{
"httpCode":"200"
},
"intervalSeconds":15,
"timeoutSeconds":5,
"healthyThresholdCount":2,
"unhealthyThresholdCount":2
}
}
}
},
"K8S::ElasticLoadBalancingV2::TargetGroupBinding":{
"ns-1/ing-1-svc-named-targetport:https":{
"spec":{
"template":{
"metadata":{
"name":"k8s-ns1-svcnamed-3430e53ee8",
"namespace":"ns-1",
"creationTimestamp":null
},
"spec":{
"targetGroupARN":{
"$ref":"#/resources/AWS::ElasticLoadBalancingV2::TargetGroup/ns-1/ing-1-svc-named-targetport:https/status/targetGroupARN"
},
"targetType":"ip",
"ipAddressType":"ipv4",
"serviceRef":{
"name":"svc-named-targetport",
"port":"https"
},
"networking":{
"ingress":[
{
"from":[
{
"securityGroup":{
"groupID": "sg-auto"
}
}
],
"ports":[
{
"port": "target-port",
"protocol":"TCP"
}
]
}
]
}
}
}
}
}
}
}
}`,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
28 changes: 28 additions & 0 deletions test/e2e/ingress/vanilla_ingress_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,34 @@ var _ = Describe("vanilla ingress tests", func() {
})
})

Context("with ALB IP targets and named target port", func() {
It("with 'alb.ingress.kubernetes.io/target-type' annotation explicitly specified, one ALB shall be created and functional", func() {
appBuilder := manifest.NewFixedResponseServiceBuilder().WithTargetPortName("e2e-targetport")
ingBuilder := manifest.NewIngressBuilder()
dp, svc := appBuilder.Build(sandboxNS.Name, "app")
ingBackend := networking.IngressBackend{ServiceName: svc.Name, ServicePort: intstr.FromInt(80)}
ing := ingBuilder.
AddHTTPRoute("", networking.HTTPIngressPath{Path: "/path", Backend: ingBackend}).
WithAnnotations(map[string]string{
"kubernetes.io/ingress.class": "alb",
"alb.ingress.kubernetes.io/scheme": "internet-facing",
"alb.ingress.kubernetes.io/target-type": "ip",
}).Build(sandboxNS.Name, "ing")
resStack := fixture.NewK8SResourceStack(tf, dp, svc, ing)
resStack.Setup(ctx)
defer resStack.TearDown(ctx)

lbARN, lbDNS := ExpectOneLBProvisionedForIngress(ctx, tf, ing)

// test traffic
ExpectLBDNSBeAvailable(ctx, tf, lbARN, lbDNS)
httpExp := httpexpect.New(tf.Logger, fmt.Sprintf("http://%v", lbDNS))
httpExp.GET("/path").Expect().
Status(http.StatusOK).
Body().Equal("Hello World!")
})
})

Context("with `alb.ingress.kubernetes.io/actions.${action-name}` variant settings", func() {
It("with annotation based actions, one ALB shall be created and functional", func() {
appBuilder := manifest.NewFixedResponseServiceBuilder()
Expand Down
Loading