CM-830: Integrate trust-manager with cert-manager-operator#1914
CM-830: Integrate trust-manager with cert-manager-operator#1914chiragkyal wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
@chiragkyal: This pull request references CM-830 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 story 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. |
|
@chiragkyal: This pull request references CM-830 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 story 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. |
5728ef0 to
fcc188e
Compare
fcc188e to
624ef6b
Compare
bharath-b-rh
left a comment
There was a problem hiding this comment.
LGTM mostly. I will need a second read.
624ef6b to
76a2381
Compare
c31d38c to
b7c9665
Compare
lunarwhite
left a comment
There was a problem hiding this comment.
It looks fairly good to me 👍. I'm very glad to see that the upstream planned Bundle API changes/deprecations are mentioned in this EP. As we move toward GA (future), we might need to adjust our custom APIs based on the stabilized upstream CRD APIs accordingly.
/lgtm |
085947b to
07a4043
Compare
This enhancement describes extending cert-manager-operator to deploy and manage trust-manager as an operand. Signed-off-by: chiragkyal <[email protected]>
06b2a15 to
0a6ca23
Compare
|
|
||
| ### User Stories | ||
|
|
||
| - As an OpenShift administrator, I want to have an option to deploy trust-manager as a day-2 operation, so that I can |
There was a problem hiding this comment.
Is there any specific impact if the trust manager is delivered as default?
There was a problem hiding this comment.
Not all the customers would require trust bundle management from day0, hence an explicit opt-in would give them a choice. Customers who need trust-manager can easily enable it by creating a single TrustManager CR. This approach is consistent with how istio-csr is also delivered as an optional day-2 operand in cert-manager-operator.
If we deliver it as default, I think apart from resource overhead challenges for the customers who don't need this operand, there should not be an impact.
| - As an OpenShift administrator, I want to have an option to deploy trust-manager as a day-2 operation, so that I can | ||
| distribute CA certificates across my cluster. | ||
| - As an OpenShift administrator, I want to be able to configure trust-manager, so that only required features can be enabled. | ||
| - As an OpenShift administrator, I want to enable trust-manager to write trust bundles to Secrets in addition to ConfigMaps |
There was a problem hiding this comment.
Are there any specific requirements of why these trust bundles need to be in Secrets and not only ConfigMaps?
There was a problem hiding this comment.
It depends on how the application wants to use the trust bundle. Some applications might require to read CA bundles exclusively from Secrets. This is an optional feature controlled by secretTargets.policy. By default, trust-manager only writes to ConfigMaps. Administrators explicitly enable Secret targets when their applications require it.
| - As an OpenShift administrator, I want to use OpenShift's trusted CA bundle as the default CA package, so that trust-manager uses certificates appropriate for my cluster. | ||
| - As an OpenShift administrator, I want to filter expired certificates from trust bundles to ensure only valid certificates | ||
| are distributed. | ||
| - As an OpenShift administrator, I want to configure a custom trust namespace where trust sources (ConfigMaps/Secrets) |
There was a problem hiding this comment.
Is the custom trust namespace an application-specific namespace?
There was a problem hiding this comment.
No, the trust namespace is not application-specific. It's a centralized namespace where trust-manager reads source CA certificates from. We are making this option immutable once set.
| - `cert-manager-operator` to be extended to manage `trust-manager` along with currently managed `cert-manager` and `istio-csr`. | ||
| - New custom resource (CR) `trustmanagers.operator.openshift.io` to be made available to install and configure | ||
| the trust-manager deployment. | ||
| - Provide OpenShift-native integration for the default CA package using CNO's trusted CA bundle injection instead of the |
There was a problem hiding this comment.
Does using CNO directly be better in this case? The user might have better control on the resources that require ca injection.
There was a problem hiding this comment.
We will be using CNO to inject default CA certificates into the trust-manager Bundle. Detailed steps are mentioned in the DefaultCAPackage Implementation section.
There was a problem hiding this comment.
Does using CNO directly be better in this case?
It is specifically for making available the RHCOS bundled CA certificates, user configured certificates for proxy... in case the application need those and is optional. But for distributing CA certificate bundle specifically based on the application need, trust-manager fares better, where instead of distributing all the OpenShift Cluster trusted CA certificates, only required certificates can be made available.
The user might have better control on the resources that require ca injection.
@TrilokGeer Could you please elaborate more this.
There was a problem hiding this comment.
Just to add a bit more to the context:
Whether or not to include the DefaultCA is a parameter (useDefaultCAs) at each bundle level.
Thus, the user has the choice of which bundles will include the DefaultCA, and which will not.
|
|
||
| - Automatic cleanup of Configmap created to support `DefaultCAPackage` option, when this field is toggled or TrustManager CR is deleted. | ||
|
|
||
| - For TechPreview, the `targetNamespaces` option will not be configurable. This option controls which namespaces |
There was a problem hiding this comment.
This might result to a conflict and maybe hard to trace, where an un-intended write will override ca configurations in a namespace.
There was a problem hiding this comment.
The targetNamespaces option defines the RBAC for trust-manager. Actual writes are controlled by individual Bundle CRs through namespaceSelector field.
apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
name: my-bundle
spec:
sources:
- configMap:
name: "my-ca"
key: "ca.crt"
target:
configMap:
key: "ca-bundle.crt"
# Only writes to namespaces with this label
namespaceSelector:
matchLabels:
trust-bundle-injection: enabledThe actual write behavior is controlled by Bundle CRs, with explicit target naming at the application level.
There was a problem hiding this comment.
Additionally trust-manager doesn't overwrite the target resource, but instead only appends the configured keys. And yes, if keys with same name exists, that would be a misconfiguration.
|
|
||
| **Impact**: | ||
| - Users who create `Bundle` resources may need to migrate to `ClusterBundle` or the new namespace-scoped `Bundle` | ||
| - Helm chart values may change, leading to changes in downstream CRD values. |
There was a problem hiding this comment.
Ideally, this risk must be mitigated by using applicable defaults wherever applicable. Is there any other implementation that cannot be solved by defaults?
There was a problem hiding this comment.
The Bundle to ClusterBundle migration will happen in a phased manner in upstream. We can definitely handle Helm value changes and CRD defaults if applicable.
Migration and backward compatibility will be defined by upstream. We'll adapt based on the decision and document any required user actions.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bharath-b-rh 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: chiragkyal <[email protected]>
0a6ca23 to
002807c
Compare
|
New changes are detected. LGTM label has been removed. |
|
@chiragkyal: all tests passed! 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. |
Extend cert-manager-operator to deploy and manage trust-manager as an operand, providing a solution for trust bundle distribution on OpenShift.