search

kcp-dev / kcp

Kubernetes-like control planes for form-factors and use-cases beyond Kubernetes and container workloads.
https://kcp.io
Apache License 2.0
2.36k stars 382 forks source link

bug: Double identity decorations on URLs #2184

Closed mjudeikis closed 1 year ago

mjudeikis commented 2 years ago

Describe the bug

If you are binding resources from 3rd party APIs with identityHash, sometimes you end-up with double decorations on URL.

In example APIExport :

apiVersion: apis.kcp.dev/v1alpha1            
kind: APIExport                              
metadata:                                    
  annotations:                               
    kcp.dev/cluster: root:compute:controllers
  creationTimestamp: "2022-10-12T17:16:54Z"
  generation: 24                           
  name: faros.sh                           
spec:                                
  identity:                          
    secretRef:                       
      name: faros.sh                 
      namespace: kcp-system          
  latestResourceSchemas:             
  - today.requests.access.faros.sh   
  - today.agents.edge.faros.sh       
  - today.registrations.edge.faros.sh
  permissionClaims:          
  - group: access.faros.sh   
    resource: requests                                                            
  - group: edge.faros.sh                                                          
    resource: registrations                                                       
  - group: ""                                                                     
    resource: secrets                                                                                    
  - group: ""                                                                                            
    resource: serviceaccounts                                                                            
  - group: ""                                                                                                              
    resource: configmaps                                                                                                   
  - group: workload.kcp.dev                                                                                                
    identityHash: a2f09aebf5bc96033c19aaa1dbe2c1808542ad3ffd65b68e30fcdd3ae61c35cc                                         
    resource: synctargets

APIBinding:

apiVersion: apis.kcp.dev/v1alpha1
kind: APIBinding
metadata:
  name: faros.sh
spec:
  permissionClaims:
  - group: access.faros.sh
    resource: requests
    state: Accepted
  - group: edge.faros.sh
    resource: registrations
    state: Accepted
  - group: ""
    resource: serviceaccounts
    state: Accepted
  - group: ""
    resource: secrets
    state: Accepted
  - group: ""
    resource: configmaps
    state: Accepted
  - group: workload.kcp.dev
    resource: synctargets
    identityHash: ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d
    state: Accepted
  reference:
    workspace:
      exportName: faros.sh
      path: root:compute:controllers

