Open jcogilvie opened 11 months ago
Ref: https://cloud-native.slack.com/archives/C01TSERG0KZ/p1698085722846109
cc @crenshaw-dev
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
fieldvaluesFile
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
Once/if hashicorp/terraform-provider-kubernetes#2410 gets fixed, this may render this issue moot.
Seems like there's a PR out; hashicorp/terraform-provider-kubernetes#2437
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
╵
@GuerreiroLeonardo ATM there is none. IMO the way to go is to push for hashicorp/terraform-provider-kubernetes#2437 to get merged.
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 avaluesRef
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 = "terraform@example.com"
overwrite_on_create = true
}
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:
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. 😅
Checklist:
argocd version
.Describe the bug
The addition of
valuesObject
to theApplication
CRD in this change has used the flagx-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 ourApplication
objects incompatible with terraform'skubernetes_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 withFailed 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+:
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: