Open kate-goldenring opened 3 months ago
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.
@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 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.
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:
OTEL_EXPORTER_OTLP_ENDPOINT=http://127.0.0.1:4318 spin up
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 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 SpinApp
s.
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
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)
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)
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>
For trigger config, we have https://github.com/spinkube/spin-operator/issues/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).
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
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)
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:
Proposal: add a
SpinApp.spec.env
Workaround
Create deployment directly instead of creating a
SpinApp
CR, bypassing the Spin operator: