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

Environment Variable Substitution or CLI Flags #2893

Closed
johnnyhuy opened this issue Nov 20, 2022 · 14 comments · Fixed by #4789
Closed

Environment Variable Substitution or CLI Flags #2893

johnnyhuy opened this issue Nov 20, 2022 · 14 comments · Fixed by #4789
Labels
feature New feature or request

Comments

@johnnyhuy
Copy link

johnnyhuy commented Nov 20, 2022

Tell us about your request

Have the ability to use environment variable substitution on the ConfigMap with the usual $ syntax similar to Java's Spingboot configuration.

Or dynamically expose deterministic environment variable/CLI flags names e.g. aws.clusterName = AWS_CLUSTER_NAME similar to other controllers like External DNS.

Tell us about the problem you're trying to solve. What are you trying to do, and why is it hard?

We've lost the ability to use environment variables to substitute configurations like cluster name and cluster endpoint.

This is useful when we want to handle these values elsewhere, like a separate configmap/secret in a different namespace. There are also advanced scenarios like using External Secrets to fetch values from SSM to upsert a Kubernetes Secret.

Are you currently working around this issue?

Using older versions before #2746 otherwise, we'd have a manually paste in dynamic values like the cluster endpoint into our Helm chart to configure Karpenter correctly.

Additional Context

No response

Attachments

No response

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment
@johnnyhuy johnnyhuy added the feature New feature or request label Nov 20, 2022
@johnnyhuy johnnyhuy changed the title Environment Variables Substitution Environment Variable Substitution or CLI Flags Nov 20, 2022
@ellistarn
Copy link
Contributor

Seems like a decent feature to me. @jonathan-innis , this would solve our local dev story, too.

@jonathan-innis
Copy link
Contributor

@johnnyhuy I'm missing the use-case here. Why not just use envsubst to gen the ConfigMap (if you are using a GitOps workflow like Argo or Flux) or use Terraform locals if you are using some other IaC?

@johnnyhuy
Copy link
Author

johnnyhuy commented Nov 25, 2022

@johnnyhuy I'm missing the use-case here. Why not just use envsubst to gen the ConfigMap (if you are using a GitOps workflow like Argo or Flux) or use Terraform locals if you are using some other IaC?

Yes, we could use envsubst and template a ConfigMap or even hard-code the values in a source code repo and let Argo CD or Flux ship it. However, in both scenarios, we'd have to know what the values are after we deploy dependent infrastructure. Hence we'd do this right after infrastructure deployment e.g. creating an EKS cluster to fetch those values (tight-coupling).

Good question, We can put it into a scenario

Scenario

As an end-user, I don't know values like a unique cluster name my-cluster-abdw32109d or launch template my-launch-generated-id123 to declare them in a ConfigMap or Secret. When the underlying infrastructure gets created, we can put values into something like SSM parameters or Secrets Manager secrets. We can then use a k8s controller to fetch those values and upsert them into a ConfigMap or Secret.

Here's an example using the External Secrets controller to do just that.

Fetched the cluster name my-cluster-abdw32109d from SSM parameter /my/auto-generated/clusterName:

apiVersion: external-secrets.io/v1alpha1
kind: ExternalSecret
metadata:
  name: example-secret
spec:
  refreshInterval: 5m
  secretStoreRef:
    name: my-ssm-secret-store
    kind: ClusterSecretStore
  target:
    creationPolicy: Owner
  data:
    - secretKey: clusterName
      remoteRef:
        key: /my/auto-generated/clusterName

In return, the controller creates a Secret:

apiVersion: v1
kind: Secret
metadata:
  name: example-secret
type: Opaque
stringData:
  clusterName: my-cluster-abdw32109d

We could then hypothetically do either of these to load values into the Karpenter pod:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: karpenter
spec:
  ...
  template:
    ...
    spec:
      ...
      containers:
        - name: controller
          image: public.ecr.aws/karpenter/controller:v0.18.1
          env:
            - name: KARPENTER_CLUSTER_NAME
              valueFrom:
                secretKeyRef:
                  name: example-secret
                  key: clusterName

---

# Or this
apiVersion: apps/v1
kind: Deployment
metadata:
  name: karpenter
