vinzscam / backstage-chart

Backstage Helm Chart
27 stars 10 forks source link

Decouple the AppConfig file from ConfigMap and allow for each to be supplied separately #4

Closed ChrisJBurns closed 1 year ago

ChrisJBurns commented 2 years ago

Currently with the Chart as it is, if you deploy it into Kubernetes with a Dockerfile for the application that has something like following line in it:

CMD ["node", "packages/backend", "--config", "app-config.yaml", "--config", "app-config.production.yaml"]

It means that users are supplying the app-config via the file that is baked into the image - as opposed to using the ConfigMap as a means to inject the app-config. Now locally if you run the Image in a container, it will log that it is using both the app-config.yaml & app-config.production.yaml However when it get's to deploying this image and the Kubernetes manifests with the Helm chart, it overrides the command with the following lines. Therefore the CMD in the Dockerfile is being overriden which means both app-config.yaml & app-config.production.yaml aren't being used when the application starts up.

When I saw this, I just added the app-config to the extraAppConfig code so that it knows to use those files. However, because the volume & volumeMounts both use extraAppConfig also and trigger when it is not empty, it tries to find a ConfigMap with the name which is supplied - which in this case doesn't exist, because we are opting to use the app-config files themselves that are baked into the Image.

So there an lies my proposal, possibly we separate these out and make it configurable for users so that they can either specify which app-config files they want to use that may already be in the image, or we give them the option of being able to specify that their app-config is inside a ConfigMap. With this, we can then use the ConfigMap AppConfig in the volume and volumeMount code, and in the command, we can use the files. So we decouple the logic away from being solely ConfigMap based.

Let me know you're thoughts. Have already got this to work with the following:

  # -- AppConfig that you want your Backstage application to use
  appConfig:

    # -- AppConfig that you want to supply to your backstage application that is stored
    # inside of a ConfigMap
    asConfigMap:
      enabled: false

      # -- The different ConfigMaps to use for your AppConfig
      # Example:
      # appConfigAsConfigMap:
      #   - configMapRef: my-dev-app-config 
      #     filename: app-config.dev.yaml
      #   - configMapRef: my-prod-app-config 
      #     filename: app-config.prod.yaml
      appConfigAsConfigMap: []

    # -- AppConfig that you already have inside of the Backstage Image
    # Example:
    # appConfigFile:
    # - filename: app-config.yaml
    # - filename: app-config.production.yaml
    appConfigFile: []
vinzscam commented 2 years ago

sorry this was my mistake. If you check in the documentation, backstage.args was supposed to have ["--config", "app-config.yaml", "--config", "app-config.production.yaml"] as default value, but the value it is actually missing from values.yaml.

So there an lies my proposal, possibly we separate these out and make it configurable for users so that they can either specify which app-config files they want to use that may already be in the image, or we give them the option of being able to specify that their app-config is inside a ConfigMap.

I think users can already do that with the current chart. They could define extra configuration files in a ConfigMap and the chart automatically will append the --config option.

cmoulliard commented 2 years ago

What @ChrisJBurns is proposing basically is to have 2 ways to set up the config files to be used by the backend depending if they have been packaged within an image or embedded in a ConfigMap.

So, we should review how we define such a need part of the values file

Idea

backend:
  appConfig:
    # Configuration files packaged within the Backstage container image at the path ./
    files:
    - app-config.yaml
    - app-config.production.yaml
    # Configuration files  
    configMaps:
    - ref: my-app-config
      fileName: app-config.prod.yaml

To be of course implemented/supported differently part of the kubernetes deployment

