cephadm: improve oauth2-proxy validation error messaging

Previously, applying oauth2-proxy without required configuration
resulted in misleading field-level validation errors.

This change introduces:
- Early validation for missing spec in CLI path
- Aggregated missing field validation in OAuth2ProxySpec
- Clear and actionable error messages

Fixes:
- ceph orch apply oauth2-proxy without spec
- ceph orch apply -i <yaml> with missing required fields

Fixes: https://tracker.ceph.com/issues/76372
Signed-off-by: Shubha Jain <SHUBHA.JAIN1@ibm.com>

diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py
index 56d94fe24ab3d7bc763453171036e76826fd36d9..74f00c18caddbb02c092c63a056b68ab6627ef2d 100644 (file)
--- a/src/pybind/mgr/orchestrator/module.py
+++ b/src/pybind/mgr/orchestrator/module.py
@@ -2106,12 +2106,39 @@ Usage:
                             no_overwrite: bool = False,
                             inbuf: Optional[str] = None) -> HandleCommandResult:
         """Add a cluster gateway service (cephadm only)"""
-
-        spec = OAuth2ProxySpec(
-            placement=PlacementSpec.from_string(placement),
-            unmanaged=unmanaged,
-            https_address=https_address
+        missing_oauth2_proxy_config = (
+            'Missing required configuration for oauth2-proxy. Please provide a spec '
+            'file with required fields: provider_display_name, oidc_issuer_url, '
+            'client_id, client_secret.'
         )
+        if not inbuf or not inbuf.strip():
+            raise OrchestratorError(missing_oauth2_proxy_config)
+
+        try:
+            spec_data = yaml.safe_load(inbuf)
+        except (OSError, yaml.YAMLError):
+            raise OrchestratorValidationError('oauth2-proxy spec file must be valid YAML')
+
+        if not spec_data:
+            raise OrchestratorError(missing_oauth2_proxy_config)
+        if not isinstance(spec_data, dict):
+            raise OrchestratorValidationError(
+                'oauth2-proxy spec file must contain a single YAML object'
+            )
+
+        spec = ServiceSpec.from_json(spec_data)
+        if not isinstance(spec, OAuth2ProxySpec):
+            raise OrchestratorValidationError(
+                'oauth2-proxy spec file must define service_type: oauth2-proxy'
+            )
+
+        if https_address is not None:
+            spec.https_address = https_address
+        if placement is not None:
+            spec.placement = PlacementSpec.from_string(placement)
+        if unmanaged:
+            spec.unmanaged = unmanaged
+        spec.preview_only = dry_run
 
         spec.validate()  # force any validation exceptions to be caught correctly
 

diff --git a/src/pybind/mgr/orchestrator/tests/test_orchestrator.py b/src/pybind/mgr/orchestrator/tests/test_orchestrator.py
index 88aee08a0103c2573fe423c36af309240f167c80..67fbb59f08dbfca09139ddf9f4a3f57464629893 100644 (file)
--- a/src/pybind/mgr/orchestrator/tests/test_orchestrator.py
+++ b/src/pybind/mgr/orchestrator/tests/test_orchestrator.py
@@ -351,3 +351,74 @@ class TestApplyNvmeof:
         mock_remote.assert_called_once_with('nvmeof', 'create_pool_if_not_exists')
         mock_apply_misc.assert_called_once()
         assert res.retval == 0
+
+
+@mock.patch("orchestrator.module.OrchestratorCli._apply_misc")
+class TestApplyOAuth2Proxy:
+
+    def setup_method(self):
+        self.m = OrchestratorCli('orchestrator', 0, 0)
+
+    def test_missing_spec_raises_clear_error(self, mock_apply_misc):
+        res = self.m._apply_oauth2_proxy()
+
+        assert res.retval != 0
+        assert (
+            'Missing required configuration for oauth2-proxy. Please provide a spec file '
+            'with required fields: provider_display_name, oidc_issuer_url, client_id, '
+            'client_secret.'
+        ) in res.stderr
+        mock_apply_misc.assert_not_called()
+
+    def test_missing_required_fields_raises_combined_error(self, mock_apply_misc):
+        res = self.m._apply_oauth2_proxy(inbuf=textwrap.dedent("""
+            service_type: oauth2-proxy
+            spec:
+              oidc_issuer_url: "https://idp.example.com"
+              client_id: "oauth-client"
+              client_secret: "oauth-secret"
+            """).strip())
+
+        assert res.retval != 0
+        assert (
+            'Missing required fields for oauth2-proxy: provider_display_name.'
+        ) in res.stderr
+        mock_apply_misc.assert_not_called()
+
+    def test_valid_spec_is_applied(self, mock_apply_misc):
+        mock_apply_misc.return_value = HandleCommandResult(retval=0, stdout="Success")
+
+        res = self.m._apply_oauth2_proxy(inbuf=textwrap.dedent("""
+            service_type: oauth2-proxy
+            spec:
+              provider_display_name: "My OIDC Provider"
+              oidc_issuer_url: "https://idp.example.com"
+              client_id: "oauth-client"
+              client_secret: "oauth-secret"
+            """).strip())
+
+        assert res.retval == 0
+        mock_apply_misc.assert_called_once()
+
+
+@mock.patch("orchestrator.module.OrchestratorCli._apply_misc")
+class TestApplyOAuth2ProxyYaml:
+
+    def setup_method(self):
+        self.m = OrchestratorCli('orchestrator', 0, 0)
+
+    def test_apply_yaml_missing_required_fields(self, mock_apply_misc):
+        res = self.m.apply_misc(
+            inbuf=textwrap.dedent("""
+                service_type: oauth2-proxy
+                spec:
+                  oidc_issuer_url: "https://idp.example.com"
+                """).strip()
+        )
+
+        assert res.retval != 0
+        assert (
+            'Missing required fields for oauth2-proxy: '
+            'provider_display_name, client_id, client_secret.'
+        ) in res.stderr
+        mock_apply_misc.assert_not_called()

diff --git a/src/python-common/ceph/deployment/service_spec.py b/src/python-common/ceph/deployment/service_spec.py
index 9ac9db4587129396c691e1f971f1b2ee5abe4b5a..67d7d9e9cadd5b2539bd2daa311fcb10a2ed48d1 100644 (file)
--- a/src/python-common/ceph/deployment/service_spec.py
+++ b/src/python-common/ceph/deployment/service_spec.py
@@ -2774,6 +2774,22 @@ class OAuth2ProxySpec(ServiceSpec):
 
     def validate(self) -> None:
         super(OAuth2ProxySpec, self).validate()
+        required_values = {
+            'provider_display_name': self.provider_display_name,
+            'oidc_issuer_url': self.oidc_issuer_url,
+            'client_id': self.client_id,
+            'client_secret': self.client_secret,
+        }
+        missing_required_fields = [
+            field for field, value in required_values.items()
+            if value is None or (isinstance(value, str) and not value.strip())
+        ]
+        if missing_required_fields:
+            raise SpecValidationError(
+                'Missing required fields for oauth2-proxy: '
+                + ', '.join(missing_required_fields)
+                + '.'
+            )
         self._validate_non_empty_string(self.provider_display_name, "provider_display_name")
         self._validate_non_empty_string(self.client_id, "client_id")
         self._validate_non_empty_string(self.client_secret, "client_secret")

diff --git a/src/python-common/ceph/tests/test_service_spec.py b/src/python-common/ceph/tests/test_service_spec.py
index 1a8c011e1e12fe34a5f023c0437f22589a1e13f4..92b86216686d49b6ae9c0bd906e98b21351a186c 100644 (file)
--- a/src/python-common/ceph/tests/test_service_spec.py
+++ b/src/python-common/ceph/tests/test_service_spec.py
@@ -15,6 +15,7 @@ from ceph.deployment.service_spec import (
     IngressSpec,
     IscsiServiceSpec,
     NFSServiceSpec,
+    OAuth2ProxySpec,
     PlacementSpec,
     PrometheusSpec,
     RGWSpec,
@@ -75,6 +76,33 @@ def test_apply_grafana(spec: GrafanaSpec, raise_exception: bool, msg: str):
     else:
         spec.validate()
 
+
+@pytest.mark.parametrize(
+    "spec_kwargs, expected_missing",
+    [
+        ({}, 'provider_display_name, oidc_issuer_url, client_id, client_secret'),
+        (
+            {
+                'provider_display_name': 'My OIDC Provider',
+                'oidc_issuer_url': 'https://idp.example.com',
+            },
+            'client_id, client_secret',
+        ),
+        (
+            {
+                'provider_display_name': 'My OIDC Provider',
+                'oidc_issuer_url': 'https://idp.example.com',
+                'client_secret': 'secret',
+            },
+            'client_id',
+        ),
+    ])
+def test_oauth2_proxy_missing_required_fields(spec_kwargs, expected_missing):
+    spec = OAuth2ProxySpec(**spec_kwargs)
+    expected = f'Missing required fields for oauth2-proxy: {expected_missing}.'
+    with pytest.raises(SpecValidationError, match=re.escape(expected)):
+        spec.validate()
+
 @pytest.mark.parametrize(
     "spec, raise_exception, msg",
     [