diff --git a/charts/radix-operator/Chart.yaml b/charts/radix-operator/Chart.yaml index b92068e4a..d6612d4d7 100644 --- a/charts/radix-operator/Chart.yaml +++ b/charts/radix-operator/Chart.yaml @@ -1,7 +1,7 @@ apiVersion: v2 name: radix-operator -version: 1.27.6 -appVersion: 1.47.6 +version: 1.28.1 +appVersion: 1.48.1 kubeVersion: ">=1.24.0" description: Radix Operator keywords: diff --git a/charts/radix-operator/templates/radixapplication.yaml b/charts/radix-operator/templates/radixapplication.yaml index 04c7e3646..0a465450a 100644 --- a/charts/radix-operator/templates/radixapplication.yaml +++ b/charts/radix-operator/templates/radixapplication.yaml @@ -922,7 +922,6 @@ spec: - name - port type: object - minItems: 1 type: array x-kubernetes-list-map-keys: - name @@ -1073,7 +1072,6 @@ spec: type: object required: - name - - ports type: object type: array x-kubernetes-list-map-keys: diff --git a/json-schema/radixapplication.json b/json-schema/radixapplication.json index 10630f60f..69ef99cb5 100644 --- a/json-schema/radixapplication.json +++ b/json-schema/radixapplication.json @@ -919,7 +919,6 @@ ], "type": "object" }, - "minItems": 1, "type": "array", "x-kubernetes-list-map-keys": [ "name" @@ -1076,8 +1075,7 @@ } }, "required": [ - "name", - "ports" + "name" ], "type": "object" }, diff --git a/pkg/apis/applicationconfig/applicationconfig.go b/pkg/apis/applicationconfig/applicationconfig.go index 994013d6e..5f8a2573e 100644 --- a/pkg/apis/applicationconfig/applicationconfig.go +++ b/pkg/apis/applicationconfig/applicationconfig.go @@ -2,8 +2,6 @@ package applicationconfig import ( "context" - "encoding/json" - stderrors "errors" "fmt" "reflect" "strings" @@ -15,11 +13,9 @@ import ( "github.com/equinor/radix-operator/pkg/apis/utils" "github.com/equinor/radix-operator/pkg/apis/utils/branch" radixclient "github.com/equinor/radix-operator/pkg/client/clientset/versioned" - radixTypes "github.com/equinor/radix-operator/pkg/client/clientset/versioned/typed/radix/v1" log "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/client-go/kubernetes" ) @@ -126,7 +122,7 @@ func (app *ApplicationConfig) ApplyConfigToApplicationNamespace() error { // It compares the actual state with the desired, and attempts to // converge the two func (app *ApplicationConfig) OnSync() error { - if err := app.createEnvironments(); err != nil { + if err := app.syncEnvironments(); err != nil { log.Errorf("Failed to create namespaces for app environments %s. %v", app.config.Name, err) return err } @@ -144,102 +140,3 @@ func (app *ApplicationConfig) OnSync() error { } return app.syncSubPipelineServiceAccounts() } - -func (app *ApplicationConfig) createEnvironments() error { - var errs []error - for _, env := range app.config.Spec.Environments { - err := app.applyEnvironment(utils.NewEnvironmentBuilder(). - WithAppName(app.config.Name). - WithAppLabel(). - WithEnvironmentName(env.Name). - WithRegistrationOwner(app.registration). - WithEgressConfig(env.Egress). - // Orphaned flag will be set by the environment handler but until - // reconciliation we must ensure it is false - // Update: It seems Update method does not update status object when using real k8s client, but the fake client does. - // Only an explicit call to UpdateStatus can update status object, and this is only done by the RadixEnvironment controller. - WithOrphaned(false). - BuildRE()) - if err != nil { - errs = append(errs, err) - } - } - return stderrors.Join(errs...) -} - -// applyEnvironment creates an environment or applies changes if it exists -func (app *ApplicationConfig) applyEnvironment(newRe *radixv1.RadixEnvironment) error { - logger := log.WithFields(log.Fields{"environment": newRe.ObjectMeta.Name}) - logger.Debugf("Apply environment %s", newRe.Name) - - repository := app.radixclient.RadixV1().RadixEnvironments() - - // Get environment from cache, instead than for cluster - oldRe, err := app.kubeutil.GetEnvironment(newRe.Name) - if err != nil && errors.IsNotFound(err) { - // Environment does not exist yet - - newRe, err = repository.Create(context.TODO(), newRe, metav1.CreateOptions{}) - if err != nil { - return fmt.Errorf("failed to create RadixEnvironment object: %v", err) - } - logger.Debugf("Created RadixEnvironment: %s", newRe.Name) - - } else if err != nil { - return fmt.Errorf("failed to get RadixEnvironment object: %v", err) - - } else { - // Environment already exists - - logger.Debugf("RadixEnvironment object %s already exists, updating the object now", oldRe.Name) - err = patchDifference(repository, oldRe, newRe, logger) - if err != nil { - return err - } - } - return nil -} - -// patchDifference creates a mergepatch, comparing old and new RadixEnvironments and issues the patch to radix -func patchDifference(repository radixTypes.RadixEnvironmentInterface, oldRe *radixv1.RadixEnvironment, newRe *radixv1.RadixEnvironment, logger *log.Entry) error { - radixEnvironment := oldRe.DeepCopy() - radixEnvironment.ObjectMeta.Labels = newRe.ObjectMeta.Labels - radixEnvironment.ObjectMeta.OwnerReferences = newRe.ObjectMeta.OwnerReferences - radixEnvironment.ObjectMeta.Annotations = newRe.ObjectMeta.Annotations - radixEnvironment.ObjectMeta.Finalizers = newRe.ObjectMeta.Finalizers - radixEnvironment.ObjectMeta.DeletionTimestamp = newRe.ObjectMeta.DeletionTimestamp - radixEnvironment.Spec = newRe.Spec - radixEnvironment.Status = newRe.Status - - oldReJSON, err := json.Marshal(oldRe) - if err != nil { - return fmt.Errorf("failed to marshal old RadixEnvironment object: %v", err) - } - - radixEnvironmentJSON, err := json.Marshal(radixEnvironment) - if err != nil { - return fmt.Errorf("failed to marshal new RadixEnvironment object: %v", err) - } - - patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldReJSON, radixEnvironmentJSON, radixv1.RadixEnvironment{}) - if err != nil { - return fmt.Errorf("failed to create patch document for RadixEnvironment object: %v", err) - } - - if !isEmptyPatch(patchBytes) { - // Will perform update as patching does not seem to work for this custom resource - patchedEnvironment, err := repository.Update(context.TODO(), radixEnvironment, metav1.UpdateOptions{}) - if err != nil { - return fmt.Errorf("failed to patch RadixEnvironment object: %v", err) - } - logger.Debugf("Patched RadixEnvironment: %s", patchedEnvironment.Name) - } else { - logger.Debugf("No need to patch RadixEnvironment: %s ", newRe.Name) - } - - return nil -} - -func isEmptyPatch(patchBytes []byte) bool { - return string(patchBytes) == "{}" -} diff --git a/pkg/apis/applicationconfig/applicationconfig_test.go b/pkg/apis/applicationconfig/applicationconfig_test.go index 480dcdbd8..626660da1 100644 --- a/pkg/apis/applicationconfig/applicationconfig_test.go +++ b/pkg/apis/applicationconfig/applicationconfig_test.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes" kubefake "k8s.io/client-go/kubernetes/fake" secretproviderfake "sigs.k8s.io/secrets-store-csi-driver/pkg/client/clientset/versioned/fake" @@ -87,7 +88,8 @@ func Test_Reconciles_Radix_Environments(t *testing.T) { context.TODO(), &radixv1.RadixEnvironment{ ObjectMeta: metav1.ObjectMeta{ - Name: "any-app-qa", + Name: "any-app-qa", + Labels: labels.Set{kube.RadixAppLabel: "any-app"}, }, }, metav1.CreateOptions{}) @@ -97,7 +99,8 @@ func Test_Reconciles_Radix_Environments(t *testing.T) { context.TODO(), &radixv1.RadixEnvironment{ ObjectMeta: metav1.ObjectMeta{ - Name: "any-app-prod", + Name: "any-app-prod", + Labels: labels.Set{kube.RadixAppLabel: "any-app"}, }, }, metav1.CreateOptions{}) diff --git a/pkg/apis/applicationconfig/environment.go b/pkg/apis/applicationconfig/environment.go new file mode 100644 index 000000000..b9a345cf2 --- /dev/null +++ b/pkg/apis/applicationconfig/environment.go @@ -0,0 +1,84 @@ +package applicationconfig + +import ( + "context" + stderrors "errors" + "fmt" + + radixv1 "github.com/equinor/radix-operator/pkg/apis/radix/v1" + "github.com/equinor/radix-operator/pkg/apis/utils" + log "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/retry" +) + +func (app *ApplicationConfig) syncEnvironments() error { + var errs []error + for _, env := range app.config.Spec.Environments { + if err := app.syncEnvironment(app.buildRadixEnvironment(env)); err != nil { + errs = append(errs, err) + } + } + return stderrors.Join(errs...) +} + +func (app *ApplicationConfig) buildRadixEnvironment(env radixv1.Environment) *radixv1.RadixEnvironment { + return utils.NewEnvironmentBuilder(). + WithAppName(app.config.Name). + WithAppLabel(). + WithEnvironmentName(env.Name). + WithRegistrationOwner(app.registration). + WithEgressConfig(env.Egress). + BuildRE() +} + +// syncEnvironment creates an environment or applies changes if it exists +func (app *ApplicationConfig) syncEnvironment(radixEnvironment *radixv1.RadixEnvironment) error { + logger := log.WithFields(log.Fields{"environment": radixEnvironment.GetName()}) + logger.Debugf("Apply RadixEnvironment") + if _, err := app.getRadixEnvironment(radixEnvironment.GetName()); err != nil { + if errors.IsNotFound(err) { + return app.createRadixEnvironment(radixEnvironment, logger) + } + return fmt.Errorf("failed to get RadixEnvironment: %v", err) + } + return app.updateRadixEnvironment(radixEnvironment, logger) +} + +func (app *ApplicationConfig) createRadixEnvironment(radixEnvironment *radixv1.RadixEnvironment, logger *log.Entry) error { + created, err := app.radixclient.RadixV1().RadixEnvironments().Create(context.Background(), radixEnvironment, metav1.CreateOptions{}) + if err != nil { + return fmt.Errorf("failed to create RadixEnvironment: %v", err) + } + logger.Debugf("Created RadixEnvironment (revision %s)", created.GetResourceVersion()) + return nil +} + +// updateRadixEnvironment updates a RadixEnvironment +func (app *ApplicationConfig) updateRadixEnvironment(radixEnvironment *radixv1.RadixEnvironment, logger *log.Entry) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + existingRE, err := app.getRadixEnvironment(radixEnvironment.GetName()) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + return err + } + logger.Debugf("re-taken RadixEnvironment (revision %s)", existingRE.GetResourceVersion()) + + newRE := existingRE.DeepCopy() + newRE.Spec = radixEnvironment.Spec + // Will perform update as patching does not seem to work for this custom resource + updated, err := app.kubeutil.UpdateRadixEnvironment(newRE) + if err != nil { + return err + } + logger.Debugf("Updated RadixEnvironment (revision %s)", updated.GetResourceVersion()) + return nil + }) +} + +func (app *ApplicationConfig) getRadixEnvironment(name string) (*radixv1.RadixEnvironment, error) { + return app.kubeutil.RadixClient().RadixV1().RadixEnvironments().Get(context.Background(), name, metav1.GetOptions{}) +} diff --git a/pkg/apis/deployment/deployment_test.go b/pkg/apis/deployment/deployment_test.go index ff71c5f3f..8adee6c97 100644 --- a/pkg/apis/deployment/deployment_test.go +++ b/pkg/apis/deployment/deployment_test.go @@ -452,7 +452,7 @@ func TestObjectSynced_MultiJob_ContainsAllElements(t *testing.T) { utils.NewDeployJobComponentBuilder(). WithName(jobName). WithImage("job:latest"). - WithPort("http", 3002). + WithPorts([]v1.ComponentPort{{Name: "http", Port: 3002}}). WithEnvironmentVariable("a_variable", "a_value"). WithMonitoring(true). WithResource(map[string]string{ @@ -482,7 +482,7 @@ func TestObjectSynced_MultiJob_ContainsAllElements(t *testing.T) { utils.NewDeployJobComponentBuilder(). WithName(jobName). WithImage("job:latest"). - WithPort("http", 3002). + WithPorts([]v1.ComponentPort{{Name: "http", Port: 3002}}). WithEnvironmentVariable("a_variable", "a_value"). WithMonitoring(true). WithResource(map[string]string{ @@ -510,7 +510,7 @@ func TestObjectSynced_MultiJob_ContainsAllElements(t *testing.T) { WithSecrets([]string{outdatedSecret, remainingSecret}). WithAlwaysPullImageOnDeploy(false), utils.NewDeployJobComponentBuilder(). - WithName(jobName2), + WithName(jobName2).WithSchedulerPort(&schedulerPortCreate), ). WithComponents() @@ -586,7 +586,6 @@ func TestObjectSynced_MultiJob_ContainsAllElements(t *testing.T) { services, _ := kubeclient.CoreV1().Services(envNamespace).List(context.TODO(), metav1.ListOptions{}) expectedServices := getServicesForRadixComponents(&services.Items) var jobNames []string - if jobsExist { jobNames = []string{jobName} assert.Equal(t, 1, len(expectedServices), "Number of services wasn't as expected") @@ -3691,7 +3690,7 @@ func Test_JobScheduler_ObjectsGarbageCollected(t *testing.T) { WithEnvironment("dev"). WithJobComponents(). WithComponents( - utils.NewDeployComponentBuilder().WithName("job"), + utils.NewDeployComponentBuilder().WithName("job").WithPorts([]v1.ComponentPort{{Name: "http", Port: 8000}}), ) testTheory(&theoryData{ @@ -3707,7 +3706,7 @@ func Test_JobScheduler_ObjectsGarbageCollected(t *testing.T) { WithAppName("app"). WithEnvironment("dev"). WithJobComponents( - utils.NewDeployJobComponentBuilder().WithName("job"), + utils.NewDeployJobComponentBuilder().WithName("job").WithPorts([]v1.ComponentPort{{Name: "http", Port: 8000}}), ). WithComponents() @@ -3724,7 +3723,7 @@ func Test_JobScheduler_ObjectsGarbageCollected(t *testing.T) { WithAppName("app"). WithEnvironment("dev"). WithJobComponents( - utils.NewDeployJobComponentBuilder().WithName("compute"), + utils.NewDeployJobComponentBuilder().WithName("compute").WithPorts([]v1.ComponentPort{{Name: "http", Port: 8000}}), ). WithComponents() @@ -3741,8 +3740,8 @@ func Test_JobScheduler_ObjectsGarbageCollected(t *testing.T) { WithAppName("app"). WithEnvironment("prod"). WithJobComponents( - utils.NewDeployJobComponentBuilder().WithName("job"), - utils.NewDeployJobComponentBuilder().WithName("compute"), + utils.NewDeployJobComponentBuilder().WithName("job").WithPorts([]v1.ComponentPort{{Name: "http", Port: 8000}}), + utils.NewDeployJobComponentBuilder().WithName("compute").WithPorts([]v1.ComponentPort{{Name: "http", Port: 8000}}), ). WithComponents() diff --git a/pkg/apis/deployment/radixcomponentname.go b/pkg/apis/deployment/radixcomponentname.go index badaf028d..56d957d61 100644 --- a/pkg/apis/deployment/radixcomponentname.go +++ b/pkg/apis/deployment/radixcomponentname.go @@ -50,6 +50,19 @@ func (t RadixComponentName) GetCommonDeployComponent(rd *v1.RadixDeployment) v1. return nil } +// CommonDeployComponentHasPorts Checks id the deploy component has regular or schedule ports +func (t RadixComponentName) CommonDeployComponentHasPorts(rd *v1.RadixDeployment) bool { + if comp := t.findInDeploymentSpecComponentList(rd); comp != nil { + return len(comp.GetPorts()) > 0 + } + for _, job := range rd.Spec.Jobs { + if strings.EqualFold(job.Name, string(t)) { + return len(job.GetPorts()) > 0 || job.SchedulerPort != nil + } + } + return false +} + // ExistInDeploymentSpecComponentList checks if RadixDeployment has any component with this name func (t RadixComponentName) ExistInDeploymentSpecComponentList(rd *v1.RadixDeployment) bool { return t.findInDeploymentSpecComponentList(rd) != nil diff --git a/pkg/apis/deployment/radixcomponentname_test.go b/pkg/apis/deployment/radixcomponentname_test.go index 8c35f11c2..d51fd28cf 100644 --- a/pkg/apis/deployment/radixcomponentname_test.go +++ b/pkg/apis/deployment/radixcomponentname_test.go @@ -3,7 +3,9 @@ package deployment import ( "testing" + "github.com/equinor/radix-common/utils/pointers" "github.com/equinor/radix-operator/pkg/apis/kube" + v1 "github.com/equinor/radix-operator/pkg/apis/radix/v1" "github.com/equinor/radix-operator/pkg/apis/utils" "github.com/stretchr/testify/assert" appsv1 "k8s.io/api/apps/v1" @@ -58,6 +60,54 @@ func Test_FindCommonComponentInRD(t *testing.T) { } +func Test_CommonDeployComponentHasPorts(t *testing.T) { + rd := utils.ARadixDeployment(). + WithComponents( + utils.NewDeployComponentBuilder().WithName("component1"). + WithPorts([]v1.ComponentPort{{Name: "http", Port: 8080}}), + utils.NewDeployComponentBuilder().WithName("component2")). + WithJobComponents( + utils.NewDeployJobComponentBuilder().WithName("job1"). + WithPorts([]v1.ComponentPort{{Name: "http", Port: 8080}}). + WithSchedulerPort(pointers.Ptr[int32](8081)), + utils.NewDeployJobComponentBuilder().WithName("job2"). + WithPorts([]v1.ComponentPort{{Name: "http", Port: 8080}}), + utils.NewDeployJobComponentBuilder().WithName("job3"). + WithSchedulerPort(pointers.Ptr[int32](8081)), + utils.NewDeployJobComponentBuilder().WithName("job4"), + ). + BuildRD() + + componentName := RadixComponentName("component1") + hasPorts := componentName.CommonDeployComponentHasPorts(rd) + assert.True(t, hasPorts) + + componentName = RadixComponentName("component2") + hasPorts = componentName.CommonDeployComponentHasPorts(rd) + assert.False(t, hasPorts) + + jobName := RadixComponentName("job1") + hasPorts = jobName.CommonDeployComponentHasPorts(rd) + assert.True(t, hasPorts) + + jobName = RadixComponentName("job2") + hasPorts = jobName.CommonDeployComponentHasPorts(rd) + assert.True(t, hasPorts) + + jobName = RadixComponentName("job3") + hasPorts = jobName.CommonDeployComponentHasPorts(rd) + assert.True(t, hasPorts) + + jobName = RadixComponentName("job4") + hasPorts = jobName.CommonDeployComponentHasPorts(rd) + assert.False(t, hasPorts) + + nonExistingName := RadixComponentName("nonexisting") + hasPorts = nonExistingName.CommonDeployComponentHasPorts(rd) + assert.False(t, hasPorts) + +} + func Test_NewRadixComponentNameFromLabels(t *testing.T) { nonRadix := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/apis/deployment/service.go b/pkg/apis/deployment/service.go index 26d2aa566..d99120add 100644 --- a/pkg/apis/deployment/service.go +++ b/pkg/apis/deployment/service.go @@ -12,7 +12,11 @@ import ( func (deploy *Deployment) createOrUpdateService(deployComponent v1.RadixCommonDeployComponent) error { namespace := deploy.radixDeployment.Namespace - service := getServiceConfig(deployComponent, deploy.radixDeployment, deployComponent.GetPorts()) + ports := deployComponent.GetPorts() + if len(ports) == 0 { + return nil + } + service := getServiceConfig(deployComponent, deploy.radixDeployment, ports) return deploy.kubeutil.ApplyService(namespace, service) } @@ -28,8 +32,7 @@ func (deploy *Deployment) garbageCollectServicesNoLongerInSpec() error { continue } if deploy.isEligibleForGarbageCollectServiceForComponent(service, componentName) { - err = deploy.kubeclient.CoreV1().Services(deploy.radixDeployment.GetNamespace()).Delete(context.TODO(), service.Name, metav1.DeleteOptions{}) - if err != nil { + if err := deploy.kubeclient.CoreV1().Services(deploy.radixDeployment.GetNamespace()).Delete(context.TODO(), service.Name, metav1.DeleteOptions{}); err != nil { return err } } @@ -39,12 +42,15 @@ func (deploy *Deployment) garbageCollectServicesNoLongerInSpec() error { } func (deploy *Deployment) isEligibleForGarbageCollectServiceForComponent(service *corev1.Service, componentName RadixComponentName) bool { - // Garbage collect if service is labelled radix-job-type=job-scheduler and not defined in RD jobs if jobType, ok := NewRadixJobTypeFromObjectLabels(service); ok && jobType.IsJobScheduler() { - return !componentName.ExistInDeploymentSpecJobList(deploy.radixDeployment) + if !componentName.ExistInDeploymentSpecJobList(deploy.radixDeployment) { + return true // Garbage collect if service is labelled radix-job-type=job-scheduler and not defined in RD jobs + } + } else if !componentName.ExistInDeploymentSpec(deploy.radixDeployment) { + return true // Garbage collect service if not defined in RD components or jobs } - // Garbage collect service if not defined in RD components or jobs - return !componentName.ExistInDeploymentSpec(deploy.radixDeployment) + + return !componentName.CommonDeployComponentHasPorts(deploy.radixDeployment) } func getServiceConfig(component v1.RadixCommonDeployComponent, radixDeployment *v1.RadixDeployment, componentPorts []v1.ComponentPort) *corev1.Service { diff --git a/pkg/apis/environment/environment.go b/pkg/apis/environment/environment.go index a37c401a8..96e8428e1 100644 --- a/pkg/apis/environment/environment.go +++ b/pkg/apis/environment/environment.go @@ -1,6 +1,7 @@ package environment import ( + "context" "fmt" "github.com/equinor/radix-common/utils/slice" @@ -14,6 +15,7 @@ import ( radixclient "github.com/equinor/radix-operator/pkg/client/clientset/versioned" "github.com/sirupsen/logrus" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes" @@ -103,11 +105,22 @@ func (env *Environment) handleDeletedRadixEnvironment(re *v1.RadixEnvironment) e if err := env.handleDeletedRadixEnvironmentDependencies(re); err != nil { return err } - updatingRE := re.DeepCopy() + updatingRE, err := env.radixclient.RadixV1().RadixEnvironments().Get(context.Background(), re.GetName(), metav1.GetOptions{}) + if err != nil { + if errors.IsNotFound(err) { + return nil + } + return err + } updatingRE.ObjectMeta.Finalizers = append(re.ObjectMeta.Finalizers[:finalizerIndex], re.ObjectMeta.Finalizers[finalizerIndex+1:]...) logrus.Debugf("removed finalizer %s from the Radix environment %s in the application %s. Left finalizers: %d", kube.RadixEnvironmentFinalizer, updatingRE.Name, updatingRE.Spec.AppName, len(updatingRE.ObjectMeta.Finalizers)) - return env.kubeutil.UpdateRadixEnvironment(updatingRE) + updated, err := env.kubeutil.UpdateRadixEnvironment(updatingRE) + if err != nil { + return err + } + logrus.Debugf("updated RadixEnvironment %s revision %s", re.Name, updated.GetResourceVersion()) + return nil } func (env *Environment) handleDeletedRadixEnvironmentDependencies(re *v1.RadixEnvironment) error { diff --git a/pkg/apis/environment/status.go b/pkg/apis/environment/status.go index 728b0507f..d8cbdeb0a 100644 --- a/pkg/apis/environment/status.go +++ b/pkg/apis/environment/status.go @@ -24,17 +24,21 @@ func (env *Environment) syncStatus(re *radixv1.RadixEnvironment, time metav1.Tim func (env *Environment) updateRadixEnvironmentStatus(re *radixv1.RadixEnvironment, changeStatusFunc func(currStatus *radixv1.RadixEnvironmentStatus)) error { radixEnvironmentInterface := env.radixclient.RadixV1().RadixEnvironments() return retry.RetryOnConflict(retry.DefaultRetry, func() error { - currentEnv, err := radixEnvironmentInterface.Get(context.Background(), re.GetName(), metav1.GetOptions{}) + name := re.GetName() + currentEnv, err := radixEnvironmentInterface.Get(context.Background(), name, metav1.GetOptions{}) if err != nil { return err } changeStatusFunc(¤tEnv.Status) - _, err = radixEnvironmentInterface.UpdateStatus(context.Background(), currentEnv, metav1.UpdateOptions{}) - if err == nil && env.config.GetName() == re.GetName() { - currentEnv, err = radixEnvironmentInterface.Get(context.Background(), re.GetName(), metav1.GetOptions{}) - if err == nil { - env.config = currentEnv + updated, err := radixEnvironmentInterface.UpdateStatus(context.Background(), currentEnv, metav1.UpdateOptions{}) + if err == nil && env.config.GetName() == name { + currentEnv, err = radixEnvironmentInterface.Get(context.Background(), name, metav1.GetOptions{}) + if err != nil { + return err } + env.config = currentEnv + env.logger.Debugf("updated status of RadixEnvironment (revision %s)", updated.GetResourceVersion()) + return nil } return err }) diff --git a/pkg/apis/kube/radix_environment.go b/pkg/apis/kube/radix_environment.go index 08b4fb25e..5f0802aaa 100644 --- a/pkg/apis/kube/radix_environment.go +++ b/pkg/apis/kube/radix_environment.go @@ -53,13 +53,13 @@ func (kubeutil *Kube) ListEnvironments() ([]*radixv1.RadixEnvironment, error) { return environments, nil } -// UpdateRadixEnvironment Updates changes of Radix environment if any -func (kubeutil *Kube) UpdateRadixEnvironment(radixEnvironment *radixv1.RadixEnvironment) error { - log.Debugf("Update Radix environment %s in the application %s", radixEnvironment.Name, radixEnvironment.Spec.AppName) - _, err := kubeutil.RadixClient().RadixV1().RadixEnvironments().Update(context.TODO(), radixEnvironment, metav1.UpdateOptions{}) +// UpdateRadixEnvironment Updates changes of RadixEnvironment if any +func (kubeutil *Kube) UpdateRadixEnvironment(radixEnvironment *radixv1.RadixEnvironment) (*radixv1.RadixEnvironment, error) { + log.Debugf("Update RadixEnvironment %s in the application %s", radixEnvironment.Name, radixEnvironment.Spec.AppName) + updated, err := kubeutil.RadixClient().RadixV1().RadixEnvironments().Update(context.TODO(), radixEnvironment, metav1.UpdateOptions{}) if err != nil { - return fmt.Errorf("failed to patch Radix environment object: %v", err) + return nil, fmt.Errorf("failed to update RadixEnvironment object: %v", err) } - log.Debugf("Updated Radix environment: %s in the application %s", radixEnvironment.Name, radixEnvironment.Spec.AppName) - return err + log.Debugf("Updated RadixEnvironment: %s in the application %s", radixEnvironment.Name, radixEnvironment.Spec.AppName) + return updated, nil } diff --git a/pkg/apis/kube/rolebinding.go b/pkg/apis/kube/rolebinding.go index 0dd613e8d..f4da5ce38 100644 --- a/pkg/apis/kube/rolebinding.go +++ b/pkg/apis/kube/rolebinding.go @@ -178,7 +178,7 @@ func (kubeutil *Kube) ApplyRoleBinding(namespace string, role *rbacv1.RoleBindin func (kubeutil *Kube) ApplyClusterRoleBinding(clusterrolebinding *rbacv1.ClusterRoleBinding) error { logger = logger.WithFields(log.Fields{"clusterRoleBinding": clusterrolebinding.ObjectMeta.Name}) logger.Debugf("Apply clusterrolebinding %s", clusterrolebinding.Name) - oldClusterRoleBinding, err := kubeutil.GetClusterRoleBinding(clusterrolebinding.Name) + oldClusterRoleBinding, err := kubeutil.kubeClient.RbacV1().ClusterRoleBindings().Get(context.TODO(), clusterrolebinding.Name, metav1.GetOptions{}) if err != nil && errors.IsNotFound(err) { createdClusterRoleBinding, err := kubeutil.kubeClient.RbacV1().ClusterRoleBindings().Create(context.TODO(), clusterrolebinding, metav1.CreateOptions{}) if err != nil { diff --git a/pkg/apis/kube/roles.go b/pkg/apis/kube/roles.go index 3246bde4e..d56c9572a 100644 --- a/pkg/apis/kube/roles.go +++ b/pkg/apis/kube/roles.go @@ -70,7 +70,7 @@ func (kubeutil *Kube) ApplyRole(namespace string, role *rbacv1.Role) error { // ApplyClusterRole Creates or updates cluster-role func (kubeutil *Kube) ApplyClusterRole(clusterrole *rbacv1.ClusterRole) error { logger.Debugf("Apply clusterrole %s", clusterrole.Name) - oldClusterRole, err := kubeutil.GetClusterRole(clusterrole.GetName()) + oldClusterRole, err := kubeutil.kubeClient.RbacV1().ClusterRoles().Get(context.TODO(), clusterrole.GetName(), metav1.GetOptions{}) if err != nil && errors.IsNotFound(err) { createdClusterRole, err := kubeutil.kubeClient.RbacV1().ClusterRoles().Create(context.TODO(), clusterrole, metav1.CreateOptions{}) if err != nil { diff --git a/pkg/apis/radix/v1/radixapptypes.go b/pkg/apis/radix/v1/radixapptypes.go index 2b72eb1b7..f7ac7da95 100644 --- a/pkg/apis/radix/v1/radixapptypes.go +++ b/pkg/apis/radix/v1/radixapptypes.go @@ -326,9 +326,9 @@ type RadixComponent struct { Image string `json:"image,omitempty"` // List of ports that the component bind to. - // +kubebuilder:validation:MinItems=1 // +listType=map // +listMapKey=name + // +optional Ports []ComponentPort `json:"ports"` // Configures the monitoring endpoint exposed by the component. diff --git a/pkg/apis/radixvalidators/errors.go b/pkg/apis/radixvalidators/errors.go index 0e0da0e7f..924bb7604 100644 --- a/pkg/apis/radixvalidators/errors.go +++ b/pkg/apis/radixvalidators/errors.go @@ -18,7 +18,6 @@ var ( ErrComponentForDNSExternalAliasIsNotMarkedAsPublic = errors.New("component for dns external alias is not marked as public") ErrEnvironmentReferencedByComponentDoesNotExist = errors.New("environment referenced by component does not exist") ErrInvalidPortNameLength = errors.New("invalid port name length") - ErrPortSpecificationCannotBeEmptyForComponent = errors.New("port specification cannot be empty for component") ErrPortNameIsRequiredForPublicComponent = errors.New("port name is required for public component") ErrMonitoringPortNameIsNotFoundComponent = errors.New("monitoring port name is not found component") ErrMultipleMatchingPortNames = errors.New("multiple matching port names") @@ -59,6 +58,7 @@ var ( ErrInvalidConfigBranchName = errors.New("invalid config branch") ErrOauth = errors.New("oauth error") ErrOAuthClientIdEmpty = errors.Wrap(ErrOauth, "oauth client id empty") + ErrOAuthRequiresPublicPort = errors.Wrap(ErrOauth, "oauth requires public port") ErrOAuthProxyPrefixEmpty = errors.Wrap(ErrOauth, "oauth proxy prefix empty") ErrOAuthProxyPrefixIsRoot = errors.Wrap(ErrOauth, "oauth proxy prefix is root") ErrOAuthSessionStoreTypeInvalid = errors.Wrap(ErrOauth, "oauth session store type invalid") @@ -183,11 +183,6 @@ func InvalidPortNameLengthErrorWithMessage(value string) error { return errors.WithMessagef(ErrInvalidPortNameLength, "%s (%s) max length is %d", "port name", value, maxPortNameLength) } -// PortSpecificationCannotBeEmptyForComponentErrorWithMessage Port cannot be empty for component -func PortSpecificationCannotBeEmptyForComponentErrorWithMessage(component string) error { - return errors.WithMessagef(ErrPortSpecificationCannotBeEmptyForComponent, "port specification cannot be empty for %s", component) -} - // PortNameIsRequiredForPublicComponentErrorWithMessage Port name cannot be empty func PortNameIsRequiredForPublicComponentErrorWithMessage(publicPortName, component string) error { return errors.WithMessagef(ErrPortNameIsRequiredForPublicComponent, "%s port name is required for public component %s", publicPortName, component) @@ -405,6 +400,10 @@ func OAuthClientIdEmptyErrorWithMessage(componentName, environmentName string) e return oauthRequiredPropertyEmptyErrorWithMessage(ErrOAuthClientIdEmpty, componentName, environmentName, "clientId") } +func OAuthRequiresPublicPortErrorWithMessage(componentName, environmentName string) error { + return errors.WithMessagef(ErrOAuthRequiresPublicPort, "component %s in environment %s: required public port when oauth2 with clientId configuration is set", componentName, environmentName) +} + func OAuthProxyPrefixEmptyErrorWithMessage(componentName, environmentName string) error { return oauthRequiredPropertyEmptyErrorWithMessage(ErrOAuthProxyPrefixEmpty, componentName, environmentName, "proxyPrefix") } diff --git a/pkg/apis/radixvalidators/validate_ra.go b/pkg/apis/radixvalidators/validate_ra.go index ba443d68e..535477c01 100644 --- a/pkg/apis/radixvalidators/validate_ra.go +++ b/pkg/apis/radixvalidators/validate_ra.go @@ -443,7 +443,7 @@ func validateAuthentication(component *radixv1.RadixComponent, environments []ra errs = append(errs, err) } - errs = append(errs, validateOAuth(combinedAuth.OAuth2, component.GetName(), environment.Name)...) + errs = append(errs, validateOAuth(combinedAuth.OAuth2, component, environment.Name)...) } return errs } @@ -476,7 +476,14 @@ func validateVerificationType(verificationType *radixv1.VerificationType) error } } -func validateOAuth(oauth *radixv1.OAuth2, componentName, environmentName string) (errors []error) { +func componentHasPublicPort(component *radixv1.RadixComponent) bool { + return slice.Any(component.GetPorts(), + func(p radixv1.ComponentPort) bool { + return len(p.Name) > 0 && (p.Name == component.PublicPort || component.Public) + }) +} + +func validateOAuth(oauth *radixv1.OAuth2, component *radixv1.RadixComponent, environmentName string) (errors []error) { if oauth == nil { return } @@ -486,10 +493,12 @@ func validateOAuth(oauth *radixv1.OAuth2, componentName, environmentName string) errors = append(errors, err) return } - + componentName := component.Name // Validate ClientID if len(strings.TrimSpace(oauthWithDefaults.ClientID)) == 0 { errors = append(errors, OAuthClientIdEmptyErrorWithMessage(componentName, environmentName)) + } else if !componentHasPublicPort(component) { + errors = append(errors, OAuthRequiresPublicPortErrorWithMessage(componentName, environmentName)) } // Validate ProxyPrefix @@ -606,11 +615,6 @@ func validateJobPayload(job *radixv1.RadixJobComponent) error { func validatePorts(componentName string, ports []radixv1.ComponentPort) []error { errs := []error{} - if len(ports) == 0 { - err := PortSpecificationCannotBeEmptyForComponentErrorWithMessage(componentName) - errs = append(errs, err) - } - for _, port := range ports { if len(port.Name) > maxPortNameLength { err := InvalidPortNameLengthErrorWithMessage(port.Name) diff --git a/pkg/apis/radixvalidators/validate_ra_test.go b/pkg/apis/radixvalidators/validate_ra_test.go index d3e268b85..9c25a8e0f 100644 --- a/pkg/apis/radixvalidators/validate_ra_test.go +++ b/pkg/apis/radixvalidators/validate_ra_test.go @@ -137,12 +137,6 @@ func Test_invalid_ra(t *testing.T) { {"component name with oauth auxiliary name suffix", radixvalidators.ComponentNameReservedSuffixErrorWithMessage(oauthAuxSuffixComponentName, "component", defaults.OAuthProxyAuxiliaryComponentSuffix), func(ra *v1.RadixApplication) { ra.Spec.Components[0].Name = oauthAuxSuffixComponentName }}, - {"invalid port specification. Nil value", radixvalidators.PortSpecificationCannotBeEmptyForComponentErrorWithMessage(validRAFirstComponentName), func(ra *v1.RadixApplication) { - ra.Spec.Components[0].Ports = nil - }}, - {"invalid port specification. Empty value", radixvalidators.PortSpecificationCannotBeEmptyForComponentErrorWithMessage(validRAFirstComponentName), func(ra *v1.RadixApplication) { - ra.Spec.Components[0].Ports = []v1.ComponentPort{} - }}, {"invalid port name", radixvalidators.InvalidLowerCaseAlphaNumericDashResourceNameErrorWithMessage("port name", invalidResourceName), func(ra *v1.RadixApplication) { ra.Spec.Components[0].Ports[0].Name = invalidResourceName }}, @@ -944,6 +938,8 @@ func Test_PublicPort(t *testing.T) { ra.Spec.Components[0].PublicPort = "" ra.Spec.Components[0].Ports[0].Name = "test" ra.Spec.Components[0].Public = false + ra.Spec.Components[0].Authentication.OAuth2 = nil + ra.Spec.Components[0].EnvironmentConfig[0].Authentication.OAuth2 = nil }, isValid: true, isErrorNil: true, @@ -1037,6 +1033,24 @@ func Test_PublicPort(t *testing.T) { isValid: false, isErrorNil: false, }, + { + name: "oauth2 require public port", + updateRA: func(ra *v1.RadixApplication) { + ra.Spec.Components[0].Ports = []v1.ComponentPort{{Name: "http", Port: 1000}} + ra.Spec.Components[0].PublicPort = "" + }, + isValid: false, + isErrorNil: false, + }, + { + name: "oauth2 require ports", + updateRA: func(ra *v1.RadixApplication) { + ra.Spec.Components[0].Ports = nil + ra.Spec.Components[0].PublicPort = "" + }, + isValid: false, + isErrorNil: false, + }, } _, client := validRASetup() diff --git a/radix-operator/common/controller_test_suite.go b/radix-operator/common/controller_test_suite.go index e2bbcabd5..597c17239 100644 --- a/radix-operator/common/controller_test_suite.go +++ b/radix-operator/common/controller_test_suite.go @@ -48,7 +48,7 @@ func (s *ControllerTestSuite) SetupTest() { s.Handler = NewMockHandler(s.MockCtrl) s.Synced = make(chan bool) s.Stop = make(chan struct{}) - s.TestControllerSyncTimeout = 5 * time.Second + s.TestControllerSyncTimeout = 10 * time.Second } // TearDownTest Tear down the test suite diff --git a/radix-operator/environment/controller.go b/radix-operator/environment/controller.go index 247b7e1ac..f4832323d 100644 --- a/radix-operator/environment/controller.go +++ b/radix-operator/environment/controller.go @@ -76,11 +76,11 @@ func NewController(client kubernetes.Interface, oldRR := old.(*v1.RadixEnvironment) if deepEqual(oldRR, newRR) { - logger.Debugf("Environment object is equal to old for %s. Do nothing", newRR.GetName()) + logger.Debugf("RadixEnvironment %s (revision %s) is equal to old (revision %s). Do nothing", newRR.GetName(), newRR.GetResourceVersion(), oldRR.GetResourceVersion()) metrics.CustomResourceUpdatedButSkipped(crType) return } - + logger.Debugf("update RadixEnvironment %s (from revision %s to %s)", oldRR.GetName(), oldRR.GetResourceVersion(), newRR.GetResourceVersion()) if _, err := controller.Enqueue(cur); err != nil { utilruntime.HandleError(err) } @@ -89,12 +89,12 @@ func NewController(client kubernetes.Interface, DeleteFunc: func(obj interface{}) { radixEnvironment, converted := obj.(*v1.RadixEnvironment) if !converted { - logger.Errorf("RadixEnvironment object cast failed during deleted event received.") + logger.Errorf("RadixEnvironment object cast failed during deleted event received") return } key, err := cache.MetaNamespaceKeyFunc(radixEnvironment) if err == nil { - logger.Debugf("Environment object deleted event received for %s. Do nothing", key) + logger.Debugf("RadixEnvironment object deleted event received for %s (revision %s). Do nothing", key, radixEnvironment.GetResourceVersion()) } metrics.CustomResourceDeleted(crType) },