-
Notifications
You must be signed in to change notification settings - Fork 2
fix(kubeovn): derive MASTER_NODES from server group for multi-master support #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4e8a87d
fix(cozystack): derive MASTER_NODES from server group for multi-maste…
lexfrei ad9f8a7
fix(tests): set explicit cozystack_master_nodes in CI inventory
lexfrei 99c495b
docs(examples): show multi-master inventory in ubuntu example
lexfrei e89dcc6
docs(readme): document cozystack_master_nodes and multi-master setup
lexfrei cfc72aa
test(master-nodes): add CI test for multi-master MASTER_NODES computa…
lexfrei fb3131a
refactor(cozystack): extract master-node computation, add IP validation
lexfrei 2280a7f
fix(ci): add ansible.utils dependency for IP validation, test single-…
lexfrei cb1edb1
test(master-nodes): add negative test for hostname host keys
lexfrei c238599
fix(ci): install netaddr in E2E job for ipaddr filter
lexfrei 68dd433
fix(galaxy): declare ansible.utils dependency for ipaddr filter
lexfrei File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| --- | ||
| collections: | ||
| - name: ansible.posix | ||
| - name: ansible.utils | ||
| - name: community.general | ||
| - name: kubernetes.core |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| --- | ||
| # Compute and validate master node IPs for kube-ovn. | ||
| # Included from main.yml and tests. | ||
| - name: Compute master node IPs for kube-ovn | ||
| ansible.builtin.set_fact: | ||
| _cozystack_master_nodes: >- | ||
| {%- if cozystack_master_nodes | length > 0 -%} | ||
| {{ cozystack_master_nodes }} | ||
| {%- else -%} | ||
| {{ groups['server'] | join(',') }} | ||
|
Comment on lines
+7
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard inventory lookups to avoid pre-assert crashes. At Line 10, Proposed hardening _cozystack_master_nodes: >-
- {%- if cozystack_master_nodes | length > 0 -%}
+ {%- if (cozystack_master_nodes | default('')) | length > 0 -%}
{{ cozystack_master_nodes }}
{%- else -%}
- {{ groups['server'] | join(',') }}
+ {{ groups.get('server', []) | join(',') }}
{%- endif -%}🤖 Prompt for AI Agents |
||
| {%- endif -%} | ||
|
|
||
| - name: Validate master node IPs are non-empty | ||
| ansible.builtin.assert: | ||
| that: | ||
| - _cozystack_master_nodes | length > 0 | ||
| fail_msg: >- | ||
| Could not determine master node IPs for kube-ovn. | ||
| Set cozystack_master_nodes explicitly in your inventory. | ||
|
|
||
| - name: Validate master node IPs are valid addresses | ||
| ansible.builtin.assert: | ||
| that: | ||
| - (item | ansible.utils.ipaddr) != false | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| fail_msg: >- | ||
| '{{ item }}' is not a valid IP address in MASTER_NODES. | ||
| Inventory host keys in the 'server' group must be IPs, not hostnames. | ||
| Alternatively, set cozystack_master_nodes explicitly. | ||
| loop: "{{ _cozystack_master_nodes.split(',') }}" | ||
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| --- | ||
| # Inventory with hostname host keys (NOT IPs). | ||
| # Used to verify that IP validation correctly rejects hostnames. | ||
| cluster: | ||
| children: | ||
| server: | ||
| hosts: | ||
| node1.example.com: | ||
| ansible_connection: local | ||
| ansible_host: 127.0.0.1 | ||
| agent: | ||
| hosts: {} | ||
|
|
||
| vars: | ||
| ansible_user: runner | ||
| cozystack_api_server_host: "10.0.0.1" |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| --- | ||
| # Verify that MASTER_NODES is correctly derived from the 'server' | ||
| # inventory group in both single-master and multi-master setups. | ||
| # Uses the actual role tasks (no duplicated Jinja2 logic). | ||
| - name: Test MASTER_NODES auto-detection and template rendering | ||
| hosts: server[0] | ||
| gather_facts: false | ||
| pre_tasks: | ||
| - name: Set template defaults when not defined by inventory | ||
| ansible.builtin.set_fact: | ||
| cozystack_master_nodes: "{{ cozystack_master_nodes | default('') }}" | ||
| cozystack_api_server_host: "{{ cozystack_api_server_host | default('127.0.0.1') }}" | ||
| cozystack_api_server_port: "{{ cozystack_api_server_port | default('6443') }}" | ||
| cozystack_namespace: "{{ cozystack_namespace | default('cozy-system') }}" | ||
| cozystack_platform_variant: "{{ cozystack_platform_variant | default('isp-full-generic') }}" | ||
| cozystack_root_host: "{{ cozystack_root_host | default('') }}" | ||
| cozystack_pod_cidr: "{{ cozystack_pod_cidr | default('10.42.0.0/16') }}" | ||
| cozystack_pod_gateway: "{{ cozystack_pod_gateway | default('10.42.0.1') }}" | ||
| cozystack_svc_cidr: "{{ cozystack_svc_cidr | default('10.43.0.0/16') }}" | ||
| cozystack_join_cidr: "{{ cozystack_join_cidr | default('100.64.0.0/16') }}" | ||
| tasks: | ||
| - name: Include role master-node computation | ||
| ansible.builtin.include_tasks: | ||
| file: "{{ playbook_dir }}/../roles/cozystack/tasks/compute-master-nodes.yml" | ||
|
|
||
| - name: Assert auto-detected MASTER_NODES matches server group | ||
| ansible.builtin.assert: | ||
| that: | ||
| - >- | ||
| (_cozystack_master_nodes | split(',') | sort) == | ||
| (groups['server'] | sort) | ||
| fail_msg: >- | ||
| Expected MASTER_NODES to match server group hosts. | ||
| Got '{{ _cozystack_master_nodes }}', | ||
| expected '{{ groups['server'] | join(',') }}'. | ||
| when: (cozystack_master_nodes | default('')) | length == 0 | ||
|
|
||
| - name: Construct API server endpoint | ||
| ansible.builtin.set_fact: | ||
| _cozystack_api_endpoint: "https://{{ cozystack_api_server_host }}:{{ cozystack_api_server_port }}" | ||
|
|
||
| - name: Render Platform Package template | ||
| ansible.builtin.template: | ||
| src: "{{ playbook_dir }}/../roles/cozystack/templates/platform-package.yml.j2" | ||
| dest: /tmp/cozystack-test-platform-package.yml | ||
| mode: "0600" | ||
|
|
||
| - name: Read rendered manifest | ||
| ansible.builtin.slurp: | ||
| src: /tmp/cozystack-test-platform-package.yml | ||
| register: _rendered_manifest | ||
|
|
||
| - name: Assert rendered MASTER_NODES contains all server IPs | ||
| ansible.builtin.assert: | ||
| that: | ||
| - "'MASTER_NODES' in _rendered_content" | ||
| - "(_cozystack_master_nodes | to_json) in _rendered_content" | ||
| fail_msg: >- | ||
| Rendered template does not contain expected MASTER_NODES value. | ||
| Expected {{ _cozystack_master_nodes | to_json }} in output. | ||
| vars: | ||
| _rendered_content: "{{ _rendered_manifest.content | b64decode }}" | ||
|
|
||
| - name: Show rendered MASTER_NODES (debug) | ||
| ansible.builtin.debug: | ||
| msg: "MASTER_NODES = {{ _cozystack_master_nodes }}" | ||
|
|
||
| - name: Clean up test manifest | ||
| ansible.builtin.file: | ||
| path: /tmp/cozystack-test-platform-package.yml | ||
| state: absent | ||
| changed_when: false | ||
|
|
||
| - name: Test explicit cozystack_master_nodes override | ||
| hosts: server[0] | ||
| gather_facts: false | ||
| tasks: | ||
| - name: Set explicit master nodes override | ||
| ansible.builtin.set_fact: | ||
| cozystack_master_nodes: "192.168.1.1,192.168.1.2" | ||
|
|
||
| - name: Include role master-node computation | ||
| ansible.builtin.include_tasks: | ||
| file: "{{ playbook_dir }}/../roles/cozystack/tasks/compute-master-nodes.yml" | ||
|
|
||
| - name: Assert explicit override is used | ||
| ansible.builtin.assert: | ||
| that: | ||
| - _cozystack_master_nodes == "192.168.1.1,192.168.1.2" | ||
| fail_msg: >- | ||
| Expected explicit override '192.168.1.1,192.168.1.2', | ||
| got '{{ _cozystack_master_nodes }}'. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| --- | ||
| # Test inventory with three control-plane nodes. | ||
| # Used to verify MASTER_NODES auto-detection from the 'server' group. | ||
| cluster: | ||
| children: | ||
| server: | ||
| hosts: | ||
| 10.0.0.10: | ||
| ansible_connection: local | ||
| ansible_host: 127.0.0.1 | ||
| 10.0.0.11: | ||
| ansible_connection: local | ||
| ansible_host: 127.0.0.1 | ||
| 10.0.0.12: | ||
| ansible_connection: local | ||
| ansible_host: 127.0.0.1 | ||
| agent: | ||
| hosts: {} | ||
|
|
||
| vars: | ||
| ansible_user: runner | ||
|
|
||
| # Cozystack configuration | ||
| cozystack_api_server_host: "10.0.0.10" | ||
| cozystack_api_server_port: "6443" | ||
| cozystack_namespace: "cozy-system" | ||
| cozystack_platform_variant: "isp-full-generic" | ||
| cozystack_create_platform_package: true | ||
| cozystack_root_host: "" | ||
| cozystack_pod_cidr: "10.42.0.0/16" | ||
| cozystack_pod_gateway: "10.42.0.1" | ||
| cozystack_svc_cidr: "10.43.0.0/16" | ||
| cozystack_join_cidr: "100.64.0.0/16" |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| --- | ||
| # Single-master test inventory with IP as host key. | ||
| # Tests auto-detection path (cozystack_master_nodes is NOT set). | ||
| cluster: | ||
| children: | ||
| server: | ||
| hosts: | ||
| 127.0.0.1: | ||
| ansible_connection: local | ||
| agent: | ||
| hosts: {} | ||
|
|
||
| vars: | ||
| ansible_user: runner | ||
| cozystack_api_server_host: "127.0.0.1" |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tighten negative test assertion to prevent false positives.
At Lines 102-108, any playbook failure is considered success. This can pass even when failure is unrelated to hostname-key validation.
✅ Proposed fix (assert failure reason)
- name: Test hostname host keys are rejected run: | - if ansible-playbook tests/test-master-nodes.yml \ - --inventory tests/test-hostname-inventory.yml 2>&1; then + set +e + output="$(ansible-playbook tests/test-master-nodes.yml \ + --inventory tests/test-hostname-inventory.yml 2>&1)" + status=$? + set -e + + if [ "$status" -eq 0 ]; then echo "ERROR: Expected failure for hostname host keys, but playbook succeeded" exit 1 - else - echo "OK: Hostname host keys correctly rejected" fi + + if ! grep -q "not a valid IP address in MASTER_NODES" <<<"$output"; then + echo "ERROR: Playbook failed, but not due to hostname/IP validation" + echo "$output" + exit 1 + fi + + echo "OK: Hostname host keys correctly rejected"🤖 Prompt for AI Agents