- name: backstage-backend
  image: {{ include "backstage.image" . }}
  imagePullPolicy: {{ .Values.backstage.image.pullPolicy | quote }}
  {{- if .Values.diagnosticMode.enabled }}
  command: {{- include "common.tplvalues.render" (dict "value" .Values.diagnosticMode.command "context" $) | nindent 12 }}
  {{- else if .Values.backstage.command }}
  command: {{- include "common.tplvalues.render" (dict "value" .Values.backstage.command "context" $) | nindent 12 }}
  {{- end }}
  {{- if .Values.diagnosticMode.enabled }}
  args: {{- include "common.tplvalues.render" (dict "value" .Values.diagnosticMode.args "context" $) | nindent 12 }}
  {{- else }}
  args:
  {{- range .Values.backstage.args }}
    - {{ . | quote }}
  {{- end }}
  #
  # AppConfig packaged within the image
  #
  {{- if .Values.backstage.appConfig.files }}
  {{- range .Values.backstage.appConfig.files }}
    - "--config"
    - {{ . | quote }}
  {{- end }}
  #
  # AppConfig packaged part of the ConfigMap
  #
  {{- end }}
  {{- if .Values.backstage.appConfig.configMaps }}
  {{- range .Values.backstage.appConfig.configMaps }}
    - "--config"
    - {{ .fileName | quote }} # fileName refers to the file embedded within a ConfigMap and mounted as a volume
  {{- end }}
  {{- end }}
  {{- end }}
cmoulliard commented 2 years ago

packaged within an image

Remark: From my point of view, this is a bad practice to package the config files or tokens within a container image and on k8s such a configuration should be externalized using: configMap or secret.

ChrisJBurns commented 2 years ago

Remark: From my point of view, this is a bad practice to package the config files or tokens within a container image and on k8s such a configuration should be externalized using: configMap or secret.

@cmoulliard Agreed. Although I couldn't see anything on the Backstage docs that discouraged the packaging of the config files within the image.

vinzscam commented 2 years ago

What @ChrisJBurns is proposing basically is to have 2 ways to set up the config files to be used by the backend depending if they have been packaged within an image or embedded in a ConfigMap.

So, we should review how we define such a need part of the values file

Idea

backend:
  appConfig:
    # Configuration files packaged within the Backstage container image at the path ./
    files:
    - app-config.yaml
    - app-config.production.yaml
    # Configuration files  
    configMaps:
    - ref: my-app-config
      fileName: app-config.prod.yaml

To be of course implemented/supported differently part of the kubernetes deployment

- name: backstage-backend
  image: {{ include "backstage.image" . }}
  imagePullPolicy: {{ .Values.backstage.image.pullPolicy | quote }}
  {{- if .Values.diagnosticMode.enabled }}
  command: {{- include "common.tplvalues.render" (dict "value" .Values.diagnosticMode.command "context" $) | nindent 12 }}
  {{- else if .Values.backstage.command }}
  command: {{- include "common.tplvalues.render" (dict "value" .Values.backstage.command "context" $) | nindent 12 }}
  {{- end }}
  {{- if .Values.diagnosticMode.enabled }}
  args: {{- include "common.tplvalues.render" (dict "value" .Values.diagnosticMode.args "context" $) | nindent 12 }}
  {{- else }}
  args:
  {{- range .Values.backstage.args }}
    - {{ . | quote }}
  {{- end }}
  #
  # AppConfig packaged within the image
  #
  {{- if .Values.backstage.appConfig.files }}
  {{- range .Values.backstage.appConfig.files }}
    - "--config"
    - {{ . | quote }}
  {{- end }}
  #
  # AppConfig packaged part of the ConfigMap
  #
  {{- end }}
  {{- if .Values.backstage.appConfig.configMaps }}
  {{- range .Values.backstage.appConfig.configMaps }}
    - "--config"
    - {{ .fileName | quote }} # fileName refers to the file embedded within a ConfigMap and mounted as a volume
  {{- end }}
  {{- end }}
  {{- end }}

sorry maybe I misunderstood something, but I am still having troubles understanding what the current chart is not providing 😄

don't you reach the same result by providing the following values.yaml?

backstage:
  extraAppConfig:
    - filename: app-config.prod.yaml
      configMapRef: my-app-config

and if you want to pass extra configuration files that are already bundled in your docker image you do:

backstage:
  args: ['--config', 'my-file1.yaml', '--config', 'my-file2.yaml']
  extraAppConfig:
    - filename: app-config.prod.yaml
      configMapRef: my-app-config
