kcp-dev / contrib-tmc

An experimental add-on readding some Kubernetes compute APIs and impement transparent multi-cluster scheduling
Apache License 2.0
5 stars 3 forks source link

RESTMapper to resolve unqualified resource names (e.g. ingress) to fully qualified ones #131

Open kasturinarra opened 2 years ago

kasturinarra commented 2 years ago

Is your feature request related to a problem? Please describe. With addition of --resources=ingress to --args in syncer.yaml leads to workloadcluster deployment not being True and syncer logs report errors as below E0513 15:47:25.098968 1 syncer.go:127] Failed to retrieve GVRs from kcp: The following resource types were requested to be synced, but were not found in the KCP logical cluster: [ingress]

Describe the solution you'd like Would like RESTMapper to resolve unqualified resource names to qualified ones .

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

mra-ruiz commented 1 year ago

Hi, I'd like to try working on this issue. Is this still relevant?

ncdc commented 1 year ago

Hi @mra-ruiz I think so. Although at this week's community call, we discussed kcp-dev/kcp#2718, which is semi-related. You might want to look at them together. And/or possibly chat with @davidfestal and @jmprusi. Thanks!

davidfestal commented 1 year ago

In fact I tend to think that this issue is somewhat obsolete. kcp-dev/kcp#2718 is the right one indeed.

Now the workload sync CLI command produces some YAML that includes RBAC rules for additional resources to be applied in the physical cluster. And these RBAC rules should use the full GR. So it seems that the place where we should use the RestMapper would be inside the workload sync command, in order to grab the complete GroupResource to add in the RBAC rules of the generated YAML.

However since the workload sync command will not use the discovery of the target physical cluster to feed the RestMapper, I wonder if this all really makes sense.

Should we simply require the whole resource.group data in the whole flow, and thus remove the use of the RestMapper in the APIImporter ?

cc @ncdc

ncdc commented 1 year ago

@davidfestal I'm not fully up to speed on the full details - what is the exact issue?

davidfestal commented 1 year ago

If you pass route in the --resources arg of the workload sync command, the routes.route.openshift.io will effectively be imported by the Syncer APIImporter, because this component uses a RestMapper, and will have mapped the route partial singular name to the complete routes.route.openshift.io name.

However the Syncer will not be able to sync, since the Syncer service account will not have the right permissions, on the downstream cluster, for routes.route.openshift.io. The reason for this is that the worklod sync command will create RBAC (in the generated yaml) with the short name route and not with the full GR-based name. Which is why you'll end up with a SyncerAuthorized condition to false on the SyncTarget as detailed in https://github.com/kcp-dev/kcp/issues/2718

ncdc commented 1 year ago

Thanks. It sounds like a possible solution is to have the workload sync command use a RESTMapper against kcp. (I think it's ok that it's kcp and not the workload cluster ... right?)

davidfestal commented 1 year ago

Well, how is it going to work for routes for example, or any other resources that, at the time we run the workload sync command, isn't known on the KCP side ? It's precisely the goal of the --resources argument to allow importing additional (initially unknown to KCP) resources from the physical cluster to the KCP workspace.

ncdc commented 1 year ago

Ah, I was thinking they'd already be in kcp 😄. So, short-term, it's probably easiest to require fully qualified resource+group.

mjudeikis commented 1 year ago

/transfer-issue contrib-tmc