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

add templating support #336

Closed
wants to merge 1 commit into from
Closed

add templating support #336

wants to merge 1 commit into from

Conversation

nabuskey
Copy link
Collaborator

@nabuskey nabuskey commented Jul 8, 2024

This adds package templating support for idpbuilder. See #295

  1. Introduces a new flag -F. This specifies a yaml file with any additional data.
  2. Templates are written with #{ as the delimiter, instead of the usual {{. This is to avoid collision with other templates. I am open to changing this.
  3. Values used by core packages are available to custom packages as well. e.g. Host, Port, etc

File: pkg/controllers/custompackage/test/resources/customPackages/testDir/params.yaml

custom:
  value: value1

File: pkg/controllers/custompackage/test/resources/customPackages/testDir/app1/cm.yaml

apiVersion: v1
kind: ConfigMap
metadata:
  name: config
data:
  test1: "one"
  hostName: #{ .Host }#
  customParam: #{ .custom.value }#

Run idpbuilder:

/idpbuilder create -p pkg/controllers/custompackage/test/resources/customPackages/testDir \
  -F pkg/controllers/custompackage/test/resources/customPackages/testDir/params.yaml

This results in CM applied with the following config:

apiVersion: v1
data:
  customParam: value1
  hostName: cnoe.localtest.me
  test1: one
kind: ConfigMap
metadata:
  name: config
  namespace: my-app

We probably should add functions from https://github.com/Masterminds/sprig as well because these are very commonly used in K8s templating stuff.

Closes: #295

Signed-off-by: Manabu McCloskey <[email protected]>
@nimakaviani
Copy link
Contributor

love this PR. can't wait for this to be ready for review.

we may get some criticism for using comments as a way to template yaml files, similar to how carvel's ytt was under fire at times. we can brainstorm on the right delimeter but otherwise I love the direction here.

@omrishiv
Copy link
Contributor

omrishiv commented Jul 9, 2024

image

@nabuskey
Copy link
Collaborator Author

We need this for things that get checked into Gitea as well, not just for Kubernetes manifests. For example, in backstage templates, we currently hard code value like this: https://github.com/cnoe-io/stacks/blob/137a12b8e69067b71622097a0858f14865b6d563/ref-implementation/backstage-templates/entities/basic/skeleton/catalog-info.yaml#L12

These values don't work if you are to deploy this with a custom host or protocol like we do in Codespaces or C9. To work around this, currently we ask users to run a script to replace host, port, etc. This means they can't take advantage of the remote package feature we implemented. e.g.

Instead of

idpbuilder create -p https://github.com....

They have to do

git clone https://github.com....
# run script
some_script.sh
idpbuilder create -p ./<PATH>

I think we need this feature.

@nabuskey
Copy link
Collaborator Author

nabuskey commented Jul 12, 2024

More thoughts

We need a way to pass runtime information (hostname, protocol, port, etc) to argocd.
Ideally, we can pass such information through Kubernetes construct like CM or secrets.
Unfortunately, ArgoCD does not support it (Flux does).

Without this, packages need to implement their own way to configure their packages. e.g. the replace.sh script in ref-impl.

Two approaches I can think of.

Store them in a git repository and reference.

ArgoCD supports reading helm values from another repository.
As part of idpbuilder create command, we create a repository containing runtime information called idpbuilder-runtime.
Then use it in ArgoCD spec.
Full example below.

apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  sources:
  - repoURL: 'https://prometheus-community.github.io/helm-charts'
    chart: prometheus
    targetRevision: 15.7.1
    helm:
      valueFiles:
      - $values/core/values.yaml
  - repoURL: "https://cnoe.localtest.me:8443/gitea/giteadmin/idpbuilder-runtime.git"
    targetRevision: dev
    ref: values

This approach is only available to helm. If a user wants to use manifests or kustomize, this won't work. HOWEVER, I think exposing such information in a repository is a good idea because you can use it in ApplicationSet generators.

I think we should also expose these information in CM and/or secrets.

Templating support

We can also solve this with templating support. Drawback is that you pretty much have to have this everywhere.

networking:
  host: #{.host}
  protocol: #{.protocol}
  port: #{.protocol}