cmoulliard commented 2 years ago

don't you reach the same result by providing the following values.yaml?

No as the configuration files are packaged within the image vs externalized using a ConfigMap resource like this

apiVersion: v1
data:
  app-config.extra.yaml: |
    app:
      baseUrl: http://backstage.192.168.1.90.nip.io
      title: Backstage
    backend:
      baseUrl: http://backstage.192.168.1.90.nip.io
      cors:
        origin: http://backstage.192.168.1.90.nip.io
      database:
        client: better-sqlite3
        connection: ':memory:'
      cache:
        store: memory
    techdocs:
      builder: 'local'
      generator:
        runIn: 'local'
      publisher:
        type: 'local'
kind: ConfigMap
metadata:
  name: my-app-config
  namespace: backstage
vinzscam commented 2 years ago

but backstage.extraAppConfig.configMapRef takes already a ConfigMap reference containing an app-config.yaml's content

cmoulliard commented 2 years ago

but backstage.extraAppConfig.configMapRef takes already a ConfigMap reference containing an app-config.yaml's content

Yep but when the configuration files are packaged within the image, then they will only be available when the container will start. This is why I suppose that @ChrisJBurns would like to pass as parameters only the names of the files to be read from the image (e.g filename: image-app-config.yaml) when backstage starts

ChrisJBurns commented 2 years ago

@vinzscam Hi, sorry for late reply, busy work day.

So I did leave out a few things in my original post, these things are mainly around me trying the args solution and it still didn't seem to work.

If you clone my main branch (which the postgres PR is based on) just run the chart and you'll get the following error

> helm upgrade --install backstage . -f values.yaml --kubeconfig ~/.kube/config/tools_config --namespace backstage
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/chburns/.kube/config/tools_config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/chburns/.kube/config/tools_config
Release "backstage" does not exist. Installing it now.
coalesce.go:199: warning: cannot overwrite table with non table for customRules (map[])
Error: YAML parse error on backstage/templates/backstage-deployment.yaml: error converting YAML to JSON: yaml: line 32: mapping values are not allowed in this context

I had to comment out some of the volume parts to get something to work. I will try again tonight with the main version of this repo, but I remember there being problems with the args and extraAppConfig stuff - hence the ConfigMap ticket. Will post about it more tonight.

ChrisJBurns commented 2 years ago

Ok, so I got it to work as far as I can see with my Postgres branch. The thing I was doing wrong was most likely the arguments. This is the code that worked. I haven't tried it with any ConfigMaps. Just the --config flag.

  args:
    - --config
    - app-config.production.yaml
    - --config
    - app-config.yaml
ChrisJBurns commented 2 years ago

Ok so update on this.

I noticed whilst testing some config the args override doesn't work in the way we expect. I can confirm that I get the following in the backstage pod:

2022-06-25T19:52:25.901Z backstage info Loaded config from app-config.yaml, app-config.production.yaml, env

Which is why I made the above comment about it working. But I was a bit naive and should have probably tested it a bit more because hen I actually try to go to Catalog, it errors and in the console it has the following. image

These are the problems that I was speaking about above. So it seems that the locahost URL's that are being used in the app-config.yaml are super-seeding the production yaml.

I can confirm however the configMap changes I have on a branch in my backstage repo definitely work as expected. So maybe there is a delta there that we have to look at?

cmoulliard commented 2 years ago

Which is why I made the above comment about it working. But I was a bit naive and should have probably tested it a bit more because hen I actually try to go to Catalog, it errors and in the console it has the following.

Here are the instructions I replayed. The HTTP/HTTPS problem is over if you add csp to the app config: https://github.com/halkyonio/backstage/blob/main/README.md#instructions

ChrisJBurns commented 2 years ago

For me, the csp config is in the app-config.yaml by default. So that's not really the problem in my case.

cmoulliard commented 2 years ago

I simply added from my previous config

backend:
  ...
  cors:
    ...
    methods: [GET, POST, PUT, DELETE]
    credentials: true      
  csp:
    connect-src: ["'self'", 'http:', 'https:']
