Introduce externalMethod=WholeIP for VMs #616
Conversation
dosubot
bot
added
the
size:M
WalkthroughThis pull request introduces a new property, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kvaps
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/apps/virtual-machine/values.yaml (1)
4-4: Clarify Parameter Documentation forexternalMethodThe added documentation comment currently reads:
## @param externalMethod specify method to passthrough the traffic to the virtual machine. Allowed values: \standard` and `wholeIP``
For improved clarity and grammatical accuracy, consider changing “passthrough” to “pass through.”packages/apps/vm-instance/values.schema.json (1)
10-18: Validate New Schema Property forexternalMethodThe new
"externalMethod"property is correctly defined with type"string", a default value of"wholeIP", and an enum restricting allowed values to"standard"and"wholeIP". For further clarity, consider revising the description to use “pass through” (with a space) instead of “passthrough.”packages/apps/virtual-machine/values.schema.json (1)
10-18: Ensure Schema Consistency forexternalMethodThis schema update mirrors the one in the vm-instance package. The property’s type, default value, and enumeration are consistent with the intended behavior. A minor improvement would be to update the description to “specify method to pass through the traffic to the virtual machine” for better readability.
packages/apps/vm-instance/README.md (1)
39-44: Documentation Table Update forexternalMethodThe parameters table now includes an entry for
externalMethodwith a clear description and default value. For clarity, please consider changing “passthrough” to “pass through” in the description. Also, ensure the table rows follow the markdown lint guidelines (i.e. use both leading and trailing pipes).🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...externalMethod| specify method to passthrough the traffic to the virtual machine. All...(NOUN_VERB_CONFUSION)
packages/apps/virtual-machine/README.md (1)
41-44: Update Documentation Table Entry forexternalMethodThe added table row correctly documents the new
externalMethodparameter. For improved clarity, update the description to read “specify method to pass through the traffic to the virtual machine” instead of using “passthrough.” Additionally, verify that the table formatting complies with markdown lint rules by including trailing pipes for all rows.🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...nalMethod` | specify method to passthrough the traffic to the virtual machine. All...(NOUN_VERB_CONFUSION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/apps/virtual-machine/Makefile(1 hunks)packages/apps/virtual-machine/README.md(1 hunks)packages/apps/virtual-machine/templates/service.yaml(1 hunks)packages/apps/virtual-machine/values.schema.json(1 hunks)packages/apps/virtual-machine/values.yaml(1 hunks)packages/apps/vm-instance/Makefile(1 hunks)packages/apps/vm-instance/README.md(1 hunks)packages/apps/vm-instance/templates/service.yaml(1 hunks)packages/apps/vm-instance/values.schema.json(1 hunks)packages/apps/vm-instance/values.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/apps/vm-instance/README.md
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ... externalMethod | specify method to passthrough the traffic to the virtual machine. All...
(NOUN_VERB_CONFUSION)
[style] ~50-~50: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... of SSH public keys for authentication. Can be a single key or a list of keys. ...
(MISSING_IT_THERE)
packages/apps/virtual-machine/README.md
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...nalMethod` | specify method to passthrough the traffic to the virtual machine. All...
(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
packages/apps/vm-instance/README.md
51-51: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
🔇 Additional comments (8)
packages/apps/vm-instance/templates/service.yaml (2)
9-12: Conditional Annotation Addition:
The conditional block correctly adds thenetworking.cozystack.io/wholeIP: "true"annotation within the metadata when.Values.externalMethodis set to"wholeIP". This ensures that the service is tagged appropriately for whole-IP traffic routing. Please confirm that this behavior is in line with the default configuration specified invalues.yaml.
20-28: Port Configuration Update:
The conditional structure under theportssection properly assigns a fixed port (65535) when.Values.externalMethodequals"wholeIP", and falls back to iterating over.Values.externalPortsotherwise. This logic is consistent with the PR objective to handle different external methods for VM traffic routing.packages/apps/virtual-machine/templates/service.yaml (2)
9-12: Conditional Annotation Addition:
The changes mirror those in the vm-instance template. The conditional check adds thenetworking.cozystack.io/wholeIP: "true"annotation when.Values.externalMethodis"wholeIP", ensuring consistent behavior across the project.
20-28: Port Configuration Update:
The port assignment logic here similarly provides a fixed port of65535when the external method is"wholeIP", which aligns with the intended behavior. When the condition is not met, the ports are dynamically generated from.Values.externalPorts. This is a clear and concise implementation of the required feature.packages/apps/vm-instance/Makefile (1)
11-11: Schema Enum Update:
The newly added command updates the enumeration forexternalMethodinvalues.schema.jsonto["standard", "wholeIP"]. This enforces strict validation for the new parameter and is well-aligned with the feature’s intent.packages/apps/virtual-machine/Makefile (1)
11-11: ExternalMethod Enum Update in Schema:
This command aptly inserts the allowed enumeration forexternalMethodintovalues.schema.json. Defining the acceptable values as["standard", "wholeIP"]ensures that users can only configure valid options, which is critical for maintaining robust external access configurations.packages/apps/vm-instance/values.yaml (1)
4-8: New Parameter Addition:
The addition of theexternalMethodparameter with a default value of"wholeIP", along with an inline comment that highlights the allowed values (standardandwholeIP), is clear and aligns perfectly with the updated schema and documentation. This ensures that users are aware of the valid configurations for external traffic routing.packages/apps/virtual-machine/values.yaml (1)
7-7: Default Value forexternalMethodThe configuration sets
externalMethodto"wholeIP", which is consistent with the defaults specified in the schemas.
Signed-off-by: Andrei Kvapil <[email protected]>
| ## @param externalPorts [array] Specify ports to forward from outside the cluster | ||
| external: false | ||
| externalMethod: "wholeIP" |
There was a problem hiding this comment.
Combination of wholeIP and port-list is confusing. Consider making the port-list either optional or empty.
kvaps
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/apps/vm-instance/README.md (4)
42-42: Clarify the 'externalMethod' description
The description “specify method to passthrough the traffic to the virtual machine.” can be improved for clarity and grammatical correctness. Consider rephrasing it to: “Specifies the method to pass traffic through to the virtual machine. Allowed values:WholeIPandPortList.”Proposed diff:
-|
externalMethod| specify method to passthrough the traffic to the virtual machine. Allowed values:WholeIPandPortList|WholeIP|
+|externalMethod| Specifies the method to pass traffic through to the virtual machine. Allowed values:WholeIPandPortList|WholeIP|🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...externalMethod| specify method to passthrough the traffic to the virtual machine. All...(NOUN_VERB_CONFUSION)
46-46: Fix spelling error in 'instanceProfile' description
The term “prefferences” is misspelled. It should be changed to “preferences” for accuracy and clarity.Proposed diff:
-|
instanceProfile| Virtual Machine prefferences profile |ubuntu|
+|instanceProfile| Virtual Machine preferences profile |ubuntu|
50-50: Improve SSH keys description style
The current description “List of SSH public keys for authentication. Can be a single key or a list of keys.” could be revised for smoother readability. For example, changing it to “List of SSH public keys for authentication; can be a single key or a list of keys.” would form a more complete sentence.Proposed diff:
-|
sshKeys| List of SSH public keys for authentication. Can be a single key or a list of keys. |[]|
+|sshKeys| List of SSH public keys for authentication; can be a single key or a list of keys. |[]|🧰 Tools
🪛 LanguageTool
[style] ~50-~50: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... of SSH public keys for authentication. Can be a single key or a list of keys. ...(MISSING_IT_THERE)
51-52: Improve markdown table formatting for 'cloudInit' row
The markdown linter suggests using a trailing pipe for consistency in table formatting. Adjust thecloudInitrow to include a trailing pipe.Proposed diff:
-|
cloudInit| cloud-init user data config. See cloud-init documentation for more details. |#cloud-config -|
+|cloudInit| cloud-init user data config. See cloud-init documentation for more details. |#cloud-config|🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
51-51: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe(MD055, table-pipe-style)
52-52: Table pipe style
Expected: leading_and_trailing; Actual: trailing_only; Missing leading pipe(MD055, table-pipe-style)
52-52: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data(MD056, table-column-count)
packages/apps/virtual-machine/values.yaml (1)
4-4: Clarify Parameter Description Grammar.
The description for the new parameter currently uses “passthrough” as one word. Since “pass through” is the proper verb form, consider updating the text to “specify method to pass through the traffic to the virtual machine” for improved clarity and professionalism.packages/apps/virtual-machine/README.md (1)
42-42: Revise Parameter Description for Consistency.
Similar to the YAML file, the description here uses “passthrough” as one word. For consistency and grammatical accuracy, consider updating it to “specify method to pass through the traffic to the virtual machine.” This will enhance clarity for the users reading the documentation.🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...nalMethod` | specify method to passthrough the traffic to the virtual machine. All...(NOUN_VERB_CONFUSION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/apps/versions_map(1 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/virtual-machine/Makefile(1 hunks)packages/apps/virtual-machine/README.md(1 hunks)packages/apps/virtual-machine/templates/service.yaml(1 hunks)packages/apps/virtual-machine/values.schema.json(1 hunks)packages/apps/virtual-machine/values.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/Makefile(1 hunks)packages/apps/vm-instance/README.md(1 hunks)packages/apps/vm-instance/templates/service.yaml(1 hunks)packages/apps/vm-instance/values.schema.json(1 hunks)packages/apps/vm-instance/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/apps/vm-instance/Chart.yaml
- packages/apps/virtual-machine/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/apps/vm-instance/values.schema.json
- packages/apps/vm-instance/Makefile
- packages/apps/vm-instance/values.yaml
- packages/apps/vm-instance/templates/service.yaml
- packages/apps/virtual-machine/values.schema.json
- packages/apps/virtual-machine/Makefile
- packages/apps/virtual-machine/templates/service.yaml
🧰 Additional context used
🪛 LanguageTool
packages/apps/virtual-machine/README.md
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...nalMethod` | specify method to passthrough the traffic to the virtual machine. All...
(NOUN_VERB_CONFUSION)
packages/apps/vm-instance/README.md
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ... externalMethod | specify method to passthrough the traffic to the virtual machine. All...
(NOUN_VERB_CONFUSION)
[style] ~50-~50: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... of SSH public keys for authentication. Can be a single key or a list of keys. ...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
packages/apps/vm-instance/README.md
51-51: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
🔇 Additional comments (4)
packages/apps/versions_map (3)
112-113: LGTM! Version updates for virtual-machine package follow semantic versioning.The changes appropriately:
- Pin version 0.7.1 to commit 05857b9 for stability
- Introduce version 0.8.0 for the new
externalMethodfeature
119-120: LGTM! Version updates for vm-instance package follow semantic versioning.The changes appropriately:
- Pin version 0.4.1 to commit 05857b9 for stability
- Introduce version 0.5.0 for the new
externalMethodfeature
112-113: Verify the commit hash exists and contains the expected changes.Both packages are pinned to commit 05857b9. Let's verify this commit exists and contains the expected changes.
Also applies to: 119-120
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the commit exists and contains the expected changes # Check if the commit exists and show its changes git show 05857b95 --name-onlyLength of output: 1639
Commit 05857b9 Verified with Expected Changes
The commit exists as expected. The output confirms that commit 05857b9—introduced on January 23, 2025—includes the changes for the Kamaji package (e.g., Makefile, Dockerfile, chart updates) along with the appropriate commit message ("Fix kamaji make update (#598)"). This aligns with what’s expected for the pinned commit.packages/apps/virtual-machine/values.yaml (1)
7-7: Verify Default Value and Consistency with Design Intent.
The new parameter is set asexternalMethod: WholeIP. Please confirm that this default aligns with the overall schema and expected type (i.e. a string). Additionally, given past feedback that combining a “WholeIP” method with an active port list may be confusing, ensure that either the design documentation explicitly clarifies this choice or that appropriate validation is in place.
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
New Features
Documentation
externalMethodparameter and its usage for managing external traffic.