Skip to content

Commit d8ba563

Browse files
author
Guillaume Harvey
committed
Fix empty proxy configuration breaking CAPI validation
Fixes #790 When HTTP/HTTPS proxy is not configured, empty strings returned by generate_systemd_proxy_config() and generate_apt_proxy_config() were being base64-encoded and causing YAML parsing errors in Cluster API. The issue occurs because: 1. utils.generate_systemd_proxy_config() returns "" when no proxy is set 2. utils.generate_apt_proxy_config() returns "" when no proxy is set 3. These empty strings get base64-encoded in resources.py 4. When CAPI decodes them, empty values break cloud-init YAML parsing 5. Cluster creation fails with validation errors The fix: - Use "or '#'" fallback in resources.py when encoding proxy configs - When proxy functions return "", the fallback "#" is used instead - "#" is a comment character in both systemd and apt config files - Base64-encoding "#" is safe and doesn't break YAML parsing - Clusters with actual proxy configs continue to work normally Changes: - magnum_cluster_api/resources.py: Add "or '#'" fallback to proxy config encoding - magnum_cluster_api/tests/unit/test_resources.py: Add comprehensive unit tests Test coverage includes: 1. Clusters without proxy configuration (fallback to "#") 2. Clusters with proxy configuration (actual config encoded) 3. Critical test ensuring empty strings are never encoded This is a minimal, safe fix that: - Prevents cluster creation failures - Has no impact on existing proxy-enabled clusters - Uses a harmless comment character as fallback - Is compatible with all configuration file formats Related issue: #790 Signed-off-by: Guillaume Harvey <[email protected]>
1 parent d5c50af commit d8ba563

File tree

2 files changed

+248
-2
lines changed

2 files changed

+248
-2
lines changed

magnum_cluster_api/resources.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,13 +1121,13 @@ def get_object(self) -> dict:
11211121
{
11221122
"name": "systemdProxyConfig",
11231123
"value": base64.encode_as_text(
1124-
utils.generate_systemd_proxy_config(self.cluster)
1124+
utils.generate_systemd_proxy_config(self.cluster) or "#"
11251125
),
11261126
},
11271127
{
11281128
"name": "aptProxyConfig",
11291129
"value": base64.encode_as_text(
1130-
utils.generate_apt_proxy_config(self.cluster)
1130+
utils.generate_apt_proxy_config(self.cluster) or "#"
11311131
),
11321132
},
11331133
{

magnum_cluster_api/tests/unit/test_resources.py

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,249 @@ def test_mutate_machine_deployment(self, context, auto_scaling_enabled):
155155
else:
156156
assert md["replicas"] == self.node_group.node_count
157157
assert md["metadata"]["annotations"] == {}
158+
159+
160+
class TestClusterProxyConfiguration:
161+
"""
162+
Test that proxy configuration values are properly handled when base64-encoded.
163+
164+
This test class addresses the fix for GitHub issue #790:
165+
https://github.com/vexxhost/magnum-cluster-api/issues/790
166+
167+
The issue: When no proxy is configured, empty strings from proxy generation
168+
functions get base64-encoded, creating invalid YAML that breaks CAPI validation.
169+
170+
The fix: Use "or '#'" fallback to ensure a harmless comment character is
171+
encoded instead of an empty string when no proxy is configured.
172+
"""
173+
174+
@pytest.fixture(autouse=True)
175+
def setup(self, context, mocker):
176+
"""Setup test fixtures for proxy configuration tests."""
177+
self.context = context
178+
self.cluster = utils.get_test_cluster(context, labels={})
179+
self.cluster.cluster_template = utils.get_test_cluster_template(context)
180+
self.cluster.stack_id = "test-cluster-stack-id"
181+
182+
# Mock API clients
183+
self.api = mocker.Mock()
184+
self.pykube_api = mocker.Mock()
185+
186+
# Mock OpenStack API
187+
mock_osc = mocker.patch("magnum_cluster_api.clients.get_openstack_api")
188+
self.mock_osc_instance = mock_osc.return_value
189+
190+
# Mock Cinder volume types
191+
mock_volume_type = mocker.Mock()
192+
mock_volume_type.name = "default"
193+
self.mock_osc_instance.cinder().volume_types.default.return_value = (
194+
mock_volume_type
195+
)
196+
197+
# Mock Nova flavor lookup
198+
mock_lookup_flavor = mocker.patch("magnum_cluster_api.utils.lookup_flavor")
199+
mock_lookup_flavor.return_value = flavors.Flavor(
200+
None,
201+
{"name": "fake-flavor", "disk": 10, "ram": 1024, "vcpus": 1},
202+
)
203+
204+
# Mock Glance image lookup
205+
mock_lookup_image = mocker.patch("magnum_cluster_api.utils.lookup_image")
206+
mock_lookup_image.return_value = {"id": "fake-image-id", "hw_disk_bus": "scsi"}
207+
208+
# Mock Neutron network
209+
mock_neutron = mocker.patch("magnum.common.neutron")
210+
mock_neutron.get_external_network_id.return_value = "fake-network-id"
211+
mock_neutron.get_fixed_subnet_id.return_value = "fake-subnet-id"
212+
213+
# Mock server groups
214+
mocker.patch(
215+
"magnum_cluster_api.utils.ensure_controlplane_server_group",
216+
return_value="fake-server-group-id",
217+
)
218+
219+
def test_cluster_without_proxy_configuration(self, mocker):
220+
"""
221+
Test that clusters without proxy configuration get comment fallback.
222+
223+
When no proxy is configured:
224+
- generate_systemd_proxy_config() returns ""
225+
- generate_apt_proxy_config() returns ""
226+
- The fix ensures "#" is encoded instead of ""
227+
"""
228+
# Setup cluster without proxy
229+
self.cluster.cluster_template.http_proxy = None
230+
self.cluster.cluster_template.https_proxy = None
231+
self.cluster.cluster_template.no_proxy = None
232+
233+
# Mock the proxy generation functions to return empty strings
234+
mock_systemd_proxy = mocker.patch(
235+
"magnum_cluster_api.utils.generate_systemd_proxy_config",
236+
return_value="",
237+
)
238+
mock_apt_proxy = mocker.patch(
239+
"magnum_cluster_api.utils.generate_apt_proxy_config", return_value=""
240+
)
241+
242+
# Create Cluster resource
243+
cluster_resource = resources.Cluster(
244+
self.context, self.api, self.pykube_api, self.cluster
245+
)
246+
cluster_obj = cluster_resource.get_object()
247+
248+
# Verify proxy generation functions were called
249+
mock_systemd_proxy.assert_called_once_with(self.cluster)
250+
mock_apt_proxy.assert_called_once_with(self.cluster)
251+
252+
# Extract the proxy config variables from the cluster spec
253+
variables = cluster_obj["spec"]["topology"]["variables"]
254+
systemd_proxy_var = next(
255+
v for v in variables if v["name"] == "systemdProxyConfig"
256+
)
257+
apt_proxy_var = next(v for v in variables if v["name"] == "aptProxyConfig")
258+
259+
# Import base64 for decoding
260+
from oslo_serialization import base64
261+
262+
# Decode the base64 values
263+
systemd_decoded = base64.decode_as_text(systemd_proxy_var["value"])
264+
apt_decoded = base64.decode_as_text(apt_proxy_var["value"])
265+
266+
# Assert: When empty string is returned, fallback to "#" is used
267+
# The decoded value should be "#" (comment character) not ""
268+
assert systemd_decoded == "#", (
269+
f"Expected '#' but got '{systemd_decoded}'. "
270+
"Empty proxy config should fallback to comment character."
271+
)
272+
assert apt_decoded == "#", (
273+
f"Expected '#' but got '{apt_decoded}'. "
274+
"Empty proxy config should fallback to comment character."
275+
)
276+
277+
def test_cluster_with_proxy_configuration(self, mocker):
278+
"""
279+
Test that clusters with proxy configuration use actual proxy values.
280+
281+
When proxy is configured:
282+
- generate_systemd_proxy_config() returns systemd configuration
283+
- generate_apt_proxy_config() returns apt configuration
284+
- The actual configuration should be base64-encoded (not the fallback)
285+
"""
286+
# Setup cluster with proxy
287+
self.cluster.cluster_template.http_proxy = "http://proxy.example.com:3128"
288+
self.cluster.cluster_template.https_proxy = "https://proxy.example.com:3128"
289+
self.cluster.cluster_template.no_proxy = "localhost,127.0.0.1"
290+
291+
# Expected proxy configurations (matching what utils.py generates)
292+
expected_systemd_config = (
293+
"[Service]\n"
294+
'Environment="http_proxy=http://proxy.example.com:3128"\n'
295+
'Environment="HTTP_PROXY=http://proxy.example.com:3128"\n'
296+
'Environment="https_proxy=https://proxy.example.com:3128"\n'
297+
'Environment="HTTPS_PROXY=https://proxy.example.com:3128"\n'
298+
'Environment="no_proxy=localhost,127.0.0.1"\n'
299+
'Environment="NO_PROXY=localhost,127.0.0.1"\n'
300+
)
301+
302+
expected_apt_config = (
303+
'Acquire::http::Proxy "http://proxy.example.com:3128";\n'
304+
'Acquire::https::Proxy "https://proxy.example.com:3128";\n'
305+
)
306+
307+
# Mock the proxy generation functions to return real configurations
308+
mock_systemd_proxy = mocker.patch(
309+
"magnum_cluster_api.utils.generate_systemd_proxy_config",
310+
return_value=expected_systemd_config,
311+
)
312+
mock_apt_proxy = mocker.patch(
313+
"magnum_cluster_api.utils.generate_apt_proxy_config",
314+
return_value=expected_apt_config,
315+
)
316+
317+
# Create Cluster resource
318+
cluster_resource = resources.Cluster(
319+
self.context, self.api, self.pykube_api, self.cluster
320+
)
321+
cluster_obj = cluster_resource.get_object()
322+
323+
# Verify proxy generation functions were called
324+
mock_systemd_proxy.assert_called_once_with(self.cluster)
325+
mock_apt_proxy.assert_called_once_with(self.cluster)
326+
327+
# Extract the proxy config variables
328+
variables = cluster_obj["spec"]["topology"]["variables"]
329+
systemd_proxy_var = next(
330+
v for v in variables if v["name"] == "systemdProxyConfig"
331+
)
332+
apt_proxy_var = next(v for v in variables if v["name"] == "aptProxyConfig")
333+
334+
# Import base64 for decoding
335+
from oslo_serialization import base64
336+
337+
# Decode the base64 values
338+
systemd_decoded = base64.decode_as_text(systemd_proxy_var["value"])
339+
apt_decoded = base64.decode_as_text(apt_proxy_var["value"])
340+
341+
# Assert: Actual proxy configurations are encoded (not the fallback)
342+
assert systemd_decoded == expected_systemd_config, (
343+
"Systemd proxy config should contain actual proxy settings when configured"
344+
)
345+
assert apt_decoded == expected_apt_config, (
346+
"APT proxy config should contain actual proxy settings when configured"
347+
)
348+
349+
# Assert: The fallback "#" should NOT be present
350+
assert systemd_decoded != "#", "Should not use fallback when proxy is configured"
351+
assert apt_decoded != "#", "Should not use fallback when proxy is configured"
352+
353+
def test_empty_string_never_encoded(self, mocker):
354+
"""
355+
Critical test: Ensure empty strings are NEVER base64-encoded.
356+
357+
This is the core issue from GitHub #790:
358+
- Empty strings break YAML parsing in CAPI
359+
- Must always use "#" fallback for empty proxy configs
360+
"""
361+
# Force empty string return from proxy functions
362+
mocker.patch(
363+
"magnum_cluster_api.utils.generate_systemd_proxy_config", return_value=""
364+
)
365+
mocker.patch(
366+
"magnum_cluster_api.utils.generate_apt_proxy_config", return_value=""
367+
)
368+
369+
# Create cluster without proxy
370+
self.cluster.cluster_template.http_proxy = None
371+
self.cluster.cluster_template.https_proxy = None
372+
373+
cluster_resource = resources.Cluster(
374+
self.context, self.api, self.pykube_api, self.cluster
375+
)
376+
cluster_obj = cluster_resource.get_object()
377+
378+
# Extract variables
379+
variables = cluster_obj["spec"]["topology"]["variables"]
380+
systemd_proxy_var = next(
381+
v for v in variables if v["name"] == "systemdProxyConfig"
382+
)
383+
apt_proxy_var = next(v for v in variables if v["name"] == "aptProxyConfig")
384+
385+
from oslo_serialization import base64
386+
387+
# Decode values
388+
systemd_decoded = base64.decode_as_text(systemd_proxy_var["value"])
389+
apt_decoded = base64.decode_as_text(apt_proxy_var["value"])
390+
391+
# CRITICAL ASSERTION: Must NEVER be empty string
392+
assert systemd_decoded != "", (
393+
"CRITICAL: Empty string must not be encoded. "
394+
"This breaks CAPI YAML parsing. Must use '#' fallback."
395+
)
396+
assert apt_decoded != "", (
397+
"CRITICAL: Empty string must not be encoded. "
398+
"This breaks CAPI YAML parsing. Must use '#' fallback."
399+
)
400+
401+
# Assert correct fallback is used
402+
assert systemd_decoded == "#"
403+
assert apt_decoded == "#"

0 commit comments

Comments
 (0)