ChrisJBurns commented 2 years ago

I already have that in my app-config.yaml, but I'm not getting self-signed cert errors. It's moreso not picking up the config files as it should

ChrisJBurns commented 1 year ago

I have raised https://github.com/vinzscam/backstage-chart/pull/19 to address the issues I get above when the production app config doesn't get used in the backstage containers.

froblesmartin commented 1 year ago

Hi all! First of all, thanks a lot for the work that you are doing! 😄

What would you think about providing a key in values.yaml which would be a multiline string, and then fill the ConfigMap from it to provide the whole app-config.yaml?

E.g.: values.yaml:

...
appConfigContent: |
  app:
      baseUrl: https://backstage.my.own.org
  auth:
    environment: development
    providers:
      microsoft:
...

And then a template that would generate the ConfigMap to be used by the Deployment:

apiVersion: v1
kind: ConfigMap
metadata:
  name: backstage-app-config
data:
  app-config.yaml: {{ toYaml .Values.appConfigContent | indent 2 }}

In this way, it would be straightforward to provide the whole config directly from values.yaml instead of packing the image with it.

ChrisJBurns commented 1 year ago

@froblesmartin I'm not too against this. Although the thing I can think of that may cause problems would be where some may want to specific separate ConfigMap's? In your solution it means that only one get's created- unless of course that it is in addendum to being able to specify ConfigMap's that already exist. Would have to feel what the others think on this as well.

instead of packing the image with it.

This bit isn't necessarily true. The image may have some application config packaged with it, but it also allows for the overriding of it by specification of a ConfigMap in the values.yaml. The solution you've proposed just goes that extra step further and removes the necessity for creating a ConfigMap separately and just allows the Chart to do it for you.

vinzscam commented 1 year ago

I have raised #19 to address the issues I get above when the production app config doesn't get used in the backstage containers.

Hi @ChrisJBurns, sorry, but I still can't see what's the difference between the current solution and what is implemented in #19 😞

Let me try to be more specific. Let's say we want to start backstage using two configuration files that are included in the docker image, together with an extra configuration file stored as ConfigMap.

Let's say app-config.yaml and app-config.production.yaml are included in the docker image and app-config.extra.yaml is stored in a ConfigMap.

The values.yaml listed below should cover the case here described.

I've applied a install --dry-run using the following values.yaml against the current code:

backstage:
  image:
    registry: ""
    repository: "backstage-local"
    tag: "latest"
  args:
    - --config
    - app-config.yaml
    - --config
    - app-config.production.yaml
  extraAppConfig:
    - filename: app-config.extra.yaml
      configMapRef: extra-config
  extraEnvVars:
    - name: BACKEND_SECRET
      value: my_secret

and this is the result:

  ......

    spec:
      volumes:
        - name: extra-config
          configMap:
            name: extra-config
      containers:
        - name: backstage-backend
          image: backstage-local:latest
          imagePullPolicy: "Always"
          command:
            - node
            - packages/backend
          args:
            - "--config"
            - "app-config.yaml"
            - "--config"
            - "app-config.production.yaml"
            - "--config"
            - "app-config.extra.yaml"
          env:
            - name: APP_CONFIG_backend_listen_port
              value: "7007"
            - name: BACKEND_SECRET
              value: my_secret
          ports:
            - name: backend
              containerPort: 7007
              protocol: TCP
          volumeMounts:
            - name: extra-config
              mountPath: "/app/app-config.extra.yaml"
              subPath: app-config.extra.yaml

Later, I've applied the following values.yaml

backstage:
  image:
    registry: ""
    repository: "backstage-local"
    tag: "latest"
  appConfig:
    asConfigMap:
      enabled: true
      appConfigAsConfigMap:
        - configMapRef: extra-config
          filename: app-config.extra.yaml
    appConfigFile:
      - filename: app-config.yaml
      - filename: app-config.production.yaml
  extraEnvVars:
    - name: BACKEND_SECRET
      value: my_secret

