Skip to content

Commit b7e6666

Browse files
chetan-rnschetan-rns
authored
fix: don't change the default policy to reencrypt if the TLS secret is present (#1401)
Signed-off-by: Chetan Banavikalmutt <[email protected]>
1 parent cdf3832 commit b7e6666

File tree

4 files changed

+101
-21
lines changed

4 files changed

+101
-21
lines changed

‎controllers/argocd/route.go

+15-4
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"strings"
2121

2222
routev1 "github.com/openshift/api/route/v1"
23+
corev1 "k8s.io/api/core/v1"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2425
"k8s.io/apimachinery/pkg/util/intstr"
2526
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -231,13 +232,23 @@ func (r *ReconcileArgoCD) reconcileServerRoute(cr *argoproj.ArgoCD) error {
231232
Termination: routev1.TLSTerminationEdge,
232233
}
233234
} else {
234-
// Server is using TLS configure reencrypt.
235235
route.Spec.Port = &routev1.RoutePort{
236236
TargetPort: intstr.FromString("https"),
237237
}
238-
route.Spec.TLS = &routev1.TLSConfig{
239-
InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
240-
Termination: routev1.TLSTerminationReencrypt,
238+
239+
isTLSSecretFound := argoutil.IsObjectFound(r.Client, cr.Namespace, common.ArgoCDServerTLSSecretName, &corev1.Secret{})
240+
// Since Passthrough was the default policy in the previous versions of the operator, we don't want to
241+
// break users who have already configured a TLS secret for Passthrough.
242+
if cr.Spec.Server.Route.TLS == nil && isTLSSecretFound && route.Spec.TLS != nil && route.Spec.TLS.Termination == routev1.TLSTerminationPassthrough {
243+
route.Spec.TLS = &routev1.TLSConfig{
244+
InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
245+
Termination: routev1.TLSTerminationPassthrough,
246+
}
247+
} else {
248+
route.Spec.TLS = &routev1.TLSConfig{
249+
InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
250+
Termination: routev1.TLSTerminationReencrypt,
251+
}
241252
}
242253
}
243254

‎controllers/argocd/route_test.go

+34-3
Original file line numberDiff line numberDiff line change
@@ -497,16 +497,18 @@ func TestReconcileRouteTLSConfig(t *testing.T) {
497497
logf.SetLogger(ZapLogger(true))
498498

499499
tt := []struct {
500-
name string
501-
want routev1.TLSTerminationType
502-
updateArgoCD func(cr *argoproj.ArgoCD)
500+
name string
501+
want routev1.TLSTerminationType
502+
updateArgoCD func(cr *argoproj.ArgoCD)
503+
createResources func(k8sClient client.Client, cr *argoproj.ArgoCD)
503504
}{
504505
{
505506
name: "should set the default termination policy to renencrypt",
506507
want: routev1.TLSTerminationReencrypt,
507508
updateArgoCD: func(cr *argoproj.ArgoCD) {
508509
cr.Spec.Server.Route.Enabled = true
509510
},
511+
createResources: func(k8sClient client.Client, cr *argoproj.ArgoCD) {},
510512
},
511513
{
512514
name: "shouldn't overwrite the TLS config if it's already configured",
@@ -517,6 +519,34 @@ func TestReconcileRouteTLSConfig(t *testing.T) {
517519
Termination: routev1.TLSTerminationEdge,
518520
}
519521
},
522+
createResources: func(k8sClient client.Client, cr *argoproj.ArgoCD) {},
523+
},
524+
{
525+
// We don't want to change the default value to reencrypt if the user has already
526+
// configured a TLS secret for passthrough (previous default value).
527+
name: "shouldn't overwrite if the Route was previously configured with passthrough",
528+
want: routev1.TLSTerminationPassthrough,
529+
updateArgoCD: func(cr *argoproj.ArgoCD) {
530+
cr.Spec.Server.Route.Enabled = true
531+
},
532+
createResources: func(k8sClient client.Client, cr *argoproj.ArgoCD) {
533+
secret := &corev1.Secret{
534+
ObjectMeta: metav1.ObjectMeta{
535+
Name: common.ArgoCDServerTLSSecretName,
536+
Namespace: cr.Namespace,
537+
},
538+
}
539+
err := k8sClient.Create(context.Background(), secret)
540+
assert.NoError(t, err)
541+
542+
// create a Route with passthrough policy.
543+
route := newRouteWithSuffix("server", cr)
544+
route.Spec.TLS = &routev1.TLSConfig{
545+
Termination: routev1.TLSTerminationPassthrough,
546+
}
547+
err = k8sClient.Create(context.Background(), route)
548+
assert.NoError(t, err)
549+
},
520550
},
521551
}
522552

@@ -531,6 +561,7 @@ func TestReconcileRouteTLSConfig(t *testing.T) {
531561
fakeClient := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
532562
reconciler := makeTestReconciler(fakeClient, sch)
533563

564+
test.createResources(fakeClient, argoCD)
534565
req := reconcile.Request{
535566
NamespacedName: testNamespacedName(testArgoCDName),
536567
}

‎controllers/argocd/service.go

+16-9
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
corev1 "k8s.io/api/core/v1"
2222
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2323
"k8s.io/apimachinery/pkg/util/intstr"
24+
"sigs.k8s.io/controller-runtime/pkg/client"
2425
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
2526

2627
argoproj "github.com/argoproj-labs/argocd-operator/api/v1beta1"
@@ -210,7 +211,7 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyService(cr *argoproj.ArgoCD) erro
210211
return r.Client.Delete(context.TODO(), svc)
211212
}
212213

213-
if ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
214+
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
214215
return r.Client.Update(context.TODO(), svc)
215216
}
216217
return nil // Service found, do nothing
@@ -220,7 +221,7 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyService(cr *argoproj.ArgoCD) erro
220221
return nil //return as Ha is not enabled do nothing
221222
}
222223

