Merge pull request #3430 from xmudrii/vw-bindings

Serve both v1alpha1 and v1alpha2 APIBindings in APIExport VW, fix virtual workspace OpenAPI v3 panics

Author: kcp-ci-bot

pkg/virtual/apiexport/builder/build.go

@@ -179,7 +179,7 @@ func BuildVirtualWorkspace(
 						cancelFn:      cancelFn,
 					}, nil
 				},
-				func(ctx context.Context, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error) {
+				func(ctx context.Context, apibindingVersion string, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error) {
 					restProvider, err := provideAPIExportFilteredRestStorage(ctx, impersonatedDynamicClientGetter, clusterName, apiExportName)
 					if err != nil {
 						return nil, err
@@ -188,7 +188,7 @@ func BuildVirtualWorkspace(
 					return apiserver.CreateServingInfoFor(
 						mainConfig,
 						schemas.ApisKcpDevSchemas["apibindings"],
-						apisv1alpha1.SchemeGroupVersion.Version,
+						apibindingVersion,
 						restProvider,
 					)
 				},

pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_controller.go

@@ -62,7 +62,7 @@ func NewAPIReconciler(
 	apiResourceSchemaInformer apisv1alpha1informers.APIResourceSchemaClusterInformer,
 	apiExportInformer apisv1alpha2informers.APIExportClusterInformer,
 	createAPIDefinition CreateAPIDefinitionFunc,
-	createAPIBindingAPIDefinition func(ctx context.Context, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error),
+	createAPIBindingAPIDefinition func(ctx context.Context, apibindingVersion string, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error),
 ) (*APIReconciler, error) {
 	c := &APIReconciler{
 		kcpClusterClient: kcpClusterClient,
@@ -138,7 +138,7 @@ type APIReconciler struct {
 	queue workqueue.TypedRateLimitingInterface[string]
 
 	createAPIDefinition           CreateAPIDefinitionFunc
-	createAPIBindingAPIDefinition func(ctx context.Context, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error)
+	createAPIBindingAPIDefinition func(ctx context.Context, apibindingVersion string, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error)
 
 	mutex   sync.RWMutex // protects the map, not the values!
 	apiSets map[dynamiccontext.APIDomainKey]apidefinition.APIDefinitionSet

pkg/virtual/apiexport/controllers/apireconciler/apiexport_apireconciler_reconcile.go

@@ -244,19 +244,20 @@ func (c *APIReconciler) reconcile(ctx context.Context, apiExport *apisv1alpha2.A
 
 	// always serve apibindings, either through a claim, or with this fallback
 	if !claimsAPIBindings {
-		d, err := c.createAPIBindingAPIDefinition(ctx, clusterName, apiExport.Name)
-		if err != nil {
-			// TODO(ncdc): would be nice to expose some sort of user-visible error
-			logger.Error(err, "error creating api definition for apibindings")
-		}
+		for _, gvr := range []schema.GroupVersionResource{apisv1alpha1.SchemeGroupVersion.WithResource("apibindings"), apisv1alpha2.SchemeGroupVersion.WithResource("apibindings")} {
+			d, err := c.createAPIBindingAPIDefinition(ctx, gvr.Version, clusterName, apiExport.Name)
+			if err != nil {
+				// TODO(ncdc): would be nice to expose some sort of user-visible error
+				logger.Error(err, "error creating api definition for apibindings")
+			}
 
-		gvr := apisv1alpha2.SchemeGroupVersion.WithResource("apibindings")
-		newSet[gvr] = apiResourceSchemaApiDefinition{
-			APIDefinition: d,
-		}
-		newGVRs = append(newGVRs, gvrString(gvr))
-		if _, ok := oldSet[gvr]; ok {
-			preservedGVR = append(preservedGVR, gvrString(gvr))
+			newSet[gvr] = apiResourceSchemaApiDefinition{
+				APIDefinition: d,
+			}
+			newGVRs = append(newGVRs, gvrString(gvr))
+			if _, ok := oldSet[gvr]; ok {
+				preservedGVR = append(preservedGVR, gvrString(gvr))
+			}
 		}
 	}
 

pkg/virtual/framework/dynamic/apiserver/openapi.go

@@ -228,7 +228,9 @@ func (o *openAPIHandler) handleOpenAPIRequest(apiSet apidefinition.APIDefinition
 		}
 
 		groupPath := "apis/" + gv.Group
-		webservices[groupPath] = append(webservices[groupPath], discovery.NewAPIGroupHandler(codecs, apiGroup).WebService())
+		if _, ok := webservices[groupPath]; !ok {
+			webservices[groupPath] = append(webservices[groupPath], discovery.NewAPIGroupHandler(codecs, apiGroup).WebService())
+		}
 	}
 
 	// Build OpenAPI v3 spec for /apis/<group> webservices

test/e2e/virtual/apiexport/apiresourceschema_cowboys_versions.yaml

@@ -0,0 +1,81 @@
+apiVersion: apis.kcp.io/v1alpha1
+kind: APIResourceSchema
+metadata:
+  name: today.cowboys.wildwest.dev
+spec:
+  group: wildwest.dev
+  names:
+    kind: Cowboy
+    listKind: CowboyList
+    plural: cowboys
+    singular: cowboy
+  scope: Namespaced
+  conversion:
+    strategy: None
+  versions:
+  - name: v1alpha1
+    schema:
+      description: Cowboy is part of the wild west
+      properties:
+        apiVersion:
+          description: 'APIVersion defines the versioned schema of this representation
+            of an object. Servers should convert recognized schemas to the latest
+            internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+          type: string
+        kind:
+          description: 'Kind is a string value representing the REST resource this
+            object represents. Servers may infer this from the endpoint the client
+            submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+          type: string
+        metadata:
+          type: object
+        spec:
+          description: CowboySpec holds the desired state of the Cowboy.
+          properties:
+            intent:
+              type: string
+          type: object
+        status:
+          description: CowboyStatus communicates the observed state of the Cowboy.
+          properties:
+            result:
+              type: string
+          type: object
+      type: object
+    served: true
+    storage: true
+    subresources:
+      status: {}
+  - name: v1alpha2
+    schema:
+      description: Cowboy is part of the wild west
+      properties:
+        apiVersion:
+          description: 'APIVersion defines the versioned schema of this representation
+            of an object. Servers should convert recognized schemas to the latest
+            internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+          type: string
+        kind:
+          description: 'Kind is a string value representing the REST resource this
+            object represents. Servers may infer this from the endpoint the client
+            submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+          type: string
+        metadata:
+          type: object
+        spec:
+          description: CowboySpec holds the desired state of the Cowboy.
+          properties:
+            intent:
+              type: string
+          type: object
+        status:
+          description: CowboyStatus communicates the observed state of the Cowboy.
+          properties:
+            result:
+              type: string
+          type: object
+      type: object
+    served: true
+    storage: false
+    subresources:
+      status: {}

test/e2e/virtual/apiexport/binding_test.go

@@ -262,7 +262,7 @@ func TestAPIBindingPermissionClaimsVerbs(t *testing.T) {
 	permissionClaims := defaultPermissionsClaims(identityHash, pcModifiers...)
 
 	t.Logf("set up service provider with permission claims")
-	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClusterClient, providerPath, cfg, permissionClaims)
+	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClusterClient, false, providerPath, cfg, permissionClaims)
 
 	t.Logf("set up binding")
 	bindConsumerToProvider(ctx, t, consumerPath, providerPath, kcpClusterClient, cfg, permissionClaimsToAcceptable(permissionClaims)...)

test/e2e/virtual/apiexport/openapi_test.go

@@ -67,7 +67,7 @@ func TestAPIExportOpenAPI(t *testing.T) {
 
 	framework.AdmitWorkspaceAccess(ctx, t, kubeClusterClient, serviceProviderPath, []string{"user-1"}, nil, false)
 
-	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClients, serviceProviderPath, cfg, nil)
+	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClients, true, serviceProviderPath, cfg, nil)
 
 	t.Logf("Waiting for APIExport to have a virtual workspace URL for the bound workspace %q", consumerWorkspace.Name)
 	apiExportVWCfg := rest.CopyConfig(cfg)

test/e2e/virtual/apiexport/virtualworkspace_test.go

@@ -103,7 +103,7 @@ func TestAPIExportVirtualWorkspace(t *testing.T) {
 
 	framework.AdmitWorkspaceAccess(ctx, t, kubeClusterClient, serviceProviderPath, []string{"user-1"}, nil, false)
 
-	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClients, serviceProviderPath, cfg, nil)
+	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClients, false, serviceProviderPath, cfg, nil)
 	bindConsumerToProvider(ctx, t, consumerPath, serviceProviderPath, kcpClients, cfg)
 	createCowboyInConsumer(ctx, t, consumerPath, wildwestClusterClient)
 
@@ -133,6 +133,10 @@ func TestAPIExportVirtualWorkspace(t *testing.T) {
 	require.NoError(t, err, "error retrieving APIExport discovery")
 	require.True(t, resourceExists(resources, "apibindings"), "missing apibindings")
 
+	resources, err = discoveryVCClusterClient.ServerResourcesForGroupVersion(apisv1alpha1.SchemeGroupVersion.String())
+	require.NoError(t, err, "error retrieving APIExport discovery")
+	require.True(t, resourceExists(resources, "apibindings"), "missing apibindings")
+
 	user1VWCfg := framework.StaticTokenUserConfig("user-1", apiExportVWCfg)
 	wwUser1VC, err := wildwestclientset.NewForConfig(user1VWCfg)
 	require.NoError(t, err)
@@ -1003,18 +1007,23 @@ func defaultPermissionsClaims(identityHash string, modifiers ...func([]apisv1alp
 func setUpServiceProviderWithPermissionClaims(ctx context.Context, t *testing.T, dynamicClusterClient kcpdynamic.ClusterInterface, kcpClients kcpclientset.ClusterInterface, serviceProviderWorkspace logicalcluster.Path, cfg *rest.Config, identityHash string) {
 	t.Helper()
 
-	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClients, serviceProviderWorkspace, cfg, defaultPermissionsClaims(identityHash))
+	setUpServiceProvider(ctx, t, dynamicClusterClient, kcpClients, false, serviceProviderWorkspace, cfg, defaultPermissionsClaims(identityHash))
 }
 
-func setUpServiceProvider(ctx context.Context, t *testing.T, dynamicClusterClient kcpdynamic.ClusterInterface, kcpClients kcpclientset.ClusterInterface, serviceProviderWorkspace logicalcluster.Path, cfg *rest.Config, claims []apisv1alpha2.PermissionClaim) {
+func setUpServiceProvider(ctx context.Context, t *testing.T, dynamicClusterClient kcpdynamic.ClusterInterface, kcpClients kcpclientset.ClusterInterface, multipleVersions bool, serviceProviderWorkspace logicalcluster.Path, cfg *rest.Config, claims []apisv1alpha2.PermissionClaim) {
 	t.Helper()
 	t.Logf("Install today cowboys APIResourceSchema into service provider workspace %q", serviceProviderWorkspace)
 
 	serviceProviderClient, err := kcpclientset.NewForConfig(cfg)
 	require.NoError(t, err)
 
+	apiResPath := "apiresourceschema_cowboys.yaml"
+	if multipleVersions {
+		apiResPath = "apiresourceschema_cowboys_versions.yaml"
+	}
+
 	mapper := restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(serviceProviderClient.Cluster(serviceProviderWorkspace).Discovery()))
-	err = helpers.CreateResourceFromFS(ctx, dynamicClusterClient.Cluster(serviceProviderWorkspace), mapper, nil, "apiresourceschema_cowboys.yaml", testFiles)
+	err = helpers.CreateResourceFromFS(ctx, dynamicClusterClient.Cluster(serviceProviderWorkspace), mapper, nil, apiResPath, testFiles)
 	require.NoError(t, err)
 
 	t.Logf("Create an APIExport for it")