search

cisco-open / cluster-registry-controller

An operator that automatically synchronizes Kubernetes resources across multiple clusters
Apache License 2.0
22 stars 8 forks source link

Add K8s 1.24+ support to cluster-registry-controller #28

Closed shanchunyang0919 closed 2 years ago

shanchunyang0919 commented 2 years ago
Q A
Bug fix? no
New feature? no
API breaks? no
Deprecations? no
License Apache 2.0

What's in this PR?

Checklist

shanchunyang0919 commented 2 years ago

LGTM

I suppose the default "https://kubernetes.default.svc.cluster.local audience is enough for the ClusterRegistry ServiceAccount token.

Make sure to smoke test manually.

Tested with different audiences and "https://kubernetes.default.svc.cluster.local", none of them works. It seems like the audience need to be identical to the issuer (the "iss" field in token's claim).

pregnor commented 2 years ago

LGTM I suppose the default "https://kubernetes.default.svc.cluster.local audience is enough for the ClusterRegistry ServiceAccount token. Make sure to smoke test manually.

Tested with different audiences and "https://kubernetes.default.svc.cluster.local", none of them works. It seems like the audience need to be identical to the issuer (the "iss" field in token's claim).

But then the current implementation doesn't work either right? Because a not-specified audience you use in the code defaults to the https://kubernetes.default.svc.cluster.local audience on server side. Edit: yeah, I misunderstood this, it defaults to the issuer.

That was my fear (audiences not working properly in this case) that is why I didn't use the TokenRequest API in the Pipeline context I referenced in one of my comments. And that is why I left that comment on the review guessing the smoke tests worked before the PR submission but wanting to double check that is the case.

shanchunyang0919 commented 2 years ago

LGTM I suppose the default "https://kubernetes.default.svc.cluster.local audience is enough for the ClusterRegistry ServiceAccount token. Make sure to smoke test manually.

Tested with different audiences and "https://kubernetes.default.svc.cluster.local", none of them works. It seems like the audience need to be identical to the issuer (the "iss" field in token's claim).

But then the current implementation doesn't work either right? Because a not-specified audience you use in the code defaults to the https://kubernetes.default.svc.cluster.local audience on server side.

That was my fear (audiences not working properly in this case) that is why I didn't use the TokenRequest API in the Pipeline context I referenced in one of my comments. And that is why I left that comment on the review guessing the smoke tests worked before the PR submission but wanting to double check that is the case.

The current implementation works fine actually. Without audience field being specified, it will automatically take the issuer as the audience. It has been tested in different k8s version (v1.21 and v1.24) clusters.

pregnor commented 2 years ago

LGTM I suppose the default "https://kubernetes.default.svc.cluster.local audience is enough for the ClusterRegistry ServiceAccount token. Make sure to smoke test manually.

Tested with different audiences and "https://kubernetes.default.svc.cluster.local", none of them works. It seems like the audience need to be identical to the issuer (the "iss" field in token's claim).

But then the current implementation doesn't work either right? Because a not-specified audience you use in the code defaults to the https://kubernetes.default.svc.cluster.local audience on server side. That was my fear (audiences not working properly in this case) that is why I didn't use the TokenRequest API in the Pipeline context I referenced in one of my comments. And that is why I left that comment on the review guessing the smoke tests worked before the PR submission but wanting to double check that is the case.

The current implementation works fine actually. Without audience field being specified, it will automatically take the issuer as the audience. It has been tested in different k8s version (v1.21 and v1.24) clusters.

Ok then, thanks.

Laci21 commented 2 years ago

I'd propose to have an automatic token renewal and syncing process with the TokenRequest API on the long run. We cannot set the audience to any stricter option and Patrik, you are right that the token is stored on the cluster as a secret anyway (in the one generated by the cluster-registry), so there is not much difference there. The only thing that could be better in the long run is the expiration. But since that's not in scope for this ticket, I'm fine with the manual secret creation approach for now.

shanchunyang0919 commented 2 years ago

Thanks for reviews! @pregnor @Laci21