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

Enable configuring the Shim via the spin-operator #248

Open
kate-goldenring opened this issue Jun 5, 2024 · 12 comments
Open

Enable configuring the Shim via the spin-operator #248

kate-goldenring opened this issue Jun 5, 2024 · 12 comments
Labels
enhancement New feature or request

Comments

@kate-goldenring
Copy link

kate-goldenring commented Jun 5, 2024

Users should be able to set Pod environment variables via the SpinApp CRD. These can be used for configuring settings, such as open telemetry endpoints:

env:
 - name: NODE_IP
    valueFrom:
      fieldRef:
        fieldPath: status.hostIP
 - name: OTEL_EXPORTER_OTLP_ENDPOINT
    value: "http://$(NODE_IP):4318"

Proposal: add a SpinApp.spec.env

Workaround

Create deployment directly instead of creating a SpinApp CR, bypassing the Spin operator:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: spin-test
spec:
  replicas: 1
  selector:
    matchLabels:
      app: spin-test
  template:
    metadata:
      labels:
        app: spin-test
    spec:
      runtimeClassName: wasmtime-spin-v2
      containers:
      - name: spin-test
        image: ttl.sh/spin-foo-123:48h
        command: ["/"]
        ports:
        - containerPort: 80
        env:
       - name: NODE_IP
          valueFrom:
            fieldRef:
              fieldPath: status.hostIP
         - name: OTEL_EXPORTER_OTLP_ENDPOINT
            value: "http://$(NODE_IP):4318"
@calebschoepp
Copy link
Contributor

Thanks for filing this @kate-goldenring. SpinApp.spec.env is definitely one approach that would work, but I think we should do more design work before we pull the trigger. I'm a little hesitant to set the precedent that users can set whatever env on the underlying pod that they want. This wouldn't necessarily translate well to other executors (granted we do already have a precedent of supporting features that don't work for all executors so maybe I'm over thinking this).

An alternative would be to have something like SpinApp.spec.otel-env that only lets you set specific OTel environment variables.

I think we need to spend some time thinking about how OTel will get configured in other executors to figure out the design here.

@kate-goldenring
Copy link
Author

@calebschoepp I'd rather have a more general solution given that users will likely want to set other env vars as well. I also don't think we have a precedent for specific fields like otel-env. I could see this being something that the spin kube scaffold plugin abstracts with a flag.

I don't imagine it being too complicated for executors to reference and use the env vars set in the SpinApp CRD.

@calebschoepp
Copy link
Contributor

@calebschoepp I'd rather have a more general solution given that users will likely want to set other env vars as well. I also don't think we have a precedent for specific fields like otel-env. I could see this being something that the spin kube scaffold plugin abstracts with a flag.

I like the idea of some kind of --otel-enabled flag on the plugin.

I don't imagine it being too complicated for executors to reference and use the env vars set in the SpinApp CRD.
I'm not concerned about their ability to reference the env vars. I'm concerned executors having no meaningful equivalent.

@kate-goldenring
Copy link
Author

I'm concerned executors having no meaningful equivalent.

I see. I think we are running into an interesting distinction here between two types of environment variables in Spin:

  • Environment variable set on the runtime: OTEL_EXPORTER_OTLP_ENDPOINT=http://127.0.0.1:4318 spin up
  • Environment variable set in the Wasm instance: spin up --env key=value

In the context of the SpinApp CRD, setting env in the SpinApp spec I'd assume would map to spin up --env. This tells executors to make sure that the environment variables are set in the runtime context of the spin app when it is executed.

I am coming around to understanding how OTEL is a unique case and should not be set in an env field.

@kate-goldenring kate-goldenring changed the title Support Setting Container Environment Variables via SpinApp CRD How to Set Environment Variables via SpinApp CRD Jun 5, 2024
@calebschoepp
Copy link
Contributor

@kate-goldenring how do you feel about something like this?

Setting env on a SpinApp sets environment variables in the Wasm runtime context of the spin app when it is executed.

apiVersion: core.spinoperator.dev/v1alpha1
kind: SpinApp
metadata:
  name: simple-spinapp
spec:
  image: "ghcr.io/spinkube/containerd-shim-spin/examples/spin-rust-hello:v0.13.0"
  replicas: 1
  executor: containerd-shim-spin
  env:
    FOO: bar

