Merge pull request #1144 from tkan145/THREESCALE-12329

THREESCALE-12329 Fix access token leaked in audit log in upgraded instances

Author: tkan145

pkg/3scale/amp/component/system.go

@@ -315,8 +315,6 @@ func (system *System) buildAppEnv() []v1.EnvVar {
 
 func (system *System) buildAppMasterContainerEnv() []v1.EnvVar {
 	result := system.buildSystemBaseEnv()
-	result = append(result, helper.EnvVarFromValue("PORT", "3002"))
-	result = append(result, helper.EnvVarFromValue("TENANT_MODE", "master"))
 	if system.Options.AppMetrics {
 		result = append(result, helper.EnvVarFromValue(SystemAppPrometheusExporterPortEnvVarName, strconv.Itoa(SystemAppMasterContainerPrometheusPort)))
 	}
@@ -327,8 +325,6 @@ func (system *System) buildAppMasterContainerEnv() []v1.EnvVar {
 
 func (system *System) buildAppProviderContainerEnv() []v1.EnvVar {
 	result := system.buildSystemBaseEnv()
-	result = append(result, helper.EnvVarFromValue("PORT", "3000"))
-	result = append(result, helper.EnvVarFromValue("TENANT_MODE", "provider"))
 	if system.Options.AppMetrics {
 		result = append(result, helper.EnvVarFromValue(SystemAppPrometheusExporterPortEnvVarName, strconv.Itoa(SystemAppProviderContainerPrometheusPort)))
 	}
@@ -339,7 +335,6 @@ func (system *System) buildAppProviderContainerEnv() []v1.EnvVar {
 
 func (system *System) buildAppDeveloperContainerEnv() []v1.EnvVar {
 	result := system.buildSystemBaseEnv()
-	result = append(result, helper.EnvVarFromValue("PORT", "3001"))
 	if system.Options.AppMetrics {
 		result = append(result, helper.EnvVarFromValue(SystemAppPrometheusExporterPortEnvVarName, strconv.Itoa(SystemAppDeveloperContainerPrometheusPort)))
 	}
@@ -732,6 +727,7 @@ func (system *System) AppDeployment(ctx context.Context, k8sclient client.Client
 						{
 							Name:         SystemAppMasterContainerName,
 							Image:        containerImage,
+							Args:         []string{"env", "TENANT_MODE=master", "PORT=3002", "container-entrypoint", "bundle", "exec", "unicorn", "-c", "config/unicorn.rb", "-E", "production", "config.ru"},
 							Ports:        system.appMasterPorts(),
 							Env:          system.buildAppMasterContainerEnv(),
 							Resources:    *system.Options.AppMasterContainerResourceRequirements,
@@ -782,6 +778,7 @@ func (system *System) AppDeployment(ctx context.Context, k8sclient client.Client
 						{
 							Name:         SystemAppProviderContainerName,
 							Image:        containerImage,
+							Args:         []string{"env", "TENANT_MODE=provider", "PORT=3000", "container-entrypoint", "bundle", "exec", "unicorn", "-c", "config/unicorn.rb", "-E", "production", "config.ru"},
 							Ports:        system.appProviderPorts(),
 							Env:          system.buildAppProviderContainerEnv(),
 							Resources:    *system.Options.AppProviderContainerResourceRequirements,
@@ -832,6 +829,7 @@ func (system *System) AppDeployment(ctx context.Context, k8sclient client.Client
 						{
 							Name:         SystemAppDeveloperContainerName,
 							Image:        containerImage,
+							Args:         []string{"env", "PORT=3001", "container-entrypoint", "bundle", "exec", "unicorn", "-c", "config/unicorn.rb", "-E", "production", "config.ru"},
 							Ports:        system.appDeveloperPorts(),
 							Env:          system.buildAppDeveloperContainerEnv(),
 							Resources:    *system.Options.AppDeveloperContainerResourceRequirements,

pkg/3scale/amp/operator/helper_test.go

@@ -1,14 +1,22 @@
 package operator
 
 import (
+	"context"
 	"fmt"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	fakeclientset "k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/tools/record"
+	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1"
 	"github.com/3scale/3scale-operator/pkg/3scale/amp/component"
 	"github.com/3scale/3scale-operator/pkg/helper"
+	"github.com/3scale/3scale-operator/pkg/reconcilers"
 	"github.com/3scale/3scale-operator/version"
 )
 
@@ -126,3 +134,23 @@ func getTestTolerations(prefix string) []v1.Toleration {
 		},
 	}
 }
+
+// setupTestReconciler creates a SystemReconciler with the provided objects for testing
+func setupTestBaseReconciler(s *runtime.Scheme, apimanager *appsv1alpha1.APIManager, objs []runtime.Object) (*BaseAPIManagerLogicReconciler, k8sclient.Client) {
+	cl := fake.NewClientBuilder().
+		WithScheme(s).
+		WithRuntimeObjects(objs...).
+		Build()
+	clientAPIReader := fake.NewClientBuilder().
+		WithScheme(s).
+		WithRuntimeObjects(objs...).
+		Build()
+	clientset := fakeclientset.NewSimpleClientset()
+	recorder := record.NewFakeRecorder(10000)
+	log := logf.Log.WithName("operator_test")
+
+	baseReconciler := reconcilers.NewBaseReconciler(context.Background(), cl, s, clientAPIReader, log, clientset.Discovery(), recorder)
+	baseAPIManagerLogicReconciler := NewBaseAPIManagerLogicReconciler(baseReconciler, apimanager)
+
+	return baseAPIManagerLogicReconciler, cl
+}

pkg/3scale/amp/operator/system_reconciler.go

@@ -218,6 +218,7 @@ func (r *SystemReconciler) Reconcile() (reconcile.Result, error) {
 			r.systemAppDeploymentResourceMutator,
 			reconcilers.DeploymentPodInitContainerMutator,
 			reconcilers.DeploymentRemoveDuplicateEnvVarMutator,
+			reconcilers.DeploymentArgsMutator,
 			reconcilers.DeploymentPodContainerImageMutator,
 			r.systemZyncEnvVarMutator,
 			r.systemDatabaseTLSEnvVarMutator,

pkg/3scale/amp/operator/system_reconciler_test.go

@@ -2,108 +2,44 @@ package operator
 
 import (
 	"context"
+	"reflect"
 	"testing"
 
 	appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1"
 	"github.com/3scale/3scale-operator/pkg/3scale/amp/component"
-	"github.com/3scale/3scale-operator/pkg/reconcilers"
+	"github.com/google/go-cmp/cmp"
+	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 
-	grafanav1alpha1 "github.com/grafana-operator/grafana-operator/v4/api/integreatly/v1alpha1"
-	configv1 "github.com/openshift/api/config/v1"
-	imagev1 "github.com/openshift/api/image/v1"
-	routev1 "github.com/openshift/api/route/v1"
-	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
 	k8sappsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
 	v1 "k8s.io/api/core/v1"
 	policyv1 "k8s.io/api/policy/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	fakeclientset "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/client-go/tools/record"
-	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
 func TestSystemReconcilerCreate(t *testing.T) {
-	log := logf.Log.WithName("operator_test")
-
-	ctx := context.TODO()
 	apimanager := basicApimanagerSpecTestSystemOptions()
-	appPreHookJob := &batchv1.Job{
-		ObjectMeta: metav1.ObjectMeta{Name: component.SystemAppPreHookJobName, Namespace: apimanager.Namespace},
-		Spec: batchv1.JobSpec{
-			Template: v1.PodTemplateSpec{
-				Spec: v1.PodSpec{
-					Containers: []v1.Container{
-						{
-							Image: SystemImageURL(),
-						},
-					},
-				},
-			},
-		},
-		Status: batchv1.JobStatus{
-			Conditions: []batchv1.JobCondition{
-				{
-					Type:   batchv1.JobComplete,
-					Status: v1.ConditionTrue,
-				},
-			},
-		},
-	}
 	systemDatabaseSecret := createSystemDBSecret(apimanager.Namespace)
 	systemRedisSecret := createSystemRedisSecret(apimanager.Namespace)
+	appPreHookJob := createAppPreHookJob(apimanager.Namespace)
 
 	// Objects to track in the fake client.
 	objs := []runtime.Object{appPreHookJob, apimanager, systemDatabaseSecret, systemRedisSecret}
 
 	s := scheme.Scheme
-	s.AddKnownTypes(appsv1alpha1.GroupVersion, &appsv1alpha1.APIManager{})
-	s.AddKnownTypes(v1.SchemeGroupVersion, &v1.Secret{}, &v1.SecretList{})
-	err := v1.AddToScheme(s)
+	err := appsv1alpha1.AddToScheme(s)
 	if err != nil {
 		t.Fatal(err)
 	}
 	err = k8sappsv1.AddToScheme(s)
 	if err != nil {
 		t.Fatal(err)
 	}
-	err = imagev1.Install(s)
-	if err != nil {
-		t.Fatal(err)
-	}
-	err = routev1.Install(s)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if err := monitoringv1.AddToScheme(s); err != nil {
-		t.Fatal(err)
-	}
-	if err := grafanav1alpha1.AddToScheme(s); err != nil {
-		t.Fatal(err)
-	}
-	if err := configv1.Install(s); err != nil {
-		t.Fatal(err)
-	}
 
-	// Create a fake client to mock API calls.
-	cl := fake.NewClientBuilder().
-		WithScheme(s).
-		WithRuntimeObjects(objs...).
-		Build()
-	clientAPIReader := fake.NewClientBuilder().
-		WithScheme(s).
-		WithRuntimeObjects(objs...).
-		Build()
-	clientset := fakeclientset.NewSimpleClientset()
-	recorder := record.NewFakeRecorder(10000)
-
-	baseReconciler := reconcilers.NewBaseReconciler(ctx, cl, s, clientAPIReader, log, clientset.Discovery(), recorder)
-	baseAPIManagerLogicReconciler := NewBaseAPIManagerLogicReconciler(baseReconciler, apimanager)
+	baseAPIManagerLogicReconciler, cl := setupTestBaseReconciler(s, apimanager, objs)
 
 	reconciler := NewSystemReconciler(baseAPIManagerLogicReconciler)
 	_, err = reconciler.Reconcile()
@@ -157,36 +93,13 @@ func TestSystemReconcilerCreate(t *testing.T) {
 func TestReplicaSystemReconciler(t *testing.T) {
 	var (
 		namespace        = "operator-unittest"
-		log              = logf.Log.WithName("operator_test")
 		oneValue   int32 = 1
 		oneValue64 int64 = 1
 		twoValue   int32 = 2
 	)
 
-	appPreHookJob := &batchv1.Job{
-		ObjectMeta: metav1.ObjectMeta{Name: component.SystemAppPreHookJobName, Namespace: namespace},
-		Spec: batchv1.JobSpec{
-			Template: v1.PodTemplateSpec{
-				Spec: v1.PodSpec{
-					Containers: []v1.Container{
-						{
-							Image: SystemImageURL(),
-						},
-					},
-				},
-			},
-		},
-		Status: batchv1.JobStatus{
-			Conditions: []batchv1.JobCondition{
-				{
-					Type:   batchv1.JobComplete,
-					Status: v1.ConditionTrue,
-				},
-			},
-		},
-	}
+	appPreHookJob := createAppPreHookJob(namespace)
 
-	ctx := context.TODO()
 	s := scheme.Scheme
 
 	err := appsv1alpha1.AddToScheme(s)
@@ -197,9 +110,6 @@ func TestReplicaSystemReconciler(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := configv1.Install(s); err != nil {
-		t.Fatal(err)
-	}
 
 	cases := []struct {
 		testName                 string
@@ -220,13 +130,7 @@ func TestReplicaSystemReconciler(t *testing.T) {
 			systemRedisSecret := createSystemRedisSecret(tc.apimanager.Namespace)
 			objs := []runtime.Object{tc.apimanager, appPreHookJob, systemDatabaseSecret, systemRedisSecret}
 
-			// Create a fake client to mock API calls.
-			cl := fake.NewFakeClient(objs...)
-			clientAPIReader := fake.NewFakeClient(objs...)
-			clientset := fakeclientset.NewSimpleClientset()
-			recorder := record.NewFakeRecorder(10000)
-			baseReconciler := reconcilers.NewBaseReconciler(ctx, cl, s, clientAPIReader, log, clientset.Discovery(), recorder)
-			baseAPIManagerLogicReconciler := NewBaseAPIManagerLogicReconciler(baseReconciler, tc.apimanager)
+			baseAPIManagerLogicReconciler, cl := setupTestBaseReconciler(s, tc.apimanager, objs)
 
 			reconciler := NewSystemReconciler(baseAPIManagerLogicReconciler)
 			_, err = reconciler.Reconcile()
@@ -271,6 +175,86 @@ func TestReplicaSystemReconciler(t *testing.T) {
 	}
 }
 
+func TestSystemReconcilerUpdate(t *testing.T) {
+	apimanager := basicApimanagerSpecTestSystemOptions()
+	appPreHookJob := createAppPreHookJob(namespace)
+
+	s := scheme.Scheme
+
+	err := appsv1alpha1.AddToScheme(s)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = k8sappsv1.AddToScheme(s)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	systemDatabaseSecret := createSystemDBSecret(apimanager.Namespace)
+	systemRedisSecret := createSystemRedisSecret(apimanager.Namespace)
+	objs := []runtime.Object{apimanager, appPreHookJob, systemDatabaseSecret, systemRedisSecret}
+
+	baseAPIManagerLogicReconciler, cl := setupTestBaseReconciler(s, apimanager, objs)
+	reconciler := NewSystemReconciler(baseAPIManagerLogicReconciler)
+
+	system, err := System(reconciler.apiManager, reconciler.Client())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	oldImage := "old-system-image:v1"
+	oldArgs := []string{"foo", "bar"}
+
+	oldDeployment, err := system.AppDeployment(context.Background(), cl, oldImage)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	oldDeployment.Spec.Template.Spec.Containers[0].Args = oldArgs
+
+	// Create an system-app Deployment with old image and args
+	err = cl.Create(context.TODO(), oldDeployment)
+	if err != nil {
+		t.Errorf("error creating deployment of %s: %v", "system-app", err)
+	}
+
+	_, err = reconciler.Reconcile()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	reconciledDeployment := &k8sappsv1.Deployment{}
+	namespacedName := types.NamespacedName{
+		Name:      "system-app",
+		Namespace: namespace,
+	}
+
+	err = cl.Get(context.TODO(), namespacedName, reconciledDeployment)
+	if err != nil {
+		t.Errorf("error fetching object %s: %v", "system-app", err)
+	}
+
+	expectedDeployment, err := system.AppDeployment(context.Background(), cl, SystemImageURL())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Verify image was updated
+	actualImage := reconciledDeployment.Spec.Template.Spec.Containers[0].Image
+	expectedImage := expectedDeployment.Spec.Template.Spec.Containers[0].Image
+	if actualImage != expectedImage {
+		t.Errorf("image was not updated: expected %s, got %s", expectedImage, actualImage)
+	}
+
+	// Verify args were updated
+	actualArgs := reconciledDeployment.Spec.Template.Spec.Containers[0].Args
+	expectedArgs := expectedDeployment.Spec.Template.Spec.Containers[0].Args
+	if !reflect.DeepEqual(actualArgs, expectedArgs) {
+		t.Errorf("args were not updated:\nexpected: %v\ngot: %v\ndiff: %s",
+			expectedArgs, actualArgs, cmp.Diff(expectedArgs, actualArgs))
+	}
+}
+
 func testSystemAPIManagerCreator(appReplicas, sidekiqReplicas *int64) *appsv1alpha1.APIManager {
 	var (
 		name                  = "example-apimanager"
@@ -329,3 +313,28 @@ func createSystemDBSecret(namespace string) *v1.Secret {
 		Data: map[string][]byte{},
 	}
 }
+
+func createAppPreHookJob(namespace string) *batchv1.Job {
+	return &batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{Name: component.SystemAppPreHookJobName, Namespace: namespace},
+		Spec: batchv1.JobSpec{
+			Template: v1.PodTemplateSpec{
+				Spec: v1.PodSpec{
+					Containers: []v1.Container{
+						{
+							Image: SystemImageURL(),
+						},
+					},
+				},
+			},
+		},
+		Status: batchv1.JobStatus{
+			Conditions: []batchv1.JobCondition{
+				{
+					Type:   batchv1.JobComplete,
+					Status: v1.ConditionTrue,
+				},
+			},
+		},
+	}
+}