USHIFT-7346: C2CC upstream user documentation#7015
Open
pmtk wants to merge 3 commits into
Open
Conversation
|
@pmtk: This pull request references USHIFT-7346 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 task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
WalkthroughThis PR adds a new user documentation guide for MicroShift's Cluster-to-Cluster Connectivity (C2CC) feature, links it and the existing IPsec guide from the README index, and expands the IPsec guide with verification examples, an nftables enforcement section, and updated considerations. ChangesC2CC documentation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmtk 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/howto_c2cc_ipsec.md`:
- Line 221: The MTU guidance in the C2CC IPsec documentation is missing the
reboot requirement, which makes the example misleading. Update the text around
the MicroShift MTU example to state that changing `/etc/microshift/ovn.yaml` for
`mtu` requires a node reboot to take effect, and adjust the instructions near
the IPsec overhead and `mtu: 8900` example so they no longer imply that
restarting MicroShift alone is sufficient.
- Around line 194-202: The CIDR enforcement example in the IPsec how-to uses the
wrong nftables hook; update the rules in the c2cc_ipsec example to use the
forward path instead of input so routed pod/service traffic is actually
filtered. Keep the existing table and chain setup, but adjust the chain
definition and accompanying rule examples in the section around the Host A/Host
B CIDRs so the `enforce` chain is bound to the forwarding hook.
In `@docs/user/howto_c2cc.md`:
- Around line 423-429: The IPsec recommendation in the c2c documentation is too
broad and implies protection is always enforced, even though the Libreswan setup
can stop protecting CIDRs if the service is stopped or a connection definition
is removed. Update the text in the c2cc how-to section to tie the recommendation
to the enforced setup by referencing the IPsec guide’s shunt/connection
requirements and making it clear that IPsec only provides the intended
protection when that enforced configuration remains in place. Use the existing
“howto_c2cc” and “howto_c2cc_ipsec” guidance sections as the anchor points for
the wording.
- Around line 314-316: The fenced block showing
<service>.<namespace>.svc.<domain> is missing a language tag, causing the
markdown lint failure. Update that fenced code block in the howto content to use
a typed fence such as text so it matches the expected markdown style and
resolves MD040.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 97446b08-c708-4412-91ad-0d37a1b3de70
📒 Files selected for processing (3)
docs/user/README.mddocs/user/howto_c2cc.mddocs/user/howto_c2cc_ipsec.md
Comment on lines
+194
to
+202
| sudo nft add table inet c2cc_ipsec | ||
| sudo nft 'add chain inet c2cc_ipsec enforce { type filter hook input priority -150; policy accept; }' | ||
| sudo nft add rule inet c2cc_ipsec enforce ip daddr 10.42.0.0/16 meta ipsec missing counter drop | ||
| sudo nft add rule inet c2cc_ipsec enforce ip daddr 10.43.0.0/16 meta ipsec missing counter drop | ||
| ``` | ||
|
|
||
| On **Host B**, use its own local CIDRs (`10.45.0.0/16`, `10.46.0.0/16`). | ||
|
|
||
| With these rules in place, inbound cross-cluster traffic is accepted only when it arrived through an IPsec SA — even if IPsec is misconfigured or stopped on the sending side, unencrypted packets are dropped rather than delivered. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant document and nearby references to the packet path.
git ls-files 'docs/user/howto_c2cc_ipsec.md' 'docs/user/*.md' 'docs/contributor/*.md' | sed -n '1,120p'
echo
echo '--- howto_c2cc_ipsec.md excerpt ---'
nl -ba docs/user/howto_c2cc_ipsec.md | sed -n '160,230p'
echo
echo '--- search for nftables / input / forward references ---'
rg -n --no-heading -C 2 'hook (input|forward)|nft .*c2cc_ipsec|meta ipsec missing|cross-cluster traffic|pod or service traffic|enforce' docs/user docs/contributorRepository: openshift/microshift
Length of output: 1896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the document structure and inspect the specific section's headings.
ast-grep outline docs/user/howto_c2cc_ipsec.md --view expanded || trueRepository: openshift/microshift
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section with portable line numbering.
awk 'NR>=180 && NR<=220 { printf "%4d %s\n", NR, $0 }' docs/user/howto_c2cc_ipsec.md
echo
echo '--- surrounding nftables references in docs ---'
rg -n --no-heading -C 3 'hook input|hook forward|c2cc_ipsec|meta ipsec missing|cross-cluster traffic' docs/user/howto_c2cc_ipsec.md docs/user/howto_c2cc.md docs/contributor/network/ovn_kubernetes_traffic_flows.md docs/contributor/network/host_networking.mdRepository: openshift/microshift
Length of output: 10281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the packet path references for Cluster-to-Cluster.
rg -n --no-heading -C 4 'cross-cluster|cluster-to-cluster|forward|input|IPsec|nft' docs/user/howto_c2cc.md docs/user/howto_c2cc_ipsec.md docs/contributor/network/ovn_kubernetes_traffic_flows.mdRepository: openshift/microshift
Length of output: 28514
Use hook forward for the CIDR rules. hook input only sees traffic to the host itself; these pod/service CIDR packets are routed through the node, so this example leaves the actual C2CC data path unprotected. docs/user/howto_c2cc_ipsec.md:195-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/howto_c2cc_ipsec.md` around lines 194 - 202, The CIDR enforcement
example in the IPsec how-to uses the wrong nftables hook; update the rules in
the c2cc_ipsec example to use the forward path instead of input so routed
pod/service traffic is actually filtered. Keep the existing table and chain
setup, but adjust the chain definition and accompanying rule examples in the
section around the Host A/Host B CIDRs so the `enforce` chain is bound to the
forwarding hook.
| ## Considerations | ||
|
|
||
| - **IPsec adds overhead.** ESP tunnel mode adds approximately 36-52 bytes per packet. If you experience MTU issues, verify that path MTU discovery is working or adjust MTU settings accordingly. | ||
| - **IPsec adds overhead.** ESP tunnel mode adds roughly 73-93 bytes per packet (ESP header/trailer plus the outer IP header, depending on the cipher suite). Cross-cluster C2CC traffic travels as plain routed IP — it is not Geneve-encapsulated between the hosts — so the largest packet entering the tunnel is the pod MTU, which MicroShift by default sets equal to the MTU of the physical interface. Size the pod MTU so that it plus the ESP overhead fits within the physical MTU. For example, on a jumbo-frame network (physical MTU 9000), set the pod MTU to 8900 by creating `/etc/microshift/ovn.yaml` with `mtu: 8900` and restarting MicroShift. Also verify that path MTU discovery works between the pods and that near-MTU-sized payloads pass through the tunnel. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File context\n'
git ls-files docs/user/howto_c2cc_ipsec.md docs/user | sed -n '1,20p'
printf '\n## Relevant lines from docs/user/howto_c2cc_ipsec.md\n'
nl -ba docs/user/howto_c2cc_ipsec.md | sed -n '210,235p'
printf '\n## Search for ovn.yaml and restart guidance\n'
rg -n --hidden --no-messages -S 'ovn\.yaml|restart MicroShift|restart the host|MTU' docs pkg test .claude AGENTS.md CONTRIBUTING.md | sed -n '1,240p'
printf '\n## Search for MicroShift service references\n'
rg -n --hidden --no-messages -S 'microshift.*restart|restart.*microshift|systemctl.*microshift|microshift.service' docs pkg scripts test | sed -n '1,240p'Repository: openshift/microshift
Length of output: 857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Relevant excerpt from docs/user/howto_c2cc_ipsec.md\n'
sed -n '215,230p' docs/user/howto_c2cc_ipsec.md
printf '\n## Search for MTU and restart guidance in docs\n'
rg -n -S 'ovn\.yaml|mtu:|restart MicroShift|restart the host|physical MTU|pod MTU' docs/user docs/contributor docs -g '*.md' | sed -n '1,240p'
printf '\n## Search for service restart instructions\n'
rg -n -S 'systemctl restart microshift|restart MicroShift|MicroShift.*restart|ovn\.yaml' docs pkg scripts test -g '*.md' -g '*.go' -g '*.sh' | sed -n '1,240p'Repository: openshift/microshift
Length of output: 12304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## default_cni_plugin docs around MTU\n'
sed -n '55,85p' docs/contributor/network/default_cni_plugin.md
printf '\n## Search for ovn config load / restart behavior in code\n'
rg -n -S 'ovn\.yaml|mtu|reboot|restart' pkg/config docs/contributor/network docs/user -g '*.go' -g '*.md' | sed -n '1,240p'
printf '\n## Inspect OVN config handling\n'
sed -n '1,220p' pkg/config/ovn/ovn.goRepository: openshift/microshift
Length of output: 19855
Call out the reboot requirement.
Changing /etc/microshift/ovn.yaml for mtu requires a node reboot to take effect, so this example shouldn’t imply that restarting MicroShift is enough.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/howto_c2cc_ipsec.md` at line 221, The MTU guidance in the C2CC
IPsec documentation is missing the reboot requirement, which makes the example
misleading. Update the text around the MicroShift MTU example to state that
changing `/etc/microshift/ovn.yaml` for `mtu` requires a node reboot to take
effect, and adjust the instructions near the IPsec overhead and `mtu: 8900`
example so they no longer imply that restarting MicroShift alone is sufficient.
Comment on lines
+314
to
+316
| ``` | ||
| <service>.<namespace>.svc.<domain> | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to this fenced block.
This block will keep failing MD040 until the fence is typed.
♻️ Proposed fix
-```
+```text
<service>.<namespace>.svc.<domain>
-```
+```📝 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.
Suggested change
| ``` | |
| <service>.<namespace>.svc.<domain> | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 314-314: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/howto_c2cc.md` around lines 314 - 316, The fenced block showing
<service>.<namespace>.svc.<domain> is missing a language tag, causing the
markdown lint failure. Update that fenced code block in the howto content to use
a typed fence such as text so it matches the expected markdown style and
resolves MD040.
Source: Linters/SAST tools
Comment on lines
+423
to
+429
| - **No authentication or encryption by default.** Any host that can reach | ||
| (or spoof) the configured next-hop IP can inject traffic into the | ||
| cluster, and because SNAT is bypassed, that traffic can carry an | ||
| attacker-chosen source IP that matches NetworkPolicy allow rules. Use | ||
| [IPsec](./howto_c2cc_ipsec.md) for production deployments — it provides | ||
| both encryption and mutual authentication, and rejects unauthenticated | ||
| hosts (including host-originated traffic to pods) at the network layer. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Tie the IPsec recommendation to the enforced setup.
The linked IPsec guide says the Libreswan shunt settings alone do not protect the CIDRs if the service is stopped or a connection definition is removed, so this reads as if IPsec by itself is enough.
🔒 Proposed fix
- Use [IPsec](./howto_c2cc_ipsec.md) for production deployments — it provides both encryption and mutual authentication, and rejects unauthenticated hosts (including host-originated traffic to pods) at the network layer.
+ Use [IPsec](./howto_c2cc_ipsec.md) together with the nftables enforcement described there for production deployments — that combination provides encryption, mutual authentication, and drops unauthenticated traffic at the network layer.📝 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.
Suggested change
| - **No authentication or encryption by default.** Any host that can reach | |
| (or spoof) the configured next-hop IP can inject traffic into the | |
| cluster, and because SNAT is bypassed, that traffic can carry an | |
| attacker-chosen source IP that matches NetworkPolicy allow rules. Use | |
| [IPsec](./howto_c2cc_ipsec.md) for production deployments — it provides | |
| both encryption and mutual authentication, and rejects unauthenticated | |
| hosts (including host-originated traffic to pods) at the network layer. | |
| - **No authentication or encryption by default.** Any host that can reach | |
| (or spoof) the configured next-hop IP can inject traffic into the | |
| cluster, and because SNAT is bypassed, that traffic can carry an | |
| attacker-chosen source IP that matches NetworkPolicy allow rules. Use | |
| [IPsec](./howto_c2cc_ipsec.md) together with the nftables enforcement | |
| described there for production deployments — that combination provides | |
| encryption, mutual authentication, and drops unauthenticated traffic | |
| at the network layer. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/howto_c2cc.md` around lines 423 - 429, The IPsec recommendation in
the c2c documentation is too broad and implies protection is always enforced,
even though the Libreswan setup can stop protecting CIDRs if the service is
stopped or a connection definition is removed. Update the text in the c2cc
how-to section to tie the recommendation to the enforced setup by referencing
the IPsec guide’s shunt/connection requirements and making it clear that IPsec
only provides the intended protection when that enforced configuration remains
in place. Use the existing “howto_c2cc” and “howto_c2cc_ipsec” guidance sections
as the anchor points for the wording.
Contributor
|
@pmtk: 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. |
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 by CodeRabbit