From 457af06c7a7cae1a5040330a483bbd01f8e4fdf3 Mon Sep 17 00:00:00 2001
From: Evan Louie <evan.louie@microsoft.com>
Date: Mon, 23 Mar 2020 20:47:54 -0700
Subject: [PATCH] Fix Incorrect Backend SVC for Default IngressRoutes

- Fixed bug where the Traefik IngressRoute generated via `reconcile` for an
  `isDefault` ringed service was pointing to a non-existent backend k8s service
  when `k8sBackend` is not provided.
  - Cause: Incorrect computed value for the IngressRoute
    `spec.routes[].services[].name`; was falling back to `metadata.name` when
    `k8sBackend` was no provided -- which did does not contain the `ringName`.
  - Fix: Expand logic for computing the IngressRoute `metadata.name`.
    1. Always compute the _ringed_ version of the service name.
    2. Use the _ringed_ service name for `metadata.name` when not `isDefault` --
       use non-ringed otherwise.
    3. Use the _ringed_ service name as fallback for the backing k8s service if
       `k8sBackend` not provided -- otherwise compute the _ringed_ `k8sBackend`.
---
 src/lib/traefik/ingress-route.test.ts | 36 +++++++++++++++++++++++----
 src/lib/traefik/ingress-route.ts      | 16 ++++++++----
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/src/lib/traefik/ingress-route.test.ts b/src/lib/traefik/ingress-route.test.ts
index eb713df0e..66d0c0cef 100644
--- a/src/lib/traefik/ingress-route.test.ts
+++ b/src/lib/traefik/ingress-route.test.ts
@@ -160,7 +160,8 @@ describe("TraefikIngressRoute - Unit Tests", () => {
     },
 
     {
-      name: "configured correctly when isDefault === false",
+      name:
+        "!isDefault & k8sBackend => metadata.name === ringed(service-name) && spec.routes[].services[].name == ringed(k8sBackend)",
       actual: (): unknown =>
         create("my-service", "master", 80, "/version/and/path", {
           k8sBackend: "my-k8s-svc",
@@ -174,7 +175,7 @@ describe("TraefikIngressRoute - Unit Tests", () => {
               match: expect.stringMatching(/.*ring.*master/i),
               services: expect.arrayContaining([
                 expect.objectContaining({
-                  name: expect.stringContaining("master"),
+                  name: "my-k8s-svc-master",
                 }),
               ]),
             }),
@@ -184,7 +185,8 @@ describe("TraefikIngressRoute - Unit Tests", () => {
     },
 
     {
-      name: "configured correctly when isDefault === true",
+      name:
+        "isDefault & k8sBackend => metadata.name == serviceName && spec.routes[].services[].name == ringed(k8sBackend)",
       actual: (): unknown =>
         create("my-service", "master", 80, "/version/and/path", {
           k8sBackend: "my-k8s-svc",
@@ -198,7 +200,31 @@ describe("TraefikIngressRoute - Unit Tests", () => {
               match: expect.not.stringMatching(/.*ring.*master/i),
               services: expect.arrayContaining([
                 expect.objectContaining({
-                  name: expect.stringContaining("master"),
+                  name: "my-k8s-svc-master",
+                }),
+              ]),
+            }),
+          ]),
+        },
+      }),
+    },
+
+    {
+      name:
+        "isDefault & !k8sBackend => metadata.name == serviceName && spec.routes[].services[].name == ringed(serviceName)",
+      actual: (): unknown =>
+        create("my-service", "master", 80, "/version/and/path", {
+          isDefault: true,
+        }),
+      expected: expect.objectContaining<PartialIngressRoute>({
+        metadata: { name: "my-service" },
+        spec: {
+          routes: expect.arrayContaining([
+            expect.objectContaining({
+              match: expect.not.stringMatching(/.*ring.*master/i),
+              services: expect.arrayContaining([
+                expect.objectContaining({
+                  name: "my-service-master",
                 }),
               ]),
             }),
@@ -227,7 +253,7 @@ describe("TraefikIngressRoute - Throwable", () => {
     {
       name: "throws when meta.name is invalid: dash (-) prefix",
       actual: (): unknown =>
-        create("-invalid-serivce&name", "valid-ring", 80, "v1"),
+        create("-invalid-service&name", "valid-ring", 80, "v1"),
       throws: true,
     },
 
diff --git a/src/lib/traefik/ingress-route.ts b/src/lib/traefik/ingress-route.ts
index 810ce0a58..d51b05136 100644
--- a/src/lib/traefik/ingress-route.ts
+++ b/src/lib/traefik/ingress-route.ts
@@ -71,8 +71,13 @@ export const create = (
 ): TraefikIngressRoute => {
   const { entryPoints, k8sBackend, middlewares = [], namespace, isDefault } =
     opts ?? {};
-  const name =
-    ringName && !isDefault ? `${serviceName}-${ringName}` : serviceName;
+
+  const ringedServiceName = ringName
+    ? `${serviceName}-${ringName}`
+    : serviceName;
+
+  // IngressRoute name is _ringed_ depending on isDefault
+  const ingressName = isDefault ? serviceName : ringedServiceName;
 
   const routeMatchPathPrefix = `PathPrefix(\`${versionAndPath}\`)`;
   const routeMatchHeaders = isDefault
@@ -82,18 +87,19 @@ export const create = (
     .filter((rule): rule is NonNullable<typeof rule> => !!rule)
     .join(" && ");
 
+  // Compute the _ringed_ k8sBackend if provided - else fallback to the _ringed_ service name
   const backendService =
-    k8sBackend && ringName ? `${k8sBackend}-${ringName}` : name;
+    k8sBackend && ringName ? `${k8sBackend}-${ringName}` : ringedServiceName;
 
   // validate fields
-  dns.assertIsValid("metadata.name", name);
+  dns.assertIsValid("metadata.name", ingressName);
   dns.assertIsValid("spec.routes[].services[].name", backendService);
 
   return {
     apiVersion: "traefik.containo.us/v1alpha1",
     kind: "IngressRoute",
     metadata: {
-      name,
+      name: ingressName,
       ...(namespace ? { namespace } : {}),
     },
     spec: {