Add CLI command for binding a catalog entry to a workspace #6

dinhxuanvu · 2022-09-13T19:26:57Z

The kcp-catalog bind command will bind a catalog entry to a workspace
by creating a singular or multiple APIBinding using ExportReference
listed in spec.Exports in the catalog entry.

For example, consider the command:
kcp-catalog bind catalogentry root:catalog:catalogentry-sample

Every export referenced in catalog entry-sample in root:catalog workspace will have an APIBinding created referencing it.

Design doc: https://docs.google.com/document/d/1J31wXY1-2aCyyGFjUlusKoHDzV-zktAmRdIMjMf4OR0/edit

cmd/kcp-catalog/bind/catalogentry/cmd.go

cmd/kcp-catalog/bind/catalogentry/bind.go

ncdc · 2022-11-09T22:16:45Z

cmd/kcp-catalog/bind/catalogentry/bind.go

+
+func bindReady(bindings []apisv1alpha1.APIBinding) bool {
+	for _, binding := range bindings {
+		if binding.Status.Phase != apisv1alpha1.APIBindingPhaseBound {


(no action required) I would probably check for the InitialBindingCompleted or BindingUpToDate condition, but this works too

cmd/kcp-catalog/bind/catalogentry/cmd.go

go.mod

cmd/kcp-catalog/bind/catalogentry/bind.go

ncdc · 2022-11-14T14:00:57Z

cmd/kcp-catalog/bind/catalogentry/bind.go

+	for _, binding := range apiBindings {
+		err := kcpClient.Create(ctx, &binding)
+		if err != nil {
+			// If an APIBinding already exists, intentionally not updating it since we would not like reset AcceptablePermissionClaims.


Now that you're using a partially random name, you won't get an "already exists" error. The binding creation should succeed, but when the controllers reconcile it, they may find conflicts (you'll see them in status.conditions).

I wonder if we should check the existing APIBindings in the workspace and see if any of them carry the same ExportReferences. If that is the case, then simply not creating a new APIBinding for that particular ExportReference.

@dinhxuanvu I intentionally didn't add that check because the permission claims in the already created binding might be different. I'm not sure if just updating permission claims of an existing claim is something which we should do. There are chances wherein the permission claims in exports ref could be different than the one existing in apibinidng present in cluster. For now, I'm just overwriting the permission claims in the existing apibinding and doing an update. Not sure if that is the approach we want. cc: @ncdc

I think given the fact the new APIBinding will fail due to what Andy said early, it is not a good UX to create a binding that won't even do anything but getting into a failed state. If the existing binding is missing some permissions, that can be resolved without having to delete the extra binding that we shouldn't create in the first place. Either way, would like to hear what Andy thinks about this.

I would recommend something like this:

$ k catalog bind catalogentry root:catalog:awesomeness Binding for "awesomeness" already exists but permission claims are different: <show differences> Rerun with --update to update the existing binding's permission claims.

But that can be in a follow-up.

cmd/kcp-catalog/bind/catalogentry/bind.go

ncdc · 2022-12-01T18:16:05Z

cmd/kcp-catalog/bind/catalogentry/bind.go

+	for _, binding := range apiBindings {
+		err := kcpClient.Create(ctx, &binding)
+		if err != nil {
+			// If an APIBinding already exists, intentionally not updating it since we would not like reset AcceptablePermissionClaims.


I would recommend something like this:

$ k catalog bind catalogentry root:catalog:awesomeness Binding for "awesomeness" already exists but permission claims are different: <show differences> Rerun with --update to update the existing binding's permission claims.

But that can be in a follow-up.

cmd/kcp-catalog/bind/catalogentry/bind.go

ncdc · 2022-12-05T19:12:01Z

@varshaprasad96 got this error with your PR:

$ make
test -s /Users/angoldst/code/kcp/catalog/bin/controller-gen || GOBIN=/Users/angoldst/code/kcp/catalog/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0
/Users/angoldst/code/kcp/catalog/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
Error: err: exit status 1: stderr: go: downloading github.com/kcp-dev/controller-runtime v0.12.2-0.20220808200255-4b60fd66e5de
go: downloading github.com/kcp-dev/kcp v0.9.1
go: downloading golang.org/x/tools v0.1.10
go build github.com/kcp-dev/kcp/pkg/openapi: build constraints exclude all Go files in /Users/angoldst/go/1.18.6/pkg/mod/github.com/kcp-dev/kcp@v0.9.1/pkg/openapi

varshaprasad96 · 2022-12-06T20:03:26Z

@ncdc Adding snippets of the command after creating it locally:

When catalog entry is created successfully:

➜  catalog git:(bind-cli)  ./kcp-catalog bind catalogentry root:catalog:catalogentry-sample
Apibinding created and bound to catalog entry catalogentry-sample.

When binding already exists on cluster:

➜  catalog git:(bind-cli)  ./kcp-catalog bind catalogentry root:catalog:catalogentry-sample
Found an existing APIExport cert-manager-stable-dx467 pointing to the same export reference.
Apibinding created and bound to catalog entry catalogentry-sample.

When there is an error while creating a binding (intentionally):

➜  catalog git:(bind-cli)  ./kcp-catalog bind catalogentry root:catalog:catalogentry-sample
error: bindings for catalog entry catalogentry-sample could not be created successfully: resource name may not be empty

I have a follow up PR that have tests for these. After #5 gets merged, will open it.

ncdc · 2022-12-06T22:58:13Z

/test

openshift-ci · 2022-12-06T22:58:16Z

@ncdc: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test build
/test test

Use /test all to run all jobs.

Details

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ncdc · 2022-12-06T23:08:41Z

/test all

varshaprasad96 · 2022-12-07T08:18:33Z

go build github.com/kcp-dev/kcp/pkg/openapi: build constraints exclude all Go files in /Users/angoldst/go/1.18.6/pkg/mod/github.com/kcp-dev/kcp@v0.9.1/pkg/openapi

The error above and in CI was because controller-gen was not accepting files under cmd/ which had deps with build dependencies like (openapi). I have modified the controller-gen args in the makefile target to accept what we want. Now the following works - make test, make build and make install:

➜  catalog git:(bind-cli) make test
test -s /Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen || GOBIN=/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0
/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./controllers/...;./api/..." output:crd:artifacts:config=config/crd/bases
/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./api/..."
go fmt ./...
go vet ./...
test -s /Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/setup-envtest || GOBIN=/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
KUBEBUILDER_ASSETS="/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/k8s/1.25.0-darwin-amd64" go test ./... -coverprofile cover.out
?   	github.com/kcp-dev/catalog	[no test files]
?   	github.com/kcp-dev/catalog/api/v1alpha1	[no test files]
?   	github.com/kcp-dev/catalog/cmd/kcp-catalog	[no test files]
?   	github.com/kcp-dev/catalog/cmd/kcp-catalog/bind/catalogentry	[no test files]
ok  	github.com/kcp-dev/catalog/controllers	0.408s	coverage: 0.0% of statements

➜  catalog git:(bind-cli)  make install
test -s /Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen || GOBIN=/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0
/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./controllers/...;./api/..." output:crd:artifacts:config=config/crd/bases
test -s /Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/kustomize || { curl -Ss "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s -- 3.8.7 /Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin; }
/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/kustomize build config/crd | kubectl apply -f -
customresourcedefinition.apiextensions.k8s.io/catalogentries.catalog.kcp.dev created

➜  catalog git:(bind-cli) make build
test -s /Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen || GOBIN=/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0
/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./controllers/...;./api/..." output:crd:artifacts:config=config/crd/bases
/Users/vnarsing/go/src/github.com/kcp-dev/catalog/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./api/..."
go fmt ./...
go vet ./...
go build -o bin/manager main.go

ncdc · 2022-12-07T14:03:01Z

@varshaprasad96 great, thanks! Would you be able to squash the commits down (e.g. to get rid of ones named wip 😄)?

The kcp-catalog bind command will bind a catalog entry to a workspace by creating a singular or multiple APIBinding using ExportReference listed in spec.Exports in the catalog entry. For example, consider the command: kcp-catalog bind catalogentry root:catalog:catalogentry-sample

varshaprasad96 · 2022-12-08T07:09:43Z

@ncdc need a lgtm again to continue merging.

openshift-ci · 2022-12-08T14:03:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, ncdc

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dinhxuanvu,ncdc]

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

dinhxuanvu changed the title ~~Add CLI command for binding a catalog entry to a workspace~~ [WIP] Add CLI command for binding a catalog entry to a workspace Sep 13, 2022

dinhxuanvu force-pushed the bind-cli branch from f4bbb00 to 1e58855 Compare September 28, 2022 15:07

varshaprasad96 force-pushed the bind-cli branch from 2a3c049 to e6615c7 Compare September 30, 2022 00:19

varshaprasad96 changed the title ~~[WIP] Add CLI command for binding a catalog entry to a workspace~~ Add CLI command for binding a catalog entry to a workspace Oct 7, 2022

varshaprasad96 mentioned this pull request Oct 11, 2022

API Catalog #7

Open

5 tasks

ncdc suggested changes Oct 18, 2022

View reviewed changes

varshaprasad96 mentioned this pull request Oct 19, 2022

[WIP] Add command to list catalog entries and the APIs exposed #8

Open

varshaprasad96 force-pushed the bind-cli branch from 3079485 to d5a8f6a Compare October 31, 2022 23:44

varshaprasad96 requested a review from ncdc October 31, 2022 23:46

ncdc suggested changes Nov 9, 2022

View reviewed changes

varshaprasad96 force-pushed the bind-cli branch 3 times, most recently from cde1c4f to 828688e Compare November 11, 2022 19:45

ncdc reviewed Nov 14, 2022

View reviewed changes

cmd/kcp-catalog/bind/catalogentry/bind.go Outdated Show resolved Hide resolved

ncdc reviewed Nov 14, 2022

View reviewed changes

varshaprasad96 force-pushed the bind-cli branch from 828688e to 4d857df Compare November 14, 2022 14:08

varshaprasad96 requested a review from ncdc December 1, 2022 17:32

ncdc reviewed Dec 1, 2022

View reviewed changes

cmd/kcp-catalog/bind/catalogentry/bind.go Outdated Show resolved Hide resolved

cmd/kcp-catalog/bind/catalogentry/bind.go Outdated Show resolved Hide resolved

varshaprasad96 force-pushed the bind-cli branch from dd95891 to 47e223b Compare December 5, 2022 14:15

varshaprasad96 requested a review from ncdc December 5, 2022 14:16

ncdc reviewed Dec 5, 2022

View reviewed changes

cmd/kcp-catalog/bind/catalogentry/bind.go Outdated Show resolved Hide resolved

ncdc reviewed Dec 5, 2022

View reviewed changes

cmd/kcp-catalog/bind/catalogentry/bind.go Outdated Show resolved Hide resolved

ncdc reviewed Dec 5, 2022

View reviewed changes

cmd/kcp-catalog/bind/catalogentry/bind.go Show resolved Hide resolved

varshaprasad96 force-pushed the bind-cli branch 2 times, most recently from 48be110 to a09ffc2 Compare December 5, 2022 16:02

varshaprasad96 requested a review from ncdc December 5, 2022 18:32

ncdc approved these changes Dec 5, 2022

View reviewed changes

varshaprasad96 force-pushed the bind-cli branch from a09ffc2 to dbfa3b6 Compare December 6, 2022 20:03

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

varshaprasad96 force-pushed the bind-cli branch from 3723960 to d0ff6ee Compare December 7, 2022 14:46

ncdc approved these changes Dec 8, 2022

View reviewed changes

openshift-ci bot assigned ncdc Dec 8, 2022

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

openshift-merge-robot merged commit ceccbbb into kcp-dev:main Dec 8, 2022

Add CLI command for binding a catalog entry to a workspace #6

Add CLI command for binding a catalog entry to a workspace #6

Uh oh!

Conversation

dinhxuanvu commented Sep 13, 2022 • edited by varshaprasad96 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

dinhxuanvu Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

varshaprasad96 Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dinhxuanvu Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

ncdc Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ncdc Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varshaprasad96 commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncdc commented Dec 6, 2022

Uh oh!

openshift-ci bot commented Dec 6, 2022

Uh oh!

ncdc commented Dec 6, 2022

Uh oh!

varshaprasad96 commented Dec 7, 2022

Uh oh!

ncdc commented Dec 7, 2022

Uh oh!

varshaprasad96 commented Dec 8, 2022

Uh oh!

openshift-ci bot commented Dec 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

dinhxuanvu commented Sep 13, 2022 •

edited by varshaprasad96

Loading

varshaprasad96 Nov 16, 2022 •

edited

Loading

ncdc commented Dec 5, 2022 •

edited

Loading

varshaprasad96 commented Dec 6, 2022 •

edited

Loading