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.

Sign up for GitHub

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

Jump to bottom

fix: don't change the default policy to reencrypt if the TLS secret is already present #1401

Merged
merged 1 commit into from
Jun 6, 2024
Merged

fix: don't change the default policy to reencrypt if the TLS secret is already present #1401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions controllers/argocd/route.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
})
}
Loading