[DRAFT-WIP] TLS1.3#613
Open
desmax74 wants to merge 1 commit into
Open
Conversation
Reviewer's GuideIntroduces a TLS configurator feature that runs as a pre-install/pre-upgrade Job in the openshift-ingress-operator namespace, with corresponding Helm values, RBAC, and ClusterServiceVersion permissions, plus a minor Go toolchain patch update in the Dockerfile. Sequence diagram for TLS configurator pre-install/pre-upgrade Job executionsequenceDiagram
actor ArgoCD
participant Helm as HelmChart
participant K8sAPI as KubernetesAPI
participant SA as TLSConfiguratorServiceAccount
participant CR as TLSConfiguratorClusterRole
participant CRB as TLSConfiguratorClusterRoleBinding
participant Job as TLSConfiguratorJob
participant ConfigAPI as OpenShiftConfigAPI
ArgoCD->>Helm: Sync redhat_trusted_profile_analyzer (modules.tlsConfigurator.enabled=true)
Helm->>K8sAPI: Apply tls_configurator_ServiceAccount (hook pre-install, wave 1)
K8sAPI-->>SA: Persist ServiceAccount in openshift-ingress-operator
Helm->>K8sAPI: Apply tls_configurator_ClusterRole (hook pre-install, wave 2)
K8sAPI-->>CR: Persist ClusterRole tls-configurator
Helm->>K8sAPI: Apply tls_configurator_ClusterRoleBinding (hook pre-install, wave 2)
K8sAPI-->>CRB: Bind ServiceAccount tls-configurator to ClusterRole
Helm->>K8sAPI: Apply tls_configurator_Job (hook pre-install, wave 3)
K8sAPI-->>Job: Create Job tls-configurator
Job->>K8sAPI: Start Pod using ServiceAccount tls-configurator
Job->>ConfigAPI: GET ingresses, clusterversions
Job->>ConfigAPI: PATCH ingresses, clusterversions (set TLS1.3 ciphers and min version)
ConfigAPI-->>Job: Updated TLS configuration
Job-->>K8sAPI: Report completion status
K8sAPI-->>Helm: Job completed
Helm-->>ArgoCD: Pre-install/pre-upgrade hooks completed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
tls-configuratorimage is hard-coded in the Job manifest while also defined undermodules.tlsConfigurator.imageinvalues.yaml; consider wiring the Job template to use the Helm values so the image can be configured in one place. - The
tls-configuratorServiceAccount and RBAC are defined both inconfig/rbac/*.yamland in Helm hook templates, which risks drift and conflicts; it would be better to consolidate these to a single source of truth or clearly separate cluster-bootstrapping vs. in-chart responsibilities. - The new
rhtpa-rbac-managerClusterRole grants broad permissions (including create/delete on jobs, serviceaccounts, clusterroles, and clusterrolebindings); if possible, narrow these rules to only the specific resources and verbs needed by the controller to reduce RBAC surface area.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `tls-configurator` image is hard-coded in the Job manifest while also defined under `modules.tlsConfigurator.image` in `values.yaml`; consider wiring the Job template to use the Helm values so the image can be configured in one place.
- The `tls-configurator` ServiceAccount and RBAC are defined both in `config/rbac/*.yaml` and in Helm hook templates, which risks drift and conflicts; it would be better to consolidate these to a single source of truth or clearly separate cluster-bootstrapping vs. in-chart responsibilities.
- The new `rhtpa-rbac-manager` ClusterRole grants broad permissions (including create/delete on jobs, serviceaccounts, clusterroles, and clusterrolebindings); if possible, narrow these rules to only the specific resources and verbs needed by the controller to reduce RBAC surface area.
## Individual Comments
### Comment 1
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/init/tls-configure/020-Job.yaml" line_range="18-22" />
<code_context>
+ helm.sh/hook-delete-policy: before-hook-creation
+
+spec:
+ backoffLimit: 1000
+ completions: 1
+ parallelism: 1
</code_context>
<issue_to_address>
**suggestion (performance):** The backoffLimit of 1000 is extremely high and could cause excessive retrying in failure scenarios.
This many retries can produce noisy failures and unnecessary cluster load if the Job is misconfigured (e.g., bad image, RBAC issues). Use a smaller value (around 3–10) unless you have a documented operational need for such a high retry count.
```suggestion
spec:
backoffLimit: 5
completions: 1
parallelism: 1
ttlSecondsAfterFinished: 600
```
</issue_to_address>
### Comment 2
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/init/tls-configure/020-Job.yaml" line_range="32" />
<code_context>
+ serviceAccountName: tls-configurator
+ containers:
+ - name: tls-configurator
+ image: quay.io/mdessi/tls-configurator:latest
+ args:
+ - "--action=update"
</code_context>
<issue_to_address>
**issue:** The Job is using a hard-coded image instead of wiring it to the Helm values defined for tlsConfigurator.
The Job template should use the existing Helm values instead of a hard-coded image. Please reference `modules.tlsConfigurator.image.fullName` and `modules.tlsConfigurator.image.pullPolicy`, e.g.:
```yaml
aimage: {{ .Values.modules.tlsConfigurator.image.fullName }}
imagePullPolicy: {{ .Values.modules.tlsConfigurator.image.pullPolicy }}
```
This keeps the chart configurable and prevents drift between `values.yaml` and the template.
</issue_to_address>
### Comment 3
<location path="config/rbac/role_cluster_rbac_manager_binding.yaml" line_range="6-8" />
<code_context>
+ kind: ClusterRole
+ name: tls-configurator
+subjects:
+ - kind: ServiceAccount
+ name: tls-configurator
+ namespace: openshift-ingress-operator
</code_context>
<issue_to_address>
**question (bug_risk):** The ClusterRoleBinding refers to a ServiceAccount in a `placeholder` namespace, which looks like it may not exist as-is.
If this namespace is meant to be replaced by kustomize or another tool, that’s fine. Otherwise, update it to the actual controller manager namespace so the ClusterRoleBinding points to a real ServiceAccount and the RBAC binding is effective.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
desmax74
force-pushed
the
TLS1.3
branch
2 times, most recently
from
4ae57ec to
2f8a4ed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a TLS configurator capability and supporting RBAC to manage OpenShift ingress TLS settings, and update build tooling.
New Features:
Enhancements:
Build: