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: delete/don't create deployment resources if using .remote field #1545

Merged
merged 10 commits into from
Oct 3, 2024
8 changes: 8 additions & 0 deletions api/v1beta1/argocd_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,10 @@ func (a *ArgoCDRedisSpec) IsEnabled() bool {
return a.Enabled == nil || (a.Enabled != nil && *a.Enabled)
}

func (a *ArgoCDRedisSpec) IsRemote() bool {
return a.Remote != nil && *a.Remote != ""
}

// ArgoCDRepoSpec defines the desired state for the Argo CD repo server component.
type ArgoCDRepoSpec struct {

Expand Down Expand Up @@ -535,6 +539,10 @@ func (a *ArgoCDRepoSpec) IsEnabled() bool {
return a.Enabled == nil || (a.Enabled != nil && *a.Enabled)
}

func (a *ArgoCDRepoSpec) IsRemote() bool {
return a.Remote != nil && *a.Remote != ""
}

// ArgoCDRouteSpec defines the desired state for an OpenShift Route.
type ArgoCDRouteSpec struct {
// Annotations is the map of annotations to use for the Route resource.
Expand Down
15 changes: 13 additions & 2 deletions controllers/argocd/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ func getDexServerAddress(cr *argoproj.ArgoCD) string {

// getRepoServerAddress will return the Argo CD repo server address.
func getRepoServerAddress(cr *argoproj.ArgoCD) string {
if cr.Spec.Repo.Remote != nil && *cr.Spec.Repo.Remote != "" {
if cr.Spec.Repo.IsRemote() {
return *cr.Spec.Repo.Remote
}
return fqdnServiceRef("repo-server", common.ArgoCDDefaultRepoServerPort, cr)
Expand Down Expand Up @@ -533,6 +533,9 @@ func (r *ReconcileArgoCD) reconcileRedisDeployment(cr *argoproj.ArgoCD, useTLS b
// Deployment exists but component enabled flag has been set to false, delete the Deployment
log.Info("Redis exists but should be disabled. Deleting existing redis.")
return r.Client.Delete(context.TODO(), deploy)
} else if cr.Spec.Redis.IsRemote() {
log.Info("Redis remote exists but redis deployment should be disabled. Deleting existing redis.")
return r.Client.Delete(context.TODO(), deploy)
}
if cr.Spec.HA.Enabled {
// Deployment exists but HA enabled flag has been set to true, delete the Deployment
Expand Down Expand Up @@ -575,7 +578,7 @@ func (r *ReconcileArgoCD) reconcileRedisDeployment(cr *argoproj.ArgoCD, useTLS b
return nil // Deployment found with nothing to do, move along...
}

if cr.Spec.Redis.IsEnabled() && cr.Spec.Redis.Remote != nil && *cr.Spec.Redis.Remote != "" {
if cr.Spec.Redis.IsEnabled() && cr.Spec.Redis.IsRemote() {
svghadi marked this conversation as resolved.
Show resolved Hide resolved
log.Info("Custom Redis Endpoint. Skipping starting redis.")
return nil
}
Expand Down Expand Up @@ -1113,6 +1116,9 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor
log.Info("Existing ArgoCD Repo Server found but should be disabled. Deleting Repo Server")
// Delete existing deployment for ArgoCD Repo Server, if any ..
return r.Client.Delete(context.TODO(), existing)
} else if cr.Spec.Repo.IsRemote() {
log.Info("Repo Server remote field exists, Repo Server deployment should be disabled. Deleting Repo Server.")
return r.Client.Delete(context.TODO(), deploy)
}

changed := false
Expand Down Expand Up @@ -1187,6 +1193,11 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor
return nil // Deployment found with nothing to do, move along...
}

if cr.Spec.Redis.IsEnabled() && cr.Spec.Repo.IsRemote() {
svghadi marked this conversation as resolved.
Show resolved Hide resolved
log.Info("Custom Repo Endpoint. Skipping starting Repo Server.")
return nil
}

if !cr.Spec.Repo.IsEnabled() {
log.Info("ArgoCD Repo Server disabled. Skipping starting ArgoCD Repo Server.")
return nil
Expand Down
55 changes: 55 additions & 0 deletions controllers/argocd/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2291,3 +2291,58 @@ func TestReconcileArgoCD_reconcileRepoDeployment_serviceAccount(t *testing.T) {
})
}
}

// If `remote` field is used in CR, then the component resources should not be created
func TestReconcileArgoCD_reconcileRedisWithRemote(t *testing.T) {
cr := makeTestArgoCD()

resObjs := []client.Object{cr}
subresObjs := []client.Object{cr}
runtimeObjs := []runtime.Object{}
sch := makeTestReconcilerScheme(argoproj.AddToScheme)
cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
r := makeTestReconciler(cl, sch)

redisRemote := "https://remote.redis.instance"

cr.Spec.Redis.Remote = &redisRemote
assert.NoError(t, r.reconcileRedisDeployment(cr, false))

d := &appsv1.Deployment{}

assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-redis", Namespace: cr.Namespace}, d),
"deployments.apps \""+cr.Name+"-redis\" not found")

// once remote is set to nil, reconciliation should trigger deployment resource creation
cr.Spec.Redis.Remote = nil

assert.NoError(t, r.reconcileRedisDeployment(cr, false))
assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-redis", Namespace: cr.Namespace}, d))
}

func TestReconcileArgoCD_reconcileRepoServerWithRemote(t *testing.T) {
cr := makeTestArgoCD()

resObjs := []client.Object{cr}
subresObjs := []client.Object{cr}
runtimeObjs := []runtime.Object{}
sch := makeTestReconcilerScheme(argoproj.AddToScheme)
cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
r := makeTestReconciler(cl, sch)

repoServerRemote := "https://remote.repo-server.instance"

cr.Spec.Repo.Remote = &repoServerRemote
assert.NoError(t, r.reconcileRepoDeployment(cr, false))

d := &appsv1.Deployment{}

assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-repo-server", Namespace: cr.Namespace}, d),
"deployments.apps \""+cr.Name+"-repo-server\" not found")

// once remote is set to nil, reconciliation should trigger deployment resource creation
cr.Spec.Repo.Remote = nil

assert.NoError(t, r.reconcileRepoDeployment(cr, false))
assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-repo-server", Namespace: cr.Namespace}, d))
}
17 changes: 17 additions & 0 deletions controllers/argocd/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,9 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
if cr.Spec.HA.Enabled {
return r.Client.Delete(context.TODO(), svc)
}
if cr.Spec.Redis.IsRemote() {
return r.Client.Delete(context.TODO(), svc)
}
return nil // Service found, do nothing
}

