janus-idp / operator

Deprecated - Operator for Backstage, based on the Operator SDK framework - see https://github.com/redhat-developer/rhdh-operator
https://github.com/redhat-developer/rhdh-operator
Apache License 2.0
15 stars 15 forks source link

Define (initial) set of spec.parameters for overriding default/raw runtime configuration #21

Closed gazarenkov closed 12 months ago

gazarenkov commented 1 year ago

We initially have default (easy) and row (complex) runtime configuration options. We clearly needed "something in between" so user is able to specify some "important" parameters. The goal is to specify this parameters.

kadel commented 1 year ago

I've tried to create an example of the configuration:

spec:
  backstage:
    # default mount path for configs if not overridden by mountPath
    configMountPath: /opt/app-root/src
    appConfig:
      - configMapRef:
          # all keys in the my-configmap configmap will be mounted to /opt/app-root/src/<key>
          name: my-appconfig
      - secretRef:
          name: my-secret-config
          # can optinally override configMountPath for given config
          mountPath: /config
    extraConfig:
      - configMapRef:
          name: my-extra-configmap
      - secretRef:
          name: my-extra-secret
          mountPath: /extra
    env:
      - name: MY_ENV_VAR
        value: my-value
    envFrom:
      - configMapRef:
          name: my-configmap
      - secretRef:
          name: my-secret
    replicas: 1
    image: example.com/my-image:latest
    imagePullSecret: my-image-pull-secret
    annotations:
      my-annotation: my-value
    labels:
      my-label: my-value
    command: ["node", "packages/backend"]
    args: ["--config /config/config.yaml"]
    ingress:
      enabled: true
      className: nginx
      annotations:
        kubernetes.io/ingress.class: nginx
      host: my-app.example.com
      path: /
      tls:
        enabled: true
        secretName: my-app-tls
    route:
      enabled: true
      annotations:
        haproxy.router.openshift.io/timeout: 5m
      path: /
      tls:
        enabled: true
        caCertificate: 
        certificate:
        destinationCACertificate:
        key:
        insecureEdgeTerminationPolicy: redirect
        termination: edge
    service:
      type: ClusterIP
      annotations:
        service.beta.kubernetes.io/aws-load-balancer-type: external
      loadBalancerIP: 192.0.2.127
      clusterIP: 10.0.171.239

  postgresql:
    # if set to false will not create a postgresql database and user is expected to provide one
    # default is true
    enabled: true
    # TODO
rm3l commented 1 year ago

I've tried to create an example of the configuration:

spec:
  backstage:
    # default mount path for configs if not overridden by mountPath
    configMountPath: /opt/app-root/src
    appConfig:
      - configMapRef:
          # all keys in the my-configmap configmap will be mounted to /opt/app-root/src/<key>
          name: my-appconfig
      - secretRef:
          name: my-secret-config
          # can optinally override configMountPath for given config
          mountPath: /config
    extraConfig:
      - configMapRef:
          name: my-extra-configmap
      - secretRef:
          name: my-extra-secret
          mountPath: /extra
    env:
      - name: MY_ENV_VAR
        value: my-value
    envFrom:
      - configMapRef:
          name: my-configmap
      - secretRef:
          name: my-secret
    replicas: 1
    image: example.com/my-image:latest
    imagePullSecret: my-image-pull-secret
    annotations:
      my-annotation: my-value
    labels:
      my-label: my-value
    command: ["node", "packages/backend"]
    args: ["--config /config/config.yaml"]
    ingress:
      enabled: true
      className: nginx
      annotations:
        kubernetes.io/ingress.class: nginx
      host: my-app.example.com
      path: /
      tls:
        enabled: true
        secretName: my-app-tls
    route:
      enabled: true
      annotations:
        haproxy.router.openshift.io/timeout: 5m
      path: /
      tls:
        enabled: true
        caCertificate: 
        certificate:
        destinationCACertificate:
        key:
        insecureEdgeTerminationPolicy: redirect
        termination: edge
    service:
      type: ClusterIP
      annotations:
        service.beta.kubernetes.io/aws-load-balancer-type: external
      loadBalancerIP: 192.0.2.127
      clusterIP: 10.0.171.239

  postgresql:
    # if set to false will not create a postgresql database and user is expected to provide one
    # default is true
    enabled: true
    # TODO

Thanks @kadel - this is helpful!

A few questions:

  1. If spec.backstage.appConfig is specified, do you think we need to automatically update the container args to pass the proper --config /path/to/<key-in-configmap-or-secret>*.yaml [--config ...] (unless explicitly set by the user in the CR)? I guess the order in which the --config flags are specified matters? Or is it up to the user to set their spec.backstage.command and spec.backstage.args in the CR accordingly?
  2. For spec.postgresql, if enabled is true, would it make sense to generate a secret with the credentials and report the secret name into the CR status? Currently, we are expecting the user to explicitly specify a secret for PostgreSQL, which is used by the PostgreSQL instance and also mounted into the Backstage instance.. So in terms of user experience, this adds an additional step, which is specifying this secret. Note that a secret will anyways be needed to work with an external database (scope of https://github.com/janus-idp/operator/issues/8)
kadel commented 1 year ago
  1. If spec.backstage.appConfig is specified, do you think we need to automatically update the container args to pass the proper --config /path/to/<key-in-configmap-or-secret>*.yaml [--config ...] (unless explicitly set by the user in the CR)? I guess the order in which the --config flags are specified matters? Or is it up to the user to set their spec.backstage.command and spec.backstage.args in the CR accordingly?

Yes, my initial idea was that it will automatically add --config .... to args. The order of --config flags matter. Now as I'm thinking about it again, I'm not sure if this is a good idea. Maybe we can leave this for later and for now just do extraConfig (this one won't do anything with args

For spec.postgresql, if enabled is true, would it make sense to generate a secret with the credentials and report the secret name into the CR status?

yes that makes sense. If users opted into using PostgreSQL instance managed by our operator we should make the setup as easy as possible.

gazarenkov commented 1 year ago

@kadel I am still thinking about what we can call advanced configuration and what is rather basic one... I think we need to define what is the Persona type we consider who benefits from this "in between" (default and full) configuration option? How much he/she should understand internals of Backstage application (both k8s objects and backstage container)? Do we consider this kind of configuration to be used for "using" only or for some "experimenting" too? (inspired by proposal to override things like "command" and "args")

My question below is an attempt to understand it.

I've tried to create an example of the configuration:

spec:
  backstage:
    # default mount path for configs if not overridden by mountPath
    configMountPath: /opt/app-root/src
    appConfig:
      - configMapRef:
          # all keys in the my-configmap configmap will be mounted to /opt/app-root/src/<key>
          name: my-appconfig
      - secretRef:
          name: my-secret-config
          # can optinally override configMountPath for given config
          mountPath: /config
    extraConfig:
      - configMapRef:
          name: my-extra-configmap
      - secretRef:
          name: my-extra-secret
          mountPath: /extra
Do we really need both configMountPath and mountPaths to be configurable here?
I think if something goes wrong here it may cause the errors we would not be able to diagnose and send meaningful message about? 
    env:
      - name: MY_ENV_VAR
        value: my-value
    envFrom:
      - configMapRef:
          name: my-configmap
      - secretRef:
          name: my-secret
What could be in  "env" and "envFrom"?
    replicas: 1
    image: example.com/my-image:latest
    imagePullSecret: my-image-pull-secret
    annotations:
      my-annotation: my-value
    labels:
      my-label: my-value
What could these  "labels" and "annotations" for?
    command: ["node", "packages/backend"]
    args: ["--config /config/config.yaml"]
"command" and "arg" says that we can change the shape of application, is that what we intend to?
    ingress:
      enabled: true
Do we need "enabled" or just not to include it in the spec?
      className: nginx
      annotations:
        kubernetes.io/ingress.class: nginx
      host: my-app.example.com
      path: /
      tls:
        enabled: true
        secretName: my-app-tls
    route:
      enabled: true
Do we need "enabled" or just not to include it in the spec?
      annotations:
        haproxy.router.openshift.io/timeout: 5m
      path: /
      tls:
        enabled: true
        caCertificate: 
        certificate:
        destinationCACertificate:
        key:
        insecureEdgeTerminationPolicy: redirect
        termination: edge
"Ingress" and "Route" looks really like the full definition of this resource... Maybe we need to have the whole object to be defined and save some effort not documenting it? 
Alternatively (I played with it and came to this consideration) - Do we need to specify it at all (even as a whole object in a ConfigMap)? Networking seems to be too specific topic.
    service:
      type: ClusterIP
      annotations:
        service.beta.kubernetes.io/aws-load-balancer-type: external
      loadBalancerIP: 192.0.2.127
      clusterIP: 10.0.171.239
Same "Service" looks really like the full definition of this resource... Maybe we need to have the whole object to be defined here and save some effort documenting it? 

  postgresql:
    # if set to false will not create a postgresql database and user is expected to provide one
    # default is true
    enabled: true
    # TODO
kadel commented 1 year ago

Do we really need both configMountPath and mountPaths to be configurable here? I think if something goes wrong here it may cause the errors we would not be able to diagnose and send meaningful message about?

no we don't we can simplify it and just keep per-config/secret mountPath

What could be in "env" and "envFrom"?

environment variables used in mostly in configuration files, or environment variables used by backstage (like log level), you can see some of the env variables used in https://github.com/janus-idp/backstage-showcase/blob/main/showcase-docs/getting-started.md

"command" and "arg" says that we can change the shape of application, is that what we intend to?

yes, especially args is important for users to specify additional config files.

Do we need "enabled" or just not to include it in the spec?

no included could also mean, use it but with default values. I think that having explicit field for enabling/disabling is better.

"Ingress" and "Route" looks really like the full definition of this resource... Maybe we need to have the whole object to be defined and save some effort not documenting it?

most of the options are about setting up TLS/HTTPS, and we definitely need those, most of the the production deployments that will use our Route or Ingress definition will need define those options.

Same "Service" looks really like the full definition of this resource... Maybe we need to have the whole object to be defined here and save some effort documenting it?

if someone wants to define the whole object that will be able to use do it using rawRuntimeConfig, I picked those few options as it looks like something that people will need to configure for some production deployments.

I should probably check what options other operators expose and use that as our staring list for the generic resources like Ingress, Route and Service

jianrongzhang89 commented 12 months ago

@kadel @rm3l @gazarenkov I came across the grafana operator git repo and found something interesting. What they do is to simply include deployment etc directly in the GrafanaSpec data structure: https://github.com/grafana-operator/grafana-operator/blob/master/api/v1beta1/grafana_types.go#L67 This allows users to provide their customizations on the run time grafana objects as desired.

What about we use the same approach the backstage operator for the runtime object configuration? It has some benefits: 1) No need for us to pre-define which parameters we should support 2) Implementation is greatly simplified as we can use json merge so that the customization gets applied on the default configuration because they use the same data structure. 3) We can still add some validations to enforce certain rules if required.

