From: Shubha Jain Date: Thu, 30 Apr 2026 16:06:16 +0000 (+0530) Subject: cephadm: improve oauth2-proxy validation error messaging X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=8122f820d7458302de5be2fe685f5a5a3390285f;p=ceph.git 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 with missing required fields Fixes: https://tracker.ceph.com/issues/76372 Signed-off-by: Shubha Jain --- diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py index 56d94fe24ab3..74f00c18cadd 100644 --- 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 88aee08a0103..67fbb59f08db 100644 --- 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 9ac9db458712..67d7d9e9cadd 100644 --- 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 1a8c011e1e12..92b86216686d 100644 --- 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", [