Setting env on a SpinAppExecutor sets environment variables in the context of where the executor (shim) is running.

apiVersion: core.spinoperator.dev/v1alpha1
kind: SpinAppExecutor
metadata:
  name: containerd-shim-spin
spec:
  createDeployment: true
  deploymentConfig:
    runtimeClassName: wasmtime-spin-v2
    installDefaultCACerts: true
  env:
    OTEL_EXPORTER_OTLP_ENDPOINT=http...

For clarity we could change the names to be something like runtimeEnv and executorEnv or something like that. Then you could also set exectorEnv from the SpinApp and that would override/merge from the executor config. And you could set runtimeEnv from the SpinAppExecutor and that would set a default value for all SpinApps.

@kate-goldenring
Copy link
Author

It seems a bit backwards to have the SpinApp (which references an executor) cause an update in the executor. I could see someone configuring that in the executor from the start though. This does mean it would be on an executor level not an app level

@endocrimes
Copy link
Contributor

We deliberately avoided env as its own thing (preferring configuring variables).

If we want to expose configuration for the shim, we should create a typed set of configuration rather than doing env-string-mappings.

apiVersion: core.spinoperator.dev/v1alpha1
kind: SpinAppExecutor
metadata:
  name: containerd-shim-spin
spec:
  createDeployment: true
  deploymentConfig:
    runtimeClassName: wasmtime-spin-v2
    installDefaultCACerts: true
  o11y:
    otlpExporterEndpoint: ....

that we then render as an environment variable (or some other more structured config if possible)

@endocrimes
Copy link
Contributor

The driving goal here being: We should always aim to provide a declarative API that captures intent rather than a current implementation detail (leaving those bits for the operator to manage based on the intent)

@endocrimes endocrimes changed the title How to Set Environment Variables via SpinApp CRD Enable configuring the Shim via the spin-operator Jun 27, 2024
@endocrimes endocrimes added the enhancement New feature or request label Jun 27, 2024
@kate-goldenring
Copy link
Author

My only concern with the declarative API is whether it is over-prescriptive. Some executors may support fields that others don't. For example, say one executor doesn't support Spin apps with the SQS trigger? Where/how can a user configure the following SQS trigger runtime values?

Key Spin CLI Example Value
AWS_DEFAULT_REGION AWS_DEFAULT_REGION="us-west-2" spin up "us-west-2"
AWS_ACCESS_KEY_ID AWS_ACCESS_KEY_ID="ABC" spin up "ABC"
AWS_SECRET_ACCESS_KEY AWS_SECRET_ACCESS_KEY="123" spin up "123"
AWS_SESSION_TOKEN AWS_SESSION_TOKEN="token" spin up "token"

These should be configurable at the app level so would we expand the SpinApp CRD so that the following is possible?

apiVersion: core.spinoperator.dev/v1alpha1
kind: SpinApp
metadata:
  name: simple-spinapp
spec:
  image: "ghcr.io/spinkube/containerd-shim-spin/examples/spin-rust-hello:v0.13.0"
  replicas: 1
  executor: containerd-shim-spin
  aws:
    defaultRegion:"us-west-2"
    accessKeyId: "123"
    secretAccessKey: <ref to k8s secret>
    sessionToken: <ref to k8s secret>

@endocrimes
Copy link
Contributor

endocrimes commented Jun 27, 2024

For trigger config, we have #15, but never got around to doing design work for it. That's a somewhat separate problem though. (And I would probably prefer some kind of typed trigger.{triggername}.config.{field} in the CRD because as much as it's messy to maintain, it is far easier to understand later).

@kate-goldenring
Copy link
Author

We already have the SQS trigger in the shim so it would be good to enable using it with this trigger config

(And I would probably prefer some kind of typed trigger.{triggername}.config.{field} in the CRD because as much as it's messy to maintain, it is far easier to understand later).

This sounds like a reasonable approach to me. What would be next steps to getting more input on this? My SKIP aims to cover configuration but it may be getting a little bloated https://github.com/spinkube/skips/pull/4/files

@endocrimes
Copy link
Contributor

I'd probably drop the Spin Operator/CRD bits from SKIP04, then we can follow up with the API we wanna expose? (if you wanna avoid too much bloat)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants