Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Shubham Mishra <smishra.shubhammishra@gmail.com>
4170196 to
0e0bb22
Compare
tariq-hasan
left a comment
There was a problem hiding this comment.
Hi @shubhM13! Thanks for raising the PR. I have added some initial comments - mostly around the scope covered by the KEP.
| - [#2502](https://github.com/kubeflow/spark-operator/issues/2502) tracks future | ||
| webhook deprecation. Existing supported releases still require safe TLS | ||
| operation until that work is complete. |
There was a problem hiding this comment.
I'm actually wondering if we'd want to deprecate webhook altogether - since the webhooks are now used not only for pod mutation but also for CRD mutation and validation.
| An external PKI already owns certificate issuance, private-key rotation, and | ||
| renewal. Requiring that material to pass through the current self-signed or | ||
| cert-manager paths creates one or more of the following problems: | ||
|
|
||
| - A second component becomes responsible for a private key. | ||
| - The webhook retains Secret permissions it does not need. | ||
| - Certificate files are copied only at startup and do not follow renewal. | ||
| - A pod restart is required to serve a renewed certificate. | ||
| - Each external-PKI installation maintains a security-sensitive downstream | ||
| patch. | ||
|
|
||
| Admission webhooks are fail-closed by default. A serving-certificate or CA | ||
| rotation mistake can therefore prevent unrelated Kubernetes API operations. | ||
| The upstream contract needs to define ownership, validation, overlap, and | ||
| rollback rather than merely adding a flag that skips certificate generation. |
There was a problem hiding this comment.
It appears that a filesystem provider was already supported before v2 but was removed - #2016. The removal seems to be owed to the fact that bootstrap mechanism was prone to lead to failure modes - #855, #926, #1705, #1739, #1740, #2032, #2281.
Since the KEP addresses the various gaps by reshaping filesystem provision I am thinking this may warrant a callout in the motivation section.
There was a problem hiding this comment.
Thanks! I’ll add a motivation callout distinguishing the previous mechanism from this proposal’s read-only external filesystem consumer and connect the relevant historical failures to the proposed safeguards.
| Add `filesystem` alongside the current self-signed and cert-manager certificate | ||
| sources. The filesystem provider reads a serving certificate chain, private key, | ||
| and minimal CA bundle from configurable paths. It does not persist or copy the | ||
| external private key. | ||
|
|
||
| Certificate source and CA publication are selected independently: | ||
|
|
||
| - **Operator-owned CA publication:** the existing admission-configuration | ||
| controllers publish the validated CA bundle. | ||
| - **External CA publication:** the Spark Operator does not read or write the | ||
| admission configurations for certificate management. An injector, Helm, | ||
| GitOps controller, or administrator owns the fields. | ||
|
|
||
| Exactly one CA writer is allowed. The binary and chart reject combinations that | ||
| would create two writers. |
There was a problem hiding this comment.
At the same time I am wondering if we want to break down the scope of the KEP into multiple phases, and for phase 1, to only support filesystem serving identity with external CA ownership, and to retain full CA reconciliation as part of a phase 2 discussion. Would appreciate thoughts from @nabuskey @RobuRishabh @vjanelle @vikas-saxena02 as well.
Purpose of this PR
This adds the provisional design document for #3165. The Spark Operator webhook
currently obtains serving certificates through its self-signed or cert-manager
paths, both of which assume certificate material is stored in a Kubernetes
Secret and copied into the serving directory. Clusters whose platform PKI
delivers short-lived certificates directly as files have no supported path that
preserves live rotation and avoids unnecessary Secret ownership.
Proposed changes:
filesystemcertificate provider while preserving theexisting self-signed default and cert-manager compatibility flag.
caBundleownership, withexactly one operator or external writer.
least-privilege RBAC, observability, and test requirements.
before implementation.
Fixes #3165
Change Category
Rationale
Admission webhooks are fail-closed, and serving-certificate and trust rotation
span filesystem generations, two admission objects, API-server propagation, and
multiple webhook replicas. Agreeing on the ownership and rotation contract
before implementation avoids encoding an unsafe or incompatible API.
The two points where maintainer direction is most useful are in
Open Questions:
small bounded polling loader in Spark Operator.
static CA PEM, or only out-of-band external ownership.
Checklist
Additional Notes
Documentation-only change; no production code or generated manifests are
modified, so unit tests were not run.
Validation performed locally:
git diff --checkpasses.upstream/master.