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

feat: make environment variables, variables from secrets and configmaps configurable #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jasstkn
Copy link
Contributor

@Jasstkn Jasstkn commented Sep 7, 2022

Signed-off-by: Maria Kotlyarevskaya [email protected]

fixes #5

Scope of changes:

  • adds support for environment variables in the job definition file, e.g.:
env:
- name: LOG_LEVEL
  value: DEBUG
  • adds support for envFromConfigmap (1+) and envFromSecret(1+), e.g.:
envFromConfigmap:
- name: special-config

envFromSecret:
- name: mysecret

If configmap / secret doesn't exist in the specified namespace (if it isn't specified then in default), it throws an error like this:

./run-job -f cows.yaml
2022/09/07 20:39:37 secret mysecret-404 not found in namespace default 
./run-job -f cows.yaml
2022/09/07 20:37:45 configmap myconfigmap not found in namespace default

Tested the following configuration:

  • environment variables are specified, correctly passed to the job definition
  • added 2 configmaps, one of it is missing, error shows
  • added 2 secrets, one of it is missing, errors shows
  • added 1 configmap, 1 secret - job successfully executed:
    Environment Variables from:
      special-config  ConfigMap  Optional: false
      mysecret        Secret     Optional: false
    Environment:
      LOG_LEVEL:  DEBUG
  • bare cows.yaml works

@Jasstkn Jasstkn marked this pull request as ready for review September 7, 2022 18:47
@Jasstkn
Copy link
Contributor Author

Jasstkn commented Sep 7, 2022

@alexellis hello! This PR is ready for the review. I think that we need to add some section in README to list all of the supported options (I guess more can come, e.g. volumes and scheduling options). What do you think?

@alexellis
Copy link
Owner

envFromConfigmap:
- name: special-config

envFromSecret:
- name: mysecret

There is probably going to be a need for a namespace too, because sometime the secret you want, or multiple secrets may be in other namespaces.

@Jasstkn
Copy link
Contributor Author

Jasstkn commented Sep 8, 2022

envFromConfigmap:
- name: special-config

envFromSecret:
- name: mysecret

There is probably going to be a need for a namespace too, because sometime the secret you want, or multiple secrets may be in other namespaces.

hi @alexellis. pod can't use configmap/secret from another namespace.

source:

The Pod and the ConfigMap must be in the same namespace.

@alexellis
Copy link
Owner

I thought I'd seen a secretRef that took a namespace

@Jasstkn
Copy link
Contributor Author

Jasstkn commented Sep 8, 2022

I thought I'd seen a secretRef that took a namespace

There is no namespace in the envFrom in the Kubernetes API reference

@Jasstkn
Copy link
Contributor Author

Jasstkn commented Sep 9, 2022

@alexellis hi. do you have any unanswered questions/concerns in mind?

@Jasstkn
Copy link
Contributor Author

Jasstkn commented Sep 15, 2022

@alexellis ping 🙈

@alexellis
Copy link
Owner

Hi, I'm on vacation so the ping isn't necessary. I was going to take a look when back. I'm not sure that the design is intuitive perhaps we can make it match the k8s app more closely instead of having a bunch of different envX sections?

@Jasstkn
Copy link
Contributor Author

Jasstkn commented Sep 20, 2022

Hi, I'm on vacation so the ping isn't necessary. I was going to take a look when back. I'm not sure that the design is intuitive perhaps we can make it match the k8s app more closely instead of having a bunch of different envX sections?

Hey. Sorry for the ping - I thought you missed my message. Regarding your feedback, I was trying to simplify it but faced an issue with underlying structure of K8S API. Under the hood it has few structs inside envFrom and I didn't figure out how to parse YAML properly in this case.

@alexellis
Copy link
Owner

Under the hood it has few structs inside envFrom and I didn't figure out how to parse YAML properly in this case.

I would suggest that we own/parse our own format with our own structs, then convert it to the K8s objects at deploy time. Like we're doing for the other fields too.

Let's have a bit more of a think about how this could work. Do we have a test user or someone who wants this feature that can help design it before we commit to a config format that will be hard to change?

This is what I see in the docs

    env:
      - name: SECRET_USERNAME
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username

Looks like envFrom is also another way to get the same result:

apiVersion: v1
kind: Pod
metadata:
  name: secret-test-pod
spec:
  containers:
    - name: test-container
      image: registry.k8s.io/busybox
      command: [ "/bin/sh", "-c", "env" ]
      envFrom:
      - secretRef:
          name: mysecret
  restartPolicy: Never

We did something similar in the openfaas-cloud bootstrap project: https://github.com/openfaas/ofc-bootstrap/blob/master/example.init.yaml#L39

  ## Use DigitalOcean
  ### Create a Personal Access Token and save it into a file, with no new-lines
  - name: "digitalocean-dns"
    files:
      - name: "access-token"
        value_from: "~/Downloads/do-access-token"
    filters:
      - "do_dns01"
    namespace: "cert-manager"

  - name: "jwt-public-key"
    files:
      - name: "key.pub"
        value_from: "./tmp/key.pub"
        value_command: "openssl ec -in ./tmp/key -pubout -out ./tmp/key.pub"
    filters:
      - "auth"
    namespace: "openfaas"

So, there could be a value and valueFrom property on the env struct that we build out, where if value is set, we use that value literally, if valueFrom is set, then it's the much bigger struct with other fields.

My concern would be that the YAML spec is quite complex, perhaps we should also support files? Files require a list of volumes and volumeMounts and things start to get worse from a UX perspective.

apiVersion: v1
kind: Pod
metadata:
  name: secret-test-pod
  labels:
    name: secret-test
spec:
  volumes:
  - name: secret-volume
    secret:
      secretName: ssh-key-secret
  containers:
  - name: ssh-test-container
    image: mySshImage
    volumeMounts:
    - name: secret-volume
      readOnly: true
      mountPath: "/etc/secret-volume"

With OpenFaaS, we really make the spec a lot simpler, for instance, assuming that aws_access_key existed and was in the same namespace as the function:

functions:
  fn_name:
     secrets:
     - aws_access_key

Then any secret mentioned is mounted under /var/openfaas/secrets/NAME using a projection.

Alex

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.

[Feature] Support environment variables
2 participants