CNTRLPLANE-1752: Add PKI API to config.openshift.io/v1alpha1 #2645
Conversation
sanchezl
commented
Jan 12, 2026
sanchezl
commented
- Add PKI API to config.openshift.io/v1alpha1
- make update
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @sanchezl! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
/test required |
|
@sanchezl: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[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 |
|
/test unit |
sanchezl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sanchezl: This pull request references CNTRLPLANE-1752 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
@sanchezl: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
config/v1alpha1/types_pki.go
Outdated
| // +kubebuilder:validation:XValidation:rule="self.type == 'Unmanaged' ? (!has(self.defaults) && !has(self.categories) && !has(self.overrides)) : true",message="defaults, categories, and overrides must not be set when type is Unmanaged" | ||
| // +kubebuilder:validation:XValidation:rule="self.type == 'Default' ? (!has(self.defaults) && !has(self.categories) && !has(self.overrides)) : true",message="defaults, categories, and overrides must not be set when type is Default" | ||
| // +union | ||
| type PKIManagementPolicy struct { |
There was a problem hiding this comment.
I want to note that I am not using a discriminated union pattern here, instead it is more of a "parameterized" union pattern (at at least a kind of parameterized union where its all fields or none).
This configuration models a "predefined behavior vs. customization" choice pattern, rather than mutually exclusive options with different configuration requirements (like platform-specific settings).
If we add more predefined behaviors (e.g., type: FIPS, type: CNSACompliant), they would be additional enum values with no configuration fields. They represent different default behaviors, not different configuration structures.
If we need to extend what's configurable (e.g., adding certificate lifetime configuration), we'd add fields to PKIProfile and/or CertificateConfig. These fields would apply regardless of which predefined behavior or custom configuration is in use.
There was a problem hiding this comment.
I'm not sure I'm following why this is a better approach than a discriminated union pattern.
Discriminated unions are a common pattern in OpenShift APIs (so our users will be used to them) and they allow for discriminators with no associated configuration fields.
If we add more predefined behaviors (e.g., type: FIPS, type: CNSACompliant), they would be additional enum values with no configuration fields. They represent different default behaviors, not different configuration structures.
If we need to extend what's configurable (e.g., adding certificate lifetime configuration), we'd add fields to PKIProfile and/or CertificateConfig. These fields would apply regardless of which predefined behavior or custom configuration is in use.
It sounds like if we added any additional pre-defined behaviors any new configuration options we add would be not configurable by end-users with these pre-defined behaviors.
To me, this reads that the only time we anticipate ever being able to configure something is when type is set to Custom - in which case an end-user must supply some kind of configuration.
Is this accurate?
There was a problem hiding this comment.
Right. This is a pick a predefined policy, or supply your own. There would be no such thing as a pre-defined policy with options. I think this is distinct from "there are different kinds of policies with different kinds of fields".
There was a problem hiding this comment.
I think this is distinct from "there are different kinds of policies with different kinds of fields".
Is it though? I read this as there being different kinds of policies with different kinds of fields, just that most of the policies have no fields.
IMO, something like:
type: Custom
custom:
defaults: ...
categories: ...
overrides: ...is more intuitive that the fields are only applicable when the type is custom than something like:
type: Custom
defaults: ...
categories: ...
overrides: ...I think becomes even more true when we've used this discriminated union approach for quite some time for OpenShift APIs and is one that users are going to be familiar with.
We try to aim for consistency, where possible, in how we structure our APIs to keep the experience consistent and familiar for our users - I'm not sold on breaking that consistency here yet.
There was a problem hiding this comment.
The currently proposed approach also lends itself to an ever growing list of CEL validations to maintain instead of having one to prevent setting custom values when the type field is not Custom.
| // certificateManagement specifies how PKI configuration is managed for internally-generated certificates. | ||
| // This controls the certificate generation approach for all OpenShift components that create | ||
| // certificates internally, including certificate authorities, serving certificates, and client certificates. | ||
| // | ||
| // +required | ||
| CertificateManagement PKICertificateManagement `json:"certificateManagement,omitzero"` |
There was a problem hiding this comment.
Do we envision this API being expanded to do more than certificate management in the future?
| // - Unmanaged: Components use their existing hardcoded certificate generation behavior, exactly as if this feature did not exist. | ||
| // Each component generates certificates using whatever parameters it was using before this feature. | ||
| // While most components use RSA 2048, some may use different parameters. | ||
| // Use of this mode might prevent upgrading to the next major OpenShift release. | ||
| // + Default when upgrading from a version without this feature to ensure zero behavior change. | ||
| // | ||
| // - Default: Use OpenShift-recommended best practices for certificate generation. | ||
| // The specific parameters may evolve across OpenShift releases to adopt improved cryptographic standards. | ||
| // In the initial release, this matches Unmanaged behavior for each component. | ||
| // In future releases, this may adopt ECDSA or larger RSA keys based on industry best practices. | ||
| // Recommended for most customers who want to benefit from security improvements automatically. | ||
| // + Default when installing a fresh cluster. | ||
| // | ||
| // - Custom: Administrator explicitly configures cryptographic parameters. | ||
| // Use the custom field to specify certificate generation parameters. | ||
| // + Recommended for customers with specific compliance requirements or organizational PKI policies. |
There was a problem hiding this comment.
Instead of using the list notation, we generally try to use formats like:
// When set to Unmanaged, ....
//
// When set to Default, ...
//
// When set to Custom, ...This helps us to avoid any weirdness in formatting of the generated documentation when using list notations.
| // - Unmanaged: Components use their existing hardcoded certificate generation behavior, exactly as if this feature did not exist. | ||
| // Each component generates certificates using whatever parameters it was using before this feature. | ||
| // While most components use RSA 2048, some may use different parameters. | ||
| // Use of this mode might prevent upgrading to the next major OpenShift release. | ||
| // + Default when upgrading from a version without this feature to ensure zero behavior change. |
There was a problem hiding this comment.
Reading the description of Unmanaged I'm wondering if Unmanaged is the right terminology here.
I'm struggling a bit with naming suggestions, but something along the lines of ComponentDefaults seems more appropriate here since this is targeted at a mode where individual components of the platform are using their own defaults.
What do you think?
| // - Default: Use OpenShift-recommended best practices for certificate generation. | ||
| // The specific parameters may evolve across OpenShift releases to adopt improved cryptographic standards. | ||
| // In the initial release, this matches Unmanaged behavior for each component. | ||
| // In future releases, this may adopt ECDSA or larger RSA keys based on industry best practices. | ||
| // Recommended for most customers who want to benefit from security improvements automatically. | ||
| // + Default when installing a fresh cluster. |
There was a problem hiding this comment.
Following the same logic of there being a couple layers of "default" behavior, maybe naming this something like PlatformDefault makes more sense as applying the default broadly across all components in the "platform"?
| // + When upgrading from a version without this feature: | ||
| // + - The PKI resource is created with mode: Unmanaged to ensure zero behavior change. | ||
| // | ||
| // + When installing a fresh cluster: | ||
| // + - If no PKI configuration is provided in install-config.yaml, mode: Default is used. | ||
| // + - If PKI configuration is provided in install-config.yaml, mode: Custom is used with the specified configuration. |
There was a problem hiding this comment.
I don't think we need to include semantics about upgrading vs installing from a fresh cluster as part of the API documentation.
It reads to me as more suited towards developer documentation as opposed to end-user documentation. The GoDoc is used as our user-facing documentation for APIs.
| // + - If PKI configuration is provided in install-config.yaml, mode: Custom is used with the specified configuration. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:Enum=Unmanaged;Default;Custom |
There was a problem hiding this comment.
We generally recommend putting the enum marker on the type alias. It is effectively the same, but if the type alias is ever used in another API somewhere it will not automatically add the enum constraint unless the marker is on the type alias.
There was a problem hiding this comment.
This comment applies to all enum constraint markers in this PR.
| // defaults specifies the default certificate configuration | ||
| // for all certificates unless overridden by category or specific | ||
| // certificate configuration. | ||
| // If not specified, uses platform defaults (typically RSA 2048). |
There was a problem hiding this comment.
Is this the actual platform defaults that may be subject to change over time, or are these the component-specific defaults?
I know they are currently one and the same, but IIUC they may not be in the future.
|
|
||
| type CategoryCertificateConfig struct { | ||
| // category identifies the certificate category. | ||
| // Valid values are "SignerCertificate", "ServingCertificate", and "ClientCertificate". |
There was a problem hiding this comment.
What does each of these values mean when set?
|
|
||
| // CertificateOverride allows configuration of certificate parameters for specific named certificates. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule=`self.certificateName in ['admin-kubeconfig-signer','kubelet-bootstrap-kubeconfig-signer','kube-apiserver-localhost-signer','kube-apiserver-service-network-signer','kube-apiserver-lb-signer','root-ca','kube-apiserver-to-kubelet-signer','kube-control-plane-signer','aggregator-signer','kubelet-signer','aggregator-ca','etcd-signer','etcd-metrics-signer','service-ca','csr-signer-ca','kube-apiserver-localhost-server','kube-apiserver-service-network-server','kube-apiserver-lb-server','kube-apiserver-internal-lb-server','machine-config-server','ironic-server','etcd-peer-server','etcd-server','etcd-metrics-server','admin-kubeconfig-client','kubelet-client','kube-apiserver-to-kubelet-client','kube-control-plane-kube-controller-manager-client','kube-control-plane-kube-scheduler-client','aggregator-client','apiserver-proxy-client','journal-gatewayd-client']`,message="certificateName must be a well-known certificate name" |
There was a problem hiding this comment.
If we have a known list of values that we want to enforce, we should use an enum instead of a CEL expression. CEL expressions should only be used when necessary, and the OpenAPI schema used for CRDs already has a way to enforce enum constraints.
| // Valid values are 2048, 3072, and 4096. | ||
| // +required | ||
| // +kubebuilder:validation:Enum=2048;3072;4096 | ||
| // +kubebuilder:validation:Minimum=2048 |
There was a problem hiding this comment.
I don't think the minimum marker is required here when setting the enum constraint. I don't recall whether or not the linter picks this up for integer types though 🤔
3 participants
