-
Notifications
You must be signed in to change notification settings - Fork 1.6k
KEP-4317: (Pod Certificates) Update for beta #5565
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
Conversation
9212583 to
f881201
Compare
| Several example signers built on the alpha feature set are available in the | ||
| [mesh-examples repository](https://github.com/ahmedtd/mesh-example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just add this to a new story, something like this:
#### Story 2
I am a cluster owner. I would like to run a service in mu cluster and force it to use mTLS
for server/client authentication. I expect other applications in my cluster to be communicating
with my service. I want the cluster to provide a simple, yet secure way for these applications to
request client certificates that they can use to authenticate to the new service.
Several example signers built on the alpha feature set are available in the
[mesh-examples repository](https://github.com/ahmedtd/mesh-example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say if we would be able to reference signers already pre-existing would be possible easier for people to adopt it and give examples on writing their own if required
| * Add e2e test to exercise the feature end-to-end | ||
| * Implement out-of-tree signers for several scenarios to prove that the feature | ||
| is sufficient for real use cases. Accomplished at | ||
| [mesh-examples](https://github.com/ahmedtd/mesh-example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have something in sigs.k8s.io eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense, but we would probably have to commit to maintaining them until they are upstreamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect some of these to land in k/k in KCM eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the main question, is do we want to ship a SIG Auth project of some demo signers, that might eventually get specced out in a KEP and moved into k/k?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to skip that step and go straight into KCM. TLS (and potentially mTLS) for Kube services is something I want all clusters to have.
| If support for the new key type is introduced in version N, then we have the | ||
| following skew cases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about API version skews?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? The cases I'm discussing are about which version of the Kubernetes API each component was built against.
| 4. Disable the PodCertificaterequest feature gate. | ||
| 5. Disable the certificates/v1beta1 API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong item numbering :)
Is there a specific order of components in which to disable the FG? If Disable the PodCertificaterequest feature gate implies kubelet, it should be explicitly mentioned.
|
|
||
| ###### How can a rollout or rollback fail? Can it impact already running workloads? | ||
|
|
||
| During rollout (enabling the certificates/v1beta1 API and PodCertificate feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember this particular detail from the implementation - is it possible to do these enablements in parallel? Won't a missing API cause for the feature to be still considered disabled on the kubelet side/fail initialization of a PCR watch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, you're right, I think we need the feature gate to be enabled on all kube-apiserver replicas before it is enabled on any kubelet instance. I've updated the rollout/rollback steps to document this, and updated this section.
| --> | ||
|
|
||
| `kubelet` will report metrics that show the feature is in use: | ||
| * `podcertificate_time_to_expiration` A gauge vector, faceted on namespace, pod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot use metrics labels with unbounded dimensions, such as names. This is to prevent DoS-ing your metrics scraper as this can easily cause memory exhaustion.
Applies to all suggested metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking here is:
- There is a fixed upper limit on the number of pods served by each kubelet, and thus fixed upper limit on the pod namespace and pod name.
- Each pod probably only mounts a few certificates, and thus signer name is bounded (though not explicitly).
This could be problematic for the kube-apiserver metrics, though. I'll check and see if there are any that include namespace / pod name today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some reference metrics:
- https://kubernetes.io/docs/reference/instrumentation/metrics/#:~:text=kube_pod_resource_limit --- exported by the scheduler, faceted on namespace and pod name
- apiserver_certificates_registry_csr_requested_duration_total --- exported by kube-apiserver, faceted on signerName, but with the caveat that only kubernetes.io signers are specifically reported.
These don't seem consistent to me --- there are potentially thousands of namespaces and tens of thousands of pods within a cluster, and (likely) ten or fewer signer names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading through the code change in kubernetes/kubernetes#102523, it seems like the real problems was that Prometheus counter vectors by default continue to hang on to facets, even if they are no longer getting data. We can fix that with
I think we might need some input from SIG Instrumentation about how to handle this case. The metrics instrumentation guide says this:
Notable exceptions are exporters like kube-state-metrics, which expose per-pod or per-deployment metrics, which are theoretically unbound over time as one could constantly create new ones, with new names. However, they have a reasonable upper bound for a given size of infrastructure they refer to and its typical frequency of changes.
In general, “external” labels like pod name, node name (any object name), & namespace do not belong in the instrumentation itself (the exception being kube-state-metrics). They are to be attached to metrics by the collecting system that has the external knowledge (blog post).
I'm not quite sure how to actually take action on that guidance. It seems clear that some of these metrics I am proposing should actually be exported by kube-state-metrics, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need some input from SIG Instrumentation about how to handle this case.
That would be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isnt this state management of the metric - more reason to have the metrics in kube-state-metrics (provided the data is readily available in some API) which will do the state management implicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, but won't the information be available in the PodCertificateRequest status?
These get vacuumed very quickly, and given a PCR, you can generally only tell which pod it is related to, not which volume in the pod. That might be OK, I guess.
But I think to make diagnosis of issues via kube-state-metrics more bulletproof, we would want the pod status to have a field that maps volumes to their current refresh and expiration times.
kube-state-metrics could then export gauges showing time left until refresh and expiration for each volume of each pod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long thread. I'm everyone has commented on cardinality. I share the concern. Perhaps it's more useful to know a count of how many successful/failed refreshes by signer and how many expired/current certs by signer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've cut the metrics down to one kubelet metric, reporting a gauge of currently-tracked pod certificate projections, bucketing them by signer name and the current state of the certificate. I think this strikes a good balance between cardinality and ability to pinpoint problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new metric looks good to me 👍
|
|
||
| `kube-apiserver` will report the following metrics to help operators assess the | ||
| health of signer implementations: | ||
| * `podcertificate_processed_latencies`: A histogram vector, faceted on signer name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we don't expect there would be too many signers, or that there would be a great turnover so I guess the signer name is probably ok for these metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be keen to say this might not be the case. In a real case scenarios we could have a secure environments with KArpenter to scale out our workloads for example and further to that various signers depends on what their function would be or maybe different CAs.
|
Please request a PRR review since you are promoting to beta. |
6d57df8 to
b32a96f
Compare
| // | ||
| // Keys should be namespaced, in the form `<domainName>/<keyname>`. Signers | ||
| // should document the keys and values they support. Signers should deny | ||
| // requests that contain keys they do not recognize. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would be length and characters limits here? Do we ever expect very long keys or values? If we want to start with restricting unicode command characters, we need to state it early - it will be hard to restrict later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would do validation on the total size of the field, as well as copying whatever validation is applied to metadata annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to https://github.com/kubernetes/enhancements/pull/5565/files#r2417192704.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify the limitations in spec. In metadata key is not allowing unicode and value allows any characters. If keys are expected to be ascii (no internationalization needed) and values allow the control characters, it is fine. It just can lead to discussions like kubernetes/kubernetes#132104 in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the spec to mention that the field is subject to the same validations as the metadata.annotations field, and include a summary description of that validation.
I think I have done this by including the PRR file in the PR. |
| * Node UID | ||
| * A requested maximum duration for the certificate lifetime. | ||
| * The signer name from which a certificate is being requested. | ||
| * A free-form `map[string]string` to allow pod authors to communicate additional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK kubernetes api conventions discourage the use of maps in the API.
Is this just suggesting exposing values to metadata on the pod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this I just copied the structure of annotations. But a list of kv-pairs would also be a valid approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to keep this as the same structure as annotations, since that lets us benefit from the annotation validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRR shadow:
This is not my area of expertise but I find the PRR very well thought out.
I have some questions on the use of a map[string]string in userConfig but I'll leave that to sigs to decide.
/assign @deads2k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, nothing serious but the metrics need to be sorted out. PRR lgtm too.
| // Keys should be namespaced, in the form `<domainName>/<keyname>`. Signers | ||
| // should document the keys and values they support. Signers should deny | ||
| // requests that contain keys they do not recognize. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify that kubernetes does verify or restrict these key/value pairs in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| // Keys must be namespaced in the form `<domainName>/<keyName`>. Signer | ||
| // implementations should document what UserConfig keys they support. Signer | ||
| // implementations should reject PodCertificateRequests that contain user config that | ||
| // they do not recognize with a condition of type "Denied" and reason "InvalidUserConfig". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same doc here about kubernetes not providing any guarantees about these name/value pairs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| - serving certificates | ||
| - issuing client certificates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes to both please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are both implemented in my demo repo
| is sufficient for real use cases. These scenarios should include issuing of: | ||
| - serving certificates | ||
| - issuing client certificates | ||
| - SPIFFE certificates (?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enj opinion on requiring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should certainly make it possible, so that projects like Istio and Cilium can leverage it, if they want to. I don't think anything regarding SPIFFE should be included directly in KCM in the future though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented basic SPIFFE cert support in my demo repo. If we decide to move some of them into a sig-auth repo, we don't need to move that one.
| - SPIFFE certificates (?) | ||
| [mesh-examples](https://github.com/ahmedtd/mesh-example). | ||
| * Migrate kubelet implementation to use the beta API. | ||
| * Deprecate and remove the alpha API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't require removal for beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to remove it just to keep life simple, I think.
| --> | ||
|
|
||
| `kubelet` will report metrics that show the feature is in use: | ||
| * `podcertificate_time_to_expiration` A gauge vector, faceted on namespace, pod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long thread. I'm everyone has commented on cardinality. I share the concern. Perhaps it's more useful to know a count of how many successful/failed refreshes by signer and how many expired/current certs by signer
|
Whoops, I'm not sure how I closed this. |
|
PRR lgtm. Changes lgtm. I'm happy with metrics separated by signer name. /approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
| // | ||
| // Signers should document the keys and values they support. Signers should | ||
| // deny requests that contain keys they do not recognize. | ||
| UserAnnotations map[string]string `json:"userAnnotations,omitempty" protobuf:"bytes,11,opt,name=userAnnotations"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add something like untrusted or unverified to the name of the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've added Unverified in front of the field in PodCertificateRequestSpec. I've left the PodCertificateProjection field as "userAnnotations", since it seems weird to make the user write out that their requests are unverified.
| // | ||
| // Entries are subject to the same validation as object metadata annotations. | ||
| // Each key must be valid label key, which is a case-insensitive ASCII string, | ||
| // optionally prefixed with a DNS 1123 subdomain and a slash. No restrictions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we wanted these to be domain qualified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to reuse the same validation logic that we have adopted for annotations, since that's likely to be already well thought out. We can add an additional restriction that the key must be domain-qualified.
| // does not recognize one of the keys passed in userConfig, or if the signer | ||
| // otherwise considers the userConfig of the request to be invalid. | ||
| PodCertificateRequestConditionInvalidUserConfig string = "InvalidUserConfig" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will need to be updated to match the other field's name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, done.
| // | ||
| // Valid values are "RSA3072", "RSA4096", "ECDSAP256", | ||
| // "ECDSAP384", and "ED25519". If left empty, Kubelet defaults to "ECDSAP256". | ||
| // "ECDSAP384", and "ED25519". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we make the field required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you have to specify a value in the pod spec.
| * Add e2e test to exercise the feature end-to-end | ||
| * Implement out-of-tree signers for several scenarios to prove that the feature | ||
| is sufficient for real use cases. Accomplished at | ||
| [mesh-examples](https://github.com/ahmedtd/mesh-example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect some of these to land in k/k in KCM eventually.
| is sufficient for real use cases. These scenarios should include issuing of: | ||
| - serving certificates | ||
| - issuing client certificates | ||
| - SPIFFE certificates (?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should certainly make it possible, so that projects like Istio and Cilium can leverage it, if they want to. I don't think anything regarding SPIFFE should be included directly in KCM in the future though.
|
|
||
| ###### How can this feature be enabled / disabled in a live cluster? | ||
|
|
||
| At alpha, enabling this feature in a live cluster will require the following steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| At alpha, enabling this feature in a live cluster will require the following steps. | |
| At beta, enabling this feature in a live cluster will require the following steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments.
| * kube-apiserver will then truncate `maxExpirationSeconds` to the signer's | ||
| configured maximum duration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we removed this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, removed.
| // the signer implementation. Kubernetes does not restrict or validate this | ||
| // metadata in any way. | ||
| // | ||
| // These values are copied verbatim into the `spec.UnverifiedUserAnnotations` field of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // These values are copied verbatim into the `spec.UnverifiedUserAnnotations` field of | |
| // These values are copied verbatim into the `spec.unverifiedUserAnnotations` field of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| * Confirm that the issued chain (if one is set) consists of valid certificates, | ||
| that do indeed form a chain to each other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't check that the certs form a chain, just that all the certs are valid.
We also know that Go is going to tighten cert validation so let's be stricter on this new API from the start:
- no DNSNames entries are
""or contain..or start/end with. - EmailAddresses all pass
mail.ParseAddress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmedtd, deads2k, enj 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:
Approvers can indicate their approval by writing |
Update KEP-4317 for beta.
Concrete changes: