Add KEA DHCP provider feature for foreman-proxy#531
Conversation
| description: Power management for bare metal hosts (IPMI, Redfish) | ||
| foreman_proxy: | ||
| plugin_name: bmc | ||
| dhcp-kea-external: |
There was a problem hiding this comment.
I think the feature should be "dhcp" and then you add a parameter to select which provider to use, with kea being one of the answers
There was a problem hiding this comment.
makes way more sense to have a single dhcp feature with a provider parameter
There was a problem hiding this comment.
I played with that thought in #532 for the DNS side of things, would love to hear your opinion on the approach I took
| foreman_proxy_bmc_redfish_verify_ssl: true | ||
|
|
||
| # KEA DHCP settings (external unmanaged server) | ||
| foreman_proxy_dhcp_kea_server: localhost |
There was a problem hiding this comment.
What are the chances that localhost is a place where a Kea will actually run (outside of development setups)?
I think we should leave that undef (like in https://github.com/theforeman/foremanctl/pull/523/changes) and force the user to set a sensible value.
|
|
||
| :enabled: {{ feature_enabled }} | ||
| :use_provider: dhcp_kea_api | ||
| :server: {{ foreman_proxy_dhcp_kea_server }} |
There was a problem hiding this comment.
where does this come from? I don't see such an option in https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/
There was a problem hiding this comment.
yeah, i was just playing around with few things, needed to look again, the reason i added that in draft but thanks for reviews, also i tryna fix few things with the updated changes.
| :enabled: {{ feature_enabled }} | ||
| :use_provider: dhcp_kea_api | ||
| :server: {{ foreman_proxy_dhcp_kea_server }} | ||
| :kea_url: {{ foreman_proxy_dhcp_kea_url }} |
There was a problem hiding this comment.
also, we need kea_api_username and kea_api_password
| :use_provider: dhcp_kea_api | ||
| :server: {{ foreman_proxy_dhcp_kea_server }} | ||
| :kea_url: {{ foreman_proxy_dhcp_kea_url }} | ||
| :kea_subnet: {{ foreman_proxy_dhcp_kea_subnet }} |
There was a problem hiding this comment.
where does this come from? I don't see such an option in https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/
|
Shall we package https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api until we figure out whether we want to merge it into core or not? |
Yeah, I was thinking we can package it for now, that will be independent of our decision about potentially merging it into the core later. |
| foreman_proxy_bmc_redfish_verify_ssl: true | ||
|
|
||
| # DHCP settings | ||
| foreman_proxy_dhcp_provider: dhcp_kea_api |
There was a problem hiding this comment.
Why not just:
| foreman_proxy_dhcp_provider: dhcp_kea_api | |
| foreman_proxy_dhcp_provider: dhcp_kea |
| :kea_api_password: {{ foreman_proxy_dhcp_kea_api_password }} | ||
| {% endif %} | ||
| {% if foreman_proxy_dhcp_kea_managed_subnets is defined and foreman_proxy_dhcp_kea_managed_subnets %} | ||
| :managed_subnets: |
There was a problem hiding this comment.
There is a definition of subnets in DHCP as well: https://github.com/theforeman/smart-proxy/blob/develop/config/settings.d/dhcp.yml.example
| foreman_proxy_dhcp_kea_api_url: "{{ undef(hint='You must specify the KEA API URL') }}" | ||
| foreman_proxy_dhcp_kea_api_username: "" | ||
| foreman_proxy_dhcp_kea_api_password: "" | ||
| foreman_proxy_dhcp_kea_managed_subnets: [] |
There was a problem hiding this comment.
I think this should be:
| foreman_proxy_dhcp_kea_managed_subnets: [] | |
| foreman_proxy_dhcp_subnets: [] |
as in https://github.com/theforeman/smart-proxy/blob/develop/config/settings.d/dhcp.yml.example
Do you want to do it, or shall I? |
I'll do it |
053aa03 to
715b2e0
Compare
|
Update: Following the refinement session, we decided to integrate the KEA DHCP provider directly into smart-proxy core rather than packaging it as an external gem. I'm on it. So keeping this into draft till then. Also looking at the current CI test failures, I believe they are coming from IOP tests failures and are unrelated to the DHCP feature changes in this PR. |
16f5653 to
a4dd887
Compare
| type: Boolean | ||
| foreman_proxy_dhcp_kea_api_url: | ||
| parameter: --foreman-proxy-dhcp-kea-api-url | ||
| help: KEA Control Agent API URL (e.g., http://kea-server:8000/) |
There was a problem hiding this comment.
Nit: https example
| help: KEA Control Agent API URL (e.g., http://kea-server:8000/) | |
| help: KEA Control Agent API URL (e.g., https://kea-server) |
There was a problem hiding this comment.
(foreman_proxy_dhcp_kea_verify_ssl) is true by default, so this should be https
| help: KEA Control Agent API URL (e.g., http://kea-server:8000/) | ||
| foreman_proxy_dhcp_kea_api_username: | ||
| parameter: --foreman-proxy-dhcp-kea-api-username | ||
| help: Optional HTTP Basic Auth username for KEA API |
There was a problem hiding this comment.
Do we want to make it optional? It's not really secure IMO
There was a problem hiding this comment.
I have updated it but auth is genuinely optional for kea control agent (many deployments run without auth on internal networks), but i agree we should encourage it for security. so fixed it
|
|
||
| # KEA DHCP provider settings (when foreman_proxy_dhcp_provider is dhcp_kea) | ||
| foreman_proxy_dhcp_kea_api_url: "{{ undef(hint='You must specify the KEA API URL') }}" | ||
| foreman_proxy_dhcp_kea_api_username: "{{ undef(hint='Optional HTTP Basic Auth username for KEA API') }}" |
There was a problem hiding this comment.
If the parameter is optional, why raise an error when no value is provided?
There was a problem hiding this comment.
So I removed optional from the hint text to reduce any confusion but the undef() is never actually raised because the template checks if you see
{% if foreman_proxy_dhcp_kea_api_username is defined and foreman_proxy_dhcp_kea_api_username %}
:dhcp_kea_api_username: {{ foreman_proxy_dhcp_kea_api_username }}
{% else %}
:dhcp_kea_api_username: ~
{% endif %}
is defined before using it. I added just to satisfy the linter earlier. do you have any other suggestons?
| help: Optional HTTP Basic Auth password for KEA API | ||
| foreman_proxy_dhcp_kea_verify_ssl: | ||
| parameter: --foreman-proxy-dhcp-kea-verify-ssl | ||
| help: Verify SSL certificates when connecting to KEA API |
There was a problem hiding this comment.
Mention the default value (true)
| # KEA DHCP provider settings (when foreman_proxy_dhcp_provider is dhcp_kea) | ||
| foreman_proxy_dhcp_kea_api_url: "{{ undef(hint='You must specify the KEA API URL') }}" | ||
| foreman_proxy_dhcp_kea_api_username: "{{ undef(hint='Optional HTTP Basic Auth username for KEA API') }}" | ||
| foreman_proxy_dhcp_kea_api_password: "{{ undef(hint='Optional HTTP Basic Auth password for KEA API') }}" |
| parameter: --bmc-redfish-verify-ssl | ||
| help: Verify SSL certificates for Redfish BMC connections. | ||
| type: Boolean | ||
| foreman_proxy_dhcp_kea_api_url: |
There was a problem hiding this comment.
I don't see a parameter for DHCP subnets.
There was a problem hiding this comment.
yes, now we can specify this
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739
a4dd887 to
1db2d43
Compare
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739
| :dhcp_kea_password: ~ | ||
| {% endif %} | ||
| :dhcp_kea_verify_ssl: {{ foreman_proxy_dhcp_kea_verify_ssl | default(true) | lower }} | ||
| :dhcp_kea_lease_timeout: {{ foreman_proxy_dhcp_kea_lease_timeout | default(60) }} |
There was a problem hiding this comment.
The default value should be set in the src/roles/foreman_proxy/defaults/main.yaml
| :dhcp_kea_password: ~ | ||
| {% endif %} | ||
| :dhcp_kea_verify_ssl: {{ foreman_proxy_dhcp_kea_verify_ssl | default(true) | lower }} | ||
| :dhcp_kea_lease_timeout: {{ foreman_proxy_dhcp_kea_lease_timeout | default(60) }} |
There was a problem hiding this comment.
Question: Do we want to give users a chance to edit this value via foremanctl? If yes, then we need another parameter for it.
There was a problem hiding this comment.
Yes, both values are exposed as CLI parameters
| {% else %} | ||
| :dhcp_kea_password: ~ | ||
| {% endif %} | ||
| :dhcp_kea_verify_ssl: {{ foreman_proxy_dhcp_kea_verify_ssl | default(true) | lower }} |
There was a problem hiding this comment.
Is this needed when the default value is already set in src/roles/foreman_proxy/defaults/main.yaml?
| :dhcp_kea_verify_ssl: {{ foreman_proxy_dhcp_kea_verify_ssl | default(true) | lower }} | |
| :dhcp_kea_verify_ssl: {{ foreman_proxy_dhcp_kea_verify_ssl }} |
1db2d43 to
883858e
Compare
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739 Fix Rubocop failures Simplify KEA plugin config loading
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739 Fix Rubocop failures Simplify KEA plugin config loading
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739 Fix Rubocop failures Simplify KEA plugin config loading
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739 Fix Rubocop failures Simplify KEA plugin config loading
ea7ba9f to
7ea157e
Compare
Ref: theforeman/foremanctl#531 (comment) ISC DHCP reached end-of-life in October 2022 and is no longer maintained. ISC officially recommends migrating to KEA DHCP as the replacement. This commit adds Smart Proxy DHCP provider for ISC KEA DHCP server using the KEA Control Agent API, providing a migration path for users currently using the deprecated dhcp_isc provider. Based on the `smart_proxy_dhcp_kea_api` gem by Sam McCarthy with author's permission to integrate into Smart Proxy core. https://gitlab.surrey.ac.uk/sm0049/smart-proxy-dhcp-kea-api/-/issues/3 Fixes: https://redhat.atlassian.net/browse/SAT-27739 Fix Rubocop failures Simplify KEA plugin config loading
ad89208 to
00439b3
Compare
| - name: Configure Kea DHCP4 server | ||
| ansible.builtin.copy: | ||
| dest: /etc/kea/kea-dhcp4.conf | ||
| content: | |
There was a problem hiding this comment.
Can we make it a file/template?
| name: kea-ctrl-agent | ||
| state: started | ||
|
|
||
| - name: Wait for Kea Control Agent to be ready |
There was a problem hiding this comment.
This is failing for me, repeatedly.
TASK [Wait for Kea Control Agent to be ready] ****************************************************************************************************
[ERROR]: Task failed: Module failed: Timeout when waiting for 127.0.0.1:8000
Origin: /home/lstejska/projects/foremanctl/development/playbooks/kea/kea.yaml:115:7
113 state: started
114
115 - name: Wait for Kea Control Agent to be ready
^ column 7
fatal: [quadlet]: FAILED! => {
"changed": false,
"elapsed": 30
}
MSG:
Timeout when waiting for 127.0.0.1:8000
PLAY RECAP ***************************************************************************************************************************************
quadlet : ok=12 changed=9 unreachable=0 failed=1 skipped=0 rescued=0 ignored=0
before I ran:
./foremanctl deploy --initial-admin-password=changeme --tuning development --add-feature foreman-proxy --add-feature dhcp --foreman-proxy-dhcp-kea-url http://127.0.0.1:8000
There was a problem hiding this comment.
I see it for me as well, taking a look on it
There was a problem hiding this comment.
Also you don't need to include --add-feature foreman-proxy, I have added it as a dependency in the dhcp with expand_features that automatically includes all dependencies.
| - name: Create installer certificates | ||
| if: contains(matrix.certificate_source, 'installer') | ||
| run: | | ||
| ./forge installer-certs |
There was a problem hiding this comment.
Why is this needed?
For me it's failing with:
forge: error: argument action: invalid choice: 'installer-certs' (choose from 'custom-certs', 'deploy-dev', 'kea', 'lock', 'mock-installer', 'remote-database', 'security', 'setup-repositories', 'smoker', 'sos', 'test', 'vms'
There was a problem hiding this comment.
I see that's a leftover dead code. I should have dropped earlier. Let's see now
ed48493 to
ac5d9bb
Compare
Adds the dhcp-kea-external feature to enable Smart Proxy integration with external KEA DHCP servers.
Since it defaults to dhcp_kea, we don't need to pass it at all
ac5d9bb to
8a1408e
Compare
Why are you introducing these changes? (Problem description, related links)
ISC DHCP reached end-of-life in October 2022 and has been removed from RHEL 10. This adds support for KEA DHCP as the replacement.
What are the changes introduced in this pull request?
dhcp-kea-externalfeature definition tosrc/features.yamldhcp_kea_api.yml.j2How to test this pull request
Steps to reproduce:
./foremanctl features | grep dhcp-kea-external./foremanctl deploy --add-feature dhcp-kea-external./foremanctl features --list-enabled | grep dhcp-kea-externalpodman exec foreman-proxy ls -la /etc/foreman-proxy/settings.d/dhcp_kea_api.ymlpython -m pytest tests/foreman_proxy_test.py::test_dhcp_kea_feature_present -vChecklist