@nabuskey
Copy link
Collaborator Author

nabuskey commented Jul 12, 2024

Another approach is to use argocd custom plugin as suggested by Omri.

In the plugin, we can look for specific supported variables, then substitute them with values in secrets or CM. This can be done with a simple envsubst command in bash. This gives us portability. Meaning packages published can be used in idpbuilder as well as non-idpbuilder environments.

In this approach, we cannot substitute all environment variables. We MUST set specific variables we support. For example, files like this https://github.com/cnoe-io/stacks/blob/137a12b8e69067b71622097a0858f14865b6d563/ref-implementation/backstage/manifests/install.yaml#L111 must be checked into git as-is. NO substitution.

This is only supported by ArgoCD, and we will need another solution if we are to support another CD solution in the future. e.g. flux. This also means we will introduce a dependency to an image and possibly maintain it. Not a big issue imo but does add a bit of overhead.

@nimakaviani
Copy link
Contributor

nimakaviani commented Jul 12, 2024

Thanks, very interesting summary here!

for this statement:

Ideally, we can pass such information through Kubernetes construct like CM or secrets.
Unfortunately, ArgoCD does not support it (Flux does).

is this something that we can contribute back to Argo CD so that we have consistency between Argo CD and flux? looks like if we land on something that is more generic, we wont corner ourselves into having to write and maintain custom plugins or do the more tricky approach of relying on helm values to cross reference configs.

We have done lots of collaboration with the Argo community previously, and I am curious what's the amount of effort to enable passing configs to ArgoCD via CM or secrets.

@nabuskey
Copy link
Collaborator Author

nabuskey commented Jul 12, 2024

is this something that we can contribute back to Argo CD so that we have consistency between Argo CD and flux? looks like if we land on something that is more generic, we wont corner ourselves into having to write and maintain custom plugins or do the more tricky approach of relying on helm values to cross reference configs.

This is a fairly often requested feature: argoproj/argo-cd#12060

But solving this doesn't solve our problem. This applies to helm only. Does not work for kustomize or pure manifests. Idpbuilder must support all cases (helm, kustomize, and manifests) for custom packages.

This also does not solve our problem like this: https://github.com/cnoe-io/stacks/blob/137a12b8e69067b71622097a0858f14865b6d563/ref-implementation/backstage-templates/entities/basic/skeleton/catalog-info.yaml#L12. e.g. Pushing non-k8s files (does not go through argocd) to git.

It really comes down to these questions:

  1. How do I pass runtime information (hostname, port, protocol, path routing) to custom packages ( the -p flag) that goes through ArgoCD?
  2. How do I pass runtime information to files in Gitea? These files do not go through ArgoCD. Like Backstage Templates. These values must reflect runtime values.
  3. Do we want to support custom parameters for custom packages? Some packages may require parameters from users at runtime.

In this PR, I implemented support for templating and passing custom values. Although it does solve all the points above, it is essentially what Helm does at its core. It also makes it so that you can't use them as-is without idpbuilder. That is, idpbuilder must first render them before they can be used IF a user wants to use them without idpbuilder (not sure if this is a valid concern tbh).

Are we reinventing the wheel? Is this the right approach?

@blakeromano
Copy link
Contributor

I think that the general concept of building our own templates is going to break the transition path from local development to production. I think folks will want to rely on the main open source components (ArgoCD, Backstage, etc;) to do templating and manage it within their system using their standards. I think the problem I have is I don't see folks using this outside of a local development environment. And if they use this it will hinder their ability to transition their setup to a remote cluster, specifically production.

The conversation came up as to if we would use this in a remote cluster and really in my dev cycle if I am using a remote cluster I want it as close to production as I want.

I think really where I think is "Are people using IDPBuilder as their deployment tool or as a tool in a production environment" and I think the answer is no even if we get to a point where it is production ready because they are going to be married to a CI tool managing Git files and such which is really where I think we should rely on the Open Source side to worry about templating.

ArgoCD with Applicationsets supports templating out how the application is made and with Backstage there's ways to use nunjucks to do templating.

I think the maintenance overhead of this and breaking the local development to production or real-world usage is why I don't see the value in this personally.

@csantanapr
Copy link
Contributor

I would prefer to use something native to ArgoCD like ApplicationSets to pass custom data
I would make idpbuilder create the in-cluster ArgoCD secret, then use the cluster generator to pull the data into an Application spec, this in turns can pass down the data to a helm chart or kustomize to override.

If not using cluster generator, you could put the data file in git, then use the ApplicationSet git generator to pull in the customer data from plain yaml file seating in git.

In the Application CR you would do the overrides with

helm:
  valuesObect
    custom: {{.metadata.annotations.custom}}

or

kustomize:
  patches:
  - target:
      kind: Deployment
      name: external-dns
    patch: |-
      - op: add
        path: /spec/template/spec/containers/0/args/3
        value: --txt-owner-id={{.metadata.annotations.custom}}

This is an example on how I'm using both generators to pull information from cluster generator and git generator
https://github.com/csantanapr/cnoe-examples/blob/main/examples/gitops-bridge/platform/addons/charts/gitops-bridge/templates/applicationsets.yaml

# Source: gitops-bridge/templates/applicationsets.yaml
apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: gatekeeper
  namespace: argocd
spec:
  syncPolicy:
    preserveResourcesOnDeletion: true
  goTemplate: true
  goTemplateOptions:
    - missingkey=error
  generators:

    - matrix:
        generators:
          - clusters:
              selector:
                matchLabels:
                  argocd.argoproj.io/secret-type: cluster
          - git:
              repoURL: '{{.metadata.annotations.addons_repo_url}}'
              revision: HEAD
              files:
                - path: "**/stack{{.metadata.annotations.platform_stack_version}}.yaml"
  template:
    metadata:
      name: addon-gatekeeper
    spec:
      project: default
      sources:
      - repoURL: '{{.metadata.annotations.addons_repo_url}}'
        targetRevision: HEAD
        ref: values
      - chart: '{{.addons.gatekeeper.chart}}'
        repoURL: '{{.addons.gatekeeper.repoUrl}}'
        targetRevision: '{{.addons.gatekeeper.targetRevision}}'
        helm:
          releaseName: '{{.addons.gatekeeper.releaseName}}'
          ignoreMissingValueFiles: true
          valueFiles:
              - $values/values/common/{{.addons.gatekeeper.chart}}/values.yaml
              - $values/values/{{.metadata.labels.environment}}/{{.addons.gatekeeper.chart}}/values.yaml
              - $values/values/clusters/{{.name}}/{{.addons.gatekeeper.chart}}/values.yaml
      destination:
        namespace: '{{.addons.gatekeeper.namespace}}'
        name: '{{.name}}'
      syncPolicy:
        automated:
          prune: false
        retry:
          limit: 100
        syncOptions:
          - CreateNamespace=true
          - ServerSideApply=true  # Big CRDs.

@cmoulliard
Copy link
Contributor

cmoulliard commented Aug 5, 2024

Before to propose a solution, I think that we should start first to think about the user story we will look like to support top of idpbuilder, next to review the technical solutions available, challenge them (pro/con) and finally take a decision.

It is also important that we clarify what we want to do here as the process to customize an internal package could be different from what we could also propose and support for external packages (= user's packages, etc).

I also agree with the fact that, when we create a cluster and we deploy the internal packages, then we need a way to customize some of the existing YAML resources as some key(s)/value(s) highly depend on the information like: hostname, port, DNS name, etc ... to simplify the maintenance of the existing code and replace what we coded in some places with a templating engine

We should also think about the idea to adopt an idpbuilder configuration file - appConfiguration file as used by backstage or Tanzu application Platform - https://github.com/halkyonio/tap/blob/main/scripts/tap.sh#L432-L509 or CNCF Kusionstack - cncf/sandbox#83 and https://www.kusionstack.io/docs/concepts/app-configuration

@nabuskey @nimakaviani

@nabuskey
Copy link
Collaborator Author

Closing this due to inactivity. Feel free to reopen.

@nabuskey nabuskey closed this Oct 29, 2024
@cmoulliard
Copy link
Contributor

To be complete here is the PR (= status is draft) developed by argocd to support the proposition reported here and that we could rename into dynamic application parameters: argoproj/argo-cd#12050

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: Templating
6 participants