Simplify provider init (#6706)

improvement(framework): always run `prepareEnvironment` handler

Previously, Garden would first run the `getEnvironmentStatus` handler
and depending on the results, either run the `prepareEnvironment`
handler or not.

Now we always run the `prepareEnvironment` handler (if the provider
status is not cached).

The reason for this change is that the previous flow is an
unneeded abstraction that doesn't serve any purpose at this point and
makes the provider initialisation harder to reason about.

The specific motivation is that there's bug in the Kubernetes
provider where some resources aren't created when the
environment is prepared. It's actually more work to first check
the statuses of these resources, return those, and then create
them. In particular since we have helpers to create them in
an idempotent manner.

That's why now we just always call `prepareEnvironment` and have that
handler figure out what needs to be done.

We still keep the `getEnvironmentStatus` handler for status-only
commands but now it doesn't have side effects. Whereas before the
status handler would e.g. create a namespace.

Plugins can then decide to use that if needed. E.g. the `prepareEnvironment`
handler for the Terraform plugins uses `getEnvironmentStatus` to decide
what needs to happen.

We'll fix the actual bug that prompted this change in a follow up
commit.

core/src/plugin/handlers/Provider/getEnvironmentStatus.ts

@@ -33,11 +33,11 @@ export interface EnvironmentStatusMap {
 
 export const getEnvironmentStatus = () => ({
   description: dedent`
-    Check if the current environment is ready for use by this plugin. Use this action in combination
-    with \`prepareEnvironment\`.
+    Check if the current environment is ready for use by this plugin. Only called
+    with commands that set \`statusOnly: true\`.
 
-    Called before \`prepareEnvironment\`. If this returns \`ready: true\`, the
-    \`prepareEnvironment\` action is not called.
+    This handler MUST NOT have side effects and should only return the status of the
+    environment.
   `,
   paramsSchema: projectActionParamsSchema(),
   resultSchema: environmentStatusSchema(),

core/src/plugin/handlers/Provider/prepareEnvironment.ts

@@ -8,17 +8,13 @@
 
 import type { PluginActionParamsBase } from "../../base.js"
 import { projectActionParamsSchema } from "../../base.js"
-import type { EnvironmentStatus } from "./getEnvironmentStatus.js"
+import { type EnvironmentStatus } from "./getEnvironmentStatus.js"
 import { dedent } from "../../../util/string.js"
 import { joi } from "../../../config/common.js"
 import { environmentStatusSchema } from "../../../config/status.js"
 import type { GenericProviderConfig } from "../../../config/provider.js"
 
-export interface PrepareEnvironmentParams<
-  C extends GenericProviderConfig = any,
-  T extends EnvironmentStatus = EnvironmentStatus,
-> extends PluginActionParamsBase<C> {
-  status: T
+export interface PrepareEnvironmentParams<C extends GenericProviderConfig = any> extends PluginActionParamsBase<C> {
   force: boolean
 }
 
@@ -31,7 +27,11 @@ export const prepareEnvironment = () => ({
     Make sure the environment is set up for this plugin. Use this action to do any bootstrapping required
     before deploying services.
 
-    Called ahead of any runtime actions (such as \`deployService\`, and \`testModule\`), unless \`getEnvironmentStatus\` returns \`ready: true\`.
+    This handler MUST BE idempotent since it's called with any command that executes runtime
+    actions (such as Deploy and Test actions).
+
+    If the plugin implements the \`getEnvironmentStatus\` handler then it can be used by this
+    handler to determine whether something needs to be done or whether it can return early.
   `,
   paramsSchema: projectActionParamsSchema().keys({
     force: joi.boolean().description("Force re-configuration of the environment."),

core/src/plugins/exec/exec.ts

@@ -87,30 +87,33 @@ execProvider.addHandler("getEnvironmentStatus", async ({ ctx }) => {
 
 execProvider.addHandler("prepareEnvironment", async ({ ctx, log }) => {
   const execLog = log.createLog({ name: "exec" })
-  if (ctx.provider.config.initScript) {
-    try {
-      execLog.info("Running init script")
-      const result = await runScript({
-        log: execLog,
-        cwd: ctx.projectRoot,
-        script: ctx.provider.config.initScript,
-      })
-      return { status: { ready: true, outputs: { initScript: { log: result.stdout.trim() } } } }
-    } catch (err) {
-      // Unexpected error (failed to execute script, as opposed to script returning an error code)
-      if (!(err instanceof ChildProcessError)) {
-        throw err
-      }
-
-      throw new RuntimeError({
-        message: dedent`
+
+  if (!ctx.provider.config.initScript) {
+    return { status: { ready: true, outputs: {} } }
+  }
+
+  try {
+    execLog.info("Running init script")
+    const result = await runScript({
+      log: execLog,
+      cwd: ctx.projectRoot,
+      script: ctx.provider.config.initScript,
+    })
+
+    return { status: { ready: true, outputs: { initScript: { log: result.stdout.trim() } } } }
+  } catch (err) {
+    // Unexpected error (failed to execute script, as opposed to script returning an error code)
+    if (!(err instanceof ChildProcessError)) {
+      throw err
+    }
+
+    throw new RuntimeError({
+      message: dedent`
           exec provider init script exited with code ${err.details.code}. Script output:
           ${err.details.output}
         `,
-      })
-    }
+    })
   }
-  return { status: { ready: true, outputs: {} } }
 })
 
 export const gardenPlugin = execPlugin

core/src/plugins/kubernetes/commands/cluster-init.ts

@@ -33,7 +33,6 @@ export const clusterInit: PluginCommand = {
         ctx,
         log,
         force: true,
-        status,
       })
     }
 

core/src/plugins/kubernetes/init.ts

@@ -9,10 +9,10 @@
 import { KubeApi, KubernetesError } from "./api.js"
 import {
   getAppNamespace,
-  prepareNamespaces,
+  prepareNamespace,
   deleteNamespaces,
-  getNamespaceStatus,
   getSystemNamespace,
+  namespaceExists,
 } from "./namespace.js"
 import type { KubernetesPluginContext, KubernetesConfig, KubernetesProvider, ProviderSecretRef } from "./config.js"
 import type {
@@ -37,7 +37,7 @@ import type { KubernetesResource } from "./types.js"
 import type { PrimitiveMap } from "../../config/common.js"
 import { getIngressApiVersion, supportedIngressApiVersions } from "./container/ingress.js"
 import type { Log } from "../../logger/log-entry.js"
-import { ingressControllerInstall, ingressControllerReady } from "./nginx/ingress-controller.js"
+import { ensureIngressController, ingressControllerReady } from "./nginx/ingress-controller.js"
 import { styles } from "../../logger/styles.js"
 import { isTruthy } from "../../util/util.js"
 
@@ -61,9 +61,9 @@ interface KubernetesEnvironmentDetail {
 export type KubernetesEnvironmentStatus = EnvironmentStatus<KubernetesProviderOutputs, KubernetesEnvironmentDetail>
 
 /**
- * Checks system service statuses (if provider has system services)
+ * Checks if namespace and secrets exist and whether ingress is ready (if applicable).
  *
- * Returns ready === true if all the above are ready.
+ * TODO @eysi: Check secret statuses
  */
 export async function getEnvironmentStatus({
   ctx,
@@ -72,8 +72,9 @@ export async function getEnvironmentStatus({
   const k8sCtx = <KubernetesPluginContext>ctx
   const provider = k8sCtx.provider
   const api = await KubeApi.factory(log, ctx, provider)
-
-  const namespace = await getNamespaceStatus({ ctx: k8sCtx, log, provider: k8sCtx.provider, skipCreate: true })
+  // TODO @eysi: Avoid casting (the namespace is ensured though when the provider config is resolved)
+  const namespaceName = provider.config.namespace!.name
+  const namespaceReady = await namespaceExists(api, ctx, namespaceName)
 
   const detail: KubernetesEnvironmentDetail = {
     systemReady: true,
@@ -82,31 +83,21 @@ export async function getEnvironmentStatus({
   }
 
   const result: KubernetesEnvironmentStatus = {
-    ready: namespace.state === "ready",
+    ready: namespaceReady,
     detail,
     outputs: {
-      "app-namespace": namespace.namespaceName,
+      "app-namespace": namespaceName,
       "default-hostname": provider.config.defaultHostname || null,
     },
   }
 
   if (provider.config.setupIngressController === "nginx") {
     const ingressControllerReadiness = await ingressControllerReady(ctx, log)
-    result.ready = ingressControllerReadiness && namespace.state === "ready"
+    result.ready = ingressControllerReadiness && namespaceReady
     detail.systemReady = ingressControllerReadiness
     log.info(
       `${styles.highlight("nginx")} Ingress Controller ${ingressControllerReadiness ? "is ready" : "is not ready"}`
     )
-  } else {
-    // We only need to warn about missing ingress classes if we're not using garden installed nginx
-    const ingressApiVersion = await getIngressApiVersion(log, api, supportedIngressApiVersions)
-    const ingressWarnings = await getIngressMisconfigurationWarnings(
-      provider.config.ingressClass,
-      ingressApiVersion,
-      log,
-      api
-    )
-    ingressWarnings.forEach((w) => log.warn(w))
   }
 
   return result
@@ -145,26 +136,47 @@ export async function getIngressMisconfigurationWarnings(
 
 /**
  * Deploys system services (if any) and creates the default app namespace.
+ *
+ * TODO @eysi: Ensure secrets here
  */
 export async function prepareEnvironment(
-  params: PrepareEnvironmentParams<KubernetesConfig, KubernetesEnvironmentStatus>
+  params: PrepareEnvironmentParams<KubernetesConfig>
 ): Promise<PrepareEnvironmentResult> {
-  const { ctx, log, status } = params
+  const { ctx, log } = params
   const k8sCtx = <KubernetesPluginContext>ctx
   const provider = k8sCtx.provider
   const config = provider.config
+  const api = await KubeApi.factory(log, ctx, provider)
+
   // create default app namespace if it doesn't exist
-  await prepareNamespaces({ ctx, log })
+  const nsStatus = await prepareNamespace({ ctx, log })
   // make sure that the system namespace exists
   await getSystemNamespace(ctx, ctx.provider, log)
 
   // TODO-0.13/TODO-0.14: remove this option for remote kubernetes clusters?
   if (config.setupIngressController === "nginx") {
-    // Install nginx ingress controller
-    await ingressControllerInstall(k8sCtx, log)
+    await ensureIngressController(k8sCtx, log)
+  } else {
+    // We only need to warn about missing ingress classes if we're not using Garden installed nginx
+    const ingressApiVersion = await getIngressApiVersion(log, api, supportedIngressApiVersions)
+    const ingressWarnings = await getIngressMisconfigurationWarnings(
+      provider.config.ingressClass,
+      ingressApiVersion,
+      log,
+      api
+    )
+    ingressWarnings.forEach((w) => log.warn(w))
   }
 
-  return { status: { ready: true, outputs: status.outputs } }
+  return {
+    status: {
+      ready: true,
+      outputs: {
+        "app-namespace": nsStatus["app-namespace"].namespaceName,
+        "default-hostname": provider.config.defaultHostname || null,
+      },
+    },
+  }
 }
 
 export async function cleanupEnvironment({

core/src/plugins/kubernetes/local/local.ts

@@ -67,7 +67,13 @@ async function prepareEnvironment(
   const { ctx, log } = params
   const provider = ctx.provider
 
-  const result = await _prepareEnvironmentBase(params)
+  // This should be set in the configureProvider handler but we need the
+  // plugin context to get the cluster type
+  if (!provider.config.clusterType) {
+    provider.config.clusterType = await getClusterType(ctx, log)
+  }
+
+  const prepareEnvResult = await _prepareEnvironmentBase(params)
 
   if (provider.config.clusterType === "minikube") {
     await setMinikubeDockerEnv()
@@ -76,7 +82,7 @@ async function prepareEnvironment(
     await configureMicrok8sAddons(log, microk8sAddons)
   }
 
-  return result
+  return prepareEnvResult
 }
 
 async function getClusterType(ctx: KubernetesPluginContext, log: Log): Promise<LocalKubernetesClusterType> {

core/src/plugins/kubernetes/namespace.ts

@@ -278,7 +278,7 @@ export function clearNamespaceCache(provider: KubernetesProvider) {
 /**
  * Used by both the remote and local plugin
  */
-export async function prepareNamespaces({ ctx, log }: GetEnvironmentStatusParams) {
+export async function prepareNamespace({ ctx, log }: GetEnvironmentStatusParams) {
   const k8sCtx = <KubernetesPluginContext>ctx
 
   try {

core/src/plugins/kubernetes/nginx/default-backend.ts

@@ -16,7 +16,7 @@ import { getDefaultGardenIngressControllerDefaultBackendImagePath } from "../con
 import { GardenIngressComponent } from "./ingress-controller-base.js"
 
 export class GardenDefaultBackend extends GardenIngressComponent {
-  override async install(ctx: KubernetesPluginContext, log: Log): Promise<void> {
+  override async ensure(ctx: KubernetesPluginContext, log: Log): Promise<void> {
     const { deployment, service } = defaultBackendGetManifests(ctx)
     const status = await this.getStatus(ctx, log)
     if (status === "ready") {

core/src/plugins/kubernetes/nginx/ingress-controller-base.ts

@@ -11,7 +11,10 @@ import type { KubernetesPluginContext } from "../config.js"
 import type { DeployState } from "../../../types/service.js"
 
 export abstract class GardenIngressComponent {
-  abstract install(ctx: KubernetesPluginContext, log: Log): Promise<void>
+  /**
+   * Install ingress controller if not ready. Idempotent.
+   */
+  abstract ensure(ctx: KubernetesPluginContext, log: Log): Promise<void>
 
   abstract getStatus(ctx: KubernetesPluginContext, log: Log): Promise<DeployState>
 

core/src/plugins/kubernetes/nginx/ingress-controller.ts

@@ -18,7 +18,7 @@ import { GardenIngressComponent } from "./ingress-controller-base.js"
 import type { DeployState } from "../../../types/service.js"
 
 class NoOpGardenIngressController extends GardenIngressComponent {
-  override install(_ctx: KubernetesPluginContext, _log: Log): Promise<void> {
+  override ensure(_ctx: KubernetesPluginContext, _log: Log): Promise<void> {
     return Promise.resolve(undefined)
   }
 
@@ -61,8 +61,8 @@ export async function ingressControllerReady(ctx: KubernetesPluginContext, log:
   return await getGardenIngressController(ctx).ready(ctx, log)
 }
 
-export async function ingressControllerInstall(ctx: KubernetesPluginContext, log: Log) {
-  await getGardenIngressController(ctx).install(ctx, log)
+export async function ensureIngressController(ctx: KubernetesPluginContext, log: Log) {
+  await getGardenIngressController(ctx).ensure(ctx, log)
 }
 
 export async function ingressControllerUninstall(ctx: KubernetesPluginContext, log: Log) {

core/src/plugins/kubernetes/nginx/nginx-helm.ts

@@ -31,7 +31,7 @@ type _HelmValue = number | string | boolean | object | null | undefined
 export abstract class HelmGardenIngressController extends GardenIngressComponent {
   private readonly defaultBackend = new GardenDefaultBackend()
 
-  override async install(ctx: KubernetesPluginContext, log: Log): Promise<void> {
+  override async ensure(ctx: KubernetesPluginContext, log: Log): Promise<void> {
     const ingressControllerReady = await this.ready(ctx, log)
     if (ingressControllerReady) {
       return
@@ -66,7 +66,7 @@ export abstract class HelmGardenIngressController extends GardenIngressComponent
     ]
 
     log.info(`Installing ${styles.highlight("nginx")} in ${styles.highlight(namespace)} namespace...`)
-    await this.defaultBackend.install(ctx, log)
+    await this.defaultBackend.ensure(ctx, log)
     await helm({ ctx, namespace, log, args, emitLogEvents: false })
 
     const nginxHelmMainResource = getNginxHelmMainResource(values)

core/src/plugins/kubernetes/nginx/nginx-kind.ts

@@ -24,7 +24,7 @@ const nginxKindMainResource = {
 }
 
 export class KindGardenIngressController extends GardenIngressComponent {
-  override async install(ctx: KubernetesPluginContext, log: Log): Promise<void> {
+  override async ensure(ctx: KubernetesPluginContext, log: Log): Promise<void> {
     const status = await this.getStatus(ctx, log)
     if (status === "ready") {
       return

core/src/plugins/kubernetes/nginx/nginx-microk8s.ts

@@ -15,7 +15,7 @@ import { waitForResources } from "../status/status.js"
 import { GardenIngressComponent } from "./ingress-controller-base.js"
 
 export class Microk8sGardenIngressController extends GardenIngressComponent {
-  override async install(ctx: KubernetesPluginContext, log: Log): Promise<void> {
+  override async ensure(ctx: KubernetesPluginContext, log: Log): Promise<void> {
     const provider = ctx.provider
 
     const status = await this.getStatus(ctx, log)

core/src/plugins/kubernetes/nginx/nginx-minikube.ts

@@ -15,7 +15,7 @@ import { checkResourceStatus, waitForResources } from "../status/status.js"
 import { GardenIngressComponent } from "./ingress-controller-base.js"
 
 export class MinikubeGardenIngressController extends GardenIngressComponent {
-  override async install(ctx: KubernetesPluginContext, log: Log): Promise<void> {
+  override async ensure(ctx: KubernetesPluginContext, log: Log): Promise<void> {
     const provider = ctx.provider
     const status = await this.getStatus(ctx, log)
     if (status === "ready") {