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

Jump to bottom

Retrieve ports for rules that use ExternalName Services #985

Merged
merged 5 commits into from
Dec 11, 2020
Merged

Retrieve ports for rules that use ExternalName Services #985

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adc2a33
fix(parser) add port logic for ExternalName
Dec 10, 2020
9007139
test(parser) test port picking for ExternalNames
Dec 10, 2020
7a7ed6c
pr: suggestion
Dec 11, 2020
07c5d06
test(parser) add basic ExternalName E2E test
Dec 11, 2020
cea5675
pr: strip protocol
Dec 11, 2020
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
16 changes: 16 additions & 0 deletions internal/ingress/controller/parser/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,26 @@ func tcpIngressToNetworkingTLS(tls []configurationv1beta1.IngressTLS) []networki
func findPort(svc *corev1.Service, wantPort kongstate.PortDef) (*corev1.ServicePort, error) {
switch wantPort.Mode {
case kongstate.PortModeByNumber:
// ExternalName Services have no port declaration of their own
// We must assume that the user-requested port is valid and construct a ServicePort from it
if svc.Spec.Type == corev1.ServiceTypeExternalName {
servicePort := corev1.ServicePort{
Protocol: "TCP",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Protocol: "TCP",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainest

I think it's fine to leave the stub configuration here since we'll ignore it when creating the Kong configuration later anyway

In such case we should be able to remove the protocol field whatsoever, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Tests pass with it gone, but our coverage isn't great, and my concern is that there's something elsewhere that will try to make use of it and panic if it's nil. The original 0.8 code did include it, so IMO safest thing is to just let that persist. However, grep doesn't show anything in our code that looks like it could use it.

Port: wantPort.Number,
TargetPort: intstr.FromInt(int(wantPort.Number)),
}
return &servicePort, nil
rainest marked this conversation as resolved.
Show resolved Hide resolved
}
for _, port := range svc.Spec.Ports {
if port.Port == wantPort.Number {
return &port, nil
}
}

case kongstate.PortModeByName:
if svc.Spec.Type == corev1.ServiceTypeExternalName {
return nil, fmt.Errorf("rules with an ExternalName service must specify numeric ports")
}
for _, port := range svc.Spec.Ports {
if port.Name == wantPort.Name {
return &port, nil
Expand All @@ -171,6 +184,9 @@ func findPort(svc *corev1.Service, wantPort kongstate.PortDef) (*corev1.ServiceP
}

case kongstate.PortModeImplicit:
if svc.Spec.Type == corev1.ServiceTypeExternalName {
return nil, fmt.Errorf("rules with an ExternalName service must specify numeric ports")
}
if len(svc.Spec.Ports) != 1 {
return nil, fmt.Errorf("in implicit mode, service must have exactly 1 port, has %d", len(svc.Spec.Ports))
}
Expand Down
54 changes: 54 additions & 0 deletions internal/ingress/controller/parser/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3668,6 +3668,20 @@ func TestPickPort(t *testing.T) {
},
}

svc2 := corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "service-2",
Namespace: "foo-namespace",
Annotations: map[string]string{
annotations.IngressClassKey: annotations.DefaultIngressClass,
},
},
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeExternalName,
ExternalName: "external.example.com",
},
}

endpointList := []*corev1.Endpoints{
{
ObjectMeta: metav1.ObjectMeta{Name: "service-0", Namespace: "foo-namespace"},
Expand Down Expand Up @@ -3738,6 +3752,46 @@ func TestPickPort(t *testing.T) {
},
wantTarget: "1.1.1.1:111",
},
{
name: "port by number external name",
objs: store.FakeObjects{
Services: []*corev1.Service{&svc2},
Endpoints: endpointList,

IngressesV1: []*networkingv1.Ingress{
{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "foo-namespace",
Annotations: map[string]string{annotations.IngressClassKey: annotations.DefaultIngressClass},
},
Spec: networkingv1.IngressSpec{
Rules: []networkingv1.IngressRule{
{
Host: "example.com",
IngressRuleValue: networkingv1.IngressRuleValue{
HTTP: &networkingv1.HTTPIngressRuleValue{
Paths: []networkingv1.HTTPIngressPath{
{
Path: "/externalname",
Backend: networkingv1.IngressBackend{
Service: &networkingv1.IngressServiceBackend{
Name: "service-2",
Port: networkingv1.ServiceBackendPort{Number: 222},
},
},
},
},
},
},
},
},
},
},
},
},
wantTarget: "external.example.com:222",
},
{
name: "port by name",
objs: store.FakeObjects{
Expand Down