Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtual/apiexport: serve wildcard apibindings #1563

Merged

Conversation

sttts
Copy link
Member

@sttts sttts commented Jul 19, 2022

Virtual workspaces should make the defining objects of workspaces visible. For APIExports, this is the APIBindings, for initializing workspace this would be the Workspace.

This PR adds the APIBindings (both wildcard requests and normal requests) to the APIExport VW.

It does that by adding a "reflexive claim label" to APIBindings marking the binding visible for the corresponding APIExport. The APIExport VW then can filter via a "IN" label selector, matching either the reflexive label or a real claim label:

// ToReflexiveAPIBindingLabelKeyAndValue returns label key and value that is set (as fallback for filtering)
// on APIBindings that point to the given APIExport and the binding has not accepted a claim to it.
func ToReflexiveAPIBindingLabelKeyAndValue(exportClusterName logicalcluster.Name, exportName string) (string, string) {
  claimHash := toBase62([28]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6, 7})
  exportHash := toBase62(sha256.Sum224([]byte(exportClusterName.Join(exportName).String())))
  return apisv1alpha1.APIExportPermissionClaimLabelPrefix + exportHash, claimHash
}

This way the APIExport owners will always see "their own" bindings, but are able to claim access to all bindings, overriding the reflexive labels.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from aa75aae to 87b4206 Compare July 19, 2022 12:38
@openshift-ci openshift-ci bot requested review from ncdc and stevekuznetsov July 19, 2022 12:39
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 87b4206 to beadb4a Compare July 26, 2022 11:38
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from beadb4a to 85afb43 Compare July 27, 2022 20:55
@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 27, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 85afb43 to a0911ae Compare July 28, 2022 21:15
@sttts sttts changed the title WIP: virtual/apiexport: serve wildcard apibindings virtual/apiexport: serve wildcard apibindings Jul 28, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch 5 times, most recently from 088814c to 1adfa3f Compare August 2, 2022 17:46
@sttts
Copy link
Member Author

sttts commented Aug 2, 2022

This PR is getting interesting (and it is just an example of more to come):

We want to make APIBindings available to the owner of the corresponding APIExport. But the owner can also claim permission on APIBindings. The later shows a superset of bindings (all, not only those belonging to the APIExport addressed by the VW).

So the challenging bit: label selectors do not allow disjunction. So we cannot say "show me all bindings for the current export, plus those that are claimed". At the same time, we cannot easily do disjunction on the storage layer in the VW. So this is tricky.

What I could imagine: now we set the claim label to:

func ToLabelKeyAndValue(permissionClaim apisv1alpha1.PermissionClaim) (string, string, error) {
	bytes, err := json.Marshal(permissionClaim)
	if err != nil {
		return "", "", err
	}
	hash := fmt.Sprintf("%x", sha256.Sum224(bytes))
	labelKeyHashLength := validation.LabelValueMaxLength - len(apisv1alpha1.APIExportPermissionClaimLabelPrefix)
	return apisv1alpha1.APIExportPermissionClaimLabelPrefix + hash[0:labelKeyHashLength], hash, nil
}

We could change that a little to

claimed.internal.apis.kcp.dev/<hash(root:org:ws:export-name)>: <hash(claim)>

With that we can express what the APIExport VW needs: claimed.internal.apis.kcp.dev/<hash(root:org:ws:export-name)> IN { <hash(claim)>, some-constant }

Then admission can either set the claim hash, if there is one, or the constant if there isn't and it's a binding against the APIExport.

@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 1adfa3f to 2d0514a Compare August 3, 2022 12:29
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 2d0514a to 8d2ed6a Compare August 15, 2022 14:08
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2022
Copy link

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have some questions but overall makes sense and looks good

I wonder if a comment explaining what API export/build BuildVirtualWorkspace is doing and what someone should expect in the return may be valuable?

@shawn-hurley
Copy link

Another question, around the options, is there ever a time when someone would want the APIExport VW but not have the bindings?

@sttts
Copy link
Member Author

sttts commented Aug 16, 2022

Another question, around the options, is there ever a time when someone would want the APIExport VW but not have the bindings?

No. With this PR the APIBinding is part of the virtual workspace API contract. You cannot specify and should ne depend on what you cannot see in a VW.

@sttts
Copy link
Member Author

sttts commented Aug 17, 2022

@shawn-hurley has a point with #1563 (comment).

/hold

Will turn into a hash after vacation.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from bb710ea to 8db6aac Compare September 1, 2022 11:48
@sttts
Copy link
Member Author

sttts commented Sep 1, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 8db6aac to a89c119 Compare September 1, 2022 11:56
@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 9dd8890 to 802d071 Compare September 1, 2022 12:36
Copy link
Member

@ncdc ncdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submitting what i have so far so you don't have to wait for them all at the end. will continue reviewing.

// Verify the labels
value, found := apiBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey]
if apiBinding.Spec.Reference.Workspace == nil && found {
return admission.NewForbidden(a, fmt.Errorf("metadata.labels.%s must not be set", apisv1alpha1.InternalAPIBindingExportLabelKey))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the field helpers (e.g. field.NewInvalid(field.NewPath - whatever is most appropriate here)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

logicalcluster.New(apiBinding.Spec.Reference.Workspace.Path),
apiBinding.Spec.Reference.Workspace.ExportName,
); value != expected {
return admission.NewForbidden(a, fmt.Errorf("metadata.labels.%s must be set to %q", apisv1alpha1.InternalAPIBindingExportLabelKey, expected))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the field helpers (e.g. field.NewInvalid(field.NewPath - whatever is most appropriate here)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sttts sttts force-pushed the sttts-virtual-apiexport-apibindings branch from 6854549 to 82188c9 Compare September 2, 2022 08:32
@sttts
Copy link
Member Author

sttts commented Sep 2, 2022

Linker killed 🤷‍♂️

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 2, 2022

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022
@openshift-merge-robot openshift-merge-robot merged commit 5c82b22 into kcp-dev:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants