diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 2830f59e1..f6049f66b 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -382,6 +382,13 @@ func TestUpdateAnnotationsRemoveExisting(t *testing.T) { assert.DeepEqual(t, expected, actual) } +func TestString(t *testing.T) { + vt := ConfigMapVolumeSourceType + assert.Equal(t, "config-map", vt.String()) + vt = -1 + assert.Equal(t, "unknown", vt.String()) +} + // // ========================================================================================================= diff --git a/pkg/serving/revision_template_test.go b/pkg/serving/revision_template_test.go index 80ca2583e..2998c2323 100644 --- a/pkg/serving/revision_template_test.go +++ b/pkg/serving/revision_template_test.go @@ -18,9 +18,11 @@ import ( "testing" "gotest.tools/v3/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/serving/pkg/apis/autoscaling" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" ) type scalingInfoTest struct { @@ -71,3 +73,29 @@ func TestScalingInfo(t *testing.T) { } } + +func TestAnnotations(t *testing.T) { + m := &metav1.ObjectMeta{} + m.Annotations = map[string]string{UserImageAnnotationKey: "mockImageVal", + autoscaling.TargetAnnotationKey: "1", + autoscaling.TargetUtilizationPercentageKey: "2", + autoscaling.WindowAnnotationKey: "mockWindowVal"} + assert.Equal(t, "mockImageVal", UserImage(m)) + assert.Equal(t, 1, *ConcurrencyTarget(m)) + assert.Equal(t, 2, *ConcurrencyTargetUtilization(m)) + assert.Equal(t, "mockWindowVal", AutoscaleWindow(m)) +} + +func TestPort(t *testing.T) { + revisionSpec := &servingv1.RevisionSpec{ + PodSpec: corev1.PodSpec{}, + ContainerConcurrency: new(int64), + TimeoutSeconds: new(int64), + } + assert.Equal(t, (*int32)(nil), Port(revisionSpec)) + revisionSpec.PodSpec.Containers = append(revisionSpec.PodSpec.Containers, corev1.Container{}) + assert.Equal(t, (*int32)(nil), Port(revisionSpec)) + port := corev1.ContainerPort{ContainerPort: 42} + revisionSpec.PodSpec.Containers[0].Ports = []corev1.ContainerPort{port} + assert.Equal(t, (int32)(42), *Port(revisionSpec)) +} diff --git a/pkg/serving/v1/apply_test.go b/pkg/serving/v1/apply_test.go index 20b6d78d5..c1526f094 100644 --- a/pkg/serving/v1/apply_test.go +++ b/pkg/serving/v1/apply_test.go @@ -16,6 +16,7 @@ package v1 import ( "context" + "fmt" "reflect" "testing" @@ -47,6 +48,9 @@ func TestApplyServiceCreate(t *testing.T) { serving.AddReactor("get", "services", func(a clienttesting.Action) (bool, runtime.Object, error) { name := a.(clienttesting.GetAction).GetName() + if name == "new-service-fail" { + return true, nil, errors.NewInternalError(fmt.Errorf("mock internal error")) + } return true, nil, errors.NewNotFound(servingv1.Resource("service"), name) }) @@ -59,6 +63,15 @@ func TestApplyServiceCreate(t *testing.T) { hasChanged, err := client.ApplyService(context.Background(), serviceNew) assert.NilError(t, err) assert.Assert(t, hasChanged, "service has changed") + + serviceNew = newServiceWithImage("new-service-fail", "test/image") + serving.AddReactor("get", "services", + func(a clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.NewInternalError(fmt.Errorf("mock internal error")) + }) + hasChanged, err = client.ApplyService(context.Background(), serviceNew) + assert.ErrorType(t, err, errors.IsInternalError) + assert.Assert(t, !hasChanged) } func TestApplyServiceUpdate(t *testing.T) { @@ -66,15 +79,37 @@ func TestApplyServiceUpdate(t *testing.T) { serviceOld := newServiceWithImage("my-service", "test/image") serviceNew := newServiceWithImage("my-service", "test/new-image") + serviceConflict := newServiceWithImage("conflict-service", "test/image") + serviceErr := newServiceWithImage("err-service", "test/image") serving.AddReactor("get", "services", func(a clienttesting.Action) (bool, runtime.Object, error) { name := a.(clienttesting.GetAction).GetName() - assert.Equal(t, name, "my-service") - return true, serviceOld, nil + var svc *servingv1.Service + var err error + switch name { + case "my-service": + svc = serviceOld + case "conflict-service": + svc = serviceConflict + case "err-service": + svc = serviceErr + err = errors.NewInternalError(fmt.Errorf("internal error")) + default: + t.FailNow() + } + return true, svc, err }) serving.AddReactor("patch", "services", func(a clienttesting.Action) (bool, runtime.Object, error) { + name := a.(clienttesting.GetAction).GetName() + conflictErr := errors.NewConflict(servingv1.Resource("service"), "conflict-service", fmt.Errorf("error patching service")) + if name == "conflict-service" { + return true, serviceConflict, conflictErr + } + if name == "err-service" { + return true, serviceErr, conflictErr + } serviceNew.Generation = 2 serviceNew.Status.ObservedGeneration = 1 return true, serviceNew, nil @@ -83,6 +118,24 @@ func TestApplyServiceUpdate(t *testing.T) { hasChanged, err := client.ApplyService(context.Background(), serviceNew) assert.NilError(t, err) assert.Assert(t, hasChanged, "service has changed") + + serviceOld.SetAnnotations(map[string]string{}) + hasChanged, err = client.ApplyService(context.Background(), serviceNew) + assert.NilError(t, err) + assert.Assert(t, hasChanged, "service has changed") + + serviceOld.SetAnnotations(map[string]string{corev1.LastAppliedConfigAnnotation: "never"}) + hasChanged, err = client.ApplyService(context.Background(), serviceNew) + assert.ErrorContains(t, err, "Invalid JSON") + assert.Assert(t, !hasChanged, "service has not changed") + + hasChanged, err = client.ApplyService(context.Background(), serviceConflict) + assert.ErrorType(t, err, errors.IsConflict) + assert.Assert(t, !hasChanged, "service has not changed") + + hasChanged, err = client.ApplyService(context.Background(), serviceErr) + assert.ErrorType(t, err, errors.IsInternalError) + assert.Assert(t, !hasChanged, "service has not changed") } func newServiceWithImage(name string, image string) *servingv1.Service { diff --git a/pkg/serving/v1/client_test.go b/pkg/serving/v1/client_test.go index ef3969a01..49219a02c 100644 --- a/pkg/serving/v1/client_test.go +++ b/pkg/serving/v1/client_test.go @@ -20,6 +20,8 @@ import ( "testing" "time" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "knative.dev/pkg/apis" duck "knative.dev/pkg/apis/duck/v1" @@ -135,6 +137,20 @@ func TestListService(t *testing.T) { } +func TestListServiceError(t *testing.T) { + serving, client := setup() + + serving.AddReactor("list", "services", + func(a clienttesting.Action) (bool, runtime.Object, error) { + assert.Equal(t, testNamespace, a.GetNamespace()) + return true, nil, apierrors.NewInternalError(fmt.Errorf("internal error")) + }) + + listServices, err := client.ListServices(context.Background()) + assert.ErrorType(t, err, apierrors.IsInternalError) + assert.Assert(t, listServices == nil) +} + func TestCreateService(t *testing.T) { serving, client := setup() @@ -193,18 +209,90 @@ func TestUpdateService(t *testing.T) { }) } +func TestUpdateServiceWithRetry(t *testing.T) { + serving, client := setup() + serviceUpdate := newService("update-service") + serviceUpdate.ObjectMeta.Generation = 2 + + conflictService := newService("conflict-service") + + serving.AddReactor("update", "services", + func(a clienttesting.Action) (bool, runtime.Object, error) { + assert.Equal(t, testNamespace, a.GetNamespace()) + name := a.(clienttesting.UpdateAction).GetObject().(metav1.Object).GetName() + if name == serviceUpdate.Name { + serviceReturn := newService("update-service") + return true, serviceReturn, nil + } + if name == conflictService.Name { + return true, nil, apierrors.NewConflict(servingv1.Resource("service"), "conflict-service", fmt.Errorf("error updating because of conflict")) + } + return true, nil, fmt.Errorf("error while updating service %s", name) + }) + t.Run("updating a service without error", func(t *testing.T) { + _, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) { + svc.Name = "update-service" + return svc, nil + }, 10) + assert.NilError(t, err) + }) + t.Run("updating a service with conflict error", func(t *testing.T) { + _, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) { + svc.Name = "conflict-service" + return svc, nil + }, 10) + assert.ErrorContains(t, err, "conflict") + }) + + t.Run("updating a service with update func error", func(t *testing.T) { + _, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) { + svc.Name = "update-service" + return svc, fmt.Errorf("update error") + }, 10) + assert.ErrorContains(t, err, "update error") + }) + + serving.AddReactor("get", "services", + func(a clienttesting.Action) (bool, runtime.Object, error) { + name := a.(clienttesting.GetAction).GetName() + if name == serviceUpdate.Name { + serviceUpdate.DeletionTimestamp = &metav1.Time{Time: time.Now()} + return true, serviceUpdate, nil + } + return true, nil, errors.NewNotFound(servingv1.Resource("service"), name) + }) + + t.Run("updating a service with deletion error", func(t *testing.T) { + _, err := client.UpdateServiceWithRetry(context.Background(), "update-service", func(svc *servingv1.Service) (*servingv1.Service, error) { + svc.Name = "update-service" + return svc, nil + }, 10) + assert.ErrorContains(t, err, "marked for deletion") + }) + + t.Run("updating a service with not found error", func(t *testing.T) { + _, err := client.UpdateServiceWithRetry(context.Background(), "unknown-service", func(svc *servingv1.Service) (*servingv1.Service, error) { + svc.Name = "unknown-service" + return svc, nil + }, 10) + assert.ErrorContains(t, err, "unknown") + }) +} + func TestDeleteService(t *testing.T) { serving, client := setup() const ( serviceName = "test-service" nonExistingServiceName = "no-service" deletedServiceName = "deleted-service" + errServiceName = "err-service" ) + delErr := fmt.Errorf("failed to delete service") serving.AddReactor("get", "services", func(a clienttesting.Action) (bool, runtime.Object, error) { name := a.(clienttesting.GetAction).GetName() - if name == serviceName { + if name == serviceName || name == errServiceName { // Don't handle existing service, just continue to next return false, nil, nil } @@ -224,6 +312,9 @@ func TestDeleteService(t *testing.T) { if name == serviceName { return true, nil, nil } + if name == errServiceName { + return true, nil, delErr + } return false, nil, nil }) serving.AddWatchReactor("services", @@ -256,6 +347,11 @@ func TestDeleteService(t *testing.T) { assert.ErrorContains(t, err, "marked for deletion") assert.ErrorContains(t, err, deletedServiceName) }) + + t.Run("delete existing service returns an error", func(t *testing.T) { + err := client.DeleteService(context.Background(), errServiceName, time.Duration(10)*time.Second) + assert.ErrorType(t, err, delErr) + }) } func TestDeleteServiceNoWait(t *testing.T) { @@ -393,6 +489,11 @@ func TestDeleteRevision(t *testing.T) { assert.NilError(t, err) }) + t.Run("deleting revision with timeout returns no error", func(t *testing.T) { + err := client.DeleteRevision(context.Background(), revisionName, time.Second*10) + assert.NilError(t, err) + }) + t.Run("trying to delete non-existing revision returns error", func(t *testing.T) { err := client.DeleteRevision(context.Background(), nonExistingRevisionName, 0) assert.ErrorContains(t, err, "not found") @@ -495,6 +596,23 @@ func TestListRevisions(t *testing.T) { }) } +func TestListRevisionsError(t *testing.T) { + serving, client := setup() + + serving.AddReactor("list", "revisions", + func(a clienttesting.Action) (bool, runtime.Object, error) { + assert.Equal(t, testNamespace, a.GetNamespace()) + return true, nil, apierrors.NewInternalError(fmt.Errorf("internal error")) + }) + + t.Run("list revisions returns an internal error", func(t *testing.T) { + + revisions, err := client.ListRevisions(context.Background()) + assert.ErrorType(t, err, apierrors.IsInternalError) + assert.Assert(t, revisions == nil) + }) +} + func TestListRevisionForService(t *testing.T) { fakeServing, client := setup() @@ -609,27 +727,59 @@ func TestListRoutes(t *testing.T) { }) } +func TestListRoutesError(t *testing.T) { + serving, client := setup() + + serving.AddReactor("list", "routes", + func(a clienttesting.Action) (bool, runtime.Object, error) { + assert.Equal(t, testNamespace, a.GetNamespace()) + return true, nil, apierrors.NewInternalError(fmt.Errorf("internal error")) + }) + + t.Run("list routes returns an internal error", func(t *testing.T) { + + routes, err := client.ListRoutes(context.Background()) + assert.ErrorType(t, err, apierrors.IsInternalError) + assert.Assert(t, routes == nil) + }) +} + func TestWaitForService(t *testing.T) { serving, client := setup() serviceName := "test-service" + notFoundServiceName := "not-found-service" + internalErrorServiceName := "internal-error-service" serving.AddWatchReactor("services", func(a clienttesting.Action) (bool, watch.Interface, error) { watchAction := a.(clienttesting.WatchAction) - _, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name") + val, found := watchAction.GetWatchRestrictions().Fields.RequiresExactMatch("metadata.name") if !found { return true, nil, fmt.Errorf("no field selector on metadata.name found") } - w := wait.NewFakeWatch(getServiceEvents(serviceName)) + w := wait.NewFakeWatch(getServiceEvents(val)) w.Start() return true, w, nil }) serving.AddReactor("get", "services", func(a clienttesting.Action) (bool, runtime.Object, error) { getAction := a.(clienttesting.GetAction) - assert.Equal(t, getAction.GetName(), serviceName) - return true, newService(serviceName), nil + var err error + var svc *servingv1.Service + switch getAction.GetName() { + case serviceName: + err = nil + svc = newService(serviceName) + case notFoundServiceName: + err = apierrors.NewNotFound(servingv1.Resource("service"), notFoundServiceName) + case internalErrorServiceName: + err = apierrors.NewInternalError(fmt.Errorf(internalErrorServiceName)) + default: + t.Log("Service name didn't match any of the given patterns") + t.FailNow() + } + return true, svc, err }) t.Run("wait on a service to become ready with success", func(t *testing.T) { @@ -637,6 +787,16 @@ func TestWaitForService(t *testing.T) { assert.NilError(t, err) assert.Assert(t, duration > 0) }) + t.Run("wait on a service to become ready with not found error", func(t *testing.T) { + err, duration := client.WaitForService(context.Background(), notFoundServiceName, 60*time.Second, wait.NoopMessageCallback()) + assert.NilError(t, err) + assert.Assert(t, duration > 0) + }) + t.Run("wait on a service to become ready with internal error", func(t *testing.T) { + err, duration := client.WaitForService(context.Background(), internalErrorServiceName, 60*time.Second, wait.NoopMessageCallback()) + assert.ErrorType(t, err, apierrors.IsInternalError) + assert.Assert(t, duration == 0) + }) } type baseRevisionCase struct { @@ -758,3 +918,13 @@ func getServiceEvents(name string) []watch.Event { {Type: watch.Modified, Object: wait.CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")}, } } + +func TestCreateRevision(t *testing.T) { + _, client := setup() + rev := newRevision("mockRevision") + assert.NilError(t, client.CreateRevision(context.Background(), rev)) + rev.Labels = map[string]string{ + "mockKey": "mockVal", + } + assert.NilError(t, client.UpdateRevision(context.Background(), rev)) +} diff --git a/pkg/serving/v1/gitops_test.go b/pkg/serving/v1/gitops_test.go index 41613cbee..066968f8e 100644 --- a/pkg/serving/v1/gitops_test.go +++ b/pkg/serving/v1/gitops_test.go @@ -51,6 +51,7 @@ func TestGitOpsOperations(t *testing.T) { fooSvc := libtest.BuildServiceWithOptions("foo", servingtest.WithConfigSpec(buildConfiguration())) barSvc := libtest.BuildServiceWithOptions("bar", servingtest.WithConfigSpec(buildConfiguration())) fooUpdateSvc := libtest.BuildServiceWithOptions("foo", servingtest.WithConfigSpec(buildConfiguration()), servingtest.WithEnv(corev1.EnvVar{Name: "a", Value: "mouse"})) + mockUpdateSvc := libtest.BuildServiceWithOptions("mock", servingtest.WithConfigSpec(buildConfiguration()), servingtest.WithEnv(corev1.EnvVar{Name: "a", Value: "mouse"})) fooserviceList := getServiceList([]servingv1.Service{*barSvc, *fooSvc}) allServices := getServiceList([]servingv1.Service{*barSvc, *barSvc, *fooSvc}) @@ -129,6 +130,11 @@ func TestGitOpsOperations(t *testing.T) { _, err := fooclient.GetService(context.Background(), "foo") assert.ErrorType(t, err, apierrors.IsNotFound) }) + + t.Run("update service foo", func(t *testing.T) { + _, err := fooclient.UpdateService(context.Background(), mockUpdateSvc) + assert.ErrorType(t, err, apierrors.IsNotFound) + }) } func TestGitOpsSingleFile(t *testing.T) { @@ -139,6 +145,8 @@ func TestGitOpsSingleFile(t *testing.T) { fooclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "test.yaml")) barclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "test.yml")) bazclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "test.json")) + mockclient := NewKnServingGitOpsClient("", filepath.Join(tmpDir, "mockfile")) + mockDirclient := NewKnServingGitOpsClient("", tmpDir) // set up test services testSvc := libtest.BuildServiceWithOptions("test", servingtest.WithConfigSpec(buildConfiguration())) @@ -202,6 +210,12 @@ func TestGitOpsSingleFile(t *testing.T) { result, err = bazclient.ListServices(context.Background()) assert.NilError(t, err) assert.DeepEqual(t, svcList, result) + + _, err = mockclient.ListServices(context.Background()) + assert.ErrorContains(t, err, "no such file") + + _, err = mockDirclient.ListServices(context.Background()) + assert.NilError(t, err) }) t.Run("delete service foo", func(t *testing.T) { err := fooclient.DeleteService(context.Background(), "test", 5*time.Second)