🌱 Replicating SyncTarget-related rbac and logicalclusters objects #2933

davidfestal · 2023-03-29T16:09:16Z

Summary

The SyncTarget-related cluster roles and cluster role bindings, as well as logical clusters, should be replicated in order to have the Syncer virtual workspace work correctly.

Related issue(s)

This is a split of PR #2675

pkg/reconciler/workload/replicateclusterrole/replicateclusterrole_controller.go

davidfestal · 2023-03-29T18:55:42Z

/test e2e-sharded

tmc/pkg/server/controllers.go

test/e2e/reconciler/cache/replication_test.go

davidfestal · 2023-03-30T15:06:40Z

Just remove the api-changed label since the changes initially done to the API have been removed, since they were finally unnecessary.

davidfestal · 2023-03-30T15:36:23Z

/test e2e-shared

p0lyn0mial

just 2 nits, lgtm other than that, thx!

test/e2e/reconciler/cache/replication_test.go

p0lyn0mial · 2023-03-31T11:44:02Z

test/e2e/reconciler/cache/replication_test.go

@@ -678,14 +683,23 @@ func (b *replicateResourceScenario) verifyResourceReplicationHelper(ctx context.
 		}
 		unstructured.RemoveNestedField(originalResource.Object, "metadata", "resourceVersion")
 		unstructured.RemoveNestedField(cachedResource.Object, "metadata", "resourceVersion")
+
+		// TODO(davidfestal): find out why the generation is not equal, specially for rbacv1.


OK. I think I found it: kcp-dev/kubernetes#132

Once this PR merges we need to set the field to true in

kcp/pkg/cache/server/config.go

Line 203 in 0d6a2ae

ExtraConfig: apiextensionsapiserver.ExtraConfig{

I think we could wait until the PR merges and test it in your PR, wdyt?

sttts · 2023-04-03T11:19:26Z

pkg/reconciler/workload/replicateclusterrole/replicateclusterrole_controller.go

+		}
+		resources := sets.NewString(rule.Resources...)
+		verbs := sets.NewString(rule.Verbs...)
+		if resources.Has("synctargets") && verbs.Has("sync") && len(rule.ResourceNames) == 1 {


len(rule.ResourceNames) == 1 is logically wrong. Multiple resource names are possible and a user could use those for custom authorization of a syncer.

would > 0 be OK ?
I can also of course completely remove this condition (which was the case initially)

But considering the case when a single service account would grant access to the VWs for all the SyncTargets seems strange to me.

The RBAC semantics is well defined. If RBAC authorizes syncing, you must replicate the role. Don't let this logic be influenced by how the cli command sets things up.

I removed this, and also now take in account the * in the rules, as done for the APIExport-related rbac objects.

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2023-04-03T16:45:36Z

/test e2e-shared

davidfestal · 2023-04-03T17:23:01Z

/test e2e-shared

p0lyn0mial · 2023-04-04T06:28:13Z

pkg/reconciler/workload/replicateclusterrole/replicateclusterrole_controller.go

+		}
+		resources := sets.NewString(rule.Resources...)
+		verbs := sets.NewString(rule.Verbs...)
+		if (resources.Has("synctargets") || resources.Has("*")) && (verbs.Has("sync") || verbs.Has("*")) {


shouldn't we match on Verbs: []string{"sync"}, Resources: []string{"synctargets"} only?

Well, in fact I came back to what @sttts had done for APIExports (quite similar use-case): https://github.com/kcp-dev/kcp/blob/main/pkg/reconciler/apis/replicateclusterrole/replicateclusterrole_controller.go#L58

That mainly covers all the CRs that would allow the sync verb for synctargets resources

OK, thanks for explaining.

p0lyn0mial · 2023-04-04T06:28:44Z

pkg/reconciler/workload/replicateclusterrole/replicateclusterrole_controller.go

+func HasSyncRule(clusterName logicalcluster.Name, cr *rbacv1.ClusterRole) bool {
+	for _, rule := range cr.Rules {
+		apiGroups := sets.NewString(rule.APIGroups...)
+		if !apiGroups.Has(workload.GroupName) && !apiGroups.Has("*") {


shouldn't we match on APIGroups: []string{"workload.kcp.io"} only?

Well, in fact I came back to what @sttts had done for APIExports (quite similar use-case): https://github.com/kcp-dev/kcp/blob/main/pkg/reconciler/apis/replicateclusterrole/replicateclusterrole_controller.go#L58

That mainly covers all the CRs that would allow the sync verb for synctargets resources

p0lyn0mial · 2023-04-04T08:36:27Z

/lgtm

davidfestal · 2023-04-04T08:38:02Z

/approve

openshift-ci · 2023-04-04T08:38:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidfestal

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~~ [davidfestal]

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

davidfestal requested a review from sttts March 29, 2023 16:09

openshift-ci bot requested review from csams and jmprusi March 29, 2023 16:09

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

stevekuznetsov reviewed Mar 29, 2023

View reviewed changes

pkg/reconciler/workload/replicateclusterrole/replicateclusterrole_controller.go Outdated Show resolved Hide resolved

davidfestal force-pushed the replicate-synctarget-related-rbac branch 2 times, most recently from a80fec5 to 1e6ee77 Compare March 29, 2023 18:43

davidfestal force-pushed the replicate-synctarget-related-rbac branch from 1e6ee77 to cf8a625 Compare March 30, 2023 10:51

davidfestal requested a review from p0lyn0mial March 30, 2023 12:20

davidfestal force-pushed the replicate-synctarget-related-rbac branch 2 times, most recently from 49b5281 to bcc511f Compare March 30, 2023 12:55

p0lyn0mial reviewed Mar 30, 2023

View reviewed changes

tmc/pkg/server/controllers.go Show resolved Hide resolved

test/e2e/reconciler/cache/replication_test.go Outdated Show resolved Hide resolved

davidfestal mentioned this pull request Mar 30, 2023

Unify workload[s] names in the tmc controller.go #2936

Closed

davidfestal force-pushed the replicate-synctarget-related-rbac branch from bcc511f to ae8c468 Compare March 30, 2023 15:04

davidfestal removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 30, 2023

p0lyn0mial reviewed Mar 31, 2023

View reviewed changes

test/e2e/reconciler/cache/replication_test.go Outdated Show resolved Hide resolved

test/e2e/reconciler/cache/replication_test.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Mar 31, 2023

View reviewed changes

davidfestal mentioned this pull request Apr 3, 2023

UPSTREAM: <carry>: apiextensions-apiserver allow for disabling generation repair kcp-dev/kubernetes#132

Open

sttts reviewed Apr 3, 2023

View reviewed changes

davidfestal added 3 commits April 3, 2023 18:34

Replicating SyncTarget-related rbac objects

e69e7c4

Signed-off-by: David Festal <dfestal@redhat.com>

Replication: Add e2e tests for workloads-related rbac objects

5e15fe7

Signed-off-by: David Festal <dfestal@redhat.com>

Replicate logicalclusters containing synctargets

63e6eb5

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal force-pushed the replicate-synctarget-related-rbac branch from ae8c468 to 63e6eb5 Compare April 3, 2023 16:34

davidfestal changed the title ~~🌱 Replicating SyncTarget-related rbac objects~~ 🌱 Replicating SyncTarget-related rbac and logicalclusters objects Apr 3, 2023

p0lyn0mial reviewed Apr 4, 2023

View reviewed changes

openshift-ci bot assigned p0lyn0mial Apr 4, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2023

openshift-merge-robot merged commit e65e243 into kcp-dev:main Apr 4, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Replicating SyncTarget-related rbac and logicalclusters objects #2933

🌱 Replicating SyncTarget-related rbac and logicalclusters objects #2933

davidfestal commented Mar 29, 2023 •

edited

davidfestal commented Mar 29, 2023

davidfestal commented Mar 30, 2023

davidfestal commented Mar 30, 2023

p0lyn0mial left a comment

p0lyn0mial Mar 31, 2023

p0lyn0mial Mar 31, 2023

sttts Apr 3, 2023

davidfestal Apr 3, 2023

davidfestal Apr 3, 2023

sttts Apr 3, 2023

davidfestal Apr 3, 2023 •

edited

davidfestal commented Apr 3, 2023

davidfestal commented Apr 3, 2023

p0lyn0mial Apr 4, 2023

davidfestal Apr 4, 2023

p0lyn0mial Apr 4, 2023

p0lyn0mial Apr 4, 2023

davidfestal Apr 4, 2023 •

edited

p0lyn0mial commented Apr 4, 2023

davidfestal commented Apr 4, 2023

openshift-ci bot commented Apr 4, 2023

🌱 Replicating SyncTarget-related rbac and logicalclusters objects #2933

🌱 Replicating SyncTarget-related rbac and logicalclusters objects #2933

Conversation

davidfestal commented Mar 29, 2023 • edited

Summary

Related issue(s)

davidfestal commented Mar 29, 2023

davidfestal commented Mar 30, 2023

davidfestal commented Mar 30, 2023

p0lyn0mial left a 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

Choose a reason for hiding this comment

davidfestal Apr 3, 2023 • edited

Choose a reason for hiding this comment

davidfestal commented Apr 3, 2023

davidfestal commented Apr 3, 2023

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

davidfestal Apr 4, 2023 • edited

Choose a reason for hiding this comment

p0lyn0mial commented Apr 4, 2023

davidfestal commented Apr 4, 2023

openshift-ci bot commented Apr 4, 2023

davidfestal commented Mar 29, 2023 •

edited

davidfestal Apr 3, 2023 •

edited

davidfestal Apr 4, 2023 •

edited