Schema addition of valuesObject breaks kubernetes_manifest resource due to ambiguity

[jcogilvie]

Checklist:

• I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
• I've included steps to reproduce the bug.
• I've pasted the output of argocd version.

Describe the bug

The addition of valuesObject to the Application CRD in this change has used the flag x-kubernetes-preserve-unknown-fields: true. This is problematic for trying to represent the CRD schema in rigorously typed languages such as terraform.

In particular, the ambiguity of the valuesObject CRD schema addition renders our Application objects incompatible with terraform's kubernetes_manifest resources. The bug linked shows that tf will attempt to delete and recreate the manifest any time something changes, which is obviously not ideal; worse still, my own code doesn't even get that far. It just crashes with Failed to transform Object element into Object element type. (Terraform bug nonsense omitted since this is the argo repo.)

This is problematic for our infra-as-code, so I'm stuck running argo on v2.7 CRDs until something changes.

I submit that such a change (i.e. introducing ambiguity) should not be allowed without a CRD version increment, even if it's technically just additive. However, we are where we are, and there's no going back now, so I have a suggestion for a go-forward solution that I'll leave in a comment below.

To Reproduce

Try to deploy this in terraform against argo 2.8+:

resource "kubernetes_manifest" "raw" {

  field_manager {
    name            = "tf-argo-app"
    force_conflicts = var.raw_manifest_force_conflicts
  }

  dynamic "wait" {
    for_each = var.wait_for_sync && var.automatic_sync_enabled ? { wait = true } : {}
    content {
      fields = {
        "status.sync.status"   = "Synced"
        "status.health.status" = "Healthy"
      }
    }
  }

  timeouts {
    create = var.raw_manifest_create_timeout_duration
    update = var.raw_manifest_update_timeout_duration
    delete = var.raw_manifest_delete_timeout_duration
  }

  manifest = {
    apiVersion = "argoproj.io/v1alpha1"
    kind       = "Application"

    metadata = {
      name        = var.service_name
      namespace   = "argocd"
      labels      = local.labels
      annotations = local.annotations
    }

    spec = {
      project = local.project_name
      destination = {
        server    = var.destination_cluster
        namespace = var.namespace
      }
      revisionHistoryLimit = var.revision_history_limit
      sources = [for source, config in local.sources_map : {
        repoURL        = config.repo_url
        path           = config.path
        chart          = config.chart
        targetRevision = config.target_revision
        helm = {
          releaseName = config.helm.release_name
          values      = config.helm.values
        }
      }]

      syncPolicy = merge({
        retry = {
          limit = var.sync_retry_limit
          backoff = {
            duration    = var.sync_retry_backoff_base_duration
            maxDuration = var.sync_retry_backoff_max_duration
            factor      = var.sync_retry_backoff_factor
          }
        }
      }, local.sync_policy)
    }
  }
}

Redeploy it against the argo 2.7 CRD schema and watch it succeed.

Expected behavior

The schema is not ambiguous.

Screenshots

Version
I'm not using the CLI, but here's the json from the GUI:

{
    "Version": "v2.8.2+dbdfc71",
    "BuildDate": "2023-08-24T20:05:39Z",
    "GitCommit": "dbdfc712702ce2f781910a795d2e5385a4f5a0f9",
    "GitTreeState": "clean",
    "GoVersion": "go1.20.6",
    "Compiler": "gc",
    "Platform": "linux/amd64",
    "KustomizeVersion": "v5.1.0 2023-06-19T16:58:18Z",
    "HelmVersion": "v3.12.1+gf32a527",
    "KubectlVersion": "v0.24.2",
    "JsonnetVersion": "v0.20.0"
}

[jcogilvie]

Ref: https://cloud-native.slack.com/archives/C01TSERG0KZ/p1698085722846109

cc @crenshaw-dev

[jcogilvie]

Here's my take at an alternate solution that solves the patching functionality issue without ambiguity in the main Application schema (by essentially moving it elsewhere):

What if the addition was in the form of an additional CRD? A theoreticaly ApplicationValues could be as ambiguous as it needed to be, provide patch functionality; Application takes a valuesRef with a name/space pointer; we terraform people have no need to use it.

Pros:

• ApplicationValues might help alleviate some of the etcd size of a multisource app given that values are (have been?) emitted into the status field
• separation of concerns between application and configuration
  • we already support something like this by taking a valuesFile reference; a new CRD containing app values could cement this separation of concerns, and allow for novel use cases like reuse or templating within an ApplicationSet

[blakepettersson]

Once/if hashicorp/terraform-provider-kubernetes#2410 gets fixed, this may render this issue moot.

[blakepettersson]

Seems like there's a PR out; hashicorp/terraform-provider-kubernetes#2437

[GuerreiroLeonardo]

Having the same issue, with the following definition:

resource "kubernetes_manifest" "helm" {
  for_each = var.helm
  manifest = {
    "apiVersion" = "argoproj.io/v1alpha1"
    "kind"       = "Application"
    "metadata" = {
      "name"      = "${each.key}-${var.environment}"
      "namespace" = "argocd"
      "annotations" = {
        "notifications.argoproj.io/subscribe.teams" = "pipelineTeam"
      }
      "labels" = {
        "env"                 = var.environment
        "argocd/app-selector" = "${each.value.sourceRepoName}"
      }
    }
    "spec" = {
      "destination" = {
        "namespace" = each.value.namespace
        "server"    = var.k8s_server
      }
      "project" = each.value.project
      "sources" = [
        {
          "path"           = "./"
          "repoURL"        = "${var.repoURL_base}${each.value.repoURL}"
          "targetRevision" = "${var.environment}_latest"
          "ref"            = "values"
        },
        {
          "path"           = "./"
          "repoURL"        = "${var.repoURL_base}${each.value.templateRepoURL}"
          "targetRevision" = "main"
          "helm" = {
            "valueFiles" = [
              "./values.yaml",
              "$values/values.yaml",
              "$values/env/values.${var.environment}.yaml",
              "./values-pods.yaml",
            ]
            "parameters" = [
              {
                "name" : "clientId",
                "value" : "${lookup(data.azuread_application.applications, "${each.key}").application_id}"
              },
              {
                "name" : "tenantId",
                "value" : "${data.azurerm_client_config.current.tenant_id}"
              },
            ]
          }
        },
      ]
    }
  }
}

