Skip to content

Commit

Permalink
fix: don't change the default policy to reencrypt if the TLS secret i…
Browse files Browse the repository at this point in the history 
…s present (argoproj-labs#1401)

Signed-off-by: Chetan Banavikalmutt <chetanrns1997@gmail.com>
Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>
  • Loading branch information
@chetan-rns @anandf
chetan-rns authored and anandf committed Jun 6, 2024
1 parent cdf3832 commit c4fe28f
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 21 deletions.
19 changes: 15 additions & 4 deletions controllers/argocd/route.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"strings"

routev1 "github.com/openshift/api/route/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -231,13 +232,23 @@ func (r *ReconcileArgoCD) reconcileServerRoute(cr *argoproj.ArgoCD) error {
Termination: routev1.TLSTerminationEdge,
}
} else {
// Server is using TLS configure reencrypt.
route.Spec.Port = &routev1.RoutePort{
TargetPort: intstr.FromString("https"),
}
route.Spec.TLS = &routev1.TLSConfig{
InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
Termination: routev1.TLSTerminationReencrypt,

isTLSSecretFound := argoutil.IsObjectFound(r.Client, cr.Namespace, common.ArgoCDServerTLSSecretName, &corev1.Secret{})
// Since Passthrough was the default policy in the previous versions of the operator, we don't want to
// break users who have already configured a TLS secret for Passthrough.
if cr.Spec.Server.Route.TLS == nil && isTLSSecretFound && route.Spec.TLS != nil && route.Spec.TLS.Termination == routev1.TLSTerminationPassthrough {
route.Spec.TLS = &routev1.TLSConfig{
InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
Termination: routev1.TLSTerminationPassthrough,
}
} else {
route.Spec.TLS = &routev1.TLSConfig{
InsecureEdgeTerminationPolicy: routev1.InsecureEdgeTerminationPolicyRedirect,
Termination: routev1.TLSTerminationReencrypt,
}
}
}

Expand Down
37 changes: 34 additions & 3 deletions controllers/argocd/route_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,16 +497,18 @@ func TestReconcileRouteTLSConfig(t *testing.T) {
logf.SetLogger(ZapLogger(true))

tt := []struct {
name string
want routev1.TLSTerminationType
updateArgoCD func(cr *argoproj.ArgoCD)
name string
want routev1.TLSTerminationType
updateArgoCD func(cr *argoproj.ArgoCD)
createResources func(k8sClient client.Client, cr *argoproj.ArgoCD)
}{
{
name: "should set the default termination policy to renencrypt",
want: routev1.TLSTerminationReencrypt,
updateArgoCD: func(cr *argoproj.ArgoCD) {
cr.Spec.Server.Route.Enabled = true
},
createResources: func(k8sClient client.Client, cr *argoproj.ArgoCD) {},
},
{
name: "shouldn't overwrite the TLS config if it's already configured",
Expand All @@ -517,6 +519,34 @@ func TestReconcileRouteTLSConfig(t *testing.T) {
Termination: routev1.TLSTerminationEdge,
}
},
createResources: func(k8sClient client.Client, cr *argoproj.ArgoCD) {},
},
{
// We don't want to change the default value to reencrypt if the user has already
// configured a TLS secret for passthrough (previous default value).
name: "shouldn't overwrite if the Route was previously configured with passthrough",
want: routev1.TLSTerminationPassthrough,
updateArgoCD: func(cr *argoproj.ArgoCD) {
cr.Spec.Server.Route.Enabled = true
},
createResources: func(k8sClient client.Client, cr *argoproj.ArgoCD) {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: common.ArgoCDServerTLSSecretName,
Namespace: cr.Namespace,
},
}
err := k8sClient.Create(context.Background(), secret)
assert.NoError(t, err)

// create a Route with passthrough policy.
route := newRouteWithSuffix("server", cr)
route.Spec.TLS = &routev1.TLSConfig{
Termination: routev1.TLSTerminationPassthrough,
}
err = k8sClient.Create(context.Background(), route)
assert.NoError(t, err)
},
},
}

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

test.createResources(fakeClient, argoCD)
req := reconcile.Request{
NamespacedName: testNamespacedName(testArgoCDName),
}
Expand Down
25 changes: 16 additions & 9 deletions controllers/argocd/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

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

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

ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())