against your code. This is what I get:

   ......

   spec:
      volumes:
        - name: extra-config
          configMap:
            name: extra-config
      containers:
        - name: backstage-backend
          image: backstage-local:latest
          imagePullPolicy: "Always"
          command:
            - node
            - packages/backend
          args:
            - "--config"
            - "app-config.yaml"
            - "--config"
            - "app-config.production.yaml"
            - "--config"
            - "app-config.extra.yaml"
          env:
            - name: BACKEND_SECRET
              value: my_secret
          ports:
            - name: backend
              containerPort: 7007
              protocol: TCP
          volumeMounts:
            - name: extra-config
              mountPath: "/app/app-config.extra.yaml"
              subPath: app-config.extra.yaml

Apart from APP_CONFIG_backend_listen_port which was removed in your PR, I can't spot any difference 🤔

ChrisJBurns commented 1 year ago

@vinzscam I'll give it a go with the values you provided, however, my issue stemmed from trying to use the current implementation, and although the deployment yaml looked the same - as you've pointed out - and although the pods say that the app-config.production.yaml is being loaded, I notice that it doesn't actually work when trying to use backstage. This is what I was describing in this comment.

It's a bit funky, and am not particularly sure what's going on :( but the code in my PR seems to work, but when I try it with the chart thats on the main branch it doesn't. I'll give it one more go with your values that you've provided and see if that works, just to rule out any possible issue with the values I used before. If it doesn't work, I think we'll have to do a bit more digging about why one method works and the other doesn't, even though the same deployment yaml is generated in k8s

ChrisJBurns commented 1 year ago

@vinzscam So, just gave this a try with the config you specified above, this seems to work for me - from what I can see. I don't see any of the errors in the console like described here. I'm assuming it was due to a bad values file that I provided at the time. Possibly I weren't passing the --config flags in at the args value correctly. I think this is one of the reasons why I refactored it out slightly into it's own appConfig values block - just so it reduces the chances of someone not passing in the config flag correctly (like me 😝 ) and it leaves the chart do it automatically.

I'm happy to leave what is in the main branch as it seems to work for me now. However, going forward, if we find that folks that use the chart for the first time trip over this same issue, possibly we look at the PR i've linked, or we improve the docs a bit to make it obvious to anyone of any skill level.

Thanks again, I can close for now until told otherwise!

froblesmartin commented 1 year ago

@froblesmartin I'm not too against this. Although the thing I can think of that may cause problems would be where some may want to specific separate ConfigMap's? In your solution it means that only one get's created- unless of course that it is in addendum to being able to specify ConfigMap's that already exist. Would have to feel what the others think on this as well.

instead of packing the image with it.

This bit isn't necessarily true. The image may have some application config packaged with it, but it also allows for the overriding of it by specification of a ConfigMap in the values.yaml. The solution you've proposed just goes that extra step further and removes the necessity for creating a ConfigMap separately and just allows the Chart to do it for you.

Hi @ChrisJBurns ! Sorry for the long delay. What I was suggesting is now in the PR: https://github.com/vinzscam/backstage-chart/pull/24

Please, let me know your thoughts on it 😃

ChrisJBurns commented 1 year ago

@vinzscam @cmoulliard What do you think about @froblesmartin PR above?

cmoulliard commented 1 year ago

LGTM. One suggestion. We should perhaps as this is the case within the backstage contrib chart provides a default AppConfig File template , a configMap and mount it within the deployment

froblesmartin commented 1 year ago

LGTM. One suggestion. We should perhaps as this is the case within the backstage contrib chart provides a default AppConfig File template , a configMap and mount it within the deployment

Yeah, that would be nice for people getting it working out of the box to try it out. But I think that should be an optional feature, and do not mount it always. That makes the chart flexible and allows for various use cases.

I would suggest adding that in a separate PR than mine as that would be a different use case.

cmoulliard commented 1 year ago

I would suggest adding that in a separate PR than mine as that would be a different use case.

I agree. To use it out of box, then we could propose a boolean value part of the values.yml file