What do you think?

rm3l commented 12 months ago

Discussion (Nov 30, 2023):

We discussed the Grafana Operator's generic approach and concluded we would just wait and see how users use our operator. We might consider this in the future if we see a need. For now, let's focus on those minimal viable fields that would make sense for a Backstage app:

spec:
  backstage:

    appConfig:
      # optional 
      mountPath:  /opt/app-root/src
      - configMapRef:
          # all keys in the my-configmap configmap will be mounted to /opt/app-root/src/<key>
          name: my-appconfig
      - secretRef:
          name: my-secret-config

    extraConfig:
      # optional 
     mountPath: /opt/app-root/src
      - configMapRef:
          name: my-extra-configmap
      - secretRef:
          name: my-extra-secret

    env:
      - name: MY_ENV_VAR
        value: my-value
    envFrom:
      - configMapRef:
          name: my-configmap
      - secretRef:
          name: my-secret

    replicas: 1

    # image and imagePullSecret should apply to all containers (main and init)
    image: example.com/my-image:latest
    imagePullSecret: my-image-pull-secret

# Postponed
#    annotations:
#      # do not override default annotations
 #     my-annotation: my-value
#    labels:
 #     # do not override default labels
  #    my-label: my-value

# Postponed
#    command: ["node", "packages/backend"]
 #   args: ["--config /config/config.yaml"]

# Ingress Postponed for now
#   ingress:
#      enabled: true
 #     className: nginx
  #    annotations:
   #     kubernetes.io/ingress.class: nginx
 #     host: my-app.example.com
 #     path: /
 #     tls:
  #      enabled: true
   #     secretName: my-app-tls

# Priority - only allow modifying host and tls config
    route:
      enabled: true
      annotations:
        haproxy.router.openshift.io/timeout: 5m
      path: /
      tls:
        enabled: true
        caCertificate: 
        certificate:
        destinationCACertificate:
        key:
        insecureEdgeTerminationPolicy: redirect
        termination: edge

# Postponed
 #   service:
  #    type: ClusterIP
 #     annotations:
 #       service.beta.kubernetes.io/aws-load-balancer-type: external
 #     loadBalancerIP: 192.0.2.127
 #     clusterIP: 10.0.171.239

# Not needed, but we should be able to disable it
  postgresql:
    # if set to false will not create a postgresql database and user is expected to provide one
    # default is true
    enabled: true
    # TODO
rm3l commented 12 months ago

Closing this issue as we agreed on a set of spec parameters that would make sense for the CR.

https://github.com/janus-idp/operator/issues/26 is the follow-up issue to implement this.