223-
ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())
224+
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())
224225

225226
svc.Spec.Selector = map[string]string{
226227
common.ArgoCDKeyName: nameWithSuffix("redis-ha-haproxy", cr),
@@ -266,7 +267,7 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
266267
if !cr.Spec.Redis.IsEnabled() {
267268
return r.Client.Delete(context.TODO(), svc)
268269
}
269-
if ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
270+
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
270271
return r.Client.Update(context.TODO(), svc)
271272
}
272273
if cr.Spec.HA.Enabled {
@@ -279,7 +280,7 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
279280
return nil //return as Ha is enabled do nothing
280281
}
281282

282-
ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())
283+
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())
283284

284285
svc.Spec.Selector = map[string]string{
285286
common.ArgoCDKeyName: nameWithSuffix("redis", cr),
@@ -308,7 +309,7 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
308309
//
309310
// When this method returns true, the svc resource will need to be updated on
310311
// the cluster.
311-
func ensureAutoTLSAnnotation(svc *corev1.Service, secretName string, enabled bool) bool {
312+
func ensureAutoTLSAnnotation(k8sClient client.Client, svc *corev1.Service, secretName string, enabled bool) bool {
312313
var autoTLSAnnotationName, autoTLSAnnotationValue string
313314

314315
// We currently only support OpenShift for automatic TLS
@@ -323,6 +324,12 @@ func ensureAutoTLSAnnotation(svc *corev1.Service, secretName string, enabled boo
323324
if autoTLSAnnotationName != "" {
324325
val, ok := svc.Annotations[autoTLSAnnotationName]
325326
if enabled {
327+
// Don't request a TLS certificate from the OpenShift Service CA if the secret already exists.
328+
isTLSSecretFound := argoutil.IsObjectFound(k8sClient, svc.Namespace, secretName, &corev1.Secret{})
329+
if !ok && isTLSSecretFound {
330+
log.Info(fmt.Sprintf("skipping AutoTLS on service %s since the TLS secret is already present", svc.Name))
331+
return false
332+
}
326333
if !ok || val != secretName {
327334
log.Info(fmt.Sprintf("requesting AutoTLS on service %s", svc.ObjectMeta.Name))
328335
svc.Annotations[autoTLSAnnotationName] = autoTLSAnnotationValue
@@ -348,7 +355,7 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
348355
if !cr.Spec.Repo.IsEnabled() {
349356
return r.Client.Delete(context.TODO(), svc)
350357
}
351-
if ensureAutoTLSAnnotation(svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS()) {
358+
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS()) {
352359
return r.Client.Update(context.TODO(), svc)
353360
}
354361
return nil // Service found, do nothing
@@ -358,7 +365,7 @@ func (r *ReconcileArgoCD) reconcileRepoService(cr *argoproj.ArgoCD) error {
358365
return nil
359366
}
360367

361-
ensureAutoTLSAnnotation(svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS())
368+
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS())
362369

363370
svc.Spec.Selector = map[string]string{
364371
common.ArgoCDKeyName: nameWithSuffix("repo-server", cr),
@@ -417,7 +424,7 @@ func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
417424
if !cr.Spec.Server.IsEnabled() {
418425
return r.Client.Delete(context.TODO(), svc)
419426
}
420-
if ensureAutoTLSAnnotation(svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
427+
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
421428
return r.Client.Update(context.TODO(), svc)
422429
}
423430
return nil // Service found, do nothing
@@ -427,7 +434,7 @@ func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
427434
return nil
428435
}
429436

430-
ensureAutoTLSAnnotation(svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS())
437+
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS())
431438

432439
svc.Spec.Ports = []corev1.ServicePort{
433440
{

‎controllers/argocd/service_test.go

+36-5
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,39 @@
11
package argocd
22

33
import (
4+
"context"
45
"testing"
56

67
"github.com/stretchr/testify/assert"
8+
corev1 "k8s.io/api/core/v1"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"k8s.io/apimachinery/pkg/runtime"
11+
"sigs.k8s.io/controller-runtime/pkg/client"
712

13+
argoproj "github.com/argoproj-labs/argocd-operator/api/v1beta1"
814
"github.com/argoproj-labs/argocd-operator/common"
915
)
1016

1117
func TestEnsureAutoTLSAnnotation(t *testing.T) {
1218
a := makeTestArgoCD()
19+
resObjs := []client.Object{a}
20+
subresObjs := []client.Object{a}
21+
runtimeObjs := []runtime.Object{}
22+
sch := makeTestReconcilerScheme(argoproj.AddToScheme)
23+
fakeClient := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
1324
t.Run("Ensure annotation will be set for OpenShift", func(t *testing.T) {
1425
routeAPIFound = true
1526
svc := newService(a)
1627

1728
// Annotation is inserted, update is required
18-
needUpdate := ensureAutoTLSAnnotation(svc, "some-secret", true)
29+
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", true)
1930
assert.Equal(t, needUpdate, true)
2031
atls, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
2132
assert.Equal(t, ok, true)
2233
assert.Equal(t, atls, "some-secret")
2334

2435
// Annotation already set, doesn't need update
25-
needUpdate = ensureAutoTLSAnnotation(svc, "some-secret", true)
36+
needUpdate = ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", true)
2637
assert.Equal(t, needUpdate, false)
2738
})
2839
t.Run("Ensure annotation will be unset for OpenShift", func(t *testing.T) {
@@ -32,21 +43,41 @@ func TestEnsureAutoTLSAnnotation(t *testing.T) {
3243
svc.Annotations[common.AnnotationOpenShiftServiceCA] = "some-secret"
3344

3445
// Annotation getting removed, update required
35-
needUpdate := ensureAutoTLSAnnotation(svc, "some-secret", false)
46+
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", false)
3647
assert.Equal(t, needUpdate, true)
3748
_, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
3849
assert.Equal(t, ok, false)
3950

4051
// Annotation does not exist, no update required
41-
needUpdate = ensureAutoTLSAnnotation(svc, "some-secret", false)
52+
needUpdate = ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", false)
4253
assert.Equal(t, needUpdate, false)
4354
})
4455
t.Run("Ensure annotation will not be set for non-OpenShift", func(t *testing.T) {
4556
routeAPIFound = false
4657
svc := newService(a)
47-
needUpdate := ensureAutoTLSAnnotation(svc, "some-secret", true)
58+
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", true)
4859
assert.Equal(t, needUpdate, false)
4960
_, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
5061
assert.Equal(t, ok, false)
5162
})
63+
t.Run("Ensure annotation will not be set if the TLS secret is already present", func(t *testing.T) {
64+
routeAPIFound = true
65+
svc := newService(a)
66+
secret := &corev1.Secret{
67+
ObjectMeta: metav1.ObjectMeta{
68+
Name: "some-secret",
69+
Namespace: svc.Namespace,
70+
},
71+
}
72+
err := fakeClient.Create(context.Background(), secret)
73+
assert.NoError(t, err)
74+
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, secret.Name, true)
75+
assert.Equal(t, needUpdate, false)
76+
_, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
77+
assert.Equal(t, ok, false)
78+
79+
// Annotation does not exist, no update required
80+
needUpdate = ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", false)
81+
assert.Equal(t, needUpdate, false)
82+
})
5283
}

0 commit comments

Comments
 (0)