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

Support setting HTTP trigger listen address in env var #138

Merged

Conversation

kate-goldenring
Copy link
Collaborator

Enables setting the shim's HTTP trigger listen address in SPIN_HTTP_LISTEN_ADDR (same naming scheme as Spin CLI). This can now be set in a container spec, which follows the approach of configuring the Spin runtime via container environment variables that is laid out in this draft SKIP.

closes #52
related #128

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rajatjindal
Copy link
Member

LGTM :)

@kate-goldenring
Copy link
Collaborator Author

@radu-matei @jsturtevant @Mossaka @devigned it would be great to get this merged before the next release if possible. There are two commented LGTMs but a review would be appreciated

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 lgtm.

One thing to consider for future, but might be hard to change at this point, is that the default port is 80. Perhaps, we may want to consider a higher port range default.

Also, it might be nice to represent the informational container spec field of ports.containerPorts with the value of the SPIN_HTTP_LISTEN_ADDR_ENV via the Spin operator when creating the SpinApp, so it's more clear to folks.

@kate-goldenring kate-goldenring merged commit 6fdfa30 into spinkube:main Jun 20, 2024
9 checks passed
@kate-goldenring
Copy link
Collaborator Author

Also, it might be nice to represent the informational container spec field of ports.containerPorts with the value of the SPIN_HTTP_LISTEN_ADDR_ENV via the Spin operator when creating the SpinApp, so it's more clear to folks.

@devigned, to clarify, are you suggesting adding a ports.containerPorts field to the SpinApp CRD? I agree that we need a clean way to set this in the SpinApp

@devigned
Copy link
Contributor

@kate-goldenring hmm... not necessarily adding ports.containerPorts to the SpinApp since it is an array and it would probably better represented in a more Spin domain specific singular value field. I was suggesting that when the SpinApp is processed by the operator that the end product, the resulting pod / container spec would have that informational field. For example:

apiVersion: ...
kind: SpinApp
metadata:
  name: my-app
spec:
  httpListeningPort: 3000 # this drives configuration of the pod / container spec and informs the pod.spec.ports.containerPort
  otherStuff: ...
---
apiVersion: v1
kind: Pod
metadata:
  name: my-app
spec:
  containers:
  - name: spin-app
    ports:
      - containerPort: 3000 # this is only for information

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

Successfully merging this pull request may close these issues.

unable to run a pod with two spin apps side-by-side
5 participants