any Ideas on a workaround?

Error:

│ Error: Failed to transform List value into Tuple of different length
│ 
│   with module.argocd[0].kubernetes_manifest.helm["nowpayments-api-nestjs"],
│   on .terraform/modules/argocd/helm.tf line 1, in resource "kubernetes_manifest" "helm":
│    1: resource "kubernetes_manifest" "helm" {
│ 
│ Error: %!s(<nil>)
│ ...at attribute:
│ spec.sources
╵
╷
│ Error: Failed to transform Object element into Object element type
│ 
│   with module.argocd[0].kubernetes_manifest.helm["nowpayments-api-nestjs"],
│   on .terraform/modules/argocd/helm.tf line 1, in resource "kubernetes_manifest" "helm":
│    1: resource "kubernetes_manifest" "helm" {
│ 
│ Error (see above) at attribute:
│ spec.sources
╵
╷
│ Error: Failed to transform Object element into Object element type
│ 
│   with module.argocd[0].kubernetes_manifest.helm["nowpayments-api-nestjs"],
│   on .terraform/modules/argocd/helm.tf line 1, in resource "kubernetes_manifest" "helm":
│    1: resource "kubernetes_manifest" "helm" {
│ 
│ Error (see above) at attribute:
│ spec
╵

[blakepettersson]

@GuerreiroLeonardo ATM there is none. IMO the way to go is to push for hashicorp/terraform-provider-kubernetes#2437 to get merged.

[blakepettersson]

hashicorp/terraform-provider-kubernetes#2437 got merged! 🎉 🎉 Hopefully this will come to a k8s provider release soon.

Here's my take at an alternate solution that solves the patching functionality issue without ambiguity in the main Application schema (by essentially moving it elsewhere):

What if the addition was in the form of an additional CRD? A theoreticaly ApplicationValues could be as ambiguous as it needed to be, provide patch functionality; Application takes a valuesRef with a name/space pointer; we terraform people have no need to use it.

Pros:

* `ApplicationValues` might help alleviate some of the etcd size of a multisource app given that values are (have been?) emitted into the `status` field

* separation of concerns between application and configuration
  
  * we already support something like this by taking a `valuesFile` reference; a new CRD containing app values could cement this separation of concerns, and allow for novel use cases like reuse or templating within an `ApplicationSet`

I have another take 😄 I've looked into this issue, and think the right way to handle this is not to add another CRD - rather that we should use the tools that we already have at our disposal.

A similar effect to what you propose can be achieved by using the Github Terraform Provider (hopefully there are similar Terraform providers for other SCMs). With this you can keep Helm values out of Application Specs.

locals {
  helm_values = {
    foo = "bar"
    bla = {
      one = "two"
      two = "aaa"
    }
  }
}

# Multiple Application Sources with Helm value files from external Git repository
resource "argocd_application" "multiple_sources" {
  metadata {
    name      = "helm-app-with-external-values"
    namespace = "argocd"
  }

  spec {
    project = "default"

    source {
      repo_url        = "https://charts.helm.sh/stable"
      chart           = "wordpress"
      target_revision = "9.0.3"
      helm {
        value_files = ["$values/${github_repository_file.foo.file}"]
      }
    }

    source {
      repo_url        = github_repository.foo.http_clone_url
      target_revision = "HEAD"
      ref             = "values"
    }

    destination {
      server    = "https://kubernetes.default.svc"
      namespace = "default"
    }
  }
}

resource "github_repository" "foo" {
  name      = "tf-test"
  auto_init = true
}

resource "github_repository_file" "foo" {
  repository          = github_repository.foo.name
  branch              = "main"
  file                = "foo/values.yaml"
  content             = yamlencode(local.helm_values)
  commit_message      = "Managed by Terraform"
  commit_author       = "Terraform User"
  commit_email        = "[email protected]"
  overwrite_on_create = true
}

[jcogilvie]

We've talked through this in person before, so you know I think abiguity in your existing (or really any) CRD schema is the fundamental problem here.

The "do it in github" solution presents a number of fundamentally github-flavored problems. The github provider isn't a first class citizen at github; it's written by one dev in her spare time who happens to work there. As such, it suffers from the following issues:

• it is tremendously, painfully slow at scale.
• authentication as a github app expires after some duration and won't refresh; you have to rekick the tf run.
• github_repository_file requires the github authenticated user in your terraform context to have excessive permissions to end-around branch protection and the PR process.

I know these things because we've made extensive use of that provider in other contexts and I am full of regret. 😅

[andrii-korotkov-verkada]

Did it make it into recent argocd versions as of now?

[blakepettersson]

@andrii-korotkov-verkada the underlying Terraform ambiguity issue is likely resolved with version 2.33 of the k8s Terraform provider.

As for the stuff @jcogilvie is commenting on, this has no current resolution and is still up for debate.