🌱 Enable TLS for Flower addon SuperLink-SuperNode communication - #112
Merged
openshift-merge-bot[bot] merged 4 commits intoMar 31, 2026
Merged
Conversation
Add TLS support for securing gRPC communication between SuperLink (hub) and SuperNode (managed clusters). Includes a certificate generation script and Helm chart conditional TLS configuration. - Add hack/generate-certs.sh to generate CA + server cert as K8s Secrets - SuperLink: mount server cert, switch from --insecure to --ssl-* flags - SuperNode: distribute CA cert via AddOnTemplate Secret, use --root-certificates - Add tls.enabled toggle (default: false) for backward compatibility - Helm lookup with fail guard ensures clear error if certs not generated Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yanmxa 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 |
Add instructions for configuring TLS in ~/.flwr/config.toml when submitting FL jobs via flwr run to the TLS-secured SuperLink control API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
WalkthroughMarked TLS as enabled in README roadmap; added Helm TLS values and conditional template logic for CA and server certs; added a cert-generation script; added docs describing how to generate certs, create Secrets, and enable TLS for SuperNode–SuperLink communication. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flower-addon/charts/flower-addon/templates/superlink.yaml`:
- Around line 62-66: When tls.enabled is true the template mounts a Secret named
by .Values.tls.serverCert.secretName but does not validate it; add a Helm lookup
+ fail guard (the same lookup/fail pattern used for the CA Secret) at the top of
this template so installation fails fast if the server-cert Secret is missing.
Specifically, introduce a lookup on Secret with the namespace and
.Values.tls.serverCert.secretName and call fail with a clear message if the
lookup returns nil before the volumes: - name: tls-certs block that references
.Values.tls.serverCert.secretName.
In `@flower-addon/hack/generate-certs.sh`:
- Around line 111-116: The script currently uploads the CA private key into the
cluster Secret "flower-tls-ca" (via --from-file=ca.key), which is unnecessary
and increases blast radius; change the kubectl create secret generic invocation
in generate-certs.sh to only include the public certificate
(--from-file=ca.crt="${CERT_DIR}/ca.crt") and remove the --from-file=ca.key
option so the CA private key stays only on disk (CERT_DIR) and is not stored in
the Secret.
In `@flower-addon/README.md`:
- Around line 76-78: Update the README note to call out that the shown commands
assume the default SuperLink namespace and advise operators to either run the
commands with --namespace to match their deployment or set the Helm value
superlink.namespace to the same namespace; mention that mismatched namespaces
will break the CA lookup in addon-template.yaml and the SuperLink Secret mount
(flower-superlink-tls). Ensure the text around the two Secret descriptions (and
the repeated section at lines ~103-105) instructs users to use the same
namespace or show an example using --namespace to keep the CA and Secret
references consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5643c90b-cad8-4d17-b3d0-cdb5fe2bd8fe
📒 Files selected for processing (5)
flower-addon/README.mdflower-addon/charts/flower-addon/templates/addon-template.yamlflower-addon/charts/flower-addon/templates/superlink.yamlflower-addon/charts/flower-addon/values.yamlflower-addon/hack/generate-certs.sh
Move the Enable TLS section from README into a dedicated doc file under docs/ for consistency with other guides, and reference it from the Roadmap section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
- Add lookup/fail guard for server-cert Secret in superlink.yaml to fail fast when the Secret is missing (mirrors existing CA Secret validation) - Update TLS docs to clarify namespace assumptions and guide users to pass matching --namespace when overriding superlink.namespace Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Meng Yan <myan@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
flower-addon/charts/flower-addon/templates/superlink.yaml (1)
25-27: Consider adding security context hardening.Static analysis flagged that the
superlinkcontainer lacks security context configuration. While out of scope for this TLS-focused PR, consider adding these hardening measures in a follow-up:containers: - name: superlink securityContext: readOnlyRootFilesystem: true runAsNonRoot: true allowPrivilegeEscalation: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flower-addon/charts/flower-addon/templates/superlink.yaml` around lines 25 - 27, Add a securityContext to the superlink container in the Helm template to harden runtime privileges: update the containers entry for the "superlink" container in superlink.yaml to include a securityContext block with readOnlyRootFilesystem: true, runAsNonRoot: true and allowPrivilegeEscalation: false (optionally make these values configurable via .Values.superlink.securityContext so they can be toggled in values.yaml).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@flower-addon/charts/flower-addon/templates/superlink.yaml`:
- Around line 25-27: Add a securityContext to the superlink container in the
Helm template to harden runtime privileges: update the containers entry for the
"superlink" container in superlink.yaml to include a securityContext block with
readOnlyRootFilesystem: true, runAsNonRoot: true and allowPrivilegeEscalation:
false (optionally make these values configurable via
.Values.superlink.securityContext so they can be toggled in values.yaml).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc289dcd-1196-4595-8e2a-f3b69533d215
📒 Files selected for processing (2)
flower-addon/charts/flower-addon/templates/superlink.yamlflower-addon/docs/enable-tls.md
✅ Files skipped from review due to trivial changes (1)
- flower-addon/docs/enable-tls.md
|
/lgtm |
openshift-merge-bot
Bot
merged commit Mar 31, 2026
dce6471
into
open-cluster-management-io:main
19 checks passed
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
2 participants
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
hack/generate-certs.shscript to generate CA + server cert and create K8s Secrets--insecureto--ssl-*TLS flags--root-certificatestls.enabledtoggle invalues.yaml(default:false) for backward compatibilitylookupwithfailguard for clear error if certs not generated before installUsage
Verified
End-to-end TLS verification passed with CIFAR-10 federated learning (FedAvg, 3 rounds, 2 managed clusters):
--ssl-ca-certfile,--ssl-certfile,--ssl-keyfile--root-certificates(CA cert distributed via AddOnTemplate Secret)flwr runsubmits jobs over TLS using~/.flwr/config.tomlwithinsecure = falseandroot-certificatesTest plan
tls.enabled=false) — verify existing insecure behavior unchangedgenerate-certs.sh— verify secrets created inflower-systemtls.enabled=true) — verify SuperLink and SuperNode args🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores