✨ Check for identityHash in APIExport admission and support multiple versions for APIs in permission claims #2169

hasheddan · 2022-10-11T13:54:55Z

Summary

This patch set includes two primary changes:

Update the APIResourceSchema generation to consider multiple versions of
an API and append the schemas to one ARS rather than generating a
separate ARS for each.

Signed-off-by: hasheddan georgedanielmangum@gmail.com

Adds an APIExport admission plugin that, for the time-being, only
validates that any permissionClaims for types that are not built-in
include an identityHash.

Signed-off-by: hasheddan georgedanielmangum@gmail.com

The first can be observed when creating an APIExport with permissionClaims on FlowSchemas or PriorityLevelConfigurations.

Before:

🤖 (kxp) k -s https://192.168.1.189:6443/services/apiexport/root/kxp.crossplane.io/clusters/* api-versions
apiextensions.crossplane.io/v1
apiextensions.crossplane.io/v1alpha1
flowcontrol.apiserver.k8s.io/v1beta1
pkg.crossplane.io/v1
pkg.crossplane.io/v1alpha1
pkg.crossplane.io/v1beta1
secrets.crossplane.io/v1alpha1
v1

After:

🤖 (kxp) k -s https://192.168.1.189:6443/services/apiexport/root/kxp.crossplane.io/clusters/* api-versions
apiextensions.crossplane.io/v1
apiextensions.crossplane.io/v1alpha1
flowcontrol.apiserver.k8s.io/v1beta1
flowcontrol.apiserver.k8s.io/v1beta2
pkg.crossplane.io/v1
pkg.crossplane.io/v1alpha1
pkg.crossplane.io/v1beta1
secrets.crossplane.io/v1alpha1
v1

The second can be observed by trying to include a permissionClaim for something like Deployment (not built-in) without an identityHash:

🤖 (kxp) cat bad-export.yaml 
apiVersion: apis.kcp.dev/v1alpha1
kind: APIExport
metadata:
  name: data.my.domain
spec:
  latestResourceSchemas:
    - today.widgets.data.my.domain
  permissionClaims:
    - group: ""
      resource: "secrets"
    - group: ""
      resource: "configmaps"
    - group: ""
      resource: "namespaces"
    - group: "apps"
      resource: "deployments"
🤖 (kxp) k apply -f bad-export.yaml 
Error from server (Forbidden): error when creating "bad-export.yaml": apiexports.apis.kcp.dev "data.my.domain" is forbidden: spec.permissionClaims.[3].identityHash: Invalid value: "": identityHash is required for API types that are not built-in

Related issue(s)

Fixes #2161
xref #2152 (more work to be done on exposing conditions for other failure modes)

hasheddan · 2022-10-11T14:00:29Z

pkg/virtual/syncer/controllers/apireconciler/syncer_apireconciler_reconcile.go

@@ -57,7 +51,7 @@ func (c *APIReconciler) reconcile(ctx context.Context, apiDomainKey dynamicconte
 	}

 	// add built-in apiResourceSchema
-	for _, apiResourceSchema := range syncerSchemas {
+	for _, apiResourceSchema := range syncerbuiltin.SyncerSchemas {


I think the process of how we add the ARS to the sync target set can be improved here as we are essentially duplicating these for every apiSet even though we know they are always present. That being said, I opted to not include in this PR as it was already carrying a fairly large diff. Planning to address in follow-up.

hasheddan · 2022-10-11T14:01:33Z

pkg/virtual/syncer/schemas/builtin/builtin_test.go

+	"testing"
+
+	"github.com/stretchr/testify/require"
+	_ "k8s.io/kubernetes/pkg/apis/core/install"


We could consider moving this anonymous import into builtin.go as inclusion of the package without also importing core install will cause init() to panic.

Sounds like we should?

I'm happy to, though in theory this is a change in behavior that is not strictly how all consumers would have to behave today. In practice, kcp is installing the core types into the legacy schema before this init() runs, but not all consumers have to. In general I don't love this pattern of registering schemas in init() calls, and would lean towards going ahead and moving the anonymous import (as you suggest) to cover the 99% case here.

@sttts what do you prefer?

@sttts any thoughts here?

don't have much context here. But in general it is highly discourage to use the legacyscheme in any way, especially in new code like kcp. Create dedicated schemes and add the group you want via the AddToScheme methods.

hasheddan · 2022-10-11T14:50:45Z

Need to cleanup some of these e2e tests that I believe are correctly failing now.

Update the APIResourceSchema generation to consider multiple versions of an API and append the schemas to one ARS rather than generating a separate ARS for each. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Moves the APIExport built-in API ARS generation to a dedicated builtin schema package and consume it in the APIExport reconciler. Methods are broken out into a check of whether an ARS exists and a getter for the actual ARS to accommodate more flexible use-cases. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Moves the syncer built-in schema generation to a dedicated package such that it can be consumed in multiple locations. Updates the syncer API reconciler to use it as first consumer. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Fixes up virtual worksapce e2e test to consume internal APIs from built-in package. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

ncdc · 2022-10-11T21:30:09Z

pkg/admission/apiexport/admission.go

+				field.Invalid(
+					field.NewPath("spec").
+						Child("permissionClaims").
+						Child(fmt.Sprintf("[%d]", i)).


You can use .Index here

pkg/admission/apiexport/admission_test.go

ncdc · 2022-10-12T14:40:58Z

pkg/admission/apiexport/admission.go

+type APIExportAdmission struct {
+	*admission.Handler
+
+	isBuiltInFn func(apisv1alpha1.GroupResource) bool


nit: I would call this isBuiltIn

ncdc · 2022-10-12T14:41:25Z

pkg/admission/apiexport/admission.go

+}
+
+// NewAPIExportAdmission constructs a new APIExportAdmission admission plugin.
+func NewAPIExportAdmission(isInternal func(apisv1alpha1.GroupResource) bool) *APIExportAdmission {


Suggested change

func NewAPIExportAdmission(isInternal func(apisv1alpha1.GroupResource) bool) *APIExportAdmission {

func NewAPIExportAdmission(isBuiltIn func(apisv1alpha1.GroupResource) bool) *APIExportAdmission {

ncdc · 2022-10-12T14:44:17Z

pkg/admission/apiexport/admission_test.go

+func updateAttr(name string, obj runtime.Object, kind, resource string) admission.Attributes {
+	return admission.NewAttributesRecord(
+		helpers.ToUnstructuredOrDie(obj),
+		nil,


Since this is an update, I know it doesn't matter for the code under test, but should we fill in the "old" object here?

Happy to 👍🏻

ncdc · 2022-10-12T14:45:02Z

pkg/admission/apiexport/admission_test.go

+		kind        string
+		resource    string
+		hasIdentity bool
+		isBuiltIn   func(apisv1alpha1.GroupResource) bool


Unless you're intending to vary the return based on the input, I think this can be a normal bool and you can set the function pointer once, inside t.Run?

pkg/admission/apiexport/admission_test.go

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

ncdc · 2022-10-12T14:50:21Z

pkg/virtual/syncer/schemas/builtin/builtin_test.go

+	"testing"
+
+	"github.com/stretchr/testify/require"
+	_ "k8s.io/kubernetes/pkg/apis/core/install"


Sounds like we should?

Adds an APIExport admission plugin that, for the time-being, only validates that any permissionClaims for types that are not built-in include an identityHash. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Registers the APIExport admission plugin as a default. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Adds hasheddan as a reviewer for the APIExport admission plugin. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Cleans up some unecessary conditional logic in APIExport reconciler. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

ncdc · 2022-10-12T15:24:00Z

/lgtm

Would like to hear Stefan's thoughts on the import question

hasheddan · 2022-10-13T12:57:23Z

/assign @sttts

sttts · 2022-10-16T15:30:27Z

pkg/virtual/syncer/schemas/builtin/builtin.go

+
+func init() {
+	genericcontrolplane.Install(legacyscheme.Scheme)
+	schemes := []*runtime.Scheme{legacyscheme.Scheme}


what is this used for? don't like to see the legacyscheme in our code.

@sttts this isn’t strictly new in this patch set, just being relocated from the syncer API reconciler. I presume the legacy scheme is being used as the sync target is assumed to be Kubernetes API-compatible. Happy to update this to explicitly add each type in the internal API list below to the scheme.

I would recommend doing a follow-up to deal with legacyscheme usage as a whole

@ncdc happy to open a tracking issue and tackle 👍🏻

@hasheddan yes let's do that please, thanks!

ncdc · 2022-10-17T18:26:27Z

Approving. Keep merging & iterate on 😄
/approve

openshift-ci · 2022-10-17T18:26:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/admission/OWNERS~~ [ncdc]
~~pkg/virtual/OWNERS~~ [ncdc]
~~test/OWNERS~~ [ncdc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from csams and shawn-hurley October 11, 2022 13:55

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 11, 2022

hasheddan commented Oct 11, 2022

View reviewed changes

hasheddan force-pushed the admission-apiexport branch from 2355d9c to 332e7fc Compare October 11, 2022 14:10

hasheddan added 4 commits October 11, 2022 10:55

Consider multiple versions of the same API when generating ARS

1117949

Update the APIResourceSchema generation to consider multiple versions of an API and append the schemas to one ARS rather than generating a separate ARS for each. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Fix virtual workspace test that checks for internal API drift

26651cd

Fixes up virtual worksapce e2e test to consume internal APIs from built-in package. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan force-pushed the admission-apiexport branch 3 times, most recently from 30c0882 to 870ca54 Compare October 11, 2022 17:35

ncdc reviewed Oct 11, 2022

View reviewed changes

pkg/admission/apiexport/admission_test.go Show resolved Hide resolved

hasheddan force-pushed the admission-apiexport branch from 870ca54 to 223bf7b Compare October 11, 2022 22:16

hasheddan requested a review from ncdc October 12, 2022 10:28

ncdc reviewed Oct 12, 2022

View reviewed changes

hasheddan added 4 commits October 12, 2022 11:16

Add APIExport admission plugin

0db3e56

Adds an APIExport admission plugin that, for the time-being, only validates that any permissionClaims for types that are not built-in include an identityHash. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Register APIExport admission plugin as loaded by default

36e18f6

Registers the APIExport admission plugin as a default. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Add hasheddan as APIExport admission reviewer

44ed9c3

Adds hasheddan as a reviewer for the APIExport admission plugin. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Clean up logic in the virtual workspace APiExport reconciler

5cfb111

Cleans up some unecessary conditional logic in APIExport reconciler. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan force-pushed the admission-apiexport branch from 223bf7b to 5cfb111 Compare October 12, 2022 15:16

openshift-ci bot assigned ncdc Oct 12, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2022

openshift-ci bot assigned sttts Oct 13, 2022

sttts reviewed Oct 16, 2022

View reviewed changes

hasheddan requested a review from sttts October 17, 2022 16:07

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2022

openshift-merge-robot merged commit b993bd9 into kcp-dev:main Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Check for identityHash in APIExport admission and support multiple versions for APIs in permission claims #2169

✨ Check for identityHash in APIExport admission and support multiple versions for APIs in permission claims #2169

hasheddan commented Oct 11, 2022

hasheddan Oct 11, 2022

hasheddan Oct 11, 2022

ncdc Oct 12, 2022

hasheddan Oct 12, 2022

ncdc Oct 12, 2022

hasheddan Oct 15, 2022

sttts Oct 16, 2022

hasheddan commented Oct 11, 2022

ncdc Oct 11, 2022

ncdc Oct 12, 2022

ncdc Oct 12, 2022

ncdc Oct 12, 2022

hasheddan Oct 12, 2022

ncdc Oct 12, 2022

ncdc Oct 12, 2022

ncdc commented Oct 12, 2022

hasheddan commented Oct 13, 2022

sttts Oct 16, 2022

hasheddan Oct 16, 2022

ncdc Oct 17, 2022

hasheddan Oct 17, 2022

ncdc Oct 17, 2022

ncdc commented Oct 17, 2022

openshift-ci bot commented Oct 17, 2022

	func NewAPIExportAdmission(isInternal func(apisv1alpha1.GroupResource) bool) *APIExportAdmission {
	func NewAPIExportAdmission(isBuiltIn func(apisv1alpha1.GroupResource) bool) *APIExportAdmission {

✨ Check for identityHash in APIExport admission and support multiple versions for APIs in permission claims #2169

✨ Check for identityHash in APIExport admission and support multiple versions for APIs in permission claims #2169

Conversation

hasheddan commented Oct 11, 2022

Summary

Related issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Oct 12, 2022

hasheddan commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Oct 17, 2022

openshift-ci bot commented Oct 17, 2022