And we have 2 requests from verbose level 8: THis one we can see singple "decorations with identity" Path:"/apis/edge.faros.sh/v1alpha1/namespaces/default/registrations:b833587a82b737a93c56b67771d0a987b671e64b762ac5ce6614d237d8f8bb5b

] startRequest(RequestDigest{RequestInfo: &request.RequestInfo{IsResourceRequest:true, Path:"/apis/edge.faros.sh/v1alpha1/namespaces/default/registrations:b833587a82b737a93c56b67771d0a987b671e64b762ac5ce6614d237d8f8bb5b", Verb:"list", APIPrefix:"apis", APIGroup:"edge.faros.sh", APIVersion:"v1alpha1", Namespace:"default", Resource:"registrations:b833587a82b737a93c56b67771d0a987b671e64b762ac5ce6614d237d8f8bb5b", Subresource:"
", Name:"", Parts:[]string{"registrations:b833587a82b737a93c56b67771d0a987b671e64b762ac5ce6614d237d8f8bb5b"}}, User: &user.DefaultInfo{Name:"system:apiserver", UID:"2fcd6aed-1c40-4a5d-963e-0ab665c7b579", Groups:[]string{"system:masters"}, Extra:map[string][]string(nil)}})

And second one we have double: /apis/workload.kcp.dev/v1alpha1/synctargets:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d

 startRequest(RequestDigest{RequestInfo: &request.RequestInfo{IsResourceRequest:true, Path:"/apis/workload.kcp.dev/v1alpha1/synctargets:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d", Verb:"list", APIPrefix:"apis", APIGroup:"workload.kcp.dev", APIVersion:"v1alpha1", Namespace:"", Resource:"synctargets:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d", Subresource:"", Name:"", Parts:[]string{"synctargets:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d:ffe94b78bc62522c08e39bf04b2b24b9f27c175fce8c3f0457e42ff5fbef3b1d"}}, User: &user.DefaultInfo{Name:"system:apiserver", UID:"2fcd6aed-1c40-4a5d-963e-0ab665c7b579", Groups:[]string{"system:masters"}, Extra:map[string][]string(nil)}})

This leads to 404 being returned from storage.

Existing decorations was here for a long time: https://github.com/kcp-dev/kcp/blob/bc50273a7a1fe2f192ccf4bb4c00120d45607e7d/pkg/server/bootstrap/identity.go#L285

And we added second decorator here: https://github.com/kcp-dev/kcp/blob/bc50273a7a1fe2f192ccf4bb4c00120d45607e7d/pkg/virtual/framework/forwardingregistry/store.go#L279

Fix is quite simple:

if !strings.Contains(comps[5],":"){
    comps[5] += ":" + id
}

But the fact we do this in 2 locations but not all code hits decorateWildcardPathsWithResourceIdentities and some hits boths. I didn't yet went all-in to find out how bussiness logic is constructed so this happens, but this small fix fixed beahviour for double decorations.

Steps To Reproduce

See description.

Expected Behaviour

No double decorations

Additional Context

https://gist.github.com/mjudeikis/39869eed948f6ab7278c0b5b7999d6a5

:exploding_head:

nrb commented 2 years ago

Can you provide client-side steps to reproduce please?

mjudeikis commented 2 years ago

Reproduced without any golang code:

  1. Set KCP loglevel to 12
  2. Start KCP
  3. Get identity hash for workloads api: 
    k get apiexport workload.kcp.dev -o yaml | grep identityHash
identityHash: 5bc5ce02d4ee55afecf01e551e3433bdb2545cfb832316626f28fbef0c2d75d8
  4. Get into workspace we gonna use for exporting. k kcp workspace use root:compute
  5. Create export for our API (https://gist.github.com/mjudeikis/4d7fb8710f71648497b761138daec371) 
    k create -f repro/export.yaml
  6. Create consumer workspace: 
    k kcp workspace use root
Current workspace is "root".
k kcp workspace create user --enter
  7. Create binding for consumer: 
    k create -f repro/apibinding.yaml (https://gist.github.com/mjudeikis/4d7fb8710f71648497b761138daec371)
  8. Get back to exporter workspace and check resources: 
    [mjudeikis@unknown kcp]$ k kcp workspace use root:compute
Current workspace is "root:compute".
[mjudeikis@unknown kcp]$ k get apiexport st-test -o yaml | grep virtual -A 2
virtualWorkspaces:
- url: https://192.168.1.138:6443/services/apiexport/root:compute/st-test

    Check resources:

    k -s  https://192.168.1.138:6443/services/apiexport/root:compute/st-test/clusters/* api-resources
NAME          SHORTNAMES   APIVERSION                  NAMESPACED   KIND
secrets                    v1                          true         Secret
synctargets                workload.kcp.dev/v1alpha1   false        SyncTarget

If you do:

k -s  https://192.168.1.138:6443/services/apiexport/root:compute/st-test/clusters/* get secrets -A
# some results
# and 
k -s  https://192.168.1.138:6443/services/apiexport/root:compute/st-test/clusters/* get synctargets
Error from server (NotFound): Unable to list "workload.kcp.dev/v1alpha1, Resource=synctargets": the server could not find the requested resource

And in the logs:

I1019 10:58:29.554639 2437476 apf_filter.go:176] Handle(RequestDigest{RequestInfo: &request.RequestInfo{IsResourceRequest:true, Path:"/apis/workload.kcp.dev/v1alpha1/synctargets:5bc5ce02d4ee55afecf01e551e3433bdb2545cfb832316626f28fbef0c2d75d8:5bc5ce02d4ee55afecf01e551e3433bdb2545cfb832316626f28fbef0c2d75d8", Verb:"list", APIPrefix:"apis", APIGroup:"workload.kcp.dev", APIVersion:"v1alpha1", Namespace:"", Resource:"synctargets", Subresource:"", Name:"", Parts:[]string{"synctargets:5bc5ce02d4ee55afecf01e551e3433bdb2545cfb832316626f28fbef0c2d75d8:5bc5ce02d4ee55afecf01e551e3433bdb2545cfb832316626f28fbef0c2d75d8"}}, User: &user.DefaultInfo{Name:"system:apiserver", UID:"7ffc9153-a2b7-4060-9816-676fa07afd75", Groups:[]string{"system:masters"}, Extra:map[string][]string(nil)}}) => fsName="exempt", distMethod=(*v1beta2.FlowDistinguisherMethod)(nil), plName="exempt", isExempt=true, queued=false, Finish() => panicking=false idle=false

If you apply fix: kcp/pkg/server/bootstrap/identity.go

if !strings.Contains(comps[5],":"){
    comps[5] += ":" + id
}

I think all this is because both Export and Binding are using non-native API which requires identity and it just coinsidence it is workloads API.

Happy to raise PR with fix if people thing this is adequate, but I have doubts (and lack of overall flow know-how)

cc @sttts @stevekuznetsov @ncdc

embik commented 2 years ago

I also encountered something related where the virtual workspaces for built-in APIExports (like tenancy.kcp.dev) were not working until I applied https://github.com/faroshq/kcp/commit/e4f885c4ec6eceb603271771b105d348f0f7acf6:

Before patch:

$ kubectl -s 'https://my-kcp-instance:443/services/apiexport/root/tenancy.kcp.dev/clusters/*' get clusterworkspaces
Error from server (NotFound): Unable to list "tenancy.kcp.dev/v1alpha1, Resource=clusterworkspaces": the server could not find the requested resource

After patch:

$ kubectl -s 'https://my-kcp-instance:443/services/apiexport/root/tenancy.kcp.dev/clusters/*' get clusterworkspaces
NAME      PHASE   TYPE        URL                                                              AGE
compute   Ready   universal   https://my-kcp-instance:443/clusters/root:compute   3h13m
users     Ready   homeroot    https://my-kcp-instance:443/clusters/root:users     3h13m
mjudeikis commented 2 years ago

I can now determine this happens only with SyncTargets, and not user provided APIExports. Tried to reproduce in the tests using Cowboys API with

root:root-test:
  APIResourceSchema: today.cowboys.wildwest.dev
  export: today-cowboys
root:root-test:consumer1:
    binding: cowboys (from root:root-test)
    export: cowboys-republisher (republisher with some secret)
root:root-test:consumer2:
    binding: cowboys-republished from consumer1 
    binding: cowboys (from root:root-test)

And I can see it working, but trying to extend same objects with Workspaces synctargets one, fails. I think it might be related so something particular how synctargets are being called....

mjudeikis commented 2 years ago

And not even all API calls. Few identity dumps from logs

(string) (len=76) "synctargets:8bf1f379c5b1714ed9be09a09bcac3c07e7224aaaf1b76fded8850bc9664d8ad"
(string) (len=141) "synctargets:8bf1f379c5b1714ed9be09a09bcac3c07e7224aaaf1b76fded8850bc9664d8ad:8bf1f379c5b1714ed9be09a09bcac3c07e7224aaaf1b76fded8850bc9664d8ad"
(string) (len=87) "clusterworkspaceshards:1b772017d81b11b18c9bbebe85c0c09a0fb43c3b8423ec9f4fd1f4639029c135"
(string) (len=83) "apiresourceimports:f6789b73a5d7d63c7a9334b2321a613013d200f672b63cfe24de608e4aa65178"
(string) (len=82) "clusterworkspaces:5089ac7bbf4758b59a535aa6782f70f0a2b3131fd21504ce09253e1f060b6dcf"
stevekuznetsov commented 2 years ago

If it happens with SyncTarget and is due to it being served from our system workspace, it should also apply to e.g. Placement - sounds like you have enough to write that e2e and fix the issue!

sttts commented 2 years ago

We should never accept claiming exported resources without an identity. If this works, we are in trouble and should fix it.