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.

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

Handle the case when ingress name has dot #532

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 32 additions & 6 deletions pkg/reconciler/ingress/resources/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"hash/adler32"
"regexp"
"sort"
"strings"

Expand All @@ -41,7 +42,11 @@ import (
)

// GatewayHTTPPort is the HTTP port the gateways listen on.
const GatewayHTTPPort = 80
const (
GatewayHTTPPort = 80
dns1123LabelMaxLength = 63 // Public for testing only.
dns1123LabelFmt = "[a-zA-Z0-9](?:[-a-zA-Z0-9]*[a-zA-Z0-9])?"
)

var httpServerPortName = "http-server"

Expand All @@ -58,6 +63,8 @@ var placeholderServer = istiov1alpha3.Server{
},
}

var dns1123LabelRegexp = regexp.MustCompile("^" + dns1123LabelFmt + "$")

// GetServers gets the `Servers` from `Gateway` that belongs to the given Ingress.
func GetServers(gateway *v1alpha3.Gateway, ing *v1alpha1.Ingress) []*istiov1alpha3.Server {
servers := []*istiov1alpha3.Server{}
Expand Down Expand Up @@ -87,7 +94,7 @@ func belongsToIngress(server *istiov1alpha3.Server, ing *v1alpha1.Ingress) bool
if len(portNameSplits) != 2 {
return false
}
return portNameSplits[0] == ing.GetNamespace()+"/"+ing.GetName()
return portNameSplits[0] == portNamePrefix(ing.GetNamespace(), ing.GetName())
}

// SortServers sorts `Server` according to its port name.
Expand Down Expand Up @@ -189,6 +196,16 @@ func makeWildcardGateways(ctx context.Context, originWildcardSecrets map[string]
return gateways, nil
}

// IsDNS1123Label tests for a string that conforms to the definition of a label in
// DNS (RFC 1123).
// This function is copied from https://github.com/istio/istio/blob/806fb24bc121bf93ea06f6a38b7ccb3d78d1f326/pkg/config/labels/instance.go#L97
// We directly copy this function instead of importing it into vendor and using it because
// if this function is changed in the upstream (for example, Istio allows the dot in the future), we don't want to
// import the change without awareness because it could break the compatibility of Gateway name generation.
func isDNS1123Label(value string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I think technically we can either use istio one or k8s one.

But I am inclined to copy the function into this repo because if there is any change to this function in the upstream (e.g. allowing dot in the future), then the gateway name could be changed implicitly and we may not be aware of this.
If the version of the function we use in the our code is not compatible with the requirement of Istio or k8s in the future, then our E2E tests should be failed and we will get notice about it. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point, but this is not some random function, it follows RFC 1123. It is not going to change.
Up to you.

return len(value) <= dns1123LabelMaxLength && dns1123LabelRegexp.MatchString(value)
}

// WildcardGatewayName creates the name of wildcard Gateway.
func WildcardGatewayName(secretName, gatewayServiceNamespace, gatewayServiceName string) string {
return fmt.Sprintf("wildcard-%x", adler32.Checksum([]byte(secretName+"-"+gatewayServiceNamespace+"-"+gatewayServiceName)))
Expand Down Expand Up @@ -263,8 +280,12 @@ func getGatewayServices(ctx context.Context, svcLister corev1listers.ServiceList
// GatewayName create a name for the Gateway that is built based on the given Ingress and bonds to the
// given ingress gateway service.
func GatewayName(accessor kmeta.Accessor, gatewaySvc *corev1.Service) string {
prefix := accessor.GetName()
if !isDNS1123Label(prefix) {
prefix = fmt.Sprint(adler32.Checksum([]byte(prefix)))
}
gatewayServiceKey := fmt.Sprintf("%s/%s", gatewaySvc.Namespace, gatewaySvc.Name)
return fmt.Sprint(accessor.GetName()+"-", adler32.Checksum([]byte(gatewayServiceKey)))
return fmt.Sprint(prefix+"-", adler32.Checksum([]byte(gatewayServiceKey)))
}

// MakeTLSServers creates the expected Gateway TLS `Servers` based on the given IngressTLS.
Expand All @@ -284,12 +305,10 @@ func MakeTLSServers(ing *v1alpha1.Ingress, ingressTLS []v1alpha1.IngressTLS, gat
credentialName = targetSecret(originSecret, ing)
}

port := ing.GetNamespace() + "/" + ing.GetName()

servers[i] = &istiov1alpha3.Server{
Hosts: tls.Hosts,
Port: &istiov1alpha3.Port{
Name: fmt.Sprintf("%s:%d", port, i),
Name: fmt.Sprintf(portNamePrefix(ing.GetNamespace(), ing.GetName())+":%d", i),
Number: 443,
Protocol: "HTTPS",
},
Expand All @@ -304,6 +323,13 @@ func MakeTLSServers(ing *v1alpha1.Ingress, ingressTLS []v1alpha1.IngressTLS, gat
return SortServers(servers), nil
}

func portNamePrefix(prefix, suffix string) string {
if !isDNS1123Label(suffix) {
suffix = fmt.Sprint(adler32.Checksum([]byte(suffix)))
}
return prefix + "/" + suffix
}

// MakeHTTPServer creates a HTTP Gateway `Server` based on the HTTPProtocol
// configuration.
func MakeHTTPServer(httpProtocol network.HTTPProtocol, hosts []string) *istiov1alpha3.Server {
Expand Down
49 changes: 49 additions & 0 deletions pkg/reconciler/ingress/resources/gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ var ingressResource = v1alpha1.Ingress{
Spec: ingressSpec,
}

var ingressResourceWithDotName = v1alpha1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "ingress.com",
Namespace: "test-ns",
},
Spec: ingressSpec,
}

func TestGetServers(t *testing.T) {
servers := GetServers(&gateway, &ingressResource)
expected := []*istiov1alpha3.Server{{
Expand Down Expand Up @@ -799,6 +807,47 @@ func TestMakeIngressTLSGateways(t *testing.T) {
}},
},
}},
}, {
name: "ingress name has dot",

ia: &ingressResourceWithDotName,
originSecrets: originSecrets,
gatewayService: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "istio-ingressgateway",
Namespace: "istio-system",
},
Spec: corev1.ServiceSpec{
Selector: selector,
},
},
want: []*v1alpha3.Gateway{{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%d-%d", adler32.Checksum([]byte("ingress.com")), adler32.Checksum([]byte("istio-system/istio-ingressgateway"))),
Namespace: "test-ns",
OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(&ingressResourceWithDotName)},
Labels: map[string]string{
networking.IngressLabelKey: "ingress.com",
},
},
Spec: istiov1alpha3.Gateway{
Selector: selector,
Servers: []*istiov1alpha3.Server{{
Hosts: []string{"host1.example.com"},
Port: &istiov1alpha3.Port{
Name: fmt.Sprintf("test-ns/%d:0", adler32.Checksum([]byte("ingress.com"))),
Number: 443,
Protocol: "HTTPS",
},
Tls: &istiov1alpha3.ServerTLSSettings{
Mode: istiov1alpha3.ServerTLSSettings_SIMPLE,
ServerCertificate: corev1.TLSCertKey,
PrivateKey: corev1.TLSPrivateKeyKey,
CredentialName: targetSecret(&secret, &ingressResourceWithDotName),
},
}},
},
}},
}, {
name: "error to make gateway because of incorrect originSecrets",
ia: &ingressResource,
Expand Down