fix aggregator path filtering to include /

This commit is contained in:
Jefftree 2024-03-06 13:01:18 -05:00
parent 546f7c3086
commit 8373f3035a
3 changed files with 49 additions and 47 deletions

View File

@ -199,7 +199,7 @@ func (s *specAggregator) updateServiceLocked(name string) error {
} }
group := specInfo.apiService.Spec.Group group := specInfo.apiService.Spec.Group
version := specInfo.apiService.Spec.Version 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}) }, cached.Result[*spec.Swagger]{Value: result, Etag: etag, Err: err})
specInfo.spec.Store(filteredResult) specInfo.spec.Store(filteredResult)
return err return err

View File

@ -39,7 +39,7 @@ func TestBasicPathsMerged(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -52,8 +52,8 @@ func TestBasicPathsMerged(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/foo/v1") expectPath(t, swagger, "/apis/foo/v1/")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
} }
func TestAddUpdateAPIService(t *testing.T) { 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/apiservicegroup/v1/path1")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
t.Log("Update APIService OpenAPI") t.Log("Update APIService OpenAPI")
handler.openapi = &spec.Swagger{ handler.openapi = &spec.Swagger{
@ -127,7 +127,7 @@ func TestAddUpdateAPIService(t *testing.T) {
// aggregated OpenAPI is also updated. // aggregated OpenAPI is also updated.
expectPath(t, swagger, "/apis/apiservicegroup/v1/path2") expectPath(t, swagger, "/apis/apiservicegroup/v1/path2")
expectNoPath(t, swagger, "/apis/apiservicegroup/v1/path1") 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 // Tests that an APIService that registers OpenAPI will only have the OpenAPI
@ -140,7 +140,7 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -171,7 +171,8 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ 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{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/a": {}, "/apis/a/": {},
"/apis/apiservicegroup/v1": {}, "/apis/apiservicegroup/v1/": {},
"/apis/apiservicegroup/v2": {}, "/apis/apiservicegroup/v2/": {},
}, },
}, },
}, },
@ -207,10 +208,11 @@ func TestAPIServiceOpenAPIServiceMismatch(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/apiservicegroup/v1") expectPath(t, swagger, "/apis/apiservicegroup/v1/")
expectPath(t, swagger, "/apis/apiservicegroup/v2") expectPath(t, swagger, "/apis/apiservicegroup/v2/")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
expectNoPath(t, swagger, "/apis/a") expectNoPath(t, swagger, "/apis/a/")
expectNoPath(t, swagger, "/apis/apiservicegroup/v1beta1/")
t.Logf("Remove APIService %s", apiService.Name) t.Logf("Remove APIService %s", apiService.Name)
s.RemoveAPIService(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. // 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") 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") expectNoPath(t, swagger, "/apis/a")
} }
@ -232,7 +234,7 @@ func TestAddRemoveAPIService(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -254,7 +256,7 @@ func TestAddRemoveAPIService(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/apiservicegroup/v1": {}, "/apis/apiservicegroup/v1/": {},
}, },
}, },
}, },
@ -271,8 +273,8 @@ func TestAddRemoveAPIService(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/apiservicegroup/v1") expectPath(t, swagger, "/apis/apiservicegroup/v1/")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
t.Logf("Remove APIService %s", apiService.Name) t.Logf("Remove APIService %s", apiService.Name)
s.RemoveAPIService(apiService.Name) s.RemoveAPIService(apiService.Name)
@ -282,8 +284,8 @@ func TestAddRemoveAPIService(t *testing.T) {
t.Error(err) t.Error(err)
} }
// Ensure that the if the APIService is added then removed, the OpenAPI disappears from the aggregated OpenAPI as well. // 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") expectNoPath(t, swagger, "/apis/apiservicegroup/v1/")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
} }
func TestUpdateAPIService(t *testing.T) { func TestUpdateAPIService(t *testing.T) {
@ -293,7 +295,7 @@ func TestUpdateAPIService(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -315,7 +317,7 @@ func TestUpdateAPIService(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/apiservicegroup/v1": {}, "/apis/apiservicegroup/v1/": {},
}, },
}, },
}, },
@ -340,8 +342,8 @@ func TestUpdateAPIService(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/apiservicegroup/v1") expectPath(t, swagger, "/apis/apiservicegroup/v1/")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
t.Logf("Updating APIService %s", apiService.Name) t.Logf("Updating APIService %s", apiService.Name)
if err := s.AddUpdateAPIService(apiService, handler2); err != nil { if err := s.AddUpdateAPIService(apiService, handler2); err != nil {
@ -356,8 +358,8 @@ func TestUpdateAPIService(t *testing.T) {
t.Error(err) 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. // 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") expectNoPath(t, swagger, "/apis/apiservicegroup/v1/")
expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1") expectPath(t, swagger, "/apis/apiregistration.k8s.io/v1/")
} }
func TestFailingAPIServiceSkippedAggregation(t *testing.T) { func TestFailingAPIServiceSkippedAggregation(t *testing.T) {
@ -367,7 +369,7 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -391,7 +393,7 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/failed/v1": {}, "/apis/failed/v1/": {},
}, },
}, },
}, },
@ -412,7 +414,7 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/success/v1": {}, "/apis/success/v1/": {},
}, },
}, },
}, },
@ -437,9 +439,9 @@ func TestFailingAPIServiceSkippedAggregation(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/foo/v1") expectPath(t, swagger, "/apis/foo/v1/")
expectNoPath(t, swagger, "/apis/failed/v1") expectNoPath(t, swagger, "/apis/failed/v1/")
expectPath(t, swagger, "/apis/success/v1") expectPath(t, swagger, "/apis/success/v1/")
} }
func TestAPIServiceFailSuccessTransition(t *testing.T) { func TestAPIServiceFailSuccessTransition(t *testing.T) {
@ -449,7 +451,7 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -473,7 +475,7 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/apiservicegroup/v1": {}, "/apis/apiservicegroup/v1/": {},
}, },
}, },
}, },
@ -491,8 +493,8 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/foo/v1") expectPath(t, swagger, "/apis/foo/v1/")
expectNoPath(t, swagger, "/apis/apiservicegroup/v1") expectNoPath(t, swagger, "/apis/apiservicegroup/v1/")
t.Log("Transition APIService to not return error") t.Log("Transition APIService to not return error")
handler.returnErr = false handler.returnErr = false
@ -504,8 +506,8 @@ func TestAPIServiceFailSuccessTransition(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/foo/v1") expectPath(t, swagger, "/apis/foo/v1/")
expectPath(t, swagger, "/apis/apiservicegroup/v1") expectPath(t, swagger, "/apis/apiservicegroup/v1/")
} }
func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) { func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) {
@ -515,7 +517,7 @@ func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/foo/v1": {}, "/apis/foo/v1/": {},
}, },
}, },
}, },
@ -542,7 +544,7 @@ func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/failed/v1": {}, "/apis/failed/v1/": {},
}, },
}, },
}, },
@ -567,8 +569,8 @@ func TestFailingAPIServiceDoesNotBlockAdd(t *testing.T) {
if err != nil { if err != nil {
t.Error(err) t.Error(err)
} }
expectPath(t, swagger, "/apis/foo/v1") expectPath(t, swagger, "/apis/foo/v1/")
expectNoPath(t, swagger, "/apis/failed/v1") expectNoPath(t, swagger, "/apis/failed/v1/")
} }
type openAPIHandler struct { type openAPIHandler struct {
@ -619,7 +621,7 @@ func buildAndRegisterSpecAggregator(delegationHandlers []http.Handler, mux commo
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ Paths: map[string]spec.PathItem{
"/apis/apiregistration.k8s.io/v1": {}, "/apis/apiregistration.k8s.io/v1/": {},
}, },
}, },
}, },

View File

@ -119,7 +119,7 @@ func TestFetchingOpenAPIBeforeReady(t *testing.T) {
SwaggerProps: spec.SwaggerProps{ SwaggerProps: spec.SwaggerProps{
Paths: &spec.Paths{ Paths: &spec.Paths{
Paths: map[string]spec.PathItem{ 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) require.NoError(t, err)
var openapi spec.Swagger var openapi spec.Swagger
require.NoError(t, openapi.UnmarshalJSON(b)) 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 true, nil
} }
return false, nil return false, nil