svc.Spec.Selector = map[string]string{
common.ArgoCDKeyName: nameWithSuffix("redis-ha-haproxy", cr),
Expand Down Expand Up @@ -266,7 +267,7 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
if !cr.Spec.Redis.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}
if ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS()) {
return r.Client.Update(context.TODO(), svc)
}
if cr.Spec.HA.Enabled {
Expand All @@ -279,7 +280,7 @@ func (r *ReconcileArgoCD) reconcileRedisService(cr *argoproj.ArgoCD) error {
return nil //return as Ha is enabled do nothing
}

ensureAutoTLSAnnotation(svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRedisServerTLSSecretName, cr.Spec.Redis.WantsAutoTLS())

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

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

ensureAutoTLSAnnotation(svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS())
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDRepoServerTLSSecretName, cr.Spec.Repo.WantsAutoTLS())

svc.Spec.Selector = map[string]string{
common.ArgoCDKeyName: nameWithSuffix("repo-server", cr),
Expand Down Expand Up @@ -417,7 +424,7 @@ func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
if !cr.Spec.Server.IsEnabled() {
return r.Client.Delete(context.TODO(), svc)
}
if ensureAutoTLSAnnotation(svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
if ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS()) {
return r.Client.Update(context.TODO(), svc)
}
return nil // Service found, do nothing
Expand All @@ -427,7 +434,7 @@ func (r *ReconcileArgoCD) reconcileServerService(cr *argoproj.ArgoCD) error {
return nil
}

ensureAutoTLSAnnotation(svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS())
ensureAutoTLSAnnotation(r.Client, svc, common.ArgoCDServerTLSSecretName, cr.Spec.Server.WantsAutoTLS())

svc.Spec.Ports = []corev1.ServicePort{
{
Expand Down
41 changes: 36 additions & 5 deletions controllers/argocd/service_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,28 +1,39 @@
package argocd

import (
"context"
"testing"

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

argoproj "github.com/argoproj-labs/argocd-operator/api/v1beta1"
"github.com/argoproj-labs/argocd-operator/common"
)

func TestEnsureAutoTLSAnnotation(t *testing.T) {
a := makeTestArgoCD()
resObjs := []client.Object{a}
subresObjs := []client.Object{a}
runtimeObjs := []runtime.Object{}
sch := makeTestReconcilerScheme(argoproj.AddToScheme)
fakeClient := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
t.Run("Ensure annotation will be set for OpenShift", func(t *testing.T) {
routeAPIFound = true
svc := newService(a)

// Annotation is inserted, update is required
needUpdate := ensureAutoTLSAnnotation(svc, "some-secret", true)
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", true)
assert.Equal(t, needUpdate, true)
atls, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
assert.Equal(t, ok, true)
assert.Equal(t, atls, "some-secret")

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

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

// Annotation does not exist, no update required
needUpdate = ensureAutoTLSAnnotation(svc, "some-secret", false)
needUpdate = ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", false)
assert.Equal(t, needUpdate, false)
})
t.Run("Ensure annotation will not be set for non-OpenShift", func(t *testing.T) {
routeAPIFound = false
svc := newService(a)
needUpdate := ensureAutoTLSAnnotation(svc, "some-secret", true)
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", true)
assert.Equal(t, needUpdate, false)
_, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
assert.Equal(t, ok, false)
})
t.Run("Ensure annotation will not be set if the TLS secret is already present", func(t *testing.T) {
routeAPIFound = true
svc := newService(a)
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "some-secret",
Namespace: svc.Namespace,
},
}
err := fakeClient.Create(context.Background(), secret)
assert.NoError(t, err)
needUpdate := ensureAutoTLSAnnotation(fakeClient, svc, secret.Name, true)
assert.Equal(t, needUpdate, false)
_, ok := svc.Annotations[common.AnnotationOpenShiftServiceCA]
assert.Equal(t, ok, false)

// Annotation does not exist, no update required
needUpdate = ensureAutoTLSAnnotation(fakeClient, svc, "some-secret", false)
assert.Equal(t, needUpdate, false)
})
}

0 comments on commit c4fe28f

Please sign in to comment.