From 858a48bc98a7653714c447cf3fc627b49863fd11 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Wed, 29 Jan 2025 13:00:34 +0200 Subject: [PATCH 01/22] adding context cancellation to get cmd and cancelling the context before printFinalStatus on sync command Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 3ce81351b33a1..f054d4c79e7d2 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -337,6 +337,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com refresh bool hardRefresh bool output string + timeout int showParams bool showOperation bool appNamespace string @@ -382,7 +383,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com `), Run: func(c *cobra.Command, args []string) { - ctx := c.Context() + ctx, cancel := context.WithCancel(c.Context()) if len(args) == 0 { c.HelpFunc()(c, args) os.Exit(1) @@ -393,6 +394,13 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com appName, appNs := argo.ParseFromQualifiedName(args[0], appNamespace) + if timeout != 0 { + time.AfterFunc(time.Duration(timeout)*time.Second, func() { + fmt.Println("Context cancelled due to timeout") + cancel() + }) + } + app, err := appIf.Get(ctx, &application.ApplicationQuery{ Name: &appName, Refresh: getRefreshType(refresh, hardRefresh), @@ -462,6 +470,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com }, } command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide|tree") + command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.") command.Flags().BoolVar(&showOperation, "show-operation", false, "Show application operation") command.Flags().BoolVar(&showParams, "show-params", false, "Show application parameters and overrides") command.Flags().BoolVar(&refresh, "refresh", false, "Refresh application data when retrieving") @@ -2588,8 +2597,8 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, fmt.Println("This is the state of the app after `wait` timed out:") } - printFinalStatus(app) cancel() + printFinalStatus(app) if printSummary { fmt.Println() From af3434328c84d473f19ed7a217229a64af8285ea Mon Sep 17 00:00:00 2001 From: Jagpreet Singh Tamber Date: Tue, 28 Jan 2025 19:00:43 -0500 Subject: [PATCH 02/22] feat: add support for azure workload identity in Microsoft Entra SSO (#21433) Signed-off-by: Jagpreet Singh Tamber Signed-off-by: Alexandre Gaudreault Co-authored-by: Alexandre Gaudreault Signed-off-by: Almo Elda --- .../user-management/microsoft.md | 153 ++++++++-------- util/oidc/oidc.go | 91 +++++++++- util/oidc/oidc_test.go | 164 +++++++++++++++++- util/settings/settings.go | 13 ++ util/settings/settings_test.go | 49 ++++++ util/test/testutil.go | 61 ++++++- 6 files changed, 441 insertions(+), 90 deletions(-) diff --git a/docs/operator-manual/user-management/microsoft.md b/docs/operator-manual/user-management/microsoft.md index 72a3a3ce77980..ac431bdc78c0e 100644 --- a/docs/operator-manual/user-management/microsoft.md +++ b/docs/operator-manual/user-management/microsoft.md @@ -3,76 +3,10 @@ !!! note "" Entra ID was formerly known as Azure AD. -* [Entra ID SAML Enterprise App Auth using Dex](#entra-id-saml-enterprise-app-auth-using-dex) * [Entra ID App Registration Auth using OIDC](#entra-id-app-registration-auth-using-oidc) +* [Entra ID SAML Enterprise App Auth using Dex](#entra-id-saml-enterprise-app-auth-using-dex) * [Entra ID App Registration Auth using Dex](#entra-id-app-registration-auth-using-dex) -## Entra ID SAML Enterprise App Auth using Dex -### Configure a new Entra ID Enterprise App - -1. From the `Microsoft Entra ID` > `Enterprise applications` menu, choose `+ New application` -2. Select `Non-gallery application` -3. Enter a `Name` for the application (e.g. `Argo CD`), then choose `Add` -4. Once the application is created, open it from the `Enterprise applications` menu. -5. From the `Users and groups` menu of the app, add any users or groups requiring access to the service. - ![Azure Enterprise SAML Users](../../assets/azure-enterprise-users.png "Azure Enterprise SAML Users") -6. From the `Single sign-on` menu, edit the `Basic SAML Configuration` section as follows (replacing `my-argo-cd-url` with your Argo URL): - - **Identifier (Entity ID):** https://``/api/dex/callback - - **Reply URL (Assertion Consumer Service URL):** https://``/api/dex/callback - - **Sign on URL:** https://``/auth/login - - **Relay State:** `` - - **Logout Url:** `` - ![Azure Enterprise SAML URLs](../../assets/azure-enterprise-saml-urls.png "Azure Enterprise SAML URLs") -7. From the `Single sign-on` menu, edit the `User Attributes & Claims` section to create the following claims: - - `+ Add new claim` | **Name:** email | **Source:** Attribute | **Source attribute:** user.mail - - `+ Add group claim` | **Which groups:** All groups | **Source attribute:** Group ID | **Customize:** True | **Name:** Group | **Namespace:** `` | **Emit groups as role claims:** False - - *Note: The `Unique User Identifier` required claim can be left as the default `user.userprincipalname`* - ![Azure Enterprise SAML Claims](../../assets/azure-enterprise-claims.png "Azure Enterprise SAML Claims") -8. From the `Single sign-on` menu, download the SAML Signing Certificate (Base64) - - Base64 encode the contents of the downloaded certificate file, for example: - - `$ cat ArgoCD.cer | base64` - - *Keep a copy of the encoded output to be used in the next section.* -9. From the `Single sign-on` menu, copy the `Login URL` parameter, to be used in the next section. - -### Configure Argo to use the new Entra ID Enterprise App - -1. Edit `argocd-cm` and add the following `dex.config` to the data section, replacing the `caData`, `my-argo-cd-url` and `my-login-url` your values from the Entra ID App: - - data: - url: https://my-argo-cd-url - dex.config: | - logger: - level: debug - format: json - connectors: - - type: saml - id: saml - name: saml - config: - entityIssuer: https://my-argo-cd-url/api/dex/callback - ssoURL: https://my-login-url (e.g. https://login.microsoftonline.com/xxxxx/a/saml2) - caData: | - MY-BASE64-ENCODED-CERTIFICATE-DATA - redirectURI: https://my-argo-cd-url/api/dex/callback - usernameAttr: email - emailAttr: email - groupsAttr: Group - -2. Edit `argocd-rbac-cm` to configure permissions, similar to example below. - - Use Entra ID `Group IDs` for assigning roles. - - See [RBAC Configurations](../rbac.md) for more detailed scenarios. - - # example policy - policy.default: role:readonly - policy.csv: | - p, role:org-admin, applications, *, */*, allow - p, role:org-admin, clusters, get, *, allow - p, role:org-admin, repositories, get, *, allow - p, role:org-admin, repositories, create, *, allow - p, role:org-admin, repositories, update, *, allow - p, role:org-admin, repositories, delete, *, allow - g, "84ce98d1-e359-4f3b-85af-985b458de3c6", role:org-admin # (azure group assigned to role) - ## Entra ID App Registration Auth using OIDC ### Configure a new Entra ID App registration #### Add a new Entra ID App registration @@ -96,7 +30,18 @@ ![Azure App registration's Authentication](../../assets/azure-app-registration-authentication.png "Azure App registration's Authentication") #### Add credentials a new Entra ID App registration - +##### Using Workload Identity Federation (Recommended) +1. **Label the Pods:** Add the `azure.workload.identity/use: "true"` label to the `argocd-server` pods. +2. **Add Annotation to Service Account:** Add `azure.workload.identity/client-id: "$CLIENT_ID"` annotation to the `argocd-server` service account using the details from application created in previous step. +3. From the `Certificates & secrets` menu, navigate to `Federated credentials`, then choose `+ Add credential` +4. Choose `Federated credential scenario` as `Kubernetes Accessing Azure resources` + - Enter Cluster Issuer URL, refer to [retrieve the OIDC issuer URL](https://learn.microsoft.com/en-us/azure/aks/workload-identity-deploy-cluster#retrieve-the-oidc-issuer-url) documentation + - Enter namespace as the namespace where the argocd is deployed + - Enter service account name as `argocd-server` + - Enter a unique name + - Click Add. + +##### Using Client Secret 1. From the `Certificates & secrets` menu, choose `+ New client secret` 2. Enter a `Name` for the secret (e.g. `ArgoCD-SSO`). - Make sure to copy and save generated value. This is a value for the `client_secret`. @@ -129,7 +74,9 @@ name: Azure issuer: https://login.microsoftonline.com/{directory_tenant_id}/v2.0 clientID: {azure_ad_application_client_id} - clientSecret: $oidc.azure.clientSecret + clientSecret: $oidc.azure.clientSecret // if using client secret for authentication + azure: + useWorkloadIdentity: true // if using azure workload identity for authentication requestedIDTokenClaims: groups: essential: true @@ -139,7 +86,7 @@ - profile - email -2. Edit `argocd-secret` and configure the `data.oidc.azure.clientSecret` section: +2. Skip this step if using azure workload identity. Edit `argocd-secret` and configure the `data.oidc.azure.clientSecret` section: Secret -> argocd-secret @@ -177,6 +124,72 @@ Refer to [operator-manual/argocd-rbac-cm.yaml](https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/argocd-rbac-cm.yaml) for all of the available variables. +## Entra ID SAML Enterprise App Auth using Dex +### Configure a new Entra ID Enterprise App + +1. From the `Microsoft Entra ID` > `Enterprise applications` menu, choose `+ New application` +2. Select `Non-gallery application` +3. Enter a `Name` for the application (e.g. `Argo CD`), then choose `Add` +4. Once the application is created, open it from the `Enterprise applications` menu. +5. From the `Users and groups` menu of the app, add any users or groups requiring access to the service. + ![Azure Enterprise SAML Users](../../assets/azure-enterprise-users.png "Azure Enterprise SAML Users") +6. From the `Single sign-on` menu, edit the `Basic SAML Configuration` section as follows (replacing `my-argo-cd-url` with your Argo URL): + - **Identifier (Entity ID):** https://``/api/dex/callback + - **Reply URL (Assertion Consumer Service URL):** https://``/api/dex/callback + - **Sign on URL:** https://``/auth/login + - **Relay State:** `` + - **Logout Url:** `` + ![Azure Enterprise SAML URLs](../../assets/azure-enterprise-saml-urls.png "Azure Enterprise SAML URLs") +7. From the `Single sign-on` menu, edit the `User Attributes & Claims` section to create the following claims: + - `+ Add new claim` | **Name:** email | **Source:** Attribute | **Source attribute:** user.mail + - `+ Add group claim` | **Which groups:** All groups | **Source attribute:** Group ID | **Customize:** True | **Name:** Group | **Namespace:** `` | **Emit groups as role claims:** False + - *Note: The `Unique User Identifier` required claim can be left as the default `user.userprincipalname`* + ![Azure Enterprise SAML Claims](../../assets/azure-enterprise-claims.png "Azure Enterprise SAML Claims") +8. From the `Single sign-on` menu, download the SAML Signing Certificate (Base64) + - Base64 encode the contents of the downloaded certificate file, for example: + - `$ cat ArgoCD.cer | base64` + - *Keep a copy of the encoded output to be used in the next section.* +9. From the `Single sign-on` menu, copy the `Login URL` parameter, to be used in the next section. + +### Configure Argo to use the new Entra ID Enterprise App + +1. Edit `argocd-cm` and add the following `dex.config` to the data section, replacing the `caData`, `my-argo-cd-url` and `my-login-url` your values from the Entra ID App: + + data: + url: https://my-argo-cd-url + dex.config: | + logger: + level: debug + format: json + connectors: + - type: saml + id: saml + name: saml + config: + entityIssuer: https://my-argo-cd-url/api/dex/callback + ssoURL: https://my-login-url (e.g. https://login.microsoftonline.com/xxxxx/a/saml2) + caData: | + MY-BASE64-ENCODED-CERTIFICATE-DATA + redirectURI: https://my-argo-cd-url/api/dex/callback + usernameAttr: email + emailAttr: email + groupsAttr: Group + +2. Edit `argocd-rbac-cm` to configure permissions, similar to example below. + - Use Entra ID `Group IDs` for assigning roles. + - See [RBAC Configurations](../rbac.md) for more detailed scenarios. + + # example policy + policy.default: role:readonly + policy.csv: | + p, role:org-admin, applications, *, */*, allow + p, role:org-admin, clusters, get, *, allow + p, role:org-admin, repositories, get, *, allow + p, role:org-admin, repositories, create, *, allow + p, role:org-admin, repositories, update, *, allow + p, role:org-admin, repositories, delete, *, allow + g, "84ce98d1-e359-4f3b-85af-985b458de3c6", role:org-admin # (azure group assigned to role) + ## Entra ID App Registration Auth using Dex Configure a new AD App Registration, as above. diff --git a/util/oidc/oidc.go b/util/oidc/oidc.go index bebb59605cb28..2633cd8f308c3 100644 --- a/util/oidc/oidc.go +++ b/util/oidc/oidc.go @@ -1,6 +1,7 @@ package oidc import ( + "context" "encoding/hex" "encoding/json" "errors" @@ -14,6 +15,7 @@ import ( "os" "path" "strings" + "sync" "time" gooidc "github.com/coreos/go-oidc/v3/oidc" @@ -60,6 +62,8 @@ type ClientApp struct { clientID string // OAuth2 client secret of this application clientSecret string + // Use Azure Workload Identity for clientID auth instead of clientSecret + useAzureWorkloadIdentity bool // Callback URL for OAuth2 responses (e.g. https://argocd.example.com/auth/callback) redirectURI string // URL of the issuer (e.g. https://argocd.example.com/api/dex) @@ -81,6 +85,17 @@ type ClientApp struct { provider Provider // clientCache represent a cache of sso artifact clientCache cache.CacheClient + // properties for azure workload identity. + azure azureApp +} + +type azureApp struct { + // federated azure token for the service account + assertion string + // expiry of the token + expires time.Time + // mutex for parallelism for reading the token + mtx *sync.RWMutex } func GetScopesOrDefault(scopes []string) []string { @@ -102,14 +117,16 @@ func NewClientApp(settings *settings.ArgoCDSettings, dexServerAddr string, dexTl return nil, err } a := ClientApp{ - clientID: settings.OAuth2ClientID(), - clientSecret: settings.OAuth2ClientSecret(), - redirectURI: redirectURL, - issuerURL: settings.IssuerURL(), - userInfoPath: settings.UserInfoPath(), - baseHRef: baseHRef, - encryptionKey: encryptionKey, - clientCache: cacheClient, + clientID: settings.OAuth2ClientID(), + clientSecret: settings.OAuth2ClientSecret(), + useAzureWorkloadIdentity: settings.UseAzureWorkloadIdentity(), + redirectURI: redirectURL, + issuerURL: settings.IssuerURL(), + userInfoPath: settings.UserInfoPath(), + baseHRef: baseHRef, + encryptionKey: encryptionKey, + clientCache: cacheClient, + azure: azureApp{mtx: &sync.RWMutex{}}, } log.Infof("Creating client app (%s)", a.clientID) u, err := url.Parse(settings.URL) @@ -158,6 +175,7 @@ func (a *ClientApp) oauth2Config(request *http.Request, scopes []string) (*oauth log.Warnf("Unable to find ArgoCD URL from request, falling back to configured redirect URI: %v", err) redirectURL = a.redirectURI } + return &oauth2.Config{ ClientID: a.clientID, ClientSecret: a.clientSecret, @@ -336,6 +354,44 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, url, http.StatusSeeOther) } +// getFederatedServiceAccountToken returns the specified file's content, which is expected to be Federated Kubernetes service account token. +// Kubernetes is responsible for updating the file as service account tokens expire. +// Azure Workload Identity mutation webhook will set the environment variable AZURE_FEDERATED_TOKEN_FILE +// Content of this file will contain a federated token which can be used in assertion with Microsoft Entra Application. +func (a *azureApp) getFederatedServiceAccountToken(context.Context) (string, error) { + file, ok := os.LookupEnv("AZURE_FEDERATED_TOKEN_FILE") + if file == "" || !ok { + return "", errors.New("AZURE_FEDERATED_TOKEN_FILE env variable not found, make sure workload identity is enabled on the cluster") + } + + if _, err := os.Stat(file); errors.Is(err, os.ErrNotExist) { + return "", errors.New("AZURE_FEDERATED_TOKEN_FILE specified file does not exist") + } + + a.mtx.RLock() + if a.expires.Before(time.Now()) { + // ensure only one goroutine at a time updates the assertion + a.mtx.RUnlock() + a.mtx.Lock() + defer a.mtx.Unlock() + // double check because another goroutine may have acquired the write lock first and done the update + if now := time.Now(); a.expires.Before(now) { + content, err := os.ReadFile(file) + if err != nil { + return "", err + } + a.assertion = string(content) + // Kubernetes rotates service account tokens when they reach 80% of their total TTL. The shortest TTL + // is 1 hour. That implies the token we just read is valid for at least 12 minutes (20% of 1 hour), + // but we add some margin for safety. + a.expires = now.Add(10 * time.Minute) + } + } else { + defer a.mtx.RUnlock() + } + return a.assertion, nil +} + // HandleCallback is the callback handler for an OAuth2 login flow func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) { oauth2Config, err := a.oauth2Config(r, nil) @@ -361,12 +417,29 @@ func (a *ClientApp) HandleCallback(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } + ctx := gooidc.ClientContext(r.Context(), a.client) - token, err := oauth2Config.Exchange(ctx, code) + options := []oauth2.AuthCodeOption{} + + if a.useAzureWorkloadIdentity { + clientAssertion, err := a.azure.getFederatedServiceAccountToken(ctx) + if err != nil { + http.Error(w, fmt.Sprintf("failed to generate client assertion: %v", err), http.StatusInternalServerError) + return + } + + options = []oauth2.AuthCodeOption{ + oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), + oauth2.SetAuthURLParam("client_assertion", clientAssertion), + } + } + + token, err := oauth2Config.Exchange(ctx, code, options...) if err != nil { http.Error(w, fmt.Sprintf("failed to get token: %v", err), http.StatusInternalServerError) return } + idTokenRAW, ok := token.Extra("id_token").(string) if !ok { http.Error(w, "no id_token in token response", http.StatusInternalServerError) diff --git a/util/oidc/oidc_test.go b/util/oidc/oidc_test.go index 47bd34a6a78fd..46d1e1f7904f6 100644 --- a/util/oidc/oidc_test.go +++ b/util/oidc/oidc_test.go @@ -1,6 +1,7 @@ package oidc import ( + "context" "crypto/tls" "encoding/hex" "encoding/json" @@ -9,7 +10,9 @@ import ( "net/http/httptest" "net/url" "os" + "path/filepath" "strings" + "sync" "testing" "time" @@ -29,6 +32,18 @@ import ( "github.com/argoproj/argo-cd/v3/util/test" ) +func setupAzureIdentity(t *testing.T) { + t.Helper() + + tempDir := t.TempDir() + tokenFilePath := filepath.Join(tempDir, "token.txt") + tempFile, err := os.Create(tokenFilePath) + require.NoError(t, err) + _, err = tempFile.Write([]byte("serviceAccountToken")) + require.NoError(t, err) + t.Setenv("AZURE_FEDERATED_TOKEN_FILE", tokenFilePath) +} + func TestInferGrantType(t *testing.T) { for _, path := range []string{"dex", "okta", "auth0", "onelogin"} { t.Run(path, func(t *testing.T) { @@ -137,9 +152,7 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL), app.HandleLogin(w, req) - if !strings.Contains(w.Body.String(), "certificate signed by unknown authority") && !strings.Contains(w.Body.String(), "certificate is not trusted") { - t.Fatal("did not receive expected certificate verification failure error") - } + assert.Contains(t, w.Body.String(), "certificate signed by unknown authority") cdSettings.OIDCTLSInsecureSkipVerify = true @@ -167,7 +180,6 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL), cert, err := tls.X509KeyPair(test.Cert, test.PrivateKey) require.NoError(t, err) cdSettings.Certificate = &cert - app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour)) require.NoError(t, err) @@ -213,7 +225,6 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL), cert, err := tls.X509KeyPair(test.Cert, test.PrivateKey) require.NoError(t, err) cdSettings.Certificate = &cert - app, err := NewClientApp(cdSettings, dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour)) require.NoError(t, err) @@ -308,7 +319,6 @@ clientSecret: yyy requestedScopes: ["oidc"]`, oidcTestServer.URL), OIDCTLSInsecureSkipVerify: true, } - // The base href (the last argument for NewClientApp) is what HandleLogin will fall back to when no explicit // redirect URL is given. app, err := NewClientApp(cdSettings, "", nil, "/", cache.NewInMemoryCache(24*time.Hour)) @@ -393,7 +403,6 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL), cert, err := tls.X509KeyPair(test.Cert, test.PrivateKey) require.NoError(t, err) cdSettings.Certificate = &cert - app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour)) require.NoError(t, err) @@ -419,6 +428,144 @@ requestedScopes: ["oidc"]`, oidcTestServer.URL), }) } +func Test_azureApp_getFederatedServiceAccountToken(t *testing.T) { + app := azureApp{mtx: &sync.RWMutex{}} + + setupAzureIdentity(t) + + t.Run("before the method call assertion should be empty.", func(t *testing.T) { + assert.Equal(t, "", app.assertion) + }) + + t.Run("Fetch the token value from the file", func(t *testing.T) { + _, err := app.getFederatedServiceAccountToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "serviceAccountToken", app.assertion) + }) + + t.Run("Workload Identity Not enabled.", func(t *testing.T) { + t.Setenv("AZURE_FEDERATED_TOKEN_FILE", "") + _, err := app.getFederatedServiceAccountToken(context.Background()) + assert.ErrorContains(t, err, "AZURE_FEDERATED_TOKEN_FILE env variable not found, make sure workload identity is enabled on the cluster") + }) + + t.Run("Workload Identity invalid file", func(t *testing.T) { + t.Setenv("AZURE_FEDERATED_TOKEN_FILE", filepath.Join(t.TempDir(), "invalid.txt")) + _, err := app.getFederatedServiceAccountToken(context.Background()) + assert.ErrorContains(t, err, "AZURE_FEDERATED_TOKEN_FILE specified file does not exist") + }) + + t.Run("Concurrent access to the function", func(t *testing.T) { + currentExpiryTime := app.expires + + var wg sync.WaitGroup + numGoroutines := 10 + wg.Add(numGoroutines) + for i := 0; i < numGoroutines; i++ { + go func() { + defer wg.Done() + _, err := app.getFederatedServiceAccountToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "serviceAccountToken", app.assertion) + }() + } + wg.Wait() + + // Event with multiple concurrent calls the expiry time should not change untile it passes. + assert.Equal(t, currentExpiryTime, app.expires) + }) + + t.Run("Concurrent access to the function when the current token expires", func(t *testing.T) { + var wg sync.WaitGroup + currentExpiryTime := app.expires + app.expires = time.Now() + numGoroutines := 10 + wg.Add(numGoroutines) + for i := 0; i < numGoroutines; i++ { + go func() { + defer wg.Done() + _, err := app.getFederatedServiceAccountToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "serviceAccountToken", app.assertion) + }() + } + wg.Wait() + + assert.NotEqual(t, currentExpiryTime, app.expires) + }) +} + +func TestClientAppWithAzureWorkloadIdentity_HandleCallback(t *testing.T) { + tokenRequestAssertions := func(r *http.Request) { + err := r.ParseForm() + require.NoError(t, err) + + formData := r.Form + clientAssertion := formData.Get("client_assertion") + clientAssertionType := formData.Get("client_assertion_type") + assert.Equal(t, "serviceAccountToken", clientAssertion) + assert.Equal(t, "urn:ietf:params:oauth:client-assertion-type:jwt-bearer", clientAssertionType) + } + + oidcTestServer := test.GetAzureOIDCTestServer(t, tokenRequestAssertions) + t.Cleanup(oidcTestServer.Close) + + dexTestServer := test.GetDexTestServer(t) + t.Cleanup(dexTestServer.Close) + signature, err := util.MakeSignature(32) + require.NoError(t, err) + + setupAzureIdentity(t) + + t.Run("oidc certificate checking during oidc callback should toggle on config", func(t *testing.T) { + cdSettings := &settings.ArgoCDSettings{ + URL: "https://argocd.example.com", + ServerSignature: signature, + OIDCConfigRAW: fmt.Sprintf(` +name: Test +issuer: %s +clientID: xxx +azure: + useWorkloadIdentity: true +skipAudienceCheckWhenTokenHasNoAudience: true +requestedScopes: ["oidc"]`, oidcTestServer.URL), + } + + app, err := NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour)) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodGet, "https://argocd.example.com/auth/callback", nil) + req.Form = url.Values{ + "code": {"abc"}, + "state": {"123"}, + } + w := httptest.NewRecorder() + + app.HandleCallback(w, req) + + if !strings.Contains(w.Body.String(), "certificate signed by unknown authority") && !strings.Contains(w.Body.String(), "certificate is not trusted") { + t.Fatal("did not receive expected certificate verification failure error") + } + + cdSettings.OIDCTLSInsecureSkipVerify = true + + app, err = NewClientApp(cdSettings, dexTestServer.URL, nil, "https://argocd.example.com", cache.NewInMemoryCache(24*time.Hour)) + require.NoError(t, err) + + w = httptest.NewRecorder() + + key, err := cdSettings.GetServerEncryptionKey() + require.NoError(t, err) + encrypted, _ := crypto.Encrypt([]byte("123"), key) + req.AddCookie(&http.Cookie{Name: common.StateCookieName, Value: hex.EncodeToString(encrypted)}) + + app.HandleCallback(w, req) + + assert.NotContains(t, w.Body.String(), "certificate is not trusted") + assert.NotContains(t, w.Body.String(), "certificate signed by unknown authority") + }) +} + func TestIsValidRedirect(t *testing.T) { tests := []struct { name string @@ -594,7 +741,6 @@ func TestGenerateAppState_NoReturnURL(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/", nil) encrypted, err := crypto.Encrypt([]byte("123"), key) require.NoError(t, err) - app, err := NewClientApp(cdSettings, "", nil, "/argo-cd", cache.NewInMemoryCache(24*time.Hour)) require.NoError(t, err) @@ -714,7 +860,7 @@ func TestGetUserInfo(t *testing.T) { idpClaims: jwt.MapClaims{"sub": "randomUser", "exp": float64(time.Now().Add(5 * time.Minute).Unix())}, idpHandler: func(w http.ResponseWriter, _ *http.Request) { userInfoBytes := ` - notevenJsongarbage + notevenJsongarbage ` _, err := w.Write([]byte(userInfoBytes)) if err != nil { diff --git a/util/settings/settings.go b/util/settings/settings.go index c6dca2bbe12eb..42f3eb980938f 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -169,6 +169,7 @@ func (o *oidcConfig) toExported() *OIDCConfig { Issuer: o.Issuer, ClientID: o.ClientID, ClientSecret: o.ClientSecret, + Azure: o.Azure, CLIClientID: o.CLIClientID, UserInfoPath: o.UserInfoPath, EnableUserInfoGroups: o.EnableUserInfoGroups, @@ -197,6 +198,11 @@ type OIDCConfig struct { RootCA string `json:"rootCA,omitempty"` EnablePKCEAuthentication bool `json:"enablePKCEAuthentication,omitempty"` DomainHint string `json:"domainHint,omitempty"` + Azure *AzureOIDCConfig `json:"azure,omitempty"` +} + +type AzureOIDCConfig struct { + UseWorkloadIdentity bool `json:"useWorkloadIdentity,omitempty"` } // DEPRECATED. Helm repository credentials are now managed using RepoCredentials @@ -2024,6 +2030,13 @@ func (a *ArgoCDSettings) OAuth2ClientSecret() string { return "" } +func (a *ArgoCDSettings) UseAzureWorkloadIdentity() bool { + if oidcConfig := a.OIDCConfig(); oidcConfig != nil && oidcConfig.Azure != nil { + return oidcConfig.Azure.UseWorkloadIdentity + } + return false +} + // OIDCTLSConfig returns the TLS config for the OIDC provider. If an external provider is configured, returns a TLS // config using the root CAs (if any) specified in the OIDC config. If an external OIDC provider is not configured, // returns the API server TLS config, because the API server proxies requests to Dex. diff --git a/util/settings/settings_test.go b/util/settings/settings_test.go index b85396e4eb65d..48052833ebf0a 100644 --- a/util/settings/settings_test.go +++ b/util/settings/settings_test.go @@ -1829,6 +1829,55 @@ func TestRedirectAdditionalURLs(t *testing.T) { } } +func TestUseAzureWorkloadIdentity(t *testing.T) { + testCases := []struct { + Name string + Settings *ArgoCDSettings + ExpectedResult bool + }{ + { + Name: "UseAzureWorkloadIdentity defined and set to true", + Settings: &ArgoCDSettings{ + OIDCConfigRAW: "{ \"azure\": {\"useWorkloadIdentity\": true }}", + }, + ExpectedResult: true, + }, + { + Name: "UseAzureWorkloadIdentity defined and set to false", + Settings: &ArgoCDSettings{ + OIDCConfigRAW: "{ \"azure\": {\"useWorkloadIdentity\": false }}", + }, + ExpectedResult: false, + }, + { + Name: "UseAzureWorkloadIdentity not defined, with azure key present", + Settings: &ArgoCDSettings{ + OIDCConfigRAW: "{ \"azure\": {}}", + }, + ExpectedResult: false, + }, + { + Name: "UseAzureWorkloadIdentity not defined", + Settings: &ArgoCDSettings{ + OIDCConfigRAW: "{}", + }, + ExpectedResult: false, + }, + { + Name: "OIDC config isnot defined", + Settings: &ArgoCDSettings{}, + ExpectedResult: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + result := tc.Settings.UseAzureWorkloadIdentity() + require.Equal(t, tc.ExpectedResult, result) + }) + } +} + func TestIsImpersonationEnabled(t *testing.T) { // When there is no argocd-cm itself, // Then IsImpersonationEnabled() must return false (default value) and an error with appropriate error message. diff --git a/util/test/testutil.go b/util/test/testutil.go index 0da029fdae023..0a30036e73d3c 100644 --- a/util/test/testutil.go +++ b/util/test/testutil.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/go-jose/go-jose/v3" "github.com/golang-jwt/jwt/v5" @@ -148,7 +149,7 @@ func GetDexTestServer(t *testing.T) *httptest.Server { return ts } -func oidcMockHandler(t *testing.T, url string) func(http.ResponseWriter, *http.Request) { +func oidcMockHandler(t *testing.T, url string, tokenRequestPreHandler func(r *http.Request)) func(http.ResponseWriter, *http.Request) { t.Helper() return func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -196,6 +197,16 @@ func oidcMockHandler(t *testing.T, url string) func(http.ResponseWriter, *http.R require.NoError(t, err) _, err = w.Write(out) require.NoError(t, err) + case "/token": + if tokenRequestPreHandler != nil { + tokenRequestPreHandler(r) + } + response, err := mockTokenEndpointResponse(url) + require.NoError(t, err) + out, err := json.Marshal(response) + require.NoError(t, err) + _, err = w.Write(out) + require.NoError(t, err) default: w.WriteHeader(http.StatusNotFound) } @@ -208,7 +219,53 @@ func GetOIDCTestServer(t *testing.T) *httptest.Server { // Start with a placeholder. We need the server URL before setting up the real handler. })) ts.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - oidcMockHandler(t, ts.URL)(w, r) + oidcMockHandler(t, ts.URL, nil)(w, r) + }) + return ts +} + +func GetAzureOIDCTestServer(t *testing.T, tokenRequestPreHandler func(r *http.Request)) *httptest.Server { + t.Helper() + ts := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + // Start with a placeholder. We need the server URL before setting up the real handler. + })) + ts.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + oidcMockHandler(t, ts.URL, tokenRequestPreHandler)(w, r) }) return ts } + +type TokenResponse struct { + AccessToken string `json:"access_token"` + TokenType string `json:"token_type"` + ExpiresIn int `json:"expires_in"` + IDToken string `json:"id_token"` + RefreshToken string `json:"refresh_token"` +} + +func mockTokenEndpointResponse(issuer string) (TokenResponse, error) { + token, err := generateJWTToken(issuer) + return TokenResponse{ + AccessToken: token, + TokenType: "Bearer", + ExpiresIn: 3600, + IDToken: token, + RefreshToken: token, + }, err +} + +// Helper function to generate a JWT token +func generateJWTToken(issuer string) (string, error) { + token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ + "sub": "1234567890", + "name": "John Doe", + "iat": time.Now().Unix(), + "iss": issuer, + "exp": time.Now().Add(time.Hour).Unix(), // Set the expiration time + }) + tokenString, err := token.SignedString([]byte("secret")) + if err != nil { + return "", err + } + return tokenString, nil +} From c5153c58c9954f54ae1ef6fec02bb28e776b424d Mon Sep 17 00:00:00 2001 From: Keith Chong Date: Tue, 28 Jan 2025 21:42:33 -0500 Subject: [PATCH 03/22] fix: Update argo-ui dependency to pull in OCI icon (#18646) (#21698) Signed-off-by: Keith Chong Signed-off-by: Almo Elda --- ui/yarn.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/yarn.lock b/ui/yarn.lock index a4e410b1d30de..d21f8b147e2bd 100644 --- a/ui/yarn.lock +++ b/ui/yarn.lock @@ -2705,7 +2705,7 @@ arg@^4.1.0: "argo-ui@git+https://github.com/argoproj/argo-ui.git": version "1.0.0" - resolved "git+https://github.com/argoproj/argo-ui.git#d9a4285dc254bc473b9f26f5d72655296233f003" + resolved "git+https://github.com/argoproj/argo-ui.git#5cf36101733ce43eed57242a12389f2a7e40bd2b" dependencies: "@fortawesome/fontawesome-free" "^6.2.1" "@tippy.js/react" "^3.1.1" From 312dd15f85f5da46a1f18a77ee1cdc4ea210b3ed Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 28 Jan 2025 21:44:13 -0500 Subject: [PATCH 04/22] chore(deps): bump github.com/bmatcuk/doublestar/v4 from 4.8.0 to 4.8.1 (#21677) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Almo Elda --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 9ba36c7794447..d318bfcb6a99f 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/argoproj/notifications-engine v0.4.1-0.20241007194503-2fef5c9049fd github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 github.com/aws/aws-sdk-go v1.55.6 - github.com/bmatcuk/doublestar/v4 v4.8.0 + github.com/bmatcuk/doublestar/v4 v4.8.1 github.com/bombsimon/logrusr/v4 v4.1.0 github.com/bradleyfalzon/ghinstallation/v2 v2.13.0 github.com/casbin/casbin/v2 v2.103.0 diff --git a/go.sum b/go.sum index 2d0185e507bc7..cc4952ebfb1d7 100644 --- a/go.sum +++ b/go.sum @@ -149,8 +149,8 @@ github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kB github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM= github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ= github.com/bmatcuk/doublestar/v4 v4.6.1/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= -github.com/bmatcuk/doublestar/v4 v4.8.0 h1:DSXtrypQddoug1459viM9X9D3dp1Z7993fw36I2kNcQ= -github.com/bmatcuk/doublestar/v4 v4.8.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= +github.com/bmatcuk/doublestar/v4 v4.8.1 h1:54Bopc5c2cAvhLRAzqOGCYHYyhcDHsFF4wWIR5wKP38= +github.com/bmatcuk/doublestar/v4 v4.8.1/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/bombsimon/logrusr/v4 v4.1.0 h1:uZNPbwusB0eUXlO8hIUwStE6Lr5bLN6IgYgG+75kuh4= github.com/bombsimon/logrusr/v4 v4.1.0/go.mod h1:pjfHC5e59CvjTBIU3V3sGhFWFAnsnhOR03TRc6im0l8= github.com/bradleyfalzon/ghinstallation/v2 v2.13.0 h1:5FhjW93/YLQJDmPdeyMPw7IjAPzqsr+0jHPfrPz0sZI= From 9ef20a5431694ae8c4038988135c3e6322af71d9 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 28 Jan 2025 21:44:34 -0500 Subject: [PATCH 05/22] chore(deps): bump google.golang.org/protobuf from 1.36.3 to 1.36.4 (#21676) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Almo Elda --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index d318bfcb6a99f..289ab2082c145 100644 --- a/go.mod +++ b/go.mod @@ -94,7 +94,7 @@ require ( golang.org/x/time v0.9.0 google.golang.org/genproto/googleapis/api v0.0.0-20241104194629-dd2ea8efbc28 google.golang.org/grpc v1.69.4 - google.golang.org/protobuf v1.36.3 + google.golang.org/protobuf v1.36.4 gopkg.in/yaml.v2 v2.4.0 gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.31.0 diff --git a/go.sum b/go.sum index cc4952ebfb1d7..519c8d70a3b0f 100644 --- a/go.sum +++ b/go.sum @@ -1381,8 +1381,8 @@ google.golang.org/protobuf v1.27.1/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQ google.golang.org/protobuf v1.28.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos= google.golang.org/protobuf v1.34.2/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw= -google.golang.org/protobuf v1.36.3 h1:82DV7MYdb8anAVi3qge1wSnMDrnKK7ebr+I0hHRN1BU= -google.golang.org/protobuf v1.36.3/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE= +google.golang.org/protobuf v1.36.4 h1:6A3ZDJHn/eNqc1i+IdefRzy/9PokBTPvcqMySR7NNIM= +google.golang.org/protobuf v1.36.4/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE= gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw= gopkg.in/alexcesaro/quotedprintable.v3 v3.0.0-20150716171945-2caba252f4dc h1:2gGKlE2+asNV9m7xrywl36YYNnBG5ZQ0r/BOOxqPpmk= gopkg.in/alexcesaro/quotedprintable.v3 v3.0.0-20150716171945-2caba252f4dc/go.mod h1:m7x9LTH6d71AHyAX77c9yqWCCa3UKHcVEj9y7hAtKDk= From 9790d93d8273ffe8796e09779ad15a58f61aef1f Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Wed, 29 Jan 2025 00:02:00 -0500 Subject: [PATCH 06/22] chore(metrics)!: remove deprecated metrics (#21697) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Almo Elda --- controller/metrics/metrics.go | 43 ---------------------- controller/metrics/metrics_test.go | 24 ------------ docs/operator-manual/upgrading/2.14-3.0.md | 16 ++++++++ 3 files changed, 16 insertions(+), 67 deletions(-) diff --git a/controller/metrics/metrics.go b/controller/metrics/metrics.go index 0913763b271fd..cdbdefb6f49ad 100644 --- a/controller/metrics/metrics.go +++ b/controller/metrics/metrics.go @@ -49,8 +49,6 @@ type MetricsServer struct { const ( // MetricsPath is the endpoint to collect application metrics MetricsPath = "/metrics" - // EnvVarLegacyControllerMetrics is a env var to re-enable deprecated prometheus metrics - EnvVarLegacyControllerMetrics = "ARGOCD_LEGACY_CONTROLLER_METRICS" ) // Follow Prometheus naming practices @@ -68,28 +66,6 @@ var ( nil, ) - // Deprecated - descAppCreated = prometheus.NewDesc( - "argocd_app_created_time", - "Creation time in unix timestamp for an application.", - descAppDefaultLabels, - nil, - ) - // Deprecated: superseded by sync_status label in argocd_app_info - descAppSyncStatusCode = prometheus.NewDesc( - "argocd_app_sync_status", - "The application current sync status.", - append(descAppDefaultLabels, "sync_status"), - nil, - ) - // Deprecated: superseded by health_status label in argocd_app_info - descAppHealthStatus = prometheus.NewDesc( - "argocd_app_health_status", - "The application current health status.", - append(descAppDefaultLabels, "health_status"), - nil, - ) - syncCounter = prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "argocd_app_sync_total", @@ -381,8 +357,6 @@ func (c *appCollector) Describe(ch chan<- *prometheus.Desc) { ch <- descAppConditions } ch <- descAppInfo - ch <- descAppSyncStatusCode - ch <- descAppHealthStatus } // Collect implements the prometheus.Collector interface @@ -456,21 +430,4 @@ func (c *appCollector) collectApps(ch chan<- prometheus.Metric, app *argoappv1.A addGauge(descAppConditions, float64(count), conditionType) } } - - // Deprecated controller metrics - if os.Getenv(EnvVarLegacyControllerMetrics) == "true" { - addGauge(descAppCreated, float64(app.CreationTimestamp.Unix())) - - addGauge(descAppSyncStatusCode, boolFloat64(syncStatus == argoappv1.SyncStatusCodeSynced), string(argoappv1.SyncStatusCodeSynced)) - addGauge(descAppSyncStatusCode, boolFloat64(syncStatus == argoappv1.SyncStatusCodeOutOfSync), string(argoappv1.SyncStatusCodeOutOfSync)) - addGauge(descAppSyncStatusCode, boolFloat64(syncStatus == argoappv1.SyncStatusCodeUnknown || syncStatus == ""), string(argoappv1.SyncStatusCodeUnknown)) - - healthStatus := app.Status.Health.Status - addGauge(descAppHealthStatus, boolFloat64(healthStatus == health.HealthStatusUnknown || healthStatus == ""), string(health.HealthStatusUnknown)) - addGauge(descAppHealthStatus, boolFloat64(healthStatus == health.HealthStatusProgressing), string(health.HealthStatusProgressing)) - addGauge(descAppHealthStatus, boolFloat64(healthStatus == health.HealthStatusSuspended), string(health.HealthStatusSuspended)) - addGauge(descAppHealthStatus, boolFloat64(healthStatus == health.HealthStatusHealthy), string(health.HealthStatusHealthy)) - addGauge(descAppHealthStatus, boolFloat64(healthStatus == health.HealthStatusDegraded), string(health.HealthStatusDegraded)) - addGauge(descAppHealthStatus, boolFloat64(healthStatus == health.HealthStatusMissing), string(health.HealthStatusMissing)) - } } diff --git a/controller/metrics/metrics_test.go b/controller/metrics/metrics_test.go index 41eac978c4111..f091ebfd3f0b0 100644 --- a/controller/metrics/metrics_test.go +++ b/controller/metrics/metrics_test.go @@ -399,30 +399,6 @@ argocd_app_condition{condition="ExcludedResourceWarning",name="my-app-4",namespa } } -func TestLegacyMetrics(t *testing.T) { - t.Setenv(EnvVarLegacyControllerMetrics, "true") - - expectedResponse := ` -# HELP argocd_app_created_time Creation time in unix timestamp for an application. -# TYPE argocd_app_created_time gauge -argocd_app_created_time{name="my-app",namespace="argocd",project="important-project"} -6.21355968e+10 -# HELP argocd_app_health_status The application current health status. -# TYPE argocd_app_health_status gauge -argocd_app_health_status{health_status="Degraded",name="my-app",namespace="argocd",project="important-project"} 0 -argocd_app_health_status{health_status="Healthy",name="my-app",namespace="argocd",project="important-project"} 1 -argocd_app_health_status{health_status="Missing",name="my-app",namespace="argocd",project="important-project"} 0 -argocd_app_health_status{health_status="Progressing",name="my-app",namespace="argocd",project="important-project"} 0 -argocd_app_health_status{health_status="Suspended",name="my-app",namespace="argocd",project="important-project"} 0 -argocd_app_health_status{health_status="Unknown",name="my-app",namespace="argocd",project="important-project"} 0 -# HELP argocd_app_sync_status The application current sync status. -# TYPE argocd_app_sync_status gauge -argocd_app_sync_status{name="my-app",namespace="argocd",project="important-project",sync_status="OutOfSync"} 0 -argocd_app_sync_status{name="my-app",namespace="argocd",project="important-project",sync_status="Synced"} 1 -argocd_app_sync_status{name="my-app",namespace="argocd",project="important-project",sync_status="Unknown"} 0 -` - testApp(t, []string{fakeApp}, expectedResponse) -} - func TestMetricsSyncCounter(t *testing.T) { cancel, appLister := newFakeLister() defer cancel() diff --git a/docs/operator-manual/upgrading/2.14-3.0.md b/docs/operator-manual/upgrading/2.14-3.0.md index 0c377228423fa..34a7b7ae032b1 100644 --- a/docs/operator-manual/upgrading/2.14-3.0.md +++ b/docs/operator-manual/upgrading/2.14-3.0.md @@ -23,6 +23,22 @@ to `false` in the Argo CD ConfigMap `argocd-cm`. Read the [RBAC documentation](../rbac.md#fine-grained-permissions-for-updatedelete-action) for more detailed inforamtion. +### Removal of `argocd_app_sync_status`, `argocd_app_health_status` and `argocd_app_created_time` Metrics + +The `argocd_app_sync_status`, `argocd_app_health_status` and `argocd_app_created_time`, deprecated and disabled by +default since 1.5.0, have been removed. The information previously provided by these metrics is now available as labels +on the `argocd_app_info` metric. + +#### Detection + +Starting with 1.5.0, these metrics are only available if `ARGOCD_LEGACY_CONTROLLER_METRICS` is explicitly set to `true`. +If it is not set to true, you can safely upgrade with no changes. + +#### Migration + +If you are using these metrics, you will need to update your monitoring dashboards and alerts to use the new metric and +labels before upgrading. + ## Other changes ### Using `cluster.inClusterEnabled: "false"` From 265536cea1e9ac96f77dbb86be3c0d03bb79963d Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Wed, 29 Jan 2025 13:25:00 +0200 Subject: [PATCH 07/22] codegen Signed-off-by: Almo Elda --- docs/user-guide/commands/argocd_app_get.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/user-guide/commands/argocd_app_get.md b/docs/user-guide/commands/argocd_app_get.md index 667978ee27c85..e341d99ca1228 100644 --- a/docs/user-guide/commands/argocd_app_get.md +++ b/docs/user-guide/commands/argocd_app_get.md @@ -57,6 +57,7 @@ argocd app get APPNAME [flags] --show-params Show application parameters and overrides --source-name string Name of the source from the list of sources of the app. --source-position int Position of the source from the list of sources of the app. Counting starts at 1. (default -1) + --timeout int Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded. (default 15) ``` ### Options inherited from parent commands From ac12366a87865db93d7c44bc1a9d0072225fdca6 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Wed, 29 Jan 2025 13:27:33 +0200 Subject: [PATCH 08/22] add cancel call to defer on app get Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index f054d4c79e7d2..8b887a01a0e13 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -390,7 +390,10 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com } acdClient := headless.NewClientOrDie(clientOpts, c) conn, appIf := acdClient.NewApplicationClientOrDie() - defer argoio.Close(conn) + defer func() { + argoio.Close(conn) + cancel() + }() appName, appNs := argo.ParseFromQualifiedName(args[0], appNamespace) From 564b7cb4f26f7fcdadc92defbfe9c48e609e08f7 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Wed, 29 Jan 2025 19:47:22 +0200 Subject: [PATCH 09/22] app get timeout to uint, app sync do not print after timeout reached Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 8b887a01a0e13..5dcb1924dbbbd 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -337,7 +337,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com refresh bool hardRefresh bool output string - timeout int + timeout uint showParams bool showOperation bool appNamespace string @@ -399,7 +399,6 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com if timeout != 0 { time.AfterFunc(time.Duration(timeout)*time.Second, func() { - fmt.Println("Context cancelled due to timeout") cancel() }) } @@ -473,7 +472,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com }, } command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide|tree") - command.Flags().IntVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.") + command.Flags().UintVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.") command.Flags().BoolVar(&showOperation, "show-operation", false, "Show application operation") command.Flags().BoolVar(&showParams, "show-params", false, "Show application parameters and overrides") command.Flags().BoolVar(&refresh, "refresh", false, "Refresh application data when retrieving") @@ -2589,7 +2588,7 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, if timeout != 0 { time.AfterFunc(time.Duration(timeout)*time.Second, func() { _, appClient := acdClient.NewApplicationClientOrDie() - app, err := appClient.Get(ctx, &application.ApplicationQuery{ + _, err := appClient.Get(ctx, &application.ApplicationQuery{ Name: &appRealName, AppNamespace: &appNs, }) @@ -2601,7 +2600,6 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, } cancel() - printFinalStatus(app) if printSummary { fmt.Println() From 8449bab9ec461620911509e535610878de6e805f Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Wed, 29 Jan 2025 19:48:38 +0200 Subject: [PATCH 10/22] app get timeout default change to common variable, changed description also Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 5dcb1924dbbbd..4a568efe09b44 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -472,7 +472,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com }, } command.Flags().StringVarP(&output, "output", "o", "wide", "Output format. One of: json|yaml|wide|tree") - command.Flags().UintVar(&timeout, "timeout", 15, "Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded.") + command.Flags().UintVar(&timeout, "timeout", defaultCheckTimeoutSeconds, "Time out after this many seconds") command.Flags().BoolVar(&showOperation, "show-operation", false, "Show application operation") command.Flags().BoolVar(&showParams, "show-params", false, "Show application parameters and overrides") command.Flags().BoolVar(&refresh, "refresh", false, "Refresh application data when retrieving") From 6f13f63b6a2dd8d5e905200cfeb14049133e50be Mon Sep 17 00:00:00 2001 From: almoelda <42950693+almoelda@users.noreply.github.com> Date: Sat, 1 Feb 2025 11:54:47 +0200 Subject: [PATCH 11/22] Update cmd/argocd/commands/app.go Co-authored-by: Alexandre Gaudreault Signed-off-by: almoelda <42950693+almoelda@users.noreply.github.com> --- cmd/argocd/commands/app.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 4a568efe09b44..63beec4edba39 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -390,10 +390,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com } acdClient := headless.NewClientOrDie(clientOpts, c) conn, appIf := acdClient.NewApplicationClientOrDie() - defer func() { - argoio.Close(conn) - cancel() - }() + defer argoio.Close(conn) appName, appNs := argo.ParseFromQualifiedName(args[0], appNamespace) From 0061e3fadc349b0a8736d770bea36bc6384cb5c1 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 12:38:31 +0200 Subject: [PATCH 12/22] codegen Signed-off-by: Almo Elda --- docs/user-guide/commands/argocd_app_get.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user-guide/commands/argocd_app_get.md b/docs/user-guide/commands/argocd_app_get.md index e341d99ca1228..8b35039fa71b0 100644 --- a/docs/user-guide/commands/argocd_app_get.md +++ b/docs/user-guide/commands/argocd_app_get.md @@ -57,7 +57,7 @@ argocd app get APPNAME [flags] --show-params Show application parameters and overrides --source-name string Name of the source from the list of sources of the app. --source-position int Position of the source from the list of sources of the app. Counting starts at 1. (default -1) - --timeout int Specifies the maximum duration for the operation to complete. The command will terminate if the timeout is exceeded. (default 15) + --timeout uint Time out after this many seconds ``` ### Options inherited from parent commands From e69263c8eecabde7c3574477c9e90d6db45eae63 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 12:39:09 +0200 Subject: [PATCH 13/22] adding a test and a simulateTimeout to the mock client Signed-off-by: Almo Elda --- cmd/argocd/commands/app_test.go | 76 ++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/cmd/argocd/commands/app_test.go b/cmd/argocd/commands/app_test.go index 37b688f5cdfcd..cc7e0c8b908a5 100644 --- a/cmd/argocd/commands/app_test.go +++ b/cmd/argocd/commands/app_test.go @@ -1938,6 +1938,71 @@ apps Deployment default test Synced Healthy assert.Equalf(t, expectationSorted, outputSorted, "Incorrect output %q, should be %q (items order doesn't matter)", output, expectation) } +func TestWaitOnApplicationStatus_JSON_YAML_WideOutput_With_Timeout(t *testing.T) { + acdClient := &customAcdClient{&fakeAcdClient{simulateTimeout: 15}} + ctx := context.Background() + var selectResource []*v1alpha1.SyncOperationResource + watch := watchOpts{ + sync: false, + health: false, + operation: true, + suspended: false, + } + watch = getWatchOpts(watch) + + output, _ := captureOutput(func() error { + _, _, _ = waitOnApplicationStatus(ctx, acdClient, "app-name", 5, watch, selectResource, "") + return nil + }) + timeStr := time.Now().Format("2006-01-02T15:04:05-07:00") + + expectation := `TIMESTAMP GROUP KIND NAMESPACE NAME STATUS HEALTH HOOK MESSAGE +%s Service default service-name1 Synced Healthy +%s apps Deployment default test Synced Healthy + +The command timed out waiting for the conditions to be met. + +This is the state of the app after wait timed out: + +Name: argocd/test +Project: default +Server: local +Namespace: argocd +URL: http://localhost:8080/applications/app-name +Source: +- Repo: test + Target: master + Path: /test + Helm Values: path1,path2 + Name Prefix: prefix +SyncWindow: Sync Allowed +Sync Policy: Automated (Prune) +Sync Status: OutOfSync from master +Health Status: Progressing (health-message) + +Operation: Sync +Sync Revision: revision +Phase: +Start: 0001-01-01 00:00:00 +0000 UTC +Finished: 2020-11-10 23:00:00 +0000 UTC +Duration: 2333448h16m18.871345152s +Message: test + +GROUP KIND NAMESPACE NAME STATUS HEALTH HOOK MESSAGE + Service default service-name1 Synced Healthy +apps Deployment default test Synced Healthy +` + expectation = fmt.Sprintf(expectation, timeStr, timeStr) + expectationParts := strings.Split(expectation, "\n") + slices.Sort(expectationParts) + expectationSorted := strings.Join(expectationParts, "\n") + outputParts := strings.Split(output, "\n") + slices.Sort(outputParts) + outputSorted := strings.Join(outputParts, "\n") + // Need to compare sorted since map entries may not keep a specific order during serialization, leading to flakiness. + assert.Equalf(t, expectationSorted, outputSorted, "Incorrect output %q, should be %q (items order doesn't matter)", output, expectation) +} + type customAcdClient struct { *fakeAcdClient } @@ -1956,6 +2021,7 @@ func (c *customAcdClient) WatchApplicationWithRetry(ctx context.Context, _ strin } go func() { + time.Sleep(time.Duration(c.simulateTimeout) * time.Second) appEventsCh <- &v1alpha1.ApplicationWatchEvent{ Type: watch.Bookmark, Application: newApp, @@ -2170,7 +2236,9 @@ func (c *fakeAppServiceClient) ListResourceLinks(_ context.Context, _ *applicati return nil, nil } -type fakeAcdClient struct{} +type fakeAcdClient struct { + simulateTimeout uint +} func (c *fakeAcdClient) ClientOptions() argocdclient.ClientOptions { return argocdclient.ClientOptions{} @@ -2287,6 +2355,12 @@ func (c *fakeAcdClient) NewAccountClientOrDie() (io.Closer, accountpkg.AccountSe func (c *fakeAcdClient) WatchApplicationWithRetry(_ context.Context, _ string, _ string) chan *v1alpha1.ApplicationWatchEvent { appEventsCh := make(chan *v1alpha1.ApplicationWatchEvent) + err := os.WriteFile("outputfile.txt", []byte("Simulating timeout for seconds"), 0644) + if err != nil { + fmt.Println(err) + } + time.Sleep(time.Duration(c.simulateTimeout) * time.Second) + go func() { modifiedEvent := new(v1alpha1.ApplicationWatchEvent) modifiedEvent.Type = watch.Modified From 7c13a304749d7d2d9c5de0bb6ecd1cf87768cc90 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 12:43:50 +0200 Subject: [PATCH 14/22] timeout handling Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 63beec4edba39..6585244455d28 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -384,6 +384,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com Run: func(c *cobra.Command, args []string) { ctx, cancel := context.WithCancel(c.Context()) + defer cancel() if len(args) == 0 { c.HelpFunc()(c, args) os.Exit(1) @@ -2519,11 +2520,17 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, ctx, cancel := context.WithCancel(ctx) defer cancel() + // `app` will be used to print the final status of the app + var app *argoappv1.Application + // refresh controls whether or not we refresh the app before printing the final status. // We only want to do this when an operation is in progress, since operations are the only // time when the sync status lags behind when an operation completes refresh := false + // appUrl is declared here so that it can be used in the printFinalStatus function when the context is cancelled + appUrl := appURL(ctx, acdClient, appName) + // printSummary controls whether we print the app summary table, OperationState, and ResourceState // We don't want to print these when output type is json or yaml, as the output would become unparsable. printSummary := output != "json" && output != "yaml" @@ -2546,7 +2553,7 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, if printSummary { fmt.Println() - printAppSummaryTable(app, appURL(ctx, acdClient, appName), nil) + printAppSummaryTable(app, appUrl, nil) fmt.Println() if watch.operation { printOperationResult(app.Status.OperationState) @@ -2584,18 +2591,23 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, if timeout != 0 { time.AfterFunc(time.Duration(timeout)*time.Second, func() { - _, appClient := acdClient.NewApplicationClientOrDie() - _, err := appClient.Get(ctx, &application.ApplicationQuery{ + conn, appClient := acdClient.NewApplicationClientOrDie() + defer conn.Close() + // We want to print the final status of the app even if the conditions are not met + if printSummary { + fmt.Println() + fmt.Println("This is the state of the app after wait timed out:") + } + // Setting refresh = false because we don't want printFinalStatus to execute a refresh + refresh = false + // Updating the app object to the latest state + timedOutAppState, err := appClient.Get(ctx, &application.ApplicationQuery{ Name: &appRealName, AppNamespace: &appNs, }) errors.CheckError(err) - - if printSummary { - fmt.Println() - fmt.Println("This is the state of the app after `wait` timed out:") - } - + app = timedOutAppState + // Cancel the context to stop the watch cancel() if printSummary { From 74b736ee6ffd3772f73a140b115482773e0f0552 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 12:52:07 +0200 Subject: [PATCH 15/22] lose debug code Signed-off-by: Almo Elda --- cmd/argocd/commands/app_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cmd/argocd/commands/app_test.go b/cmd/argocd/commands/app_test.go index cc7e0c8b908a5..e89930b9306de 100644 --- a/cmd/argocd/commands/app_test.go +++ b/cmd/argocd/commands/app_test.go @@ -2355,10 +2355,6 @@ func (c *fakeAcdClient) NewAccountClientOrDie() (io.Closer, accountpkg.AccountSe func (c *fakeAcdClient) WatchApplicationWithRetry(_ context.Context, _ string, _ string) chan *v1alpha1.ApplicationWatchEvent { appEventsCh := make(chan *v1alpha1.ApplicationWatchEvent) - err := os.WriteFile("outputfile.txt", []byte("Simulating timeout for seconds"), 0644) - if err != nil { - fmt.Println(err) - } time.Sleep(time.Duration(c.simulateTimeout) * time.Second) go func() { From 09848592809fb5db41d39172569c0b33741c92b0 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 13:30:45 +0200 Subject: [PATCH 16/22] introduce getAppStateWithRetry to ensure retry without refresh after timeout Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 41 +++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 6585244455d28..429aa99896a29 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -400,14 +400,45 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com cancel() }) } + getAppStateWithRetry := func() (*argoappv1.Application, error) { - app, err := appIf.Get(ctx, &application.ApplicationQuery{ - Name: &appName, - Refresh: getRefreshType(refresh, hardRefresh), - AppNamespace: &appNs, - }) + type getResponse struct { + app *argoappv1.Application + err error + } + + ch := make(chan getResponse, 1) + + go func() { + app, err := appIf.Get(ctx, &application.ApplicationQuery{ + Name: &appName, + Refresh: getRefreshType(refresh, hardRefresh), + AppNamespace: &appNs, + }) + ch <- getResponse{app: app, err: err} + }() + + select { + case result := <-ch: + return result.app, result.err + case <-ctx.Done(): + // Timeout occurred, try again without refresh flag + // Create new context for retry request + ctx := context.Background() + app, err := appIf.Get(ctx, &application.ApplicationQuery{ + Name: &appName, + AppNamespace: &appNs, + }) + return app, err + } + } + + app, err := getAppStateWithRetry() errors.CheckError(err) + if ctx.Err() != nil { + ctx = context.Background() // Reset context for subsequent requests + } if sourceName != "" && sourcePosition != -1 { errors.CheckError(stderrors.New("Only one of source-position and source-name can be specified.")) } From 9bd679e3f3830fb15964f4a0559080d977b669ba Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 21:32:11 +0200 Subject: [PATCH 17/22] revert time sleep in app_test Signed-off-by: Almo Elda --- cmd/argocd/commands/app_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/argocd/commands/app_test.go b/cmd/argocd/commands/app_test.go index e89930b9306de..f315869ef595f 100644 --- a/cmd/argocd/commands/app_test.go +++ b/cmd/argocd/commands/app_test.go @@ -2355,8 +2355,6 @@ func (c *fakeAcdClient) NewAccountClientOrDie() (io.Closer, accountpkg.AccountSe func (c *fakeAcdClient) WatchApplicationWithRetry(_ context.Context, _ string, _ string) chan *v1alpha1.ApplicationWatchEvent { appEventsCh := make(chan *v1alpha1.ApplicationWatchEvent) - time.Sleep(time.Duration(c.simulateTimeout) * time.Second) - go func() { modifiedEvent := new(v1alpha1.ApplicationWatchEvent) modifiedEvent.Type = watch.Modified From 6c6977816923b4feeb46c8d6240363e44ddf7f2c Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 21:34:32 +0200 Subject: [PATCH 18/22] introduce lock sync on app in waitOnApplicationStatus Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 105 +++++++++++++++++++++++++++---------- 1 file changed, 77 insertions(+), 28 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 429aa99896a29..6d4b242e9f99c 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -12,6 +12,7 @@ import ( "sort" "strconv" "strings" + "sync" "text/tabwriter" "time" "unicode/utf8" @@ -2544,6 +2545,50 @@ func resourceParentChild(ctx context.Context, acdClient argocdclient.Client, app const waitFormatString = "%s\t%5s\t%10s\t%10s\t%20s\t%8s\t%7s\t%10s\t%s\n" +// AppWithLock encapsulates the application and its lock +type AppWithLock struct { + mu sync.RWMutex + app *argoappv1.Application + refresh bool +} + +// NewAppWithLock creates a new AppWithLock instance +func NewAppWithLock() *AppWithLock { + return &AppWithLock{} +} + +// SetApp safely updates the application +func (a *AppWithLock) SetApp(app *argoappv1.Application) { + a.mu.Lock() + defer a.mu.Unlock() + a.app = app +} + +// GetApp safely retrieves the application +func (a *AppWithLock) GetApp() *argoappv1.Application { + a.mu.RLock() + defer a.mu.RUnlock() + return a.app +} + +// UpdateApp safely updates the application with a modification function +func (a *AppWithLock) UpdateApp(updateFn func(*argoappv1.Application)) { + a.mu.Lock() + defer a.mu.Unlock() + if a.app != nil { + updateFn(a.app) + } +} + +// WithLock executes a function while holding the lock +func (a *AppWithLock) WithLock(fn func(*argoappv1.Application)) { + a.mu.Lock() + defer a.mu.Unlock() + if a.app != nil { + fn(a.app) + } +} + // waitOnApplicationStatus watches an application and blocks until either the desired watch conditions // are fulfilled or we reach the timeout. Returns the app once desired conditions have been filled. // Additionally return the operationState at time of fulfilment (which may be different than returned app). @@ -2551,9 +2596,7 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, ctx, cancel := context.WithCancel(ctx) defer cancel() - // `app` will be used to print the final status of the app - var app *argoappv1.Application - + appWithLock := NewAppWithLock() // refresh controls whether or not we refresh the app before printing the final status. // We only want to do this when an operation is in progress, since operations are the only // time when the sync status lags behind when an operation completes @@ -2622,30 +2665,33 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, if timeout != 0 { time.AfterFunc(time.Duration(timeout)*time.Second, func() { - conn, appClient := acdClient.NewApplicationClientOrDie() - defer conn.Close() - // We want to print the final status of the app even if the conditions are not met - if printSummary { - fmt.Println() - fmt.Println("This is the state of the app after wait timed out:") - } - // Setting refresh = false because we don't want printFinalStatus to execute a refresh - refresh = false - // Updating the app object to the latest state - timedOutAppState, err := appClient.Get(ctx, &application.ApplicationQuery{ - Name: &appRealName, - AppNamespace: &appNs, - }) - errors.CheckError(err) - app = timedOutAppState - // Cancel the context to stop the watch - cancel() + appWithLock.WithLock(func(app *argoappv1.Application) { + conn, appClient := acdClient.NewApplicationClientOrDie() + defer conn.Close() + // We want to print the final status of the app even if the conditions are not met + if printSummary { + fmt.Println() + fmt.Println("This is the state of the app after wait timed out:") + } + // Setting refresh = false because we don't want printFinalStatus to execute a refresh + refresh = false + // Updating the app object to the latest state + timedOutAppState, err := appClient.Get(ctx, &application.ApplicationQuery{ + Name: &appRealName, + AppNamespace: &appNs, + }) + errors.CheckError(err) + app = timedOutAppState + // Cancel the context to stop the watch + cancel() - if printSummary { - fmt.Println() - fmt.Println("The command timed out waiting for the conditions to be met.") - } + if printSummary { + fmt.Println() + fmt.Println("The command timed out waiting for the conditions to be met.") + } + }) }) + } w := tabwriter.NewWriter(os.Stdout, 5, 0, 2, ' ', 0) @@ -2656,11 +2702,13 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, prevStates := make(map[string]*resourceState) conn, appClient := acdClient.NewApplicationClientOrDie() defer argoio.Close(conn) + app, err := appClient.Get(ctx, &application.ApplicationQuery{ Name: &appRealName, AppNamespace: &appNs, }) errors.CheckError(err) + appWithLock.SetApp(app) // Update the app object // printFinalStatus() will refresh and update the app object, potentially causing the app's // status.operationState to be different than the version when we break out of the event loop. @@ -2668,10 +2716,11 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, // finalOperationState captures the operationState as it was seen when we met the conditions of // the wait, so the caller can rely on it to determine the outcome of the operation. // See: https://github.com/argoproj/argo-cd/issues/5592 - finalOperationState := app.Status.OperationState + finalOperationState := appWithLock.GetApp().Status.OperationState - appEventCh := acdClient.WatchApplicationWithRetry(ctx, appName, app.ResourceVersion) + appEventCh := acdClient.WatchApplicationWithRetry(ctx, appName, appWithLock.GetApp().ResourceVersion) for appEvent := range appEventCh { + app = &appEvent.Application finalOperationState = app.Status.OperationState @@ -2743,7 +2792,7 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, } _ = w.Flush() } - _ = printFinalStatus(app) + _ = printFinalStatus(appWithLock.GetApp()) return nil, finalOperationState, fmt.Errorf("timed out (%ds) waiting for app %q match desired state", timeout, appName) } From a44bde2f217d8ed3f904dbb6b3a23d06e274d10a Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 23:24:09 +0200 Subject: [PATCH 19/22] fix lock sync on app in waitOnApplicationStatus Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 73 ++++++++++++++------------------------ 1 file changed, 27 insertions(+), 46 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 6d4b242e9f99c..d2a0cf34064f5 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -2547,9 +2547,8 @@ const waitFormatString = "%s\t%5s\t%10s\t%10s\t%20s\t%8s\t%7s\t%10s\t%s\n" // AppWithLock encapsulates the application and its lock type AppWithLock struct { - mu sync.RWMutex - app *argoappv1.Application - refresh bool + mu sync.RWMutex + app *argoappv1.Application } // NewAppWithLock creates a new AppWithLock instance @@ -2571,24 +2570,6 @@ func (a *AppWithLock) GetApp() *argoappv1.Application { return a.app } -// UpdateApp safely updates the application with a modification function -func (a *AppWithLock) UpdateApp(updateFn func(*argoappv1.Application)) { - a.mu.Lock() - defer a.mu.Unlock() - if a.app != nil { - updateFn(a.app) - } -} - -// WithLock executes a function while holding the lock -func (a *AppWithLock) WithLock(fn func(*argoappv1.Application)) { - a.mu.Lock() - defer a.mu.Unlock() - if a.app != nil { - fn(a.app) - } -} - // waitOnApplicationStatus watches an application and blocks until either the desired watch conditions // are fulfilled or we reach the timeout. Returns the app once desired conditions have been filled. // Additionally return the operationState at time of fulfilment (which may be different than returned app). @@ -2665,31 +2646,30 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, if timeout != 0 { time.AfterFunc(time.Duration(timeout)*time.Second, func() { - appWithLock.WithLock(func(app *argoappv1.Application) { - conn, appClient := acdClient.NewApplicationClientOrDie() - defer conn.Close() - // We want to print the final status of the app even if the conditions are not met - if printSummary { - fmt.Println() - fmt.Println("This is the state of the app after wait timed out:") - } - // Setting refresh = false because we don't want printFinalStatus to execute a refresh - refresh = false - // Updating the app object to the latest state - timedOutAppState, err := appClient.Get(ctx, &application.ApplicationQuery{ - Name: &appRealName, - AppNamespace: &appNs, - }) - errors.CheckError(err) - app = timedOutAppState - // Cancel the context to stop the watch - cancel() - - if printSummary { - fmt.Println() - fmt.Println("The command timed out waiting for the conditions to be met.") - } + conn, appClient := acdClient.NewApplicationClientOrDie() + defer conn.Close() + // We want to print the final status of the app even if the conditions are not met + if printSummary { + fmt.Println() + fmt.Println("This is the state of the app after wait timed out:") + } + // Setting refresh = false because we don't want printFinalStatus to execute a refresh + refresh = false + // Updating the app object to the latest state + timedOutAppState, err := appClient.Get(ctx, &application.ApplicationQuery{ + Name: &appRealName, + AppNamespace: &appNs, }) + errors.CheckError(err) + // Update the app object + appWithLock.SetApp(timedOutAppState) + // Cancel the context to stop the watch + cancel() + + if printSummary { + fmt.Println() + fmt.Println("The command timed out waiting for the conditions to be met.") + } }) } @@ -2721,7 +2701,8 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, appEventCh := acdClient.WatchApplicationWithRetry(ctx, appName, appWithLock.GetApp().ResourceVersion) for appEvent := range appEventCh { - app = &appEvent.Application + appWithLock.SetApp(&appEvent.Application) + app = appWithLock.GetApp() finalOperationState = app.Status.OperationState operationInProgress := false From 49ad06f80b93560228d1ff6c097e845100455ce0 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 23:28:07 +0200 Subject: [PATCH 20/22] change app variable inside waitOnApplicationStatus.AfterFunc Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index d2a0cf34064f5..db96731065f97 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -2656,13 +2656,13 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, // Setting refresh = false because we don't want printFinalStatus to execute a refresh refresh = false // Updating the app object to the latest state - timedOutAppState, err := appClient.Get(ctx, &application.ApplicationQuery{ + app, err := appClient.Get(ctx, &application.ApplicationQuery{ Name: &appRealName, AppNamespace: &appNs, }) errors.CheckError(err) // Update the app object - appWithLock.SetApp(timedOutAppState) + appWithLock.SetApp(app) // Cancel the context to stop the watch cancel() From 13e9e8d9c65004ee99251d9121b43061aef741b5 Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Sun, 2 Feb 2025 23:31:12 +0200 Subject: [PATCH 21/22] remove unnecessary new lines Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index db96731065f97..1a2ec0a4ac989 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -2671,7 +2671,6 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, fmt.Println("The command timed out waiting for the conditions to be met.") } }) - } w := tabwriter.NewWriter(os.Stdout, 5, 0, 2, ' ', 0) @@ -2682,7 +2681,6 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, prevStates := make(map[string]*resourceState) conn, appClient := acdClient.NewApplicationClientOrDie() defer argoio.Close(conn) - app, err := appClient.Get(ctx, &application.ApplicationQuery{ Name: &appRealName, AppNamespace: &appNs, @@ -2700,7 +2698,6 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, appEventCh := acdClient.WatchApplicationWithRetry(ctx, appName, appWithLock.GetApp().ResourceVersion) for appEvent := range appEventCh { - appWithLock.SetApp(&appEvent.Application) app = appWithLock.GetApp() From 5106b195e71c588891383e4372fb281398f278ba Mon Sep 17 00:00:00 2001 From: Almo Elda Date: Mon, 3 Feb 2025 01:06:38 +0200 Subject: [PATCH 22/22] lint fix Signed-off-by: Almo Elda --- cmd/argocd/commands/app.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 1a2ec0a4ac989..1182db941e41b 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -402,7 +402,6 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com }) } getAppStateWithRetry := func() (*argoappv1.Application, error) { - type getResponse struct { app *argoappv1.Application err error