Expand All @@ -298,6 +301,11 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
},
}

if cr.Spec.Redis.IsEnabled() && cr.Spec.Redis.IsRemote() {
svghadi marked this conversation as resolved.
Show resolved Hide resolved
log.Info("Skipping service creation, redis remote is enabled")
return nil
}

if err := controllerutil.SetControllerReference(cr, svc, r.Scheme); err != nil {
return err
}
Expand Down Expand Up @@ -361,6 +369,10 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS()) {
return r.Client.Update(context.TODO(), svc)
}
if cr.Spec.Repo.IsRemote() {
log.Info("skip creating repo server service, repo remote is enabled")
return r.Client.Delete(context.TODO(), svc)
}
return nil // Service found, do nothing
}

Expand Down Expand Up @@ -388,6 +400,11 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
},
}

if cr.Spec.Repo.IsEnabled() && cr.Spec.Repo.IsRemote() {
svghadi marked this conversation as resolved.
Show resolved Hide resolved
log.Info("skip creating repo server service, repo remote is enabled")
return nil
}

if err := controllerutil.SetControllerReference(cr, svc, r.Scheme); err != nil {
return err
}
Expand Down
49 changes: 47 additions & 2 deletions controllers/argocd/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import (
"context"
"testing"

"k8s.io/apimachinery/pkg/api/errors"

"k8s.io/apimachinery/pkg/types"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

argoproj "github.com/argoproj-labs/argocd-operator/api/v1beta1"
Expand Down Expand Up @@ -131,3 +133,46 @@ func TestReconcileServerService(t *testing.T) {
assert.Equal(t, a.Spec.Server.Service.Type, serverService.Spec.Type)
})
}

// If `remote` field is used in CR, then the component resources should not be created
func TestReconcileArgoCD_reconcileRedisWithRemoteEn(t *testing.T) {
cr := makeTestArgoCD()

resObjs := []client.Object{cr}
subresObjs := []client.Object{cr}
runtimeObjs := []runtime.Object{}
sch := makeTestReconcilerScheme(argoproj.AddToScheme)
cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
r := makeTestReconciler(cl, sch)

redisRemote := "https://remote.redis.instance"

cr.Spec.Redis.Remote = &redisRemote
assert.NoError(t, r.reconcileRedisService(cr))

s := &corev1.Service{}

assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-redis", Namespace: cr.Namespace}, s),
"services \"argocd-redis\" not found")
}

func TestReconcileArgoCD_reconcileRepoServerWithRemoteEnabled(t *testing.T) {
cr := makeTestArgoCD()

resObjs := []client.Object{cr}
subresObjs := []client.Object{cr}
runtimeObjs := []runtime.Object{}
sch := makeTestReconcilerScheme(argoproj.AddToScheme)
cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
r := makeTestReconciler(cl, sch)

repoServerRemote := "https://remote.repo-server.instance"

cr.Spec.Repo.Remote = &repoServerRemote
assert.NoError(t, r.reconcileRepoService(cr))

s := &corev1.Service{}

assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: cr.Name + "-repo-server", Namespace: cr.Namespace}, s),
"services \"argocd-repo-server\" not found")
}
1 change: 1 addition & 0 deletions docs/reference/argocd.md
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,7 @@ DisableTLSVerification | false | defines whether the redis server should be acce
Image | `redis` | The container image for Redis. This overrides the `ARGOCD_REDIS_IMAGE` environment variable.
Resources | [Empty] | The container compute resources.
Version | 5.0.3 (SHA) | The tag to use with the Redis container image.
Remote | "" | Specifies the remote URL of redis running in external clusters, also disables Redis component. This field is optional.

### Redis Example

Expand Down
7 changes: 2 additions & 5 deletions docs/usage/enabling-disabling-argocd-core-components.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ In this example, only the controller component is disabled, while all other comp

# Specifying External URLs for Redis and RepoServer Components
When disabling core components like Redis or Repo Server, you may wish to provide an external URL for components running in external clusters. The remote URL can be set using the `spec.<component>.remote` flag (where the component can only be `redis` or `repo`).

Using `spec.<component>.remote` will skip the deployment & service creation.
For example,

```yaml
Expand All @@ -41,8 +41,5 @@ metadata:
name: example
spec:
repo:
enabled: false
remote: 'https://www.example.com/repo-server'
```

!!! Note: The remote flag can only be set if the enabled flag of the component is set to false.
```
50 changes: 50 additions & 0 deletions tests/k8s/1-043_validate_cleanup_when_using_remote/01-assert.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Increase the timeout for the first test because it needs to download
# a number of container images
apiVersion: kuttl.dev/v1beta1
kind: TestAssert
timeout: 720
---
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
name: example-argocd
status:
phase: Available
---
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: example-argocd-application-controller
status:
readyReplicas: 1
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: example-argocd-redis
status:
readyReplicas: 1
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: example-argocd-repo-server
status:
readyReplicas: 1
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: example-argocd-server
status:
readyReplicas: 1
---
apiVersion: v1
kind: Service
metadata:
name: example-argocd-repo-server
---
apiVersion: v1
kind: Service
metadata:
name: example-argocd-redis
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
name: example-argocd
labels:
example: basic
spec: {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
name: example-argocd
labels:
example: basic
spec:
redis:
remote: https://redis.remote.host:6379
repo:
remote: https://repo-server.remote.host:8081
30 changes: 30 additions & 0 deletions tests/k8s/1-043_validate_cleanup_when_using_remote/02-error.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Increase the timeout for the first test because it needs to download
# a number of container images
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: sleep 30
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: example-argocd-redis
status:
readyReplicas: 1
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: example-argocd-repo-server
status:
readyReplicas: 1
---
apiVersion: v1
kind: Service
metadata:
name: example-argocd-repo-server
---
apiVersion: v1
kind: Service
metadata:
name: example-argocd-redis
Loading
Loading