Skip to content
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.

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

certificatesigningrequests/v1beta1 to v1: create kubelet-serving CSR #133

Closed
wants to merge 3 commits into from

Conversation

wongma7
Copy link
Member

@wongma7 wongma7 commented Jan 4, 2022

Issue #, if available:

Description of changes:
This is #115 plus the patch suggested in #115 (comment)

IMO This should is only a partial mitigation to the removal of the legacy-unknown signer. IT's partial because it only works in clsuters where the default kubelet-serving signer is running. Notably, EKS does NOT run the default kublet-serving signer (it runs a stricter signer for security reasons, so not just anyone can get a serving cert signed by cluster CA and trusted by apiserver). So a "full" solution IMO entails fixing #94

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@wongma7 wongma7 requested a review from a team as a code owner January 4, 2022 19:35
@wongma7 wongma7 changed the title Certificatesv1 upstreamprefix certificatesigningrequests/v1beta1 to v1: create kubelet-serving CSR Jan 4, 2022
Joel Diaz and others added 3 commits January 4, 2022 11:37
needed for certificatesv1

performed by:
go get -u k8s.io/client-go
go mod vendor
go mod tidy
v1beta1 will be dropped in kube 1.22
@wongma7 wongma7 force-pushed the certificatesv1-upstreamprefix branch from fe06f60 to 8908663 Compare January 4, 2022 19:38
@nckturner
Copy link
Contributor

Should we really be using kubernetes.io/kubelet-serving signer, as the point of signer names is to create signing profiles for certificate users. Additionally, this will cause the webhook to get a certificate with a system:node name -- I"m not sure what this means for serving certificates and who will care, but it seems wrong.

@wongma7
Copy link
Member Author

wongma7 commented Jan 4, 2022

I agree it seems wrong, see my comments in #115, but is it any worse than using legacy-unknown signer? Neither CSR gets auto-approved by default so it's still up to the deployer/administrator to manually get the cert signed, the webhook is just automating most of it. The alternative is to remove this CSR creation automation altogether and leave it up entirely up to the deployer to provide the cert which arguably is what the webhook should have been doing in the first place to support cert-manager etc. but it would be disruptive to do now so this is just a quickfix since we have gone months with this being broken for k8s 1.22

@wongma7
Copy link
Member Author

wongma7 commented Jan 4, 2022

If everyone agrees we can drop this path and instead invest in supporting cert-manager/bring-your-own cert use-case like by reviving #87 but I cant provide guarantees about how long that would take whereas this partial fix can be merged today

@wongma7
Copy link
Member Author

wongma7 commented Jan 4, 2022

We would probably need to deprecate the in-cluster flag since currently it's mutually exclusive with the tls-key flag . And remove stuff like https://github.com/aws/amazon-eks-pod-identity-webhook/blob/master/hack/webhook-patch-ca-bundle.sh and edit https://github.com/aws/amazon-eks-pod-identity-webhook/blob/master/deploy/mutatingwebhook.yaml which assume that the server cert will be signed by cluster CA which won't be the case in cert-manager/ bring your own/non-CSR use-case.

@nckturner
Copy link
Contributor

nckturner commented Jan 4, 2022

Also your point:

Notably, EKS does NOT run the default kublet-serving signer

is important. EKS won't even sign these for the pod identity webhook. So we should really look into the bring your own cert use case for non-EKS, and, either use a self signed/byo cert in EKS as well, or .. introduce a signer name for it?

@nckturner
Copy link
Contributor

/cc @micahhausler

@wongma7
Copy link
Member Author

wongma7 commented Jan 4, 2022

re: EKS, right, also, I have been assuming wrongly that EKS users are all using the EKS-deployed webhook but there are some users who have deployed the webhook themselves and this change could break the webhook for them.

Anyway, after some more offline discussion I guess it's agreed we ought to deprecate this "in-cluster" CSR-based cert generation and work on the certwatcher stuff instead of merging this fix?

@windsource
Copy link

Anyhow it would be good to have a working version soon such that amazon-eks-pod-identity-webhook can be used with Kubernetes 1.22+. Currently even 1.23 has been released. I know that EKS is still on 1.21.2 but I think there are some people (like me) who use amazon-eks-pod-identity-webhook with other Kubernetes distributions like K3S or RKE2.

@wongma7 wongma7 closed this Jan 20, 2022
@wongma7
Copy link
Member Author

wongma7 commented Jan 20, 2022

Solution I'm thinking of is:
if --in-cluster=true then you must provide signerName (new arg)
if --in-cluster=false then #134 should fix the bug where cert never gets reloaded if it's renewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants