fix: operator retries and error logging

src/pepr/operator/controllers/keycloak/client-sync.ts

@@ -6,11 +6,16 @@ import { Sso, UDSPackage } from "../../crd";
 import { getOwnerRef } from "../utils";
 import { Client } from "./types";
 
-const apiURL =
+let apiURL =
   "http://keycloak-http.keycloak.svc.cluster.local:8080/realms/uds/clients-registrations/default";
 const samlDescriptorUrl =
   "http://keycloak-http.keycloak.svc.cluster.local:8080/realms/uds/protocol/saml/descriptor";
 
+// Support dev mode with port-forwarded keycloak svc
+if (process.env.PEPR_MODE === "dev") {
+  apiURL = "http://localhost:8080/realms/uds/clients-registrations/default";
+}
+
 // Template regex to match clientField() references, see https://regex101.com/r/e41Dsk/3 for details
 const secretTemplateRegex = new RegExp(
   'clientField\\(([a-zA-Z]+)\\)(?:\\["?([\\w]+)"?\\]|(\\.json\\(\\)))?',
@@ -78,14 +83,12 @@ async function syncClient(
   isRetry = false,
 ) {
   Log.debug(pkg.metadata, `Processing client request: ${clientReq.clientId}`);
+  // Not including the CR data in the ref because Keycloak client IDs must be unique already
+  const name = `sso-client-${clientReq.clientId}`;
+  let client: Client;
 
   try {
-    // Not including the CR data in the ref because Keycloak client IDs must be unique already
-    const name = `sso-client-${clientReq.clientId}`;
     const token = Store.getItem(name);
-
-    let client: Client;
-
     // If an existing client is found, update it
     if (token && !isRetry) {
       Log.debug(pkg.metadata, `Found existing token for ${clientReq.clientId}`);
@@ -94,52 +97,52 @@ async function syncClient(
       Log.debug(pkg.metadata, `Creating new client for ${clientReq.clientId}`);
       client = await apiCall(clientReq);
     }
-
-    // Write the new token to the store
-    await Store.setItemAndWait(name, client.registrationAccessToken!);
-
-    // Remove the registrationAccessToken from the client object to avoid problems (one-time use token)
-    delete client.registrationAccessToken;
-
-    if (clientReq.protocol === "saml") {
-      client.samlIdpCertificate = await getSamlCertificate();
-    }
-
-    // Create or update the client secret
-    await K8s(kind.Secret).Apply({
-      metadata: {
-        namespace: pkg.metadata!.namespace,
-        // Use the CR secret name if provided, otherwise use the client name
-        name: secretName || name,
-        labels: {
-          "uds/package": pkg.metadata!.name,
-        },
-        // Use the CR as the owner ref for each VirtualService
-        ownerReferences: getOwnerRef(pkg),
-      },
-      data: generateSecretData(client, secretTemplate),
-    });
-
-    if (isAuthSvcClient) {
-      // Do things here
-    }
-
-    return name;
   } catch (err) {
     const msg =
       `Failed to process client request '${clientReq.clientId}' for ` +
-      `${pkg.metadata?.namespace}/${pkg.metadata?.name}. This can occur if a client already exists with the same ID that Pepr isn't tracking.`;
+      `${pkg.metadata?.namespace}/${pkg.metadata?.name}. Error: ${err.message}`;
     Log.error({ err }, msg);
 
     if (isRetry) {
       Log.error(`${msg}, retry failed, aborting`);
-      throw new Error(`${msg}. RETRY FAILED, aborting: ${JSON.stringify(err)}`);
+      throw new Error(msg);
     }
 
     // Retry the request
     Log.warn(`${msg}, retrying`);
     return syncClient(clientReq, pkg, true);
   }
+
+  // Write the new token to the store
+  await Store.setItemAndWait(name, client.registrationAccessToken!);
+
+  // Remove the registrationAccessToken from the client object to avoid problems (one-time use token)
+  delete client.registrationAccessToken;
+
+  if (clientReq.protocol === "saml") {
+    client.samlIdpCertificate = await getSamlCertificate();
+  }
+
+  // Create or update the client secret
+  await K8s(kind.Secret).Apply({
+    metadata: {
+      namespace: pkg.metadata!.namespace,
+      // Use the CR secret name if provided, otherwise use the client name
+      name: secretName || name,
+      labels: {
+        "uds/package": pkg.metadata!.name,
+      },
+      // Use the CR as the owner ref for each VirtualService
+      ownerReferences: getOwnerRef(pkg),
+    },
+    data: generateSecretData(client, secretTemplate),
+  });
+
+  if (isAuthSvcClient) {
+    // Do things here
+  }
+
+  return name;
 }
 
 async function apiCall(sso: Partial<Sso>, method = "POST", authToken = "") {
@@ -166,7 +169,8 @@ async function apiCall(sso: Partial<Sso>, method = "POST", authToken = "") {
   // When not creating a new client, add the client ID and registrationAccessToken
   if (authToken) {
     req.headers.Authorization = `Bearer ${authToken}`;
-    url += `/${sso.clientId}`;
+    // Ensure that we URI encode the clientId in the request URL
+    url += `/${encodeURIComponent(sso.clientId!)}`;
   }
 
   // Remove the body for DELETE requests
@@ -178,7 +182,11 @@ async function apiCall(sso: Partial<Sso>, method = "POST", authToken = "") {
   const resp = await fetch<Client>(url, req);
 
   if (!resp.ok) {
-    throw new Error(`Failed to ${method} client: ${resp.statusText}`);
+    if (resp.data) {
+      throw new Error(`${JSON.stringify(resp.statusText)}, ${JSON.stringify(resp.data)}`);
+    } else {
+      throw new Error(`${JSON.stringify(resp.statusText)}`);
+    }
   }
 
   return resp.data;

src/pepr/operator/crd/generated/package-v1alpha1.ts

@@ -547,6 +547,7 @@ export interface Status {
 export enum Phase {
   Failed = "Failed",
   Pending = "Pending",
+  PendingRetry = "Pending Retry",
   Ready = "Ready",
 }
 

src/pepr/operator/crd/sources/package/v1alpha1.ts

@@ -363,7 +363,7 @@ export const v1alpha1: V1CustomResourceDefinitionVersion = {
               type: "integer",
             },
             phase: {
-              enum: ["Pending", "Ready", "Failed"],
+              enum: ["Pending", "Pending Retry", "Ready", "Failed"],
               type: "string",
             },
             ssoClients: {

src/pepr/operator/crd/validators/package-validator.ts

@@ -86,6 +86,12 @@ export async function validator(req: PeprValidateRequest<UDSPackage>) {
       return req.Deny(`The client ID "${client.clientId}" is not unique`);
     }
     clientIDs.add(client.clientId);
+    // Don't allow illegal k8s resource names for the secret name
+    if (client.secretName && client.secretName !== sanitizeResourceName(client.secretName)) {
+      return req.Deny(
+        `The client ID "${client.clientId}" uses an invalid secret name ${client.secretName}`,
+      );
+    }
   }
 
   return req.Approve();

src/pepr/operator/reconcilers/index.spec.ts

@@ -179,9 +179,7 @@ describe("handleFailure", () => {
 
     expect(PatchStatus).toHaveBeenCalledWith({
       metadata: { namespace: "default", name: "test" },
-      status: {
-        retryAttempt: 1,
-      },
+      status: { phase: Phase.PendingRetry, retryAttempt: 1 },
     });
   });
 

src/pepr/operator/reconcilers/index.ts

@@ -108,6 +108,7 @@ export async function handleFailure(err: { status: number; message: string }, cr
     Log.error({ err }, `Reconciliation attempt ${currRetry} failed for ${identifier}, retrying...`);
 
     status = {
+      phase: Phase.PendingRetry,
       retryAttempt: currRetry,
     };
   } else {
@@ -116,6 +117,7 @@ export async function handleFailure(err: { status: number; message: string }, cr
     status = {
       phase: Phase.Failed,
       observedGeneration: metadata.generation,
+      retryAttempt: 0, // todo: make this nullable when kfc generates the type
     };
   }
 

src/pepr/operator/reconcilers/package-reconciler.ts

@@ -30,6 +30,9 @@ export async function packageReconciler(pkg: UDSPackage) {
   // Migrate the package to the latest version
   migrate(pkg);
 
+  // Update to indicate this version of pepr-core has attempted to reconcile the package once
+  uidSeen.add(pkg.metadata!.uid!);
+
   // Configure the namespace and namespace-wide network policies
   try {
     await updateStatus(pkg, { phase: Phase.Pending });
@@ -64,9 +67,6 @@ export async function packageReconciler(pkg: UDSPackage) {
       observedGeneration: metadata.generation,
       retryAttempt: 0, // todo: make this nullable when kfc generates the type
     });
-
-    // Update to indicate this version of pepr-core has reconciled the package successfully once
-    uidSeen.add(pkg.metadata!.uid!);
   } catch (err) {
     void handleFailure(err, pkg);
   }

src/pepr/tasks.yaml

@@ -6,9 +6,9 @@ tasks:
   - name: gen-crds
     description: "Generate CRDS, requires a running kubernetes cluster"
     actions:
-      - cmd: "npx ts-node src/pepr/operator/crd/register.ts"
+      - cmd: npx ts-node -e "import { registerCRDs } from './src/pepr/operator/crd/register'; registerCRDs()"
         env:
-          - "PEPR_WATCH_MODE=true"
+          - "PEPR_MODE=dev"
 
       - cmd: "npx kubernetes-fluent-client crd packages.uds.dev src/pepr/operator/crd/generated"