Rework VirtualMachine, introduce cloudinit and sshKeys configuration, fix externalPorts #303
Conversation
Previously, an error occurred when trying to access .Values.service.ports in the template, leading to a nil pointer dereference. This commit corrects the issue by ensuring that service.ports is checked for existence before attempting to access it, preventing the error from occurring.
Changes:
- Updated service.yaml to use `{{- with .Values.service }}...{{- end }}` for conditional rendering of ports.
|
{{- if .Values.service.ports }} already tests for existence of .service.ports |
|
@ginn13 is that what you mean? |
- Added `user` parameter to the Helm chart with a default value of `username`. - Updated `values.yaml` to include documentation for the new `user` parameter. - Updated `values.schema.json` to validate the `user` parameter as a string. - Modified `vm.yaml` template to include `user` in the cloud-init configuration. - Fixed `service.yaml` template to correctly handle the `ports` field in `service`. - Improved documentation in `README.md` to reflect the addition of the `user` parameter.
| @@ -63,6 +63,7 @@ spec: | |||
| #cloud-config | |||
| ssh_pwauth: {{ if .Values.sshPwauth | default false }}True{{ else }}False{{ end }} | |||
| disable_root: {{ if .Values.disableRoot | default false }}True{{ else }}False{{ end }} | |||
| user: {{ .Values.user }} | |||
There was a problem hiding this comment.
Let's make these parameterns optional:
| user: {{ .Values.user }} | |
| {{- with .Values.user }} | |
| user: {{ . }} | |
| {{- end }} |
Or we can simple map cloud-config as parameter in values.yaml as is
| @@ -18,7 +18,7 @@ spec: | |||
| - name: ssh | |||
| port: 22 | |||
| targetPort: 22 | |||
| {{- if .Values.service.ports }} | |||
| {{- if (.Values.service).ports }} | |||
| sshPwauth: true | ||
| disableRoot: true | ||
| user: username | ||
| password: hackme | ||
| chpasswdExpire: false |
There was a problem hiding this comment.
What do you think about using common cloudInit config parameter for this?
There was a problem hiding this comment.
Is that what you mean?
cloudInit:
ssh_pwauth: true
disable_root: true
user: username
...
There was a problem hiding this comment.
It might be better to pass all cloud-init configuration as a string:
cloudInitUserConfig: |
#cloud-config
ssh_pwauth: true
disable_root: true
user: usernamedue to fact user configuration have no formalized spec, it can include multiple header, scripts and even jinja2 templates
https://cloudinit.readthedocs.io/en/latest/explanation/instancedata.html#example-cloud-config-with-instance-data
variuos vendors can define their own format for this, eg. Talos Linux allows to write machine-config in user-data
This config should be saved into external secret and included with secretRef into KubeVirt VM:
- name: cloudinitdisk
cloudInitNoCloud:
secretRef:
name: my-vmi-secretAs about sshKeys, cloud-init defines formalized spec for this in meta-data:
https://cloudinit.readthedocs.io/en/latest/explanation/instancedata.html#v1-public-ssh-keys
KubeVirt allows to specify them as
accessCredentials:
- sshPublicKey:
source:
secret:
secretName: my-pub-key
propagationMethod:
noCloud: {}details:
There was a problem hiding this comment.
What difference would it make? We'd have to create a secret anyway.
Signed-off-by: Andrei Kvapil <[email protected]>
WalkthroughThe updates to the virtual machine application include a version increment from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM_Config
participant Kubernetes
User->>VM_Config: Specify externalPorts and cloudInit
VM_Config->>Kubernetes: Generate VM with new configurations
Kubernetes->>Kubernetes: Create Secrets for SSH keys and cloud-init
Kubernetes->>User: Provision virtual machine with access methods
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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- packages/apps/virtual-machine/Chart.yaml (1 hunks)
- packages/apps/virtual-machine/README.md (1 hunks)
- packages/apps/virtual-machine/templates/secret.yaml (1 hunks)
- packages/apps/virtual-machine/templates/service.yaml (1 hunks)
- packages/apps/virtual-machine/templates/vm.yaml (1 hunks)
- packages/apps/virtual-machine/values.schema.json (2 hunks)
- packages/apps/virtual-machine/values.yaml (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/apps/virtual-machine/Chart.yaml
Additional context used
yamllint
packages/apps/virtual-machine/templates/secret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 9-9: trailing spaces
(trailing-spaces)
LanguageTool
packages/apps/virtual-machine/README.md
[style] ~49-~49: 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
packages/apps/virtual-machine/README.md
55-55: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
19-19: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
25-25: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
50-50: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style(MD055, table-pipe-style)
51-51: Expected: leading_and_trailing; Actual: trailing_only; Missing leading pipe
Table pipe style(MD055, table-pipe-style)
51-51: Expected: 3; Actual: 1; Too few cells, row will be missing data
Table column count(MD056, table-column-count)
Additional comments not posted (13)
packages/apps/virtual-machine/templates/secret.yaml (1)
12-21: Review and approve the cloud-init secret configuration.The template correctly creates a Kubernetes secret for cloud-init configurations if
.Values.cloudInitis provided. The use ofnindentto properly indent the cloud-init data is a good practice, ensuring that the YAML structure within the secret is correctly formatted.The cloud-init secret configuration is correctly implemented and follows best practices for Helm charts.
packages/apps/virtual-machine/templates/service.yaml (1)
16-19: Approve the dynamic port configuration in the service template.The new implementation using
.Values.externalPortsfor dynamic port configuration enhances flexibility and reduces the risk of errors by not relying on a potentially undefined.Values.service.ports. This approach allows for specifying any number of external ports directly, which is a significant improvement over the previous static method.The changes to the service port configuration are correctly implemented and improve the template's flexibility and robustness.
packages/apps/virtual-machine/values.yaml (2)
4-15: Approve the addition ofexternalPortsand its documentation.The addition of
externalPortsas a new configuration parameter allows users to specify which ports should be forwarded from outside the cluster. This is a significant enhancement for external access configurations, providing flexibility in network setups. The documentation is clear and provides sufficient detail about its usage.The implementation and documentation of
externalPortsare correctly handled and meet the objectives of enhancing VM configuration flexibility.
23-43: Review and approve the changes tosshKeysand the addition ofcloudInit.The modification of
sshKeysto be an empty array by default is a prudent security measure, encouraging users to explicitly define their SSH keys rather than relying on potentially insecure defaults. The addition ofcloudInitwith detailed documentation and examples provides users with the ability to customize VM instances extensively. This is aligned with best practices for cloud-native applications.The changes to
sshKeysand the addition ofcloudInitare well-implemented, enhancing security and customization capabilities for VM deployments.packages/apps/virtual-machine/values.schema.json (2)
62-63: Update tosshKeysdefault valueThe default value for
sshKeyshas been updated to an empty array, which is a sensible default for cases where no SSH keys are provided. This change is correctly implemented.The code changes are approved.
68-71: Addition ofcloudInitpropertyThe
cloudInitproperty has been added to allow users to specify cloud-init user data. The property is correctly defined as a string with a sensible default value. This addition enhances the flexibility of VM initialization.The code changes are approved.
packages/apps/virtual-machine/templates/vm.yaml (4)
48-52: Conditional inclusion ofcloudinitdiskThe conditional logic to include
cloudinitdiskbased on the presence of.Values.sshKeysor.Values.cloudInitis correctly implemented. This change ensures that resources are allocated dynamically based on configuration.The code changes are approved.
61-69: NewaccessCredentialsstructureThe new structure for
accessCredentialsto source SSH public keys from a Kubernetes secret is a significant improvement in terms of security and manageability. The implementation is correct and aligns with best practices for handling credentials in Kubernetes.The code changes are approved.
72-80: Modifiedvolumessection forcloudinitdiskThe modification to nest the
cloudInitNoCloudconfiguration undercloudinitdiskis a good practice, ensuring that cloud-init settings are applied only when necessary. The implementation is correct and enhances the template's flexibility.The code changes are approved.
81-83: Addition ofnetworkssectionThe addition of a
networkssection with a default network configuration is a valuable enhancement, improving the networking capabilities of the virtual machine. The implementation is straightforward and correctly done.The code changes are approved.
packages/apps/virtual-machine/README.md (3)
12-33: New section on accessing the virtual machineThe new section provides detailed instructions on accessing the virtual machine using the
virtctltool. This addition significantly enhances user guidance and is well-documented.The code changes are approved.
Tools
Markdownlint
19-19: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
25-25: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
39-51: UpdatedParameterssectionThe
Parameterssection has been updated with new and modified parameters, includingexternalPortsandcloudInit. These changes are well-documented and provide clear guidance on the new configuration options.The code changes are approved.
Tools
LanguageTool
[style] ~49-~49: 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
50-50: Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
Table pipe style(MD055, table-pipe-style)
51-51: Expected: leading_and_trailing; Actual: trailing_only; Missing leading pipe
Table pipe style(MD055, table-pipe-style)
51-51: Expected: 3; Actual: 1; Too few cells, row will be missing data
Table column count(MD056, table-column-count)
55-79: Example virtual machine configurationThe example configuration has been updated to reflect the new parameters and changes in default values. This update is crucial for users to understand how to apply the new settings effectively.
The code changes are approved.
Tools
Markdownlint
55-55: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
| {{- if .Values.sshKeys }} | ||
| --- | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ include "virtual-machine.fullname" $ }}-ssh-keys | ||
| stringData: | ||
| {{- range $k, $v := .Values.sshKeys }} | ||
| key{{ $k }}: {{ quote $v }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Ensure proper Helm syntax and address yamllint errors.
The Helm syntax error flagged by yamllint on line 1 might be a false positive, as the Helm templating syntax often conflicts with standard YAML linters. However, the trailing spaces on line 9 should be removed to adhere to best practices.
To address the yamllint errors, consider the following changes:
- key{{ $k }}: {{ quote $v }}
+ key{{ $k }}: {{ quote $v }}Additionally, ensure that the Helm syntax is correctly interpreted in your CI/CD pipeline by using a Helm-specific linter or testing the template deployment.
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.
| {{- if .Values.sshKeys }} | |
| --- | |
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: {{ include "virtual-machine.fullname" $ }}-ssh-keys | |
| stringData: | |
| {{- range $k, $v := .Values.sshKeys }} | |
| key{{ $k }}: {{ quote $v }} | |
| {{- end }} | |
| {{- end }} | |
| {{- if .Values.sshKeys }} | |
| --- | |
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: {{ include "virtual-machine.fullname" $ }}-ssh-keys | |
| stringData: | |
| {{- range $k, $v := .Values.sshKeys }} | |
| key{{ $k }}: {{ quote $v }} | |
| {{- end }} | |
| {{- end }} |
Tools
yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 9-9: trailing spaces
(trailing-spaces)
| "externalPorts": { | ||
| "type": "array", | ||
| "description": "Specify ports to forward from outside the cluster", | ||
| "default": "[]", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Addition of externalPorts property
The new externalPorts property allows users to specify ports for external access. The schema is correctly defined as an array of strings. However, the default value should be an array, not a string representation of an array.
Change the default value from "[]" to [] to correctly represent an empty array in JSON:
- "default": "[]",
+ "default": [],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.
| "externalPorts": { | |
| "type": "array", | |
| "description": "Specify ports to forward from outside the cluster", | |
| "default": "[]", | |
| "items": { | |
| "type": "string" | |
| } | |
| }, | |
| "externalPorts": { | |
| "type": "array", | |
| "description": "Specify ports to forward from outside the cluster", | |
| "default": [], | |
| "items": { | |
| "type": "string" | |
| } | |
| }, |
Previously, an error occurred when trying to access .Values.service.ports in the template, leading to a nil pointer dereference. This commit corrects the issue by ensuring that service.ports is checked for existence before attempting to access it, preventing the error from occurring.
Changes:
{{- with .Values.service }}...{{- end }}for conditional rendering of ports.Summary by CodeRabbit
New Features
externalPortsfor external access, andcloudInitfor user data configuration.virtctl.Bug Fixes
5Gito10Gi.Documentation
Chores
0.3.0to0.4.0, indicating a new release with significant changes.