From 8373f3035ade14b3c4723aae82672709fba6d372 Mon Sep 17 00:00:00 2001 From: Jefftree Date: Wed, 6 Mar 2024 13:01:18 -0500 Subject: [PATCH] fix aggregator path filtering to include / --- .../openapi/aggregator/aggregator.go | 2 +- .../openapi/aggregator/aggregator_test.go | 90 ++++++++++--------- .../openapi/openapi_apiservice_test.go | 4 +- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go index b41e268b8ab..7d4281f2346 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator.go @@ -199,7 +199,7 @@ func (s *specAggregator) updateServiceLocked(name string) error { } group := specInfo.apiService.Spec.Group version := specInfo.apiService.Spec.Version - return aggregator.FilterSpecByPathsWithoutSideEffects(result, []string{"/apis/" + group + "/" + version}), etag, nil + return aggregator.FilterSpecByPathsWithoutSideEffects(result, []string{"/apis/" + group + "/" + version + "/"}), etag, nil }, cached.Result[*spec.Swagger]{Value: result, Etag: etag, Err: err}) specInfo.spec.Store(filteredResult) return err diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator_test.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator_test.go index f1af06d0591..e8d72788d53 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator_test.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/aggregator/aggregator_test.go @@ -39,7 +39,7 @@ func TestBasicPathsMerged(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -52,8 +52,8 @@ func TestBasicPathsMerged(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/foo/v1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectPath(t, swagger, "/apis/foo/v1/") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") } func TestAddUpdateAPIService(t *testing.T) { @@ -103,7 +103,7 @@ func TestAddUpdateAPIService(t *testing.T) { } expectPath(t, swagger, "/apis/apiservicegroup/v1/path1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") t.Log("Update APIService OpenAPI") handler.openapi = &spec.Swagger{ @@ -127,7 +127,7 @@ func TestAddUpdateAPIService(t *testing.T) { // aggregated OpenAPI is also updated. expectPath(t, swagger, "/apis/apiservicegroup/v1/path2") expectNoPath(t, swagger, "/apis/apiservicegroup/v1/path1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") } // Tests that an APIService that registers OpenAPI will only have the OpenAPI @@ -140,7 +140,7 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -171,7 +171,8 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/apiservicegroup/v1": {}, + "/apis/apiservicegroup/v1/": {}, + "/apis/apiservicegroup/v1beta1/": {}, }, }, }, @@ -181,9 +182,9 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/a": {}, - "/apis/apiservicegroup/v1": {}, - "/apis/apiservicegroup/v2": {}, + "/apis/a/": {}, + "/apis/apiservicegroup/v1/": {}, + "/apis/apiservicegroup/v2/": {}, }, }, }, @@ -207,10 +208,11 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/apiservicegroup/v1") - expectPath(t, swagger, "/apis/apiservicegroup/v2") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") - expectNoPath(t, swagger, "/apis/a") + expectPath(t, swagger, "/apis/apiservicegroup/v1/") + expectPath(t, swagger, "/apis/apiservicegroup/v2/") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") + expectNoPath(t, swagger, "/apis/a/") + expectNoPath(t, swagger, "/apis/apiservicegroup/v1beta1/") t.Logf("Remove APIService %s", apiService.Name) s.RemoveAPIService(apiService.Name) @@ -221,7 +223,7 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) { } // Ensure that the if the APIService is added then removed, the OpenAPI disappears from the aggregated OpenAPI as well. expectNoPath(t, swagger, "/apis/apiservicegroup/v1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") expectNoPath(t, swagger, "/apis/a") } @@ -232,7 +234,7 @@ func TestAddRemoveAPIService(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -254,7 +256,7 @@ func TestAddRemoveAPIService(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/apiservicegroup/v1": {}, + "/apis/apiservicegroup/v1/": {}, }, }, }, @@ -271,8 +273,8 @@ func TestAddRemoveAPIService(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/apiservicegroup/v1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectPath(t, swagger, "/apis/apiservicegroup/v1/") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") t.Logf("Remove APIService %s", apiService.Name) s.RemoveAPIService(apiService.Name) @@ -282,8 +284,8 @@ func TestAddRemoveAPIService(t *testing.T) { t.Error(err) } // Ensure that the if the APIService is added then removed, the OpenAPI disappears from the aggregated OpenAPI as well. - expectNoPath(t, swagger, "/apis/apiservicegroup/v1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectNoPath(t, swagger, "/apis/apiservicegroup/v1/") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") } func TestUpdateAPIService(t *testing.T) { @@ -293,7 +295,7 @@ func TestUpdateAPIService(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -315,7 +317,7 @@ func TestUpdateAPIService(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/apiservicegroup/v1": {}, + "/apis/apiservicegroup/v1/": {}, }, }, }, @@ -340,8 +342,8 @@ func TestUpdateAPIService(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/apiservicegroup/v1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectPath(t, swagger, "/apis/apiservicegroup/v1/") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") t.Logf("Updating APIService %s", apiService.Name) if err := s.AddUpdateAPIService(apiService, handler2); err != nil { @@ -356,8 +358,8 @@ func TestUpdateAPIService(t *testing.T) { t.Error(err) } // Ensure that the if the APIService is added and then handler is modified, the new data is reflected in the aggregated OpenAPI. - expectNoPath(t, swagger, "/apis/apiservicegroup/v1") - expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") + expectNoPath(t, swagger, "/apis/apiservicegroup/v1/") + expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/") } func TestFailingAPIServiceSkippedAggregation(t *testing.T) { @@ -367,7 +369,7 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -391,7 +393,7 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/failed/v1": {}, + "/apis/failed/v1/": {}, }, }, }, @@ -412,7 +414,7 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/success/v1": {}, + "/apis/success/v1/": {}, }, }, }, @@ -437,9 +439,9 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/foo/v1") - expectNoPath(t, swagger, "/apis/failed/v1") - expectPath(t, swagger, "/apis/success/v1") + expectPath(t, swagger, "/apis/foo/v1/") + expectNoPath(t, swagger, "/apis/failed/v1/") + expectPath(t, swagger, "/apis/success/v1/") } func TestAPIServiceFailSuccessTransition(t *testing.T) { @@ -449,7 +451,7 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -473,7 +475,7 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/apiservicegroup/v1": {}, + "/apis/apiservicegroup/v1/": {}, }, }, }, @@ -491,8 +493,8 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/foo/v1") - expectNoPath(t, swagger, "/apis/apiservicegroup/v1") + expectPath(t, swagger, "/apis/foo/v1/") + expectNoPath(t, swagger, "/apis/apiservicegroup/v1/") t.Log("Transition APIService to not return error") handler.returnErr = false @@ -504,8 +506,8 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/foo/v1") - expectPath(t, swagger, "/apis/apiservicegroup/v1") + expectPath(t, swagger, "/apis/foo/v1/") + expectPath(t, swagger, "/apis/apiservicegroup/v1/") } func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) { @@ -515,7 +517,7 @@ func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/foo/v1": {}, + "/apis/foo/v1/": {}, }, }, }, @@ -542,7 +544,7 @@ func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/failed/v1": {}, + "/apis/failed/v1/": {}, }, }, }, @@ -567,8 +569,8 @@ func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) { if err != nil { t.Error(err) } - expectPath(t, swagger, "/apis/foo/v1") - expectNoPath(t, swagger, "/apis/failed/v1") + expectPath(t, swagger, "/apis/foo/v1/") + expectNoPath(t, swagger, "/apis/failed/v1/") } type openAPIHandler struct { @@ -619,7 +621,7 @@ func buildAndRegisterSpecAggregator(delegationHandlers []http.Handler, mux commo SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/apiregistration.k8s.io/v1": {}, + "/apis/apiregistration.k8s.io/v1/": {}, }, }, }, diff --git a/test/integration/apiserver/openapi/openapi_apiservice_test.go b/test/integration/apiserver/openapi/openapi_apiservice_test.go index 01f6d20da55..a12374571bb 100644 --- a/test/integration/apiserver/openapi/openapi_apiservice_test.go +++ b/test/integration/apiserver/openapi/openapi_apiservice_test.go @@ -119,7 +119,7 @@ func TestFetchingOpenAPIBeforeReady(t *testing.T) { SwaggerProps: spec.SwaggerProps{ Paths: &spec.Paths{ Paths: map[string]spec.PathItem{ - "/apis/wardle.example.com/v1alpha1": {}, + "/apis/wardle.example.com/v1alpha1/": {}, }, }, }, @@ -150,7 +150,7 @@ func TestFetchingOpenAPIBeforeReady(t *testing.T) { require.NoError(t, err) var openapi spec.Swagger require.NoError(t, openapi.UnmarshalJSON(b)) - if _, ok := openapi.Paths.Paths["/apis/wardle.example.com/v1alpha1"]; ok { + if _, ok := openapi.Paths.Paths["/apis/wardle.example.com/v1alpha1/"]; ok { return true, nil } return false, nil