diff --git a/internal/handler/operation.go b/internal/handler/operation.go index b1772b0..ea8fb80 100644 --- a/internal/handler/operation.go +++ b/internal/handler/operation.go @@ -3,6 +3,7 @@ package handler import ( "context" "fmt" + "slices" "time" "github.com/Azure/operation-cache-controller/internal/utils/reconciler" @@ -57,13 +58,7 @@ func NewOperationHandler(ctx context.Context, operation *v1alpha1.Operation, log } func (o *OperationHandler) phaseIn(phases ...string) bool { - - for _, phase := range phases { - if phase == o.operation.Status.Phase { - return true - } - } - return false + return slices.Contains(phases, o.operation.Status.Phase) } func (o *OperationHandler) EnsureNotExpired(ctx context.Context) (reconciler.OperationResult, error) { diff --git a/internal/handler/operation_test.go b/internal/handler/operation_test.go index a95c00c..562629c 100644 --- a/internal/handler/operation_test.go +++ b/internal/handler/operation_test.go @@ -42,6 +42,9 @@ var ( }, }, }, + Status: v1alpha1.OperationStatus{ + OperationID: "test-operation", + }, } emptyAppDeploymentList = &v1alpha1.AppDeploymentList{} @@ -49,7 +52,7 @@ var ( Items: []v1alpha1.AppDeployment{ { ObjectMeta: metav1.ObjectMeta{ - Name: "test-app1", + Name: "test-operation-test-app1", Namespace: "default", }, Spec: v1alpha1.AppDeploymentSpec{ @@ -64,7 +67,7 @@ var ( }, { ObjectMeta: metav1.ObjectMeta{ - Name: "test-app2", + Name: "test-operation-test-app2", Namespace: "default", }, Spec: v1alpha1.AppDeploymentSpec{ @@ -83,7 +86,7 @@ var ( Items: []v1alpha1.AppDeployment{ { ObjectMeta: metav1.ObjectMeta{ - Name: "test-app2", + Name: "test-operation-test-app2", Namespace: "default", }, Spec: v1alpha1.AppDeploymentSpec{ @@ -115,7 +118,7 @@ var ( }, { ObjectMeta: metav1.ObjectMeta{ - Name: "test-app3", + Name: "test-operation-test-app3", Namespace: "default", }, Spec: v1alpha1.AppDeploymentSpec{ @@ -277,14 +280,14 @@ func TestOperationHandler_EnsureAllAppsAreReady(t *testing.T) { operation.Status.Phase = v1alpha1.OperationPhaseReconciling appList := emptyAppDeploymentList.DeepCopy() - mockClient.EXPECT().List(ctx, appList, gomock.Any()).DoAndReturn(func(ctx context.Context, list *v1alpha1.AppDeploymentList, opts ...interface{}) error { + mockClient.EXPECT().List(ctx, appList, gomock.Any()).DoAndReturn(func(ctx context.Context, list *v1alpha1.AppDeploymentList, opts ...any) error { *list = *changedValidAppDeploymentList return nil }) scheme := runtime.NewScheme() utilruntime.Must(v1alpha1.AddToScheme(scheme)) mockClient.EXPECT().Scheme().Return(scheme).AnyTimes() - mockClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj runtime.Object, opt ...interface{}) error { + mockClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj runtime.Object, opt ...any) error { *obj.(*v1alpha1.AppDeployment) = v1alpha1.AppDeployment{} return nil }).AnyTimes() diff --git a/internal/handler/requirement.go b/internal/handler/requirement.go index a8ef720..aa2ca91 100644 --- a/internal/handler/requirement.go +++ b/internal/handler/requirement.go @@ -286,7 +286,7 @@ func (r *RequirementHandler) EnsureOperationReady(ctx context.Context) (reconcil } if r.rqutils.IsCacheMissed(r.requirement) { r.logger.V(1).Info("cache missed, creating operation") - r.requirement.Status.OperationName = r.requirement.Name + "-" + "operation" + r.requirement.Status.OperationName = r.requirement.Name // reset operation name to requirement name } // check operation status if op, err := r.getOperation(); err == nil { diff --git a/internal/utils/controller/appdeployment_job.go b/internal/utils/controller/appdeployment_job.go index 2569053..284ff66 100644 --- a/internal/utils/controller/appdeployment_job.go +++ b/internal/utils/controller/appdeployment_job.go @@ -13,23 +13,20 @@ import ( const ( JobTypeProvision = "provision" JobTypeTeardown = "teardown" + JobNamePattern = "%s-%s-%s" // jobType-appName-operationId ) var ( backOffLimit int32 = 10 ttlSecondsAfterFinished int32 = 3600 MaxAppNameLength int = 36 - MaxOpIdLength int = 18 ) -func validJName(appName, operationId, jobType string) string { - if len(appName) > MaxAppNameLength { - appName = appName[:MaxAppNameLength] - } - if len(operationId) > MaxOpIdLength { - operationId = operationId[:MaxOpIdLength] +func validJobName(appName, jobType string) string { + originName := jobType + "-" + appName + if len(originName) > MaxResourceNameLength { + return originName[:MaxResourceNameLength] } - originName := jobType + "-" + appName + "-" + operationId return originName } @@ -42,16 +39,16 @@ func TeardownJobFromAppDeploymentSpec(appDeployment *v1alpha1.AppDeployment) *ba } func GetProvisionJobName(appDeployment *v1alpha1.AppDeployment) string { - return validJName(appDeployment.Name, appDeployment.Spec.OpId, JobTypeProvision) + return validJobName(appDeployment.Name, JobTypeProvision) } func GetTeardownJobName(appDeployment *v1alpha1.AppDeployment) string { - return validJName(appDeployment.Name, appDeployment.Spec.OpId, JobTypeTeardown) + return validJobName(appDeployment.Name, JobTypeTeardown) } func jobFromAppDeploymentSpec(appDeployment *v1alpha1.AppDeployment, suffix string) *batchv1.Job { ops := jobOptions{ - name: validJName(appDeployment.Name, appDeployment.Spec.OpId, suffix), + name: validJobName(appDeployment.Name, suffix), namespace: appDeployment.Namespace, labels: appDeployment.Labels, jobSpec: appDeployment.Spec.Provision, @@ -117,14 +114,16 @@ func CheckJobStatus(ctx context.Context, job *batchv1.Job) JobStatus { } func OperationScopedAppDeployment(appName, opId string) string { + originalName := opId + "-" + appName + if len(originalName) < MaxResourceNameLength { + return originalName + } if len(appName) > MaxAppNameLength { appName = appName[:MaxAppNameLength] } - if len(opId) > MaxOpIdLength { - opId = opId[:MaxOpIdLength] - } - if opId == "" { - return appName + resisualLength := MaxResourceNameLength - len(appName) - 1 // -1 for the hyphen + if len(opId) > resisualLength { + opId = opId[:resisualLength] } return opId + "-" + appName } diff --git a/internal/utils/controller/appdeployment_job_test.go b/internal/utils/controller/appdeployment_job_test.go index db5b162..50a8125 100644 --- a/internal/utils/controller/appdeployment_job_test.go +++ b/internal/utils/controller/appdeployment_job_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "strings" "testing" "github.com/Azure/operation-cache-controller/api/v1alpha1" @@ -94,21 +95,18 @@ func TestGetProvisionJobName(t *testing.T) { }{ { name: "Valid app name and operation ID", - appName: "my-app", - opId: "op123", - expected: "provision-my-app-op123", + appName: "op123-my-app", + expected: "provision-op123-my-app", }, { name: "App name exceeds max length", - appName: "a-very-long-application-name-exceeding-limit", - opId: "op123", - expected: "provision-a-very-long-application-name-exceedi-op123", + appName: "op1234567890-a-very-long-application-name-exceeding-limit", + expected: "provision-op1234567890-a-very-long-application-name-exceeding-l", }, { name: "Operation ID exceeds max length", - appName: "my-app", - opId: "operation-id-exceeding-length", - expected: "provision-my-app-operation-id-excee", + appName: "operationid1234567890123456789012345678901234567890-my-application", + expected: "provision-operationid1234567890123456789012345678901234567890-m", }, } @@ -118,9 +116,6 @@ func TestGetProvisionJobName(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: tt.appName, }, - Spec: v1alpha1.AppDeploymentSpec{ - OpId: tt.opId, - }, } res := GetProvisionJobName(appDeployment) assert.Equal(t, tt.expected, res) @@ -138,20 +133,17 @@ func TestGetTeardownJobName(t *testing.T) { { name: "Valid app name and operation ID", appName: "my-app", - opId: "op123", - expected: "teardown-my-app-op123", + expected: "teardown-my-app", }, { name: "App name exceeds max length", - appName: "a-very-long-application-name-exceeding-limit", - opId: "op123", - expected: "teardown-a-very-long-application-name-exceedi-op123", + appName: "op1234567890-a-very-long-application-name-exceeding-limit", + expected: "teardown-op1234567890-a-very-long-application-name-exceeding-li", }, { name: "Operation ID exceeds max length", - appName: "my-app", - opId: "operation-id-exceeding-length", - expected: "teardown-my-app-operation-id-excee", + appName: "operationid1234567890123456789012345678901234567890-my-application", + expected: "teardown-operationid1234567890123456789012345678901234567890-my", }, } @@ -161,9 +153,6 @@ func TestGetTeardownJobName(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: tt.appName, }, - Spec: v1alpha1.AppDeploymentSpec{ - OpId: tt.opId, - }, } res := GetTeardownJobName(appDeployment) assert.Equal(t, tt.expected, res) @@ -172,59 +161,91 @@ func TestGetTeardownJobName(t *testing.T) { } func TestOperationScopedAppDeployment(t *testing.T) { tests := []struct { - name string - appName string - opId string - expected string + name string + appName string + opId string + want string }{ { - name: "Both appName and opId within limits", - appName: "my-app", - opId: "op123", - expected: "op123-my-app", + name: "normal case - combined length within limit", + appName: "my-app", + opId: "op-12345", + want: "op-12345-my-app", }, { - name: "appName exceeds max length", - appName: "a-very-long-application-name-exceeding-limit", - opId: "op123", - expected: "op123-a-very-long-application-name-exceedi", + name: "app name truncated when too long", + appName: strings.Repeat("a", 60), // 60 chars, exceeds MaxAppNameLength + opId: "op-12345", + want: "op-12345-" + strings.Repeat("a", MaxAppNameLength), }, { - name: "opId exceeds max length", - appName: "my-app", - opId: "operation-id-exceeding-length", - expected: "operation-id-excee-my-app", + name: "operation ID truncated when combined length exceeds limit", + appName: strings.Repeat("a", MaxAppNameLength), + opId: strings.Repeat("b", 30), // This will exceed when combined + want: strings.Repeat("b", MaxResourceNameLength-MaxAppNameLength-1) + "-" + strings.Repeat("a", MaxAppNameLength), }, { - name: "Both appName and opId exceed max length", - appName: "another-very-long-application-name-exceeding-limit", - opId: "another-operation-id-exceeding-length", - expected: "another-operation--another-very-long-application-name-e", + name: "both truncated for very long inputs", + appName: strings.Repeat("a", 100), + opId: strings.Repeat("b", 100), + want: strings.Repeat("b", MaxResourceNameLength-MaxAppNameLength-1) + "-" + strings.Repeat("a", MaxAppNameLength), }, { - name: "Empty appName and opId", - appName: "", - opId: "", - expected: "", + name: "empty app name", + appName: "", + opId: "op-12345", + want: "op-12345-", }, { - name: "Empty appName", - appName: "", - opId: "op123", - expected: "op123-", + name: "empty operation ID", + appName: "my-app", + opId: "", + want: "-my-app", }, { - name: "Empty opId", - appName: "my-app", - opId: "", - expected: "my-app", + name: "both empty", + appName: "", + opId: "", + want: "-", + }, + { + name: "exact max length", + appName: "app", + opId: strings.Repeat("x", MaxResourceNameLength-4), // -4 for "-app" + want: strings.Repeat("x", MaxResourceNameLength-4) + "-app", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - res := OperationScopedAppDeployment(tt.appName, tt.opId) - assert.Equal(t, tt.expected, res) + got := OperationScopedAppDeployment(tt.appName, tt.opId) + assert.Equal(t, tt.want, got, "OperationScopedAppDeployment() mismatch for %s", tt.name) }) } } + +func TestOperationScopedAppDeployment_LengthConstraints(t *testing.T) { + // Test that the function always respects the maximum length constraint + testCases := []struct { + appNameLen int + opIdLen int + }{ + {10, 10}, + {MaxAppNameLength, 10}, + {50, 50}, + {100, 100}, + {MaxResourceNameLength, MaxResourceNameLength}, + } + + for _, tc := range testCases { + appName := strings.Repeat("a", tc.appNameLen) + opId := strings.Repeat("b", tc.opIdLen) + + result := OperationScopedAppDeployment(appName, opId) + + if len(result) > MaxResourceNameLength { + t.Errorf("Result length %d exceeds MaxResourceNameLength %d for appName length %d and opId length %d", + len(result), MaxResourceNameLength, tc.appNameLen, tc.opIdLen) + } + } +} diff --git a/internal/utils/controller/const.go b/internal/utils/controller/const.go index 2916951..166bef4 100644 --- a/internal/utils/controller/const.go +++ b/internal/utils/controller/const.go @@ -2,11 +2,11 @@ package controller const ( LabelNameCacheKey = "operation-cache-controller.azure.github.com/cache-key" -) -const ( AnnotationNameCacheMode = "operation-cache-controller.azure.github.com/cache-mode" AnnotationNameCacheKey = "operation-cache-controller.azure.github.com/cache-key" AnnotationValueTrue = "true" AnnotationValueFalse = "false" + + MaxResourceNameLength int = 63 )