CM-862, CM-863: Add TrustManager API and Static Manifests #362
Conversation
openshift-ci
bot
added
do-not-merge/hold
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds cluster-scoped TrustManager and Bundle CRDs, generated Go client/informer/lister/apply-configuration code, embedded trust-manager Kubernetes manifests and assets, a Makefile variable plus script to update upstream trust-manager manifests, CSV/samples updates, and deepcopy/apply-config boilerplate. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (32)
🚧 Files skipped from review as they are similar to previous changes (13)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (10)pkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.go (4)
pkg/operator/informers/externalversions/operator/v1alpha1/interface.go (1)
pkg/operator/applyconfigurations/utils.go (9)
pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (1)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.go (5)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go (1)
api/operator/v1alpha1/zz_generated.deepcopy.go (2)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (3)
🔇 Additional comments (26)
✏️ Tip: You can disable this entire section by setting Comment |
chiragkyal
force-pushed
the
trust-manager-api
branch
from
a8f136c to
d918d5f
Compare
chiragkyal
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
chiragkyal
force-pushed
the
trust-manager-api
branch
from
d918d5f to
a9b8263
Compare
openshift-merge-robot
removed
the
needs-rebase
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CM-862 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 either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. This pull request references CM-863 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 either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. 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
force-pushed
the
trust-manager-api
branch
from
a9b8263 to
525ff10
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
403-688: Blocker: CSV RBAC is missing permissions for the new TrustManager (and likely Bundle) APIs
clusterPermissionsincludesoperator.openshift.iorules forcertmanagersandistiocsrs, but nottrustmanagers. If this CSV ships as-is, the operator will fail to list/watch/reconcileTrustManagerresources.Proposed fix (add API rules)
- apiGroups: - operator.openshift.io resources: - certmanagers + - trustmanagers verbs: - create - delete - get - list - patch - update - watch - apiGroups: - operator.openshift.io resources: - certmanagers/finalizers - istiocsrs/finalizers + - trustmanagers/finalizers verbs: - update - apiGroups: - operator.openshift.io resources: - certmanagers/status - istiocsrs/status + - trustmanagers/status verbs: - get - patch - update + - apiGroups: + - trust.cert-manager.io + resources: + - bundles + - bundles/status + verbs: + - get + - list + - watch
🤖 Fix all issues with AI agents
In @bindata/trust-manager/resources/deployment_trust-manager.yml:
- Line 66: The Deployment currently has an empty resources block ("resources:
{}"); update the Pod template's container spec (spec.template.spec.containers
for the trust-manager container) to define sensible CPU and memory requests and
limits (e.g., requests for baseline scheduling and limits to prevent OOM/CPU
runaway) based on expected load; ensure both request and limit fields are set
for cpu and memory for the trust-manager container, and document or comment that
values should be tuned after load testing.
In @bundle/manifests/cert-manager-operator.clusterserviceversion.yaml:
- Around line 236-268: The alm-example is inconsistent:
Bundle.spec.sources[].useDefaultCAs is true while the TrustManager example
(TrustManager.spec.trustManagerConfig) does not enable
spec.trustManagerConfig.defaultCAPackage.policy (which defaults to Disabled);
update the examples so they align by either setting
TrustManager.spec.trustManagerConfig.defaultCAPackage.policy to Enabled on the
TrustManager resource (so default CAs are allowed) or change
Bundle.spec.sources[].useDefaultCAs to false; reference the TrustManager and
Bundle symbols when making the change
(TrustManager.spec.trustManagerConfig.defaultCAPackage.policy and
Bundle.spec.sources[].useDefaultCAs).
In @bundle/manifests/operator.openshift.io_trustmanagers.yaml:
- Around line 1218-1223: The description for the TrustManager CRD's
status.conditions is incorrect (it mentions the "istio-csr agent deployment");
update the properties.status.conditions description in the TrustManager CRD
(operator.openshift.io_trustmanagers.yaml) to reference the TrustManager
resource status instead—for example: "conditions holds information about the
current observed state of the TrustManager." Ensure you replace the misleading
"istio-csr agent deployment" phrase with wording that clearly refers to the
TrustManager status.
In @bundle/manifests/trust.cert-manager.io_bundles.yaml:
- Around line 424-428: The CRD description under the `conditions` field
incorrectly says "Known condition types are `Bundle`" but the printer columns
expect `.status.conditions[?(@.type == "Synced")]`; update the `conditions`
description text to reference the actual condition type(s) used (e.g., `Synced`)
or list both expected types, ensuring the `conditions.description` string
matches the `.status.conditions[?(@.type == "Synced")]` usage in the printer
columns.
🧹 Nitpick comments (6)
api/operator/v1alpha1/trustmanager_types.go (1)
59-150: Verifyomitemptybehavior for optional nested structs (may serialize as{})
Fields likeTrustManagerSpec.ControllerConfig,TrustManagerConfig.SecretTargets, andTrustManagerConfig.DefaultCAPackageare non-pointer structs taggedomitempty. In Go’s standardencoding/json, structs are not omitted; if your serialization behaves similarly, these “optional” blocks may always appear in stored objects and patch/apply diffs. If that’s undesirable, consider switching these optional subobjects to pointers (*...).bindata/trust-manager/resources/role_trust-manager:leaderelection.yml (1)
13-23: Restrict Lease RBAC withresourceNamesfor least privilege
The deployment uses--leader-elect=truewithout explicitly specifying a lease name, which defaults to the controller name. AddresourceNames: ["trust-manager"]to the Role to limit lease access to only the required resource.Example
rules: - apiGroups: - "coordination.k8s.io" resources: - "leases" resourceNames: - "trust-manager" verbs: - "get" - "create" - "update" - "watch" - "list"bindata/trust-manager/resources/certificate_trust-manager.yml (1)
13-22: Optional: specify TLS certificate intent/rotation knobs (usages,privateKey,duration)This is structurally fine, but if the trust-manager serving cert has specific requirements, consider explicitly setting
spec.usages(e.g., server auth) andspec.privateKey(algorithm/size/rotationPolicy), and optionallyduration/renewBeforeto avoid relying on defaults across cert-manager versions.hack/update-trust-manager-manifests.sh (2)
37-38: Add safety checks before destructive rm operations.The
rm -rfcommands on Lines 37-38 could be dangerous ifMANIFESTS_PATHor the directory paths are empty or misconfigured, potentially deleting unintended files.🛡️ Recommended safety improvements
# regenerate all bindata -rm -rf bindata/trust-manager/resources -rm -f config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml +if [ -d bindata/trust-manager/resources ]; then + rm -rf bindata/trust-manager/resources +fi +if [ -f config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml ]; then + rm -f config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml +fi
1-5: Verify required tools are installed.The script depends on
helmandyqbut doesn't verify they are available before executing. This could lead to cryptic errors if these tools are not installed.🔧 Optional dependency check
#!/bin/bash set -e +# Check required tools +for cmd in helm yq; do + if ! command -v "$cmd" &> /dev/null; then + echo "Error: $cmd is not installed" >&2 + exit 1 + fi +done + TRUST_MANAGER_VERSION=${1:?"missing trust-manager version. Please specify a version from https://github.com/cert-manager/trust-manager/releases"}bindata/trust-manager/resources/deployment_trust-manager.yml (1)
14-14: Consider high availability for production deployments.The deployment runs a single replica, which means:
- No high availability during pod restarts or node failures
- Single point of failure for webhook validation
- Potential service interruption during upgrades
For production environments, consider:
- Increasing replicas to at least 2 for HA
- Adding pod anti-affinity rules to spread replicas across nodes
- Implementing PodDisruptionBudget to maintain availability during cluster maintenance
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (47)
Makefileapi/operator/v1alpha1/trustmanager_types.goapi/operator/v1alpha1/zz_generated.deepcopy.gobindata/trust-manager/resources/certificate_trust-manager.ymlbindata/trust-manager/resources/clusterrole_trust-manager.ymlbindata/trust-manager/resources/clusterrolebinding_trust-manager.ymlbindata/trust-manager/resources/deployment_trust-manager.ymlbindata/trust-manager/resources/issuer_trust-manager.ymlbindata/trust-manager/resources/role_trust-manager.ymlbindata/trust-manager/resources/role_trust-manager:leaderelection.ymlbindata/trust-manager/resources/rolebinding_trust-manager.ymlbindata/trust-manager/resources/rolebinding_trust-manager:leaderelection.ymlbindata/trust-manager/resources/service_trust-manager-metrics.ymlbindata/trust-manager/resources/service_trust-manager.ymlbindata/trust-manager/resources/serviceaccount_trust-manager.ymlbindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.ymlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_trustmanagers.yamlbundle/manifests/trust.cert-manager.io_bundles.yamlconfig/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlconfig/crd/bases/operator.openshift.io_trustmanagers.yamlconfig/crd/kustomization.yamlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yamlconfig/samples/tech-preview/kustomization.yamlconfig/samples/tech-preview/operator.openshift.io_v1alpha1_trustmanager.yamlconfig/samples/tech-preview/trust.cert-manager.io_v1alpha1_bundle.yamlhack/update-trust-manager-manifests.shpkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/assets/bindata.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/trustmanager.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/trust-manager/resources/rolebinding_trust-manager.ymlpkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gobindata/trust-manager/resources/rolebinding_trust-manager:leaderelection.ymlbindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.ymlpkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.gobindata/trust-manager/resources/role_trust-manager.ymlbindata/trust-manager/resources/issuer_trust-manager.ymlpkg/operator/informers/externalversions/generic.goMakefilepkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.goconfig/samples/tech-preview/kustomization.yamlbindata/trust-manager/resources/role_trust-manager:leaderelection.ymlhack/update-trust-manager-manifests.shconfig/samples/tech-preview/operator.openshift.io_v1alpha1_trustmanager.yamlpkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.goconfig/crd/kustomization.yamlpkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.gopkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.gobindata/trust-manager/resources/clusterrole_trust-manager.ymlconfig/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlbindata/trust-manager/resources/service_trust-manager.ymlapi/operator/v1alpha1/trustmanager_types.goconfig/crd/bases/operator.openshift.io_trustmanagers.yamlpkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/listers/operator/v1alpha1/trustmanager.gobindata/trust-manager/resources/deployment_trust-manager.ymlbundle/manifests/trust.cert-manager.io_bundles.yamlbindata/trust-manager/resources/serviceaccount_trust-manager.ymlbindata/trust-manager/resources/service_trust-manager-metrics.ymlconfig/samples/tech-preview/trust.cert-manager.io_v1alpha1_bundle.yamlpkg/operator/assets/bindata.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.goapi/operator/v1alpha1/zz_generated.deepcopy.gopkg/operator/applyconfigurations/utils.gobindata/trust-manager/resources/certificate_trust-manager.ymlpkg/operator/applyconfigurations/internal/internal.gobindata/trust-manager/resources/clusterrolebinding_trust-manager.ymlpkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.gobundle/manifests/operator.openshift.io_trustmanagers.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yaml
🧬 Code graph analysis (11)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go (1)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.go (2)
TrustManagersGetter(19-21)TrustManagerInterface(24-39)
pkg/operator/informers/externalversions/generic.go (1)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeGroupVersion(29-29)
pkg/operator/informers/externalversions/operator/v1alpha1/interface.go (1)
pkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.go (1)
TrustManagerInformer(21-24)
pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
api/operator/v1alpha1/trustmanager_types.go (1)
SecretTargetsPolicy(221-221)
pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
api/operator/v1alpha1/trustmanager_types.go (1)
DefaultCAPackagePolicy(234-234)
api/operator/v1alpha1/trustmanager_types.go (3)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeBuilder(32-32)api/operator/v1alpha1/istiocsr_types.go (1)
ControllerConfig(299-308)api/operator/v1alpha1/meta.go (1)
ConditionalStatus(43-50)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (1)
api/operator/v1alpha1/trustmanager_types.go (2)
SecretTargetsPolicy(221-221)DefaultCAPackagePolicy(234-234)
pkg/operator/listers/operator/v1alpha1/trustmanager.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (1)
TrustManager(25-31)pkg/operator/listers/operator/v1alpha1/expansion_generated.go (1)
TrustManagerListerExpansion(19-19)api/operator/v1alpha1/groupversion_info.go (1)
Resource(40-42)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
SecretTargetsConfigApplyConfiguration(11-14)api/operator/v1alpha1/trustmanager_types.go (1)
FilterExpiredCertificatesPolicy(210-210)pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
DefaultCAPackageConfigApplyConfiguration(11-13)
api/operator/v1alpha1/zz_generated.deepcopy.go (2)
api/operator/v1alpha1/trustmanager_types.go (1)
TrustManagerList(16-23)api/operator/v1alpha1/meta.go (1)
ConditionalStatus(43-50)
pkg/operator/applyconfigurations/utils.go (7)
api/operator/v1alpha1/groupversion_info.go (1)
SchemeGroupVersion(29-29)pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
DefaultCAPackageConfigApplyConfiguration(11-13)pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
SecretTargetsConfigApplyConfiguration(11-14)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (1)
TrustManagerApplyConfiguration(16-21)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (1)
TrustManagerConfigApplyConfiguration(12-23)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.go (1)
TrustManagerControllerConfigApplyConfiguration(7-10)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go (1)
TrustManagerSpecApplyConfiguration(7-10)
🔇 Additional comments (45)
Makefile (2)
9-9: LGTM! Consistent version management pattern.The addition of
TRUST_MANAGER_VERSIONfollows the established pattern for managing component versions in the Makefile.
151-151: LGTM! Properly integrates trust-manager manifest generation.The addition correctly integrates trust-manager manifest updates into the
update-manifeststarget, following the same pattern as cert-manager and istio-csr.config/crd/kustomization.yaml (1)
13-14: LGTM! CRD resources correctly added.The new TrustManager and Bundle CRD resources are properly integrated into the kustomization configuration, following the existing pattern.
config/samples/tech-preview/kustomization.yaml (1)
3-4: LGTM! Sample resources correctly referenced.The new TrustManager and Bundle sample files are properly integrated into the tech-preview kustomization.
config/samples/tech-preview/operator.openshift.io_v1alpha1_trustmanager.yaml (1)
1-10: LGTM! Well-structured TrustManager sample.The TrustManager sample provides a clear and complete configuration example with sensible defaults. The
metadata.name: clusteraligns with the singleton enforcement pattern for this cluster-scoped resource.pkg/operator/applyconfigurations/internal/internal.go (1)
46-55: LGTM! Generated schema entry is consistent.The TrustManager schema entry follows the same pattern as existing CertManager and IstioCSR entries, ensuring consistent structured-merge-diff behavior.
pkg/operator/listers/operator/v1alpha1/expansion_generated.go (1)
16-19: LGTM! Generated expansion interface is consistent.The TrustManagerListerExpansion interface follows the established pattern, enabling custom method additions to TrustManagerLister.
pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
1-27: LGTM! Generated apply-configuration follows standard patterns.The DefaultCAPackageConfigApplyConfiguration correctly implements the declarative apply-configuration pattern with fluent builder methods.
pkg/operator/listers/operator/v1alpha1/trustmanager.go (1)
1-32: LGTM! Generated lister implementation follows client-go patterns.The TrustManagerLister correctly uses ResourceIndexer with proper resource type initialization, enabling standard list/get operations for TrustManager resources.
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.go (1)
1-44: LGTM! Generated apply-configuration correctly handles map fields.The TrustManagerControllerConfigApplyConfiguration properly implements fluent builders for Labels and Annotations, with correct lazy map initialization and entry merging.
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (3)
10-18: Generated apply-config type looks consistent with client-go apply patterns
Pointer fields + embedded ConditionalStatusApplyConfiguration are what applyconfiguration-gen typically emits; no issues spotted.
29-37: Panic-on-nil inWithConditionsis OK for generated fluent APIs; just ensure callers never pass nil
This is standard for these builders, but it will crash the reconciler if misused.
58-69: Verify policy enum types match the API package definitions
WithSecretTargetsPolicy/WithDefaultCAPackagePolicytakeoperatorv1alpha1.{SecretTargetsPolicy,DefaultCAPackagePolicy}; ensure those are the canonical API types (not duplicated/aliased elsewhere) to avoid subtle conversions or schema drift.api/operator/v1alpha1/trustmanager_types.go (4)
8-10: Scheme registration for TrustManager types is correct
Registers bothTrustManagerandTrustManagerList.
25-57: Singleton enforcement via XValidation is a good guardrail
Requiring.metadata.name == "cluster"is clear and prevents accidental multi-instance creation.
152-176: SecretTargets XValidations look correct and cover both directions
The “Specific requires non-empty authorizedSecrets” and “non-Specific requires empty authorizedSecrets” constraints are complementary and should prevent ambiguous configs.
208-241: Policy enums are well-scoped and explicit
ClearEnabled/DisabledandDisabled/All/Specificoptions with kubebuilder Enum validation.bindata/trust-manager/resources/rolebinding_trust-manager.yml (1)
1-20: RoleBinding wiring is correct (Role -> ServiceAccount in same namespace)
Looks consistent for bindingRole/trust-managertoServiceAccount/trust-managerincert-manager.pkg/operator/informers/externalversions/generic.go (1)
44-45: Informer factory mapping for TrustManager GVR is correct
Usesv1alpha1.SchemeGroupVersion.WithResource("trustmanagers")and returns the expected typed informer.bindata/trust-manager/resources/issuer_trust-manager.yml (1)
1-14: Issuer manifest is straightforward and valid
A namespaced self-signed Issuer is a common pattern for generating serving certs.bindata/trust-manager/resources/service_trust-manager-metrics.yml (1)
14-22: No action required—Service selector labels and metrics port correctly match the Deployment.Service selector
app: cert-manager-trust-managermatches the Deployment pod labels, andtargetPort: 9402matches the container port exposed by the Deployment. The port namemetricsis also consistent.pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go (1)
5-9: LGTM: consistent client-gen expansion hookFollows the existing
*Expansionpattern and keeps future custom methods possible without forking generated code.pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go (1)
13-18: TrustManager client is correctly surfaced as cluster-scopedThe interface embedding and
TrustManagers()accessor match theTrustManagersGetter/TrustManagerInterfacepattern. ThenewTrustManagers(c)constructor correctly implements cluster-scoped behavior: the namespace parameter is an empty string (""at line 53 of trustmanager.go), which is the standard Kubernetes client-go pattern for cluster-scoped resources. The REST path uses no namespace segments.bindata/trust-manager/resources/rolebinding_trust-manager:leaderelection.yml (1)
13-20: The corresponding Role manifest is already included in the bundle.The
role_trust-manager:leaderelection.ymlfile exists in the same directory with the correct name and namespace. Both manifests will be applied together as part of the static bundle, so there is no risk of the install failing due to a missing Role.Likely an incorrect or invalid review comment.
bindata/trust-manager/resources/role_trust-manager.yml (1)
13-21: Thelist/watchpermissions on Secrets are upstream-mandated and necessaryThese role manifests are auto-generated from the upstream cert-manager/trust-manager Helm chart (via
hack/update-trust-manager-manifests.sh), so the RBAC permissions cannot be modified locally without forking. Thelist/watchverbs are required because the Bundle CRD allows referencing Secrets as data sources, and trust-manager must watch these Secrets for changes via Kubernetes informers. The permissions are read-only and appropriately scoped to the cert-manager namespace, matching the CRD design that limits secret access to that namespace by default.bindata/trust-manager/resources/clusterrole_trust-manager.yml (1)
33-37: Verify necessity of broad ConfigMaps permissions.The ClusterRole grants
deletepermission on all ConfigMaps cluster-wide. Ensure that trust-manager requires this level of access, as ConfigMaps may contain sensitive configuration data. If trust-manager only manages specific ConfigMaps (e.g., those with certain labels or in specific namespaces), consider using a more restrictive RBAC setup.pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go (1)
23-25: LGTM!The generated fake client method follows the standard Kubernetes client-go pattern and is consistent with the existing CertManagers() and IstioCSRs() methods.
bindata/trust-manager/resources/serviceaccount_trust-manager.yml (1)
3-3: Verify that service account token automounting is necessary.While
automountServiceAccountToken: trueis likely required for trust-manager to access the Kubernetes API, confirm this is needed. If trust-manager doesn't require API access, consider setting this tofalseto reduce the attack surface.pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.go (1)
1-37: LGTM!The generated fake TrustManager client follows standard Kubernetes client-go patterns and correctly implements the TrustManagerInterface using the FakeClientWithListAndApply pattern. The cluster-scoped nature (empty namespace on line 22) aligns with TrustManager being a cluster-level resource.
pkg/operator/informers/externalversions/operator/v1alpha1/interface.go (1)
15-16: LGTM!The generated informer interface and implementation for TrustManagers correctly follows the Kubernetes informer-gen pattern and is consistent with the existing CertManagers() implementation, both being cluster-scoped resources.
Also applies to: 40-43
bindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.yml (1)
27-30: Verify webhook availability strategy with failurePolicy: Fail.The
failurePolicy: Failsetting means that if the trust-manager webhook becomes unavailable, all Bundle CREATE/UPDATE operations will be blocked. While this is appropriate for validation, ensure that:
- The trust-manager deployment has appropriate readiness probes and health checks
- Deployment strategies account for zero-downtime upgrades
- Documentation warns operators about this behavior
Based on the deployment manifest, the readiness probe is configured (Lines 40-45 in deployment_trust-manager.yml), which is good. However, verify that the operator handles webhook availability during upgrades to prevent blocking critical operations.
bindata/trust-manager/resources/deployment_trust-manager.yml (1)
67-76: LGTM! Strong security posture.The security context configuration follows best practices:
- Read-only root filesystem prevents runtime modifications
- Non-root execution reduces attack surface
- All capabilities dropped follows least privilege
- RuntimeDefault seccomp profile adds syscall filtering
bindata/trust-manager/resources/service_trust-manager.yml (1)
1-22: LGTM! Service configuration is correct.The Service configuration properly:
- Maps port 443 (standard HTTPS) to targetPort 6443 (webhook container port)
- Uses selector
app: cert-manager-trust-managerwhich matches the Deployment pod labels- Exposes the webhook endpoint for the ValidatingWebhookConfiguration
bindata/trust-manager/resources/clusterrolebinding_trust-manager.yml (1)
1-19: LGTM! RBAC binding is correctly configured.The ClusterRoleBinding properly:
- References the trust-manager ClusterRole
- Binds to the trust-manager ServiceAccount in the cert-manager namespace
- Uses consistent naming across all RBAC resources
- Follows standard Kubernetes RBAC patterns
config/manifests/bases/cert-manager-operator.clusterserviceversion.yaml (1)
109-122: LGTM - New CRD definitions added correctly.The Bundle and TrustManager CRD entries follow the established pattern used by existing CRDs like IstioCSR. The TrustManager singleton semantics (name must be 'cluster') and deployment namespace are clearly documented.
Also applies to: 156-156
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go (1)
1-32: LGTM - Standard generated apply configuration.Auto-generated file following the established apply configuration pattern used throughout the codebase.
pkg/operator/assets/bindata.go (1)
3088-3125: LGTM - Trust-manager static manifests properly embedded.The trust-manager resources include appropriate security configurations:
- Non-root execution with read-only filesystem
- Dropped capabilities and RuntimeDefault seccomp profile
- Least-privilege RBAC with scoped secret access only in the trust namespace
pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
1-38: LGTM - Standard generated apply configuration.Auto-generated file with proper handling of the Policy field and list-append semantics for AuthorizedSecrets.
config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml (1)
1-476: LGTM - Well-structured Bundle CRD.The CRD follows Kubernetes conventions with comprehensive OpenAPI validation, proper list-type annotations, and status subresource support. The JKS deprecation notice provides clear migration guidance to PKCS12.
pkg/operator/applyconfigurations/utils.go (1)
37-68: LGTM - ForKind updated with new TrustManager types.Auto-generated additions follow the established pattern, adding all necessary apply configuration type mappings for the TrustManager API surface.
pkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.go (1)
1-85: LGTM - Standard generated informer for TrustManager.Auto-generated informer following established patterns with both legacy and context-aware list/watch implementations.
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (1)
14-246: Generated TrustManager apply-configuration looks consistent and completepkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (1)
10-117: Generated TrustManagerConfig apply-configuration looks goodapi/operator/v1alpha1/zz_generated.deepcopy.go (1)
281-650: > Likely an incorrect or invalid review comment.pkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.go (1)
1-58: Generated code follows standard Kubernetes client patterns — LGTM.This is auto-generated client code (
client-gen) that correctly implements the typed client for the cluster-scoped TrustManager resource. The empty namespace on line 53 is appropriate for cluster-scoped resources, and the generic client wiring is consistent with modern client-go conventions.
|
@chiragkyal: This pull request references CM-862 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 either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. This pull request references CM-863 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 either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. 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. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
168-176: Avoid using a real-looking email in alm-examples.Consider swapping
"aos-ci-cd@redhat.com"for a non-identifying placeholder (e.g.,user@example.com) since this is user-facing sample content.Proposed fix
- "email": "aos-ci-cd@redhat.com", + "email": "user@example.com",
🤖 Fix all issues with AI agents
In @bundle/manifests/operator.openshift.io_trustmanagers.yaml:
- Around line 1218-1223: Update the CRD description for TrustManager: replace
the incorrect phrase "istio-csr agent deployment" in the status.conditions
description with a TrustManager-specific phrase (e.g., "current state of the
TrustManager" or "current state of the TrustManager controller/deployment") so
the 'status.conditions' entry accurately describes TrustManager state; edit the
properties entry for status -> conditions in the TrustManager CRD to reflect
this corrected wording.
In @bundle/manifests/trust.cert-manager.io_bundles.yaml:
- Around line 23-39: The manifest's additionalPrinterColumns describe the
condition as "Bundle" but the jsonPath targets condition type "Synced"; update
the schema descriptions to reference the "Synced" condition (or make the
jsonPath and condition type match) so the docs and printer columns are
consistent—specifically edit the additionalPrinterColumns entries under
additionalPrinterColumns that use jsonPath .status.conditions[?(@.type ==
"Synced")] (and the duplicate block mentioned around the later occurrence) to
replace "Bundle" wording with "Synced" (or change the jsonPath to point to the
"Bundle" condition if that is the intended type).
In @config/crd/bases/operator.openshift.io_trustmanagers.yaml:
- Around line 1218-1223: The CRD description for TrustManager status.conditions
incorrectly references “istio-csr”; update the API type comment that generates
this field (the Go struct comment for TrustManagerStatus or the Conditions field
on TrustManager) to mention TrustManager instead of istio-csr, then re-run
controller-gen to regenerate the CRD base and bundle so the description in
status.conditions is corrected across outputs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (24)
api/operator/v1alpha1/zz_generated.deepcopy.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_trustmanagers.yamlbundle/manifests/trust.cert-manager.io_bundles.yamlconfig/crd/bases/operator.openshift.io_trustmanagers.yamlpkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/trustmanager.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.go
- pkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/listers/operator/v1alpha1/trustmanager.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.gobundle/manifests/trust.cert-manager.io_bundles.yamlpkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.gobundle/manifests/operator.openshift.io_trustmanagers.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_trustmanagers.yamlpkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.goapi/operator/v1alpha1/zz_generated.deepcopy.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.gopkg/operator/applyconfigurations/utils.gopkg/operator/applyconfigurations/internal/internal.go
🧬 Code graph analysis (9)
pkg/operator/listers/operator/v1alpha1/trustmanager.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (1)
TrustManager(25-31)pkg/operator/listers/operator/v1alpha1/expansion_generated.go (1)
TrustManagerListerExpansion(19-19)api/operator/v1alpha1/groupversion_info.go (1)
Resource(40-42)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (1)
api/operator/v1alpha1/trustmanager_types.go (2)
SecretTargetsPolicy(221-221)DefaultCAPackagePolicy(234-234)
pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
api/operator/v1alpha1/trustmanager_types.go (1)
DefaultCAPackagePolicy(234-234)
pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
api/operator/v1alpha1/trustmanager_types.go (1)
SecretTargetsPolicy(221-221)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (2)
TrustManagerConfig(27-29)TrustManagerConfigApplyConfiguration(12-23)api/operator/v1alpha1/istiocsr_types.go (1)
ControllerConfig(299-308)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.go (1)
TrustManagerControllerConfigApplyConfiguration(7-10)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
SecretTargetsConfigApplyConfiguration(11-14)api/operator/v1alpha1/trustmanager_types.go (1)
FilterExpiredCertificatesPolicy(210-210)pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
DefaultCAPackageConfigApplyConfiguration(11-13)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go (1)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.go (1)
TrustManagerInterface(24-39)
api/operator/v1alpha1/zz_generated.deepcopy.go (7)
pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
DefaultCAPackageConfig(17-19)pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
SecretTargetsConfig(18-20)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (1)
TrustManagerConfig(27-29)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.go (1)
TrustManagerControllerConfig(14-16)api/operator/v1alpha1/trustmanager_types.go (1)
TrustManagerList(16-23)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go (1)
TrustManagerSpec(14-16)api/operator/v1alpha1/meta.go (1)
ConditionalStatus(43-50)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (3)
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go (1)
TrustManagerSpecApplyConfiguration(7-10)pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (1)
TrustManagerStatusApplyConfiguration(12-18)pkg/operator/applyconfigurations/internal/internal.go (1)
Parser(12-21)
🔇 Additional comments (28)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go (1)
8-9: LGTM! Consistent with existing expansion patterns.The
TrustManagerExpansioninterface follows the same pattern asCertManagerExpansionandIstioCSRExpansion, providing a standard extension point for the generated client.pkg/operator/informers/externalversions/operator/v1alpha1/interface.go (2)
15-16: LGTM! Standard informer interface addition.The
TrustManagers()method addition follows the established pattern for informer interfaces.
40-43: LGTM! Correctly implements cluster-scoped informer.The implementation follows the same pattern as
CertManagers(), correctly omitting the namespace parameter to indicate that TrustManager is a cluster-scoped resource.pkg/operator/listers/operator/v1alpha1/trustmanager.go (1)
1-32: LGTM! Standard generated lister implementation.The
TrustManagerListerimplementation follows established patterns:
- Interface provides standard List/Get methods
- Implementation uses
listers.ResourceIndexergeneric- Constructor properly instantiates with resource type
All components are consistent with existing listers for CertManager and IstioCSR.
pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
1-27: LGTM! Standard apply configuration pattern.The
DefaultCAPackageConfigApplyConfigurationfollows Kubernetes conventions for apply configurations:
- Pointer field for optional
Policyvalue- Empty constructor
- Fluent builder pattern with
WithPolicymethodThe implementation correctly takes the value parameter and stores its address, enabling proper omitempty/nil semantics for declarative apply operations.
api/operator/v1alpha1/zz_generated.deepcopy.go (8)
281-294: LGTM! Correct deepcopy for simple struct.The
DefaultCAPackageConfigdeepcopy implementation correctly performs a simple struct copy, which is appropriate since the type contains only aPolicyfield (a string type) with no nested pointers or slices requiring deep copying.
553-571: LGTM! Proper slice deep copy.The
SecretTargetsConfigdeepcopy correctly handles theAuthorizedSecretsstring slice with proper nil checks, allocation, and copy operations.
588-613: LGTM! Standard Kubernetes CR deepcopy pattern.The
TrustManagerdeepcopy implementation follows the standard pattern for custom resources:
- Properly calls
DeepCopyIntoon nestedObjectMeta,Spec, andStatus- Implements
DeepCopyObjectto satisfy theruntime.Objectinterface
615-650: LGTM! Comprehensive deepcopy with proper handling of complex fields.The
TrustManagerConfigdeepcopy correctly handles all field types:
- Nested structs via
DeepCopyIntocalls- Pointer fields with nil checks and allocation
- Slices with proper allocation and element-wise copying
- Maps with proper allocation and entry copying
All patterns align with generated deepcopy best practices.
652-679: LGTM! Proper map deep copying.The
TrustManagerControllerConfigdeepcopy correctly handles bothLabelsandAnnotationsmaps with proper nil checks, allocation, and entry copying.
681-711: LGTM! Standard list deepcopy implementation.The
TrustManagerListdeepcopy follows Kubernetes conventions for list types:
- Proper handling of
ListMeta- Correct allocation and element-wise copying of the
ItemssliceDeepCopyObjectimplementation forruntime.Objectinterface
713-728: LGTM! Correct delegation to nested types.The
TrustManagerSpecdeepcopy properly delegates to theDeepCopyIntomethods of its embeddedTrustManagerConfigandControllerConfigfields.
730-744: LGTM! Proper status deepcopy.The
TrustManagerStatusdeepcopy correctly delegates toConditionalStatus.DeepCopyInto, which properly handles the nestedConditionsslice.pkg/operator/applyconfigurations/internal/internal.go (1)
46-55: Schema entry for TrustManager matches existing untyped/atomic+separable patterns (LGTM).pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go (1)
1-32: Generated TrustManagerSpec apply-configuration looks consistent with client-go apply patterns.pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
1-38: Generated SecretTargetsConfig apply-configuration is straightforward and consistent.pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (2)
1-25: Remaining TrustManagerStatus apply-configuration fields/setters are consistent with generated patterns.Also applies to: 39-69
26-37: TheWithConditionspanic-on-nil behavior is consistent with all other generated applyconfiguration builders in the repository. All fourWithConditionsimplementations (TrustManagerStatus,ConditionalStatus,IstioCSRStatus, andCertManagerStatus) use the identical pattern. This is the standard convention for these generated builders.pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go (1)
23-25: TrustManager client signature is correct. Verified that the fakeTrustManagers()method matches the real interface exactly—both returnTrustManagerInterfacewithout a namespace parameter. TrustManager is cluster-scoped per the+genclient:nonNamespacedmarker, and the status subresource is enabled (+kubebuilder:subresource:status), so the generatedUpdateStatus()andApplyStatus()methods are available. The wiring is consistent withCertManagers().pkg/operator/applyconfigurations/utils.go (1)
16-71: ForKind GVK → ApplyConfiguration mappings look correct.The new cases follow the existing pattern and returning
nilfor unknown kinds is consistent with current behavior.pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (1)
10-117: TrustManagerConfig apply-configuration builder looks good.Fluent methods correctly set pointers and safely merge
NodeSelectorentries.pkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.go (1)
17-58: TrustManager client implementation is correct and consistent with other cluster-scoped resources.The cluster-scoped namespace
""usage matches CertManager and other cluster-scoped clients in the repository. The code follows the established pattern correctly.bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
611-648: The RBAC additions for TrustManager are not required. While TrustManager is declared as an owned CRD in the CSV, the operator does not actually reconcile TrustManager resources. The only registered controller is for IstioCSR (seepkg/operator/setup_manager.go). The TrustManager client, informer, and lister code are auto-generated infrastructure that is never invoked by any controller code in the operator.Likely an incorrect or invalid review comment.
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (5)
1-21: LGTM! Generated apply configuration follows standard Kubernetes patterns.The struct definition correctly embeds
TypeMetaApplyConfigurationandObjectMetaApplyConfigurationfrom client-go, with optionalSpecandStatusfields referencing the related apply configuration types. This is consistent with the Kubernetes apply configuration generator output.
23-31: LGTM! Constructor correctly initializes the TrustManager apply configuration.
33-66: LGTM! Extraction functions follow the standard Kubernetes apply configuration pattern.The use of
managedfields.ExtractIntowith the internal parser and proper error handling is correct. The type pathcom.github.openshift.cert-manager-operator.api.operator.v1alpha1.TrustManageraligns with the module path convention.
67-241: LGTM! Fluent builder methods follow standard Kubernetes apply configuration patterns.The
IsApplyConfiguration()marker method and allWith*builders are correctly implemented. The nil-check with panic inWithOwnerReferencesis consistent with the Kubernetes generated code convention.
243-263: LGTM! Getter methods are correctly implemented.The getters properly retrieve values from the embedded configurations. The
ensureObjectMetaApplyConfigurationExists()call inGetNameandGetNamespacefollows the standard generated pattern to handle cases whereObjectMetaApplyConfigurationhasn't been initialized.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
api/operator/v1alpha1/meta.gobundle/manifests/operator.openshift.io_istiocsrs.yamlbundle/manifests/operator.openshift.io_trustmanagers.yamlconfig/crd/bases/operator.openshift.io_istiocsrs.yamlconfig/crd/bases/operator.openshift.io_trustmanagers.yaml
✅ Files skipped from review due to trivial changes (2)
- config/crd/bases/operator.openshift.io_istiocsrs.yaml
- bundle/manifests/operator.openshift.io_istiocsrs.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- bundle/manifests/operator.openshift.io_trustmanagers.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/operator/v1alpha1/meta.goconfig/crd/bases/operator.openshift.io_trustmanagers.yaml
🔇 Additional comments (4)
api/operator/v1alpha1/meta.go (1)
43-51: LGTM - Documentation update correctly generalizes the type.The change from "istio-csr agent deployment" to "operand deployment" appropriately broadens the scope of ConditionalStatus for reuse across multiple operands (e.g., istio-csr, trust-manager).
config/crd/bases/operator.openshift.io_trustmanagers.yaml (3)
1-38: CRD definition + singleton intent are clear and consistent.Group/name/scope align with a cluster-scoped singleton API, and the description sets expectations well.
1308-1314: Singleton enforcement via CEL is a good guardrail.The
.metadata.name == 'cluster'validation is the right place to prevent accidental multi-instance creation.
1123-1149: RBAC is already appropriately restricted to cluster-admin by design—no additional controls needed.While
secretTargets.policy=Allgrants cluster-wide Secret write capability to trust-manager, the TrustManager CR itself can only be updated by cluster-admin users. No explicit RBAC rules grant trustmanagers permissions to regular users; unlike CertManager (which has a certmanager-editor-role), no trustmanager-editor-role exists. The operator's own manager-role does not include permissions for trustmanagers. By default Kubernetes/OpenShift RBAC model, only cluster-admin can manage cluster-scoped resources without explicit role bindings, so no privilege escalation path exists unless the user already has cluster-admin privileges.
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
trust-manager-api
branch
from
093cbd2 to
71445da
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @api/operator/v1alpha1/trustmanager_types.go:
- Around line 59-154: The nested config structs should be pointers so omitempty
works and to preserve unset vs empty states: change
TrustManagerSpec.TrustManagerConfig to *TrustManagerConfig and
TrustManagerSpec.ControllerConfig to *TrustManagerControllerConfig, and inside
TrustManagerConfig change SecretTargets to *SecretTargetsConfig and
DefaultCAPackage to *DefaultCAPackageConfig (keep json:"...,omitempty" tags).
After updating the types (and any constructors/usage), re-run the code
generators to refresh deepcopy, CRDs and applyconfig.
In @bundle/manifests/cert-manager-operator.clusterserviceversion.yaml:
- Around line 236-269: The alm-examples show a Bundle with "useDefaultCAs: true"
but the TrustManager example (kind: TrustManager, metadata.name: "cluster") does
not enable the corresponding default CA package; update the TrustManager's
spec.trustManagerConfig to set the defaultCAPackage flag (e.g.,
defaultCAPackage: true) so the TrustManager example matches the Bundle's
useDefaultCAs usage and will work out-of-the-box; ensure you modify the
TrustManager resource (trustManagerConfig) rather than the Bundle and keep
existing fields like trustNamespace and logLevel intact.
- Around line 313-319: The CSV is missing RBAC rules for the owned CRDs; add
permission rules in the CSV's permissions section granting appropriate verbs
(get,list,watch,create,update,patch,delete) for API group operator.openshift.io
on resources trustmanagers and subresources trustmanagers/status and
trustmanagers/finalizers, and for API group trust.cert-manager.io on resources
bundles and subresources bundles/status and bundles/finalizers (include
create/update/patch/get/list/watch and delete as appropriate); ensure the rules
reference the exact resource names trustmanagers and bundles and include
subresource entries (status and finalizers) so the operator can manage those
CRDs it declares as owned.
In @config/crd/bases/operator.openshift.io_trustmanagers.yaml:
- Around line 1205-1219: spec.trustManagerConfig.trustNamespace currently only
has length checks; add a DNS_LABEL pattern validation so Kubernetes rejects
invalid namespace names (use regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$). Update the
OpenAPI validation for trustNamespace (the x-kubernetes-validations entry for
trustNamespace) to include this pattern rule, or preferably add the
corresponding kubebuilder annotation on the Go field for
Spec.TrustManagerConfig.TrustNamespace (e.g.,
+kubebuilder:validation:Pattern="<DNS_LABEL>") and re-run controller-gen to
regenerate the CRD so the x-kubernetes-validations and schema include the
DNS_LABEL pattern.
🧹 Nitpick comments (1)
api/operator/v1alpha1/trustmanager_types.go (1)
92-103: Consider adding a namespace-name Pattern validation forTrustNamespace(not just length).Proposed kubebuilder annotation
// +kubebuilder:validation:MinLength:=1 // +kubebuilder:validation:MaxLength:=63 + // +kubebuilder:validation:Pattern:="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" // +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="trustNamespace is immutable once set"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (50)
Makefileapi/operator/v1alpha1/meta.goapi/operator/v1alpha1/trustmanager_types.goapi/operator/v1alpha1/zz_generated.deepcopy.gobindata/trust-manager/resources/certificate_trust-manager.ymlbindata/trust-manager/resources/clusterrole_trust-manager.ymlbindata/trust-manager/resources/clusterrolebinding_trust-manager.ymlbindata/trust-manager/resources/deployment_trust-manager.ymlbindata/trust-manager/resources/issuer_trust-manager.ymlbindata/trust-manager/resources/role_trust-manager.ymlbindata/trust-manager/resources/role_trust-manager:leaderelection.ymlbindata/trust-manager/resources/rolebinding_trust-manager.ymlbindata/trust-manager/resources/rolebinding_trust-manager:leaderelection.ymlbindata/trust-manager/resources/service_trust-manager-metrics.ymlbindata/trust-manager/resources/service_trust-manager.ymlbindata/trust-manager/resources/serviceaccount_trust-manager.ymlbindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.ymlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_istiocsrs.yamlbundle/manifests/operator.openshift.io_trustmanagers.yamlbundle/manifests/trust.cert-manager.io_bundles.yamlconfig/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlconfig/crd/bases/operator.openshift.io_istiocsrs.yamlconfig/crd/bases/operator.openshift.io_trustmanagers.yamlconfig/crd/kustomization.yamlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yamlconfig/samples/tech-preview/kustomization.yamlconfig/samples/tech-preview/operator.openshift.io_v1alpha1_trustmanager.yamlconfig/samples/tech-preview/trust.cert-manager.io_v1alpha1_bundle.yamlhack/update-trust-manager-manifests.shpkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/assets/bindata.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/trustmanager.go
🚧 Files skipped from review as they are similar to previous changes (22)
- bindata/trust-manager/resources/certificate_trust-manager.yml
- config/crd/kustomization.yaml
- bindata/trust-manager/resources/deployment_trust-manager.yml
- config/samples/tech-preview/trust.cert-manager.io_v1alpha1_bundle.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- bindata/trust-manager/resources/rolebinding_trust-manager.yml
- hack/update-trust-manager-manifests.sh
- bindata/trust-manager/resources/clusterrole_trust-manager.yml
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- bindata/trust-manager/resources/serviceaccount_trust-manager.yml
- pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerspec.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/trustmanager.go
- bindata/trust-manager/resources/issuer_trust-manager.yml
- config/samples/tech-preview/operator.openshift.io_v1alpha1_trustmanager.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagercontrollerconfig.go
- bundle/manifests/operator.openshift.io_istiocsrs.yaml
- bindata/trust-manager/resources/rolebinding_trust-manager:leaderelection.yml
- bindata/trust-manager/resources/role_trust-manager.yml
- bindata/trust-manager/resources/clusterrolebinding_trust-manager.yml
- pkg/operator/applyconfigurations/internal/internal.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.gopkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.goconfig/samples/tech-preview/kustomization.yamlpkg/operator/informers/externalversions/generic.gopkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.goMakefilepkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.gopkg/operator/applyconfigurations/utils.gopkg/operator/assets/bindata.goconfig/crd/bases/operator.openshift.io_trustmanagers.yamlconfig/crd/bases/operator.openshift.io_istiocsrs.yamlpkg/operator/listers/operator/v1alpha1/trustmanager.goapi/operator/v1alpha1/zz_generated.deepcopy.gobundle/manifests/operator.openshift.io_trustmanagers.yamlconfig/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlapi/operator/v1alpha1/meta.gobindata/trust-manager/resources/role_trust-manager:leaderelection.ymlpkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/trust.cert-manager.io_bundles.yamlapi/operator/v1alpha1/trustmanager_types.gobindata/trust-manager/resources/service_trust-manager.ymlbindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.ymlpkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.gobindata/trust-manager/resources/service_trust-manager-metrics.ymlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yamlpkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go
🔇 Additional comments (31)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go (1)
8-9: LGTM!The new
TrustManagerExpansioninterface follows the established pattern for the existingCertManagerExpansionandIstioCSRExpansioninterfaces. This is standard client-gen boilerplate that correctly complements the new TrustManager API being introduced in this PR.Makefile (1)
9-9: LGTM!The new
TRUST_MANAGER_VERSIONvariable and the script invocation inupdate-manifestsfollow the established patterns forCERT_MANAGER_VERSIONandISTIO_CSR_VERSION.Also applies to: 151-151
pkg/operator/informers/externalversions/generic.go (1)
44-45: LGTM!The new case for
trustmanagersfollows the established pattern used bycertmanagersandistiocsrs. This is generated code that correctly integrates the TrustManager informer.bindata/trust-manager/resources/role_trust-manager:leaderelection.yml (1)
1-23: LGTM!The Role manifest correctly defines leader election permissions for the trust-manager ServiceAccount. The rules granting
get,create,update,watch,listonleasesin thecoordination.k8s.ioAPI group are standard for Kubernetes leader election.Note: The AI summary incorrectly described this as a RoleBinding, but the manifest correctly defines a Role.
bindata/trust-manager/resources/service_trust-manager-metrics.yml (1)
1-22: LGTM!The metrics Service is correctly configured with ClusterIP type, exposing port 9402 for Prometheus metrics scraping. The selector matches the expected deployment labels.
pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_trustmanager.go (1)
1-37: LGTM!This is generated client code that correctly implements the fake TrustManager client for testing. The empty namespace string (line 22) correctly indicates that TrustManager is a cluster-scoped resource, consistent with the PR objectives stating it's a "cluster-scoped TrustManager Custom Resource Definition (singleton named 'cluster')".
config/crd/bases/operator.openshift.io_istiocsrs.yaml (1)
1379-1382: Doc-only CRD description tweak looks good.pkg/operator/listers/operator/v1alpha1/trustmanager.go (1)
12-32: Verify the resource string format in the listers.New() call is consistent with other generated listers.The code uses
Resource("trustmanager")(singular). Confirm this matches the pattern used by other listers in this repository. If the convention is to use plural form (e.g.,Resource("trustmanagers")), the resource string should be corrected to ensure consistency in NotFound error messages and resource naming across the codebase.config/samples/tech-preview/kustomization.yaml (1)
1-4: Verify the referenced sample files exist in the repository. Ensure bothoperator.openshift.io_v1alpha1_trustmanager.yamlandtrust.cert-manager.io_v1alpha1_bundle.yamlare present inconfig/samples/tech-preview/before merging, as broken file references will cause the kustomization to fail.bindata/trust-manager/resources/service_trust-manager.yml (1)
1-22: Verify selector/ports match the trust-manager Deployment (labels + container port 6443). If the Deployment doesn't expose 6443 or uses different labels, the webhook service will blackhole traffic.bindata/trust-manager/resources/validatingwebhookconfiguration_trust-manager.yml (1)
1-35: CA injection target and webhook service/path alignment are correctly configured. Certificatetrust-managerexists incert-managernamespace with secrettrust-manager-tls, Servicetrust-manageris properly defined incert-manager, and the Deployment uses the upstream trust-manager image (v0.20.3) on port 6443. The ValidatingWebhookConfiguration correctly references the service with the expected webhook path. WithfailurePolicy: Fail, ensure the trust-manager Pod is healthy during rollouts to prevent blocking Bundle create/update operations.pkg/operator/applyconfigurations/operator/v1alpha1/defaultcapackageconfig.go (1)
1-27: LGTM!This is standard generated apply-configuration code following the expected fluent builder pattern for the
DefaultCAPackageConfigtype.api/operator/v1alpha1/meta.go (1)
43-51: LGTM!The
ConditionalStatustype is well-defined with appropriate kubebuilder markers for strategic merge patch semantics on the conditions field. This enables proper condition management across operand deployments.config/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml (1)
1-477: LGTM!The Bundle CRD is well-structured with:
- Proper cluster-scoped configuration
- Comprehensive OpenAPI v3 schema with appropriate validations
- Correct strategic merge markers for status conditions (
x-kubernetes-list-map-keys,x-kubernetes-list-type: map)- Useful printer columns for
kubectl get bundlesoutput- The
helm.sh/resource-policy: keepannotation appropriately prevents accidental CRD deletion during upgradespkg/operator/assets/bindata.go (4)
3088-3110: LGTM!The trust-manager Certificate manifest correctly references the self-signed Issuer and configures TLS for the webhook service.
3222-3307: LGTM - Deployment manifest is well-configured.The trust-manager Deployment includes proper security hardening:
runAsNonRoot: truewithseccompProfile: RuntimeDefault- Dropped all capabilities with read-only root filesystem
- Leader election enabled for HA scenarios
- Appropriate node selector for Linux nodes
The
resources: {}placeholder will be populated by the operator based on TrustManager CR configuration.
3614-3664: LGTM!The ValidatingWebhookConfiguration is properly configured with:
- CA injection via
cert-manager.io/inject-ca-fromannotationfailurePolicy: Failensuring invalid Bundle resources are rejected- Appropriate timeout and admission review versions
3781-3793: LGTM!The
_bindatamap and_bintreestructure are correctly updated to include all trust-manager resource assets, enabling proper asset lookup and directory traversal.Also applies to: 3916-3932
pkg/operator/applyconfigurations/operator/v1alpha1/secrettargetsconfig.go (1)
1-38: LGTM!This is standard generated apply-configuration code. The fluent builder pattern for
SecretTargetsConfigcorrectly handles both thePolicyfield (single value) andAuthorizedSecretsfield (append semantics for multiple values).pkg/operator/applyconfigurations/utils.go (1)
37-68: LGTM!The generated
ForKindswitch cases for the new TrustManager-related types follow the established pattern and are correctly wired to return the appropriateApplyConfigurationtypes.pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerstatus.go (1)
1-69: LGTM!The generated
TrustManagerStatusApplyConfigurationtype and its builder methods follow the standard Kubernetes apply configuration patterns. The nil-value panic inWithConditionsis consistent with other generated apply configurations.pkg/operator/informers/externalversions/operator/v1alpha1/trustmanager.go (1)
1-85: LGTM!The generated TrustManager informer correctly implements the
SharedIndexInformerpattern with both legacy and context-aware list/watch functions. Thecache.NamespaceIndexindefaultInformeris standard generated code and works correctly for cluster-scoped resources.bundle/manifests/operator.openshift.io_trustmanagers.yaml (1)
1-1326: LGTM!The TrustManager CRD is well-structured with:
- Proper singleton enforcement via CEL validation (
self.metadata.name == 'cluster')- Immutability protection for
trustNamespace- Sound CEL validation for
secretTargets.authorizedSecretsensuring policy consistency- Appropriate bounds on configurable values (logLevel 1-5, maxLength/maxItems constraints)
- Status subresource and informative printer columns
bundle/manifests/trust.cert-manager.io_bundles.yaml (1)
1-506: LGTM!The Bundle CRD correctly defines the trust-manager bundle resource with:
- Comprehensive schema for sources (configMap, secret, inLine, useDefaultCAs) and targets
- Appropriate validation constraints (minLength, maxItems: 100)
- JKS format deprecation notice preserved from upstream
- Proper status subresource with conditions
helm.sh/resource-policy: keepannotation for lifecycle managementconfig/crd/bases/operator.openshift.io_trustmanagers.yaml (2)
35-39: Singleton enforcement via CEL XValidation looks correct.Also applies to: 1314-1316
1124-1161: Verify namespace scoping for SecretTargets "Custom" policy implementation.The API defines
authorizedSecretsas a list of secret names only (no namespace information). If RBAC enforcement translates these directly to ClusterRoleresourceNames, secrets with the same name in different namespaces cannot be distinguished, creating potential cross-namespace access risks. Confirm whether namespace scoping is applied during RBAC rule generation, or document the security boundary clearly if same-name collisions across namespaces are intentional.api/operator/v1alpha1/zz_generated.deepcopy.go (1)
281-295: Generated DeepCopy additions for TrustManager types look consistent (nil checks, slice/map copies).Also applies to: 553-572, 588-745
config/manifests/bases/cert-manager-operator.clusterserviceversion.yaml (1)
109-122: Owned CRD entries + keyword for trust-manager are correctly added.Also applies to: 156-156
pkg/operator/applyconfigurations/operator/v1alpha1/trustmanagerconfig.go (1)
10-117: Apply-configuration builder looks standard and correct.pkg/operator/applyconfigurations/operator/v1alpha1/trustmanager.go (2)
14-31: Generated apply surface wiring (constructor + WithSpec/WithStatus) looks good.Also applies to: 227-241
55-66: The type stringcom.github.openshift.cert-manager-operator.api.operator.v1alpha1.TrustManageris properly registered in the parser schema. This is auto-generated code fromapplyconfiguration-gen, and the type is explicitly defined in theschemaYAMLalongsideCertManagerandIstioCSRwith identical structure. No runtime issue exists.Likely an incorrect or invalid review comment.
| // TrustManagerSpec defines the desired state of TrustManager. | ||
| // Note: trust-manager operand is always deployed in the cert-manager namespace. | ||
| type TrustManagerSpec struct { | ||
| // trustManagerConfig configures the trust-manager operand's behavior. | ||
| // +kubebuilder:validation:Required | ||
| // +required | ||
| TrustManagerConfig TrustManagerConfig `json:"trustManagerConfig"` | ||
|
|
||
| // controllerConfig configures the operator's behavior for resource creation. | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| ControllerConfig TrustManagerControllerConfig `json:"controllerConfig,omitempty"` | ||
| } | ||
|
|
||
| // TrustManagerConfig configures the trust-manager operand's behavior. | ||
| type TrustManagerConfig struct { | ||
| // logLevel configures the verbosity of trust-manager logging. | ||
| // Follows [Kubernetes logging guidelines](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#what-method-to-use). | ||
| // +kubebuilder:default:=1 | ||
| // +kubebuilder:validation:Minimum:=1 | ||
| // +kubebuilder:validation:Maximum:=5 | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| LogLevel int32 `json:"logLevel,omitempty"` | ||
|
|
||
| // logFormat specifies the output format for trust-manager logging. | ||
| // Supported formats are "text" and "json". | ||
| // +kubebuilder:validation:Enum:="text";"json" | ||
| // +kubebuilder:default:="text" | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| LogFormat string `json:"logFormat,omitempty"` | ||
|
|
||
| // trustNamespace is the namespace where trust-manager looks for trust sources | ||
| // (ConfigMaps and Secrets containing CA certificates). | ||
| // Defaults to "cert-manager" if not specified. | ||
| // This field is immutable once set. | ||
| // This field can have a maximum of 63 characters. | ||
| // +kubebuilder:default:="cert-manager" | ||
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=63 | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="trustNamespace is immutable once set" | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| TrustNamespace string `json:"trustNamespace,omitempty"` | ||
|
|
||
| // secretTargets configures whether trust-manager can write trust bundles to Secrets. | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| SecretTargets SecretTargetsConfig `json:"secretTargets,omitempty"` | ||
|
|
||
| // filterExpiredCertificates controls whether trust-manager filters out | ||
| // expired certificates from trust bundles before distributing them. | ||
| // When set to "Enabled", expired certificates are removed from bundles. | ||
| // When set to "Disabled", expired certificates are included (default behavior). | ||
| // +kubebuilder:default:="Disabled" | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| FilterExpiredCertificates FilterExpiredCertificatesPolicy `json:"filterExpiredCertificates,omitempty"` | ||
|
|
||
| // defaultCAPackage configures the default CA package for trust-manager. | ||
| // When enabled, the operator will use OpenShift's trusted CA bundle injection mechanism. | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| DefaultCAPackage DefaultCAPackageConfig `json:"defaultCAPackage,omitempty"` | ||
|
|
||
| // resources defines the compute resource requirements for the trust-manager pod. | ||
| // ref: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| Resources corev1.ResourceRequirements `json:"resources,omitempty"` | ||
|
|
||
| // affinity defines scheduling constraints for the trust-manager pod. | ||
| // ref: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/ | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| Affinity *corev1.Affinity `json:"affinity,omitempty"` | ||
|
|
||
| // tolerations allows the trust-manager pod to be scheduled on tainted nodes. | ||
| // ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MinItems:=0 | ||
| // +kubebuilder:validation:MaxItems:=50 | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| Tolerations []corev1.Toleration `json:"tolerations,omitempty"` | ||
|
|
||
| // nodeSelector restricts which nodes the trust-manager pod can be scheduled on. | ||
| // ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/ | ||
| // +mapType=atomic | ||
| // +kubebuilder:validation:MinProperties:=0 | ||
| // +kubebuilder:validation:MaxProperties:=50 | ||
| // +kubebuilder:validation:Optional | ||
| // +optional | ||
| NodeSelector map[string]string `json:"nodeSelector,omitempty"` | ||
| } |
There was a problem hiding this comment.
Make optional nested configs pointers; current omitempty on non-pointer structs likely won’t omit and loses “unset vs empty”.
Proposed API shape change (then re-run generators to refresh CRDs/deepcopy/applyconfig)
type TrustManagerSpec struct {
@@
- ControllerConfig TrustManagerControllerConfig `json:"controllerConfig,omitempty"`
+ ControllerConfig *TrustManagerControllerConfig `json:"controllerConfig,omitempty"`
}
type TrustManagerConfig struct {
@@
- SecretTargets SecretTargetsConfig `json:"secretTargets,omitempty"`
+ SecretTargets *SecretTargetsConfig `json:"secretTargets,omitempty"`
@@
- DefaultCAPackage DefaultCAPackageConfig `json:"defaultCAPackage,omitempty"`
+ DefaultCAPackage *DefaultCAPackageConfig `json:"defaultCAPackage,omitempty"`
}Also applies to: 156-210
🤖 Prompt for AI Agents
In @api/operator/v1alpha1/trustmanager_types.go around lines 59 - 154, The
nested config structs should be pointers so omitempty works and to preserve
unset vs empty states: change TrustManagerSpec.TrustManagerConfig to
*TrustManagerConfig and TrustManagerSpec.ControllerConfig to
*TrustManagerControllerConfig, and inside TrustManagerConfig change
SecretTargets to *SecretTargetsConfig and DefaultCAPackage to
*DefaultCAPackageConfig (keep json:"...,omitempty" tags). After updating the
types (and any constructors/usage), re-run the code generators to refresh
deepcopy, CRDs and applyconfig.
| trustNamespace: | ||
| default: cert-manager | ||
| description: |- | ||
| trustNamespace is the namespace where trust-manager looks for trust sources | ||
| (ConfigMaps and Secrets containing CA certificates). | ||
| Defaults to "cert-manager" if not specified. | ||
| This field is immutable once set. | ||
| This field can have a maximum of 63 characters. | ||
| maxLength: 63 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: trustNamespace is immutable once set | ||
| rule: oldSelf == '' || self == oldSelf | ||
| type: object |
There was a problem hiding this comment.
Add Kubernetes namespace format validation for spec.trustManagerConfig.trustNamespace (currently only length-validated).
Without a DNS_LABEL pattern (e.g., ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$), the API can accept values the operator/operand can’t use reliably.
Proposed change (prefer generating via kubebuilder annotations in the Go type and re-running controller-gen)
trustNamespace:
default: cert-manager
description: |-
@@
maxLength: 63
minLength: 1
+ pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
type: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trustNamespace: | |
| default: cert-manager | |
| description: |- | |
| trustNamespace is the namespace where trust-manager looks for trust sources | |
| (ConfigMaps and Secrets containing CA certificates). | |
| Defaults to "cert-manager" if not specified. | |
| This field is immutable once set. | |
| This field can have a maximum of 63 characters. | |
| maxLength: 63 | |
| minLength: 1 | |
| type: string | |
| x-kubernetes-validations: | |
| - message: trustNamespace is immutable once set | |
| rule: oldSelf == '' || self == oldSelf | |
| type: object | |
| trustNamespace: | |
| default: cert-manager | |
| description: |- | |
| trustNamespace is the namespace where trust-manager looks for trust sources | |
| (ConfigMaps and Secrets containing CA certificates). | |
| Defaults to "cert-manager" if not specified. | |
| This field is immutable once set. | |
| This field can have a maximum of 63 characters. | |
| maxLength: 63 | |
| minLength: 1 | |
| pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ | |
| type: string | |
| x-kubernetes-validations: | |
| - message: trustNamespace is immutable once set | |
| rule: oldSelf == '' || self == oldSelf | |
| type: object |
🤖 Prompt for AI Agents
In @config/crd/bases/operator.openshift.io_trustmanagers.yaml around lines 1205
- 1219, spec.trustManagerConfig.trustNamespace currently only has length checks;
add a DNS_LABEL pattern validation so Kubernetes rejects invalid namespace names
(use regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$). Update the OpenAPI validation for
trustNamespace (the x-kubernetes-validations entry for trustNamespace) to
include this pattern rule, or preferably add the corresponding kubebuilder
annotation on the Go field for Spec.TrustManagerConfig.TrustNamespace (e.g.,
+kubebuilder:validation:Pattern="<DNS_LABEL>") and re-run controller-gen to
regenerate the CRD so the x-kubernetes-validations and schema include the
DNS_LABEL pattern.
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
Is there any suggestions to be incorporated from EP review?
openshift-ci
bot
added
the
approved
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
trust-manager-api
branch
from
71445da to
4591f79
Compare
Till today, the API changes suggested in the EP align with this PR. If anything else comes in the future, we need to incorporate that, but nothing for now. |
|
@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. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
/lgtm
/hold till the EP is the merged.
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, chiragkyal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Summary
This PR introduces the TrustManager API and supporting static manifests, which will be managed by cert-manager-operator to handle the lifecycle of the trust-manager operand.
Key additions
TrustManagerCustom Resource Definitionhack/update-trust-manager-manifests.sh) to generate trust-manager manifests from upstreamTrustManager API Features
The TrustManager resource is a cluster-scoped singleton (name must be
cluster) that provides:Closes