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

fix(argocd_cluster): use cluster list api to avoid 403 issues with cluster get api at cluster read time #399

56 changes: 55 additions & 1 deletion argocd/resource_argocd_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ func resourceArgoCDClusterCreate(ctx context.Context, d *schema.ResourceData, me

// Cluster are unique by "server address" so we should check there is no existing cluster with this address before
existingClusters, err := si.ClusterClient.List(ctx, &clusterClient.ClusterQuery{
// Starting argo-cd server v2.8.0 filtering on list api endpoint is fixed, else it is ignored, see:
// - https://github.com/oboukili/terraform-provider-argocd/issues/266#issuecomment-1739122022
// - https://github.com/argoproj/argo-cd/pull/13363
Id: &clusterClient.ClusterID{
Type: "server",
Value: rtrimmedServer,
Expand All @@ -53,6 +56,7 @@ func resourceArgoCDClusterCreate(ctx context.Context, d *schema.ResourceData, me
return errorToDiagnostics(fmt.Sprintf("failed to list existing clusters when creating cluster %s", cluster.Server), err)
}

// Here we will filter ourselves on the list so that we are backward compatible for argo-cd server with version < v2.8.0 (see coment above)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need backward compat to versions below 2.8? That is outside of our promise, no?

Copy link
Collaborator

@the-technat the-technat Oct 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned In my review I don't think we need that compat, but since it's already coded in a way that doesn't harm we should leave it as is so far.

if len(existingClusters.Items) > 0 {
for _, existingCluster := range existingClusters.Items {
if rtrimmedServer == strings.TrimRight(existingCluster.Server, "/") {
Expand Down Expand Up @@ -103,7 +107,57 @@ func resourceArgoCDClusterRead(ctx context.Context, d *schema.ResourceData, meta
return nil
}

return argoCDAPIError("read", "cluster", d.Id(), err)
// Fix for https://github.com/oboukili/terraform-provider-argocd/issues/266
// This fix is added here as a workaround to ensure backward compatibility, as
// it is triggered only on the specific usecase where the issue happens.
// Additional remarks about this code:
// * it is a copy/paste of the code used by resourceArgoCDClusterCreate to check if
// the cluster already exists (with some obvious changes to return value and mutex type)
// * it should at term replace the `si.ClusterClient.Get` code for this method
if strings.Contains(err.Error(), "PermissionDenied") {
cluster, err := expandCluster(d)
if err != nil {
return errorToDiagnostics("failed to expand cluster", err)
}

tokenMutexClusters.RLock()

rtrimmedServer := strings.TrimRight(cluster.Server, "/")

// Cluster are unique by "server address" so we should check there is no existing cluster with this address before
existingClusters, err := si.ClusterClient.List(ctx, &clusterClient.ClusterQuery{
// Starting argo-cd server v2.8.0 filtering on list api endpoint is fixed, else it is ignored, see:
// - https://github.com/oboukili/terraform-provider-argocd/issues/266#issuecomment-1739122022
// - https://github.com/argoproj/argo-cd/pull/13363
Id: &clusterClient.ClusterID{
Type: "server",
Value: rtrimmedServer,
},
})

tokenMutexClusters.RUnlock()

if err != nil {
return errorToDiagnostics(fmt.Sprintf("failed to list existing clusters when reading cluster %s", cluster.Server), err)
}

// Here we will filter ourselves on the list so that we are backward compatible for argo-cd server with version < v2.8.0 (see coment above)
if len(existingClusters.Items) > 0 {
for _, existingCluster := range existingClusters.Items {
if rtrimmedServer == strings.TrimRight(existingCluster.Server, "/") {
// Cluster was found, return
return nil
}
}
}

// Cluster was not found, return with empty Id
d.SetId("")

return nil
} else {
return argoCDAPIError("read", "cluster", d.Id(), err)
}
}

if err = flattenCluster(c, d); err != nil {
Expand Down
100 changes: 100 additions & 0 deletions argocd/resource_argocd_cluster_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
package argocd

import (
"context"
"fmt"
"os"
"regexp"
"runtime"
"strconv"
"testing"
"time"

"github.com/argoproj/argo-cd/v2/pkg/apiclient/cluster"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/oboukili/terraform-provider-argocd/internal/provider"
mkilchhofer marked this conversation as resolved.
Show resolved Hide resolved
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/util/homedir"
Expand Down Expand Up @@ -292,6 +299,74 @@ func TestAccArgoCDCluster_invalidSameServer(t *testing.T) {
})
}

func TestAccArgoCDCluster_outsideDeletion(t *testing.T) {
clusterName := acctest.RandString(10)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
ProviderFactories: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccArgoCDClusterMetadata(clusterName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"argocd_cluster.cluster_metadata",
"info.0.connection_state.0.status",
"Successful",
),
resource.TestCheckResourceAttr(
"argocd_cluster.cluster_metadata",
"config.0.tls_client_config.0.insecure",
"true",
),
resource.TestCheckResourceAttr(
"argocd_cluster.cluster_metadata",
"name",
clusterName,
),
),
},
{
PreConfig: func() {
// delete cluster and validate refresh generates a plan
// (non-regression test for https://github.com/oboukili/terraform-provider-argocd/issues/266)
si, err := getServerInterface()
if err != nil {
t.Error(fmt.Errorf("failed to get server interface: %s", err.Error()))
}
ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second)
defer cancel()
_, err = si.ClusterClient.Delete(ctx, &cluster.ClusterQuery{Name: clusterName})
if err != nil {
t.Error(fmt.Errorf("failed to delete cluster '%s': %s", clusterName, err.Error()))
}
},
RefreshState: true,
ExpectNonEmptyPlan: true,
},
{
Config: testAccArgoCDClusterMetadata(clusterName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(
"argocd_cluster.cluster_metadata",
"info.0.connection_state.0.status",
"Successful",
),
resource.TestCheckResourceAttr(
"argocd_cluster.cluster_metadata",
"config.0.tls_client_config.0.insecure",
"true",
),
resource.TestCheckResourceAttr(
"argocd_cluster.cluster_metadata",
"name",
clusterName,
),
),
},
},
})
}

func TestAccArgoCDCluster_namespacesErrorWhenEmpty(t *testing.T) {
name := acctest.RandString(10)

Expand Down Expand Up @@ -619,3 +694,28 @@ func getInternalRestConfig() (*rest.Config, error) {

return nil, fmt.Errorf("could not find a kind-argocd cluster from the current ~/.kube/config file")
}

// build & init ArgoCD server interface
func getServerInterface() (*provider.ServerInterface, error) {
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

insecure, err := strconv.ParseBool(os.Getenv("ARGOCD_INSECURE"))
if err != nil {
return nil, fmt.Errorf("failed to parse 'ARGOCD_INSECURE' env var to bool: %s", err.Error())
}

si := provider.NewServerInterface(provider.ArgoCDProviderConfig{
ServerAddr: types.StringValue(os.Getenv("ARGOCD_SERVER")),
Insecure: types.BoolValue(insecure),
Username: types.StringValue(os.Getenv("ARGOCD_AUTH_USERNAME")),
Password: types.StringValue(os.Getenv("ARGOCD_AUTH_PASSWORD")),
})

diag := si.InitClients(ctx)
if diag.HasError() {
return nil, fmt.Errorf("failed to init clients: %v", diag.Errors())
}

return si, nil
}
Loading