spec:
  ...
  template:
    ...
    spec:
      ...
      containers:
        - name: controller
          image: public.ecr.aws/karpenter/controller:v0.18.1
          args:
            - --cluster-name=$(CLUSTER_NAME)
          env:
            - name: CLUSTER_NAME
              valueFrom:
                secretKeyRef:
                  name: example-secret
                  key: clusterName

Final thoughts

As we can see, the end-user does not need to know exactly what the auto-generated cluster name is; they'd refer to the SSM parameter instead to give us the value.

This is one scenario; another one would be another team deploying an EKS cluster and creates a Secret or ConfigMap with the cluster name in it. We could either imperatively build a script to deploy Karpenter or declaratively use environment variables or CLI flags on the Karpenter pod.

@FernandoMiguel
Copy link
Contributor

we also follow the approach where we fill in a configmap with infra details and let other deployments fetch those details

@ellistarn
Copy link
Contributor

@jonathan-innis , it strikes me that cluster name and endpoint aren't the same as other dynamic global settings. Do they make more sense as cli args?

@dudicoco
Copy link

dudicoco commented Dec 4, 2022

Can someone elaborate on the decision that was taken to move all of the env vars and cli args to a configmap?
Configuration via environment variables is much easier than dealing with configmaps.

Also the current implementation of karpenter reading all of its configuration from configmaps directly via the kube api rather than mounting the configmaps into files is very opinionated and seems like it was inspired by argocd.
This seems to be geared more towards new kubernetes users while forcing experienced and advanced users to use the opinionated setup instead of native ways of loading configs (via cli args/env vars/files).

@dudicoco
Copy link

Can someone elaborate on the decision that was taken to move all of the env vars and cli args to a configmap? Configuration via environment variables is much easier than dealing with configmaps.

Also the current implementation of karpenter reading all of its configuration from configmaps directly via the kube api rather than mounting the configmaps into files is very opinionated and seems like it was inspired by argocd. This seems to be geared more towards new kubernetes users while forcing experienced and advanced users to use the opinionated setup instead of native ways of loading configs (via cli args/env vars/files).

@jonathan-innis @ellistarn

@ellistarn
Copy link
Contributor

Open-minded about this. Curious why it's easier to configure an env car instead of a config map.

The configmap is nice because it's dynamic, but I could see static overrides as well. We originally decided to make just one mechanism because it reduces the chance for users to set both and get confused.

@dudicoco
Copy link

@ellistarn I think a configmap is necessary when you have deeply nested configuration files, for example the Prometheus config file.
The Karpenter configmap however is currently a very simple key value pairs which could easily be configured as env vars only.

The downsides of using a configmap is that you have to configure your application to watch the mounted configmap files for changes and reload your application when that happens, and in a case where your configmap file is corrupted this might cause a downtime.
It is possible to use a versioned configmap in order to gradually rollout your pods to use the new config, but that is also complicated in comparison with environment variables.

Specifically with Karpenter the main issue that bothers me is that Karpenter does not mount the configmap into a file on the disk but watches the actual configmap k8s object for changes, the k8s object name is hardcoded so we must use this name for the configmap.
We have a specific naming convention for all configmaps in our clusters and we cannot use that naming convention since Karpenter does not allow specifying the configmap name.

So to summarize:

  1. Changes to the Karpenter configmap can cause all Karpenter pods to stop working due to a bad config since there is no gradual rollout of the deployment.
  2. Karpenter does not allow specifying the configmaps names for users who would like to use different names.

@johnnyhuy
Copy link
Author

As a workaround we can do the following.

Create a Job to run a kubctl apply with envsubst so that we can handle env vars.

Job

apiVersion: batch/v1
kind: Job
metadata:
  name: karpenter-global-settings
  annotations:
    helm.sh/hook: post-install,post-upgrade
    helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
    helm.sh/hook-weight: "1"
spec:
  backoffLimit: 4
  template:
    metadata:
      labels:
        app.kubernetes.io/name: karpenter-global-settings
    spec:
      restartPolicy: OnFailure
      serviceAccount: karpenter-global-settings
      containers:
        - name: k8s
          image: alpine/k8s:1.25.13
          command:
            - /bin/sh
            - -c
          volumeMounts:
            - name: config
              mountPath: /tmp/config
          args:
            - envsubst < /tmp/config/manifest.yaml | kubectl apply -f -
          env:
            - name: CLUSTER_NAME
              valueFrom:
                secretKeyRef:
                  key: clusterName
                  name: karpenter-env
                  optional: false
            - name: CLUSTER_ENDPOINT
              valueFrom:
                secretKeyRef:
                  key: clusterEndpoint
                  name: karpenter-env
                  optional: false
            - name: AWS_DEFAULT_INSTANCE_PROFILE
              valueFrom:
                secretKeyRef:
                  key: awsDefaultInstanceProfile
                  name: karpenter-env
                  optional: false
      volumes:
        - name: config
          configMap:
            name: karpenter-global-settings-temp

ConfigMap

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: karpenter-global-settings-temp
data:
  manifest.yaml: |
    apiVersion: v1
    kind: ConfigMap
    metadata:
      name: karpenter-global-settings
    data:
      "aws.defaultInstanceProfile": ${AWS_DEFAULT_INSTANCE_PROFILE}
      "aws.clusterName": ${CLUSTER_NAME}
      "aws.clusterEndpoint": ${CLUSTER_ENDPOINT}
      "aws.enableENILimitedPodDensity": "true"
      "aws.enablePodENI": "false"
      "aws.isolatedVPC": "true"
      "aws.nodeNameConvention": "resource-name"
      "aws.vmMemoryOverheadPercent": "0.075"
      "batchIdleDuration": "1s"
      "batchMaxDuration": "10s"
      "featureGates.driftEnabled": "false"

RBAC

apiVersion: v1
kind: ServiceAccount
metadata:
  name: karpenter-global-settings
---

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: karpenter-global-settings
rules:
- apiGroups: [""]
  resources: [configmaps]
  verbs: [list, label, get, patch]
---

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: karpenter-global-settings
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: karpenter-global-settings
subjects:
- kind: ServiceAccount
  name: karpenter-global-settings
  namespace: kube-system

@morremeyer
Copy link
Contributor

We are having the same issue currently: We manage e.g. the Instance profile and ServiceAccount with terraform and want to deploy karpenter itself with the helm chart in ArgoCD (we're currently using helm_release in terraform which has terrible UX/DX).

However, we currently can't set the defaultInstanceProfile in any reliable way since this is only available in the ConfigMap and can't be set as an environment variable.

I was surprised by this, expecting every setting to be configurable via an environment variable instead of being loaded from a ConfigMap with a hard-coded name.

As a mid-term workaround, I'll be opening a PR for the chart to be able to disable the deployment of the ConfigMap, so that we can manage the ConfigMap in terraform for now.

On the long term, I'd prefer for karpenter to be fully configurable via env vars only. This would benefit everyone in multiple ways:

  • Code would likely be less complex since there does not need to be hot reloading of data from the ConfigMap
  • Changes are more predictable for users since there is no hot-reloading from the ConfigMap
  • Various deployment mechanisms would be supported for configuration since using a Secret or ConfigMap via .Values.controller.envFrom or .Values.controller.env is trivial
  • Configuration would be easier to understand for users and contributors since having configuration in environment variables is a common pattern, having it in a ConfigMap and hot reloading it in the application is far less common from what I've been seeing

Using environment variables would also be easier then implementing environment variable substitution with the ConfigMap.

@FernandoMiguel
Copy link
Contributor

@morremeyer we pass instance profile generated in terraform into a git yaml file that is used by Argo Cd deployment of karpenter with zero issues.

@morremeyer
Copy link
Contributor

@FernandoMiguel Thank you! This does not work for us since our Terraform runs are not able to commit to the repository.

@jmdeal
Copy link
Contributor

jmdeal commented Oct 30, 2023

Since the PR name has room for improvement I'll leave a brief blurb here before closing. As part of Karpenter's graduation to beta we've removed the karpenter-global-settings configmap in favor of returning to CLI args and environment variables. The configmap will continue to exist for one minor version to give users a chance to migrate but after that will be removed entirely.

@jmdeal jmdeal closed this as completed Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants