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.

Sign up for GitHub

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

Jump to bottom

✨ wire more controllers cross-shard and authz #2562

Merged

openshift-merge-robot merged 18 commits into kcp-dev:main from sttts:sttts-global-authz

Jan 11, 2023

Member

sttts commented Jan 6, 2023

replicate ClusterRoles+ClusterBindings that are labeled to be replicated
added replication label controllers for ClusterRoles+Bindings relevant to APIExport authorization
allow deep SAR again the cache server data

openshift-ci bot requested review from davidfestal and s-urbaniak

January 6, 2023 17:24

sttts force-pushed the sttts-global-authz branch from 3a213b7 to aa249d5 Compare

January 6, 2023 17:31

sttts changed the title ~~✨ wire cross-shard authorization~~ ✨ Wire cross-shard authorization

sttts changed the title ~~✨ Wire cross-shard authorization~~ WIP: ✨ Wire cross-shard authorization

openshift-ci bot added the do-not-merge/work-in-progress label

p0lyn0mial reviewed

View reviewed changes

pkg/reconciler/cache/replication/replication_controller.go

+              		globalAPIResourceSchemaIndexer:  globalKcpInformers.Apis().V1alpha1().APIResourceSchemas().Informer().GetIndexer(),
+              		globalShardIndexer:              globalKcpInformers.Core().V1alpha1().Shards().Informer().GetIndexer(),
+              		globalWorkspaceTypeIndexer:      globalKcpInformers.Tenancy().V1alpha1().WorkspaceTypes().Informer().GetIndexer(),
+              		globalClusterRoleIndexer:        globalKubeInformers.Rbac().V1().ClusterRoles().Informer().GetIndexer(),

Contributor

p0lyn0mial Jan 9, 2023

why do we need to replicate cr and crb ?

Member Author

sttts Jan 10, 2023

As explained in gmeet: these are required to authorize binding of APIExports, and for maximum permission policy, for now. There will be more reasons for other cross-workspace authorizations.

sttts force-pushed the sttts-global-authz branch from aa249d5 to 04a096b Compare

January 9, 2023 08:15

openshift-ci bot added the kind/api-change label

sttts force-pushed the sttts-global-authz branch 4 times, most recently from 65c6de3 to 3759c6f Compare

January 9, 2023 11:38

sttts changed the title ~~WIP: ✨ Wire cross-shard authorization~~ WIP ✨ wire more controllers cross-shard and authz

openshift-merge-robot added the needs-rebase label

sttts force-pushed the sttts-global-authz branch from 0011bf0 to bb9fcfc Compare

January 10, 2023 09:05

openshift-merge-robot removed the needs-rebase label

p0lyn0mial reviewed

View reviewed changes

pkg/reconciler/apis/replicationclusterrolebinding/replicationclusterrolebinding_controller.go Outdated

		@@ -0,0 +1,254 @@
		/*
		Copyright 2022 The KCP Authors.

Contributor

p0lyn0mial Jan 10, 2023

its 2023

Member Author

sttts Jan 10, 2023

done

Contributor

p0lyn0mial Jan 10, 2023

applies to other places as well

pkg/reconciler/apis/replicationclusterrolebinding/replicationclusterrolebinding_controller.go Outdated

+              )
+              const (
+              	ControllerName = "kcp-apiexport-replication-clusterrolebinding"

Contributor

p0lyn0mial Jan 10, 2023

apiexport ?

Member Author

sttts Jan 10, 2023

renamed to kcp-apis-replication-clusterrolebinding

Contributor

p0lyn0mial Jan 10, 2023

applies to the other controller as well

pkg/reconciler/apis/replicationclusterrolebinding/replicationclusterrolebinding_controller.go Outdated

+              	cluster, _, name, err := kcpcache.SplitMetaClusterNamespaceKey(key)
+              	if err != nil {
+              		runtime.HandleError(err)
+              		return nil

Contributor

p0lyn0mial Jan 10, 2023

why not to return the err?

Member Author

sttts Jan 10, 2023

then it would be requeued. Doesn't help here to fix the problem.

pkg/reconciler/apis/replicationclusterrolebinding/replicationclusterrolebinding_controller.go Outdated

+              	return true
+              }
+              func (c *controller) process(ctx context.Context, key string) error {

Contributor

p0lyn0mial Jan 10, 2023

change the signature of this method to ctx context.Context, crb *rbacv1.ClusterRoleBinding, rename to reconcile and move it to a separate file - it would unify with the rest of the controllers and allow for easier unit testing

Member Author

sttts Jan 10, 2023

done

pkg/reconciler/apis/replicationclusterrolebinding/replicationclusterrolebinding_controller.go Outdated

+              	}
+              	logger.V(4).Info("patching ClusterRoleBinding", "patch", patch)
+              	_, err = c.kubeClusterClient.Cluster(cluster.Path()).RbacV1().ClusterRoleBindings().Patch(ctx, crb.Name, types.MergePatchType, []byte(patch), metav1.PatchOptions{})

Contributor

p0lyn0mial Jan 10, 2023

wondering if we could use committer.NewCommitter

Member Author

sttts Jan 10, 2023

no, I can't. But I wrote one I can use. Stay tuned :)

p0lyn0mial reviewed

View reviewed changes

pkg/apis/core/types.go

@@ @@ -26,6 +26,11 @@ const ( @@
               	//
               	// If this annotation exists, the system will maintain the annotation value.
               	LogicalClusterPathAnnotationKey = "kcp.io/path"
+              	// ReplicateAnnotationKey is the annotation key used to indicate that a ClusterRole should be replicated.

Contributor

p0lyn0mial Jan 10, 2023

could you please remind me why it is a list?

Member Author

sttts Jan 10, 2023

there can be multiple controllers that contribute to this annotation. E.g. from another Api group.

p0lyn0mial reviewed

View reviewed changes

pkg/reconciler/cache/replication/replication_reconcile.go Outdated

+              			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
+              				obj, err := c.localClusterRoleBindingLister.Cluster(cluster).Get(name)
+              				if err != nil {
+              					return obj, true, err

Contributor

p0lyn0mial Jan 10, 2023

I think this line causes the panic, replace with return nil, false, err

Member Author

sttts Jan 10, 2023

Isn't obj nil then?

Member Author

sttts Jan 10, 2023

done

p0lyn0mial reviewed

View reviewed changes

pkg/reconciler/cache/replication/replication_reconcile.go Outdated

+              			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
+              				obj, err := c.localClusterRoleLister.Cluster(cluster).Get(name)
+              				if err != nil {
+              					return obj, true, err

Contributor

p0lyn0mial Jan 10, 2023

same here (re panic), replace with return nil, false, err

Member Author

sttts Jan 10, 2023

done

sttts force-pushed the sttts-global-authz branch from bb9fcfc to d1f1dce Compare

January 10, 2023 13:56

openshift-merge-robot added the needs-rebase label

sttts force-pushed the sttts-global-authz branch from d1f1dce to a0b03ef Compare

January 10, 2023 13:58

openshift-merge-robot removed the needs-rebase label

sttts force-pushed the sttts-global-authz branch 2 times, most recently from 32dde53 to 5d8654c Compare

January 10, 2023 14:03

sttts changed the title ~~WIP ✨ wire more controllers cross-shard and authz~~ ✨ wire more controllers cross-shard and authz

openshift-ci bot removed the do-not-merge/work-in-progress label

sttts mentioned this pull request

Community Meeting Jan 10 2023 #2566

Closed

sttts and others added 18 commits

January 11, 2023 11:15


          FIX: server: wire cache informers instead of root informers

5000d80


          authorization/maxpermissionpolicy: find export in cache server

ac5eba6


          cache: replicate ClusterRoles+ClusterRoleBindings

e60f9ad

Co-authored-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>


          authorization: wire global informers

f10b47d

Co-authored-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>


          Bump k/k

eb7b257


          cache: create rbac objects as CRDs

a5dd0eb


          admission: add global informers to SetKcpInformers

c787e9a


          admission/workspacetypeexists: wire global workspace types


          cli: remove hack to hide 404 on workspaces

6ff714e


          admission/permissionclaims: wire global APIExports


          admission/webhooks: wire global APIExports

d8480e0


          reconciler/workspace/scheduler: wire global APIExports

696c662


          apis/core: replace replication label with internal.kcp.io/replicate a…

8b5d4e4

…nnotation in core


          Makefile: add clean target

52e5255


          Make 'make lint' happy

a215912


          reconciler/committer: add commit for status-less resources like Clust…

0ce4728

…erRoles


          Address review comments

8a25737


          cmd/apigen: don't create rbacv1 schemas and export

82b52e5

sttts force-pushed the sttts-global-authz branch from 5d8654c to 82b52e5 Compare

January 11, 2023 10:16

openshift-merge-robot removed the needs-rebase label

Contributor

p0lyn0mial commented Jan 11, 2023

/lgtm

openshift-ci bot assigned p0lyn0mial

openshift-ci bot added the lgtm label

Member Author

sttts commented Jan 11, 2023

/retest

Member Author

sttts commented Jan 11, 2023

/approve

Contributor

openshift-ci bot commented Jan 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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:

~~OWNERS~~ [sttts]
~~config/crds/OWNERS~~ [sttts]
~~pkg/admission/OWNERS~~ [sttts]
~~pkg/apis/OWNERS~~ [sttts]

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 added the approved label

openshift-merge-robot merged commit ca40e16 into kcp-dev:main

hardys mentioned this pull request

🐛 Fix Makefile clean target and add .kcp cleanup #2712

Merged

xrstf mentioned this pull request

Revisit "creating a workspace under a universal type workspace is not supported" error #1932

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment