Merge pull request ceph#65567 from rkachach/fix_issue_certs_migration

mgr/cephadm: cleaning up old migration logic and improving upgrade handling with certmgr

Reviewed-by: Adam King <[email protected]>

Author: adk3798

src/pybind/mgr/cephadm/inventory.py

@@ -27,7 +27,7 @@
 from mgr_util import parse_combined_pem_file
 
 from .utils import resolve_ip, SpecialHostLabels
-from .migrations import queue_migrate_nfs_spec, queue_migrate_rgw_spec, queue_migrate_rgw_ssl_spec
+from .migrations import queue_migrate_nfs_spec, queue_migrate_rgw_spec
 
 if TYPE_CHECKING:
     from .module import CephadmOrchestrator
@@ -309,12 +309,6 @@ def load(self):
                 ):
                     queue_migrate_rgw_spec(self.mgr, j)
 
-                if (
-                        (self.mgr.migration_current or 0) < 8
-                        and j['spec'].get('service_type') == 'rgw'
-                ):
-                    queue_migrate_rgw_ssl_spec(self.mgr, j)
-
                 spec = ServiceSpec.from_json(j['spec'])
                 created = str_to_datetime(cast(str, j['created']))
                 self._specs[service_name] = spec

src/pybind/mgr/cephadm/migrations.py

@@ -7,16 +7,15 @@
 from cephadm.schedule import HostAssignment
 from cephadm.utils import SpecialHostLabels
 import rados
-from mgr_util import parse_combined_pem_file, get_cert_issuer_info
-from cephadm.tlsobject_types import CertKeyPair
+from mgr_util import get_cert_issuer_info
 
 from mgr_module import NFS_POOL_NAME
 from orchestrator import OrchestratorError, DaemonDescription
 
 if TYPE_CHECKING:
     from .module import CephadmOrchestrator
 
-LAST_MIGRATION = 9
+LAST_MIGRATION = 8
 
 logger = logging.getLogger(__name__)
 
@@ -43,9 +42,6 @@ def __init__(self, mgr: "CephadmOrchestrator"):
         r = mgr.get_store('rgw_migration_queue')
         self.rgw_migration_queue = json.loads(r) if r else []
 
-        r = mgr.get_store('rgw_ssl_migration_queue')
-        self.rgw_ssl_migration_queue = json.loads(r) if r else []
-
         # for some migrations, we don't need to do anything except for
         # incrementing migration_current.
         # let's try to shortcut things here.
@@ -126,11 +122,6 @@ def migrate(self, startup: bool = False) -> None:
             if self.migrate_7_8():
                 self.set(8)
 
-        if self.mgr.migration_current == 8:
-            logger.info('Running migration 8 -> 9')
-            if self.migrate_8_9():
-                self.set(9)
-
     def migrate_0_1(self) -> bool:
         """
         Migration 0 -> 1
@@ -455,16 +446,12 @@ def migrate_6_7(self) -> bool:
             grafana_cert = self.mgr.get_store(grafana_cert_path)
             grafana_key = self.mgr.get_store(grafana_key_path)
             if grafana_cert:
-                (org, cn) = get_cert_issuer_info(grafana_cert)
-                if org == 'Ceph':
-                    logger.info(f'Migrating {grafana_daemon.name()}/{hostname} cert/key to cert store (as cephadm-signed certs)')
-                    self.mgr.cert_mgr.register_self_signed_cert_key_pair('grafana')
-                    self.mgr.cert_mgr.save_self_signed_cert_key_pair('grafana', CertKeyPair(grafana_cert, grafana_key), host=hostname)
-                else:
+                org, _ = get_cert_issuer_info(grafana_cert)
+                if org != 'Ceph':
                     logger.info(f'Migrating {grafana_daemon.name()}/{hostname} cert/key to cert store (as custom-certs)')
                     grafana_cephadm_signed_certs = False
-                    self.mgr.cert_mgr.save_cert('grafana_ssl_cert', grafana_cert, host=hostname)
-                    self.mgr.cert_mgr.save_key('grafana_ssl_key', grafana_key, host=hostname)
+                    self.mgr.cert_mgr.save_cert('grafana_ssl_cert', grafana_cert, host=hostname, user_made=True, editable=True)
+                    self.mgr.cert_mgr.save_key('grafana_ssl_key', grafana_key, host=hostname, user_made=True, editable=True)
 
         if not grafana_cephadm_signed_certs:
             # Update the spec to specify the right certificate source
@@ -478,37 +465,6 @@ def migrate_6_7(self) -> bool:
         return True
 
     def migrate_7_8(self) -> bool:
-        logger.info(f'Starting rgw SSL/TLS migration (queue length is {len(self.rgw_ssl_migration_queue)})')
-        for s in self.rgw_ssl_migration_queue:
-
-            svc_spec = s['spec']  # this is the RGWspec
-
-            if 'spec' not in svc_spec:
-                logger.info(f"No SSL/TLS fields migration is needed for rgw spec: {svc_spec}")
-                continue
-
-            cert_field = svc_spec['spec'].get('rgw_frontend_ssl_certificate')
-            if not cert_field:
-                logger.info(f"No SSL/TLS fields migration is needed for rgw spec: {svc_spec}")
-                continue
-
-            cert_str = '\n'.join(cert_field) if isinstance(cert_field, list) else cert_field
-            ssl_cert, ssl_key = parse_combined_pem_file(cert_str)
-            new_spec = svc_spec.copy()
-            new_spec['spec'].update({
-                'rgw_frontend_ssl_certificate': None,
-                'certificate_source': CertificateSource.INLINE.value,
-                'ssl_cert': ssl_cert,
-                'ssl_key': ssl_key,
-            })
-
-            logger.info(f"Migrating {svc_spec} to new RGW SSL/TLS format {new_spec}")
-            self.mgr.spec_store.save(RGWSpec.from_json(new_spec))
-
-        self.rgw_ssl_migration_queue = []
-        return True
-
-    def migrate_8_9(self) -> bool:
         """
         Replace Promtail with Alloy.
 
@@ -588,15 +544,6 @@ def queue_migrate_rgw_spec(mgr: "CephadmOrchestrator", spec_dict: Dict[Any, Any]
     logger.info(f'Queued rgw.{service_id} for migration')
 
 
-def queue_migrate_rgw_ssl_spec(mgr: "CephadmOrchestrator", spec_dict: Dict[Any, Any]) -> None:
-    service_id = spec_dict['spec']['service_id']
-    queued = mgr.get_store('rgw_ssl_migration_queue') or '[]'
-    ls = json.loads(queued)
-    ls.append(spec_dict)
-    mgr.set_store('rgw_ssl_migration_queue', json.dumps(ls))
-    logger.info(f'Queued rgw.{service_id} for TLS migration')
-
-
 def queue_migrate_nfs_spec(mgr: "CephadmOrchestrator", spec_dict: Dict[Any, Any]) -> None:
     """
     After 16.2.5 we dropped the NFSServiceSpec pool and namespace properties.

src/pybind/mgr/cephadm/tests/test_migration.py

@@ -8,7 +8,8 @@
     RGWSpec,
     IngressSpec,
     IscsiServiceSpec,
-    GrafanaSpec
+    GrafanaSpec,
+    CertificateSource
 )
 from ceph.utils import datetime_to_str, datetime_now
 from cephadm import CephadmOrchestrator
@@ -402,25 +403,6 @@ def test_migrate_rgw_spec(cephadm_module: CephadmOrchestrator, rgw_spec_store_en
             assert 'rgw.foo' not in cephadm_module.spec_store.all_specs
 
 
-@mock.patch('cephadm.migrations.get_cert_issuer_info')
-def test_migrate_grafana_cephadm_signed(mock_get_cert_issuer_info, cephadm_module: CephadmOrchestrator):
-    mock_get_cert_issuer_info.return_value = ('Ceph', 'MockCephCN')
-
-    cephadm_module.set_store('host1/grafana_crt', 'grafana_cert1')
-    cephadm_module.set_store('host1/grafana_key', 'grafana_key1')
-    cephadm_module.set_store('host2/grafana_crt', 'grafana_cert2')
-    cephadm_module.set_store('host2/grafana_key', 'grafana_key2')
-    cephadm_module.cache.daemons = {'host1': {'grafana.host1': DaemonDescription('grafana', 'host1', 'host1')},
-                                    'host2': {'grafana.host2': DaemonDescription('grafana', 'host2', 'host2')}}
-
-    cephadm_module.migration.migrate_6_7()
-
-    assert cephadm_module.cert_mgr.get_cert('cephadm-signed_grafana_cert', host='host1')
-    assert cephadm_module.cert_mgr.get_cert('cephadm-signed_grafana_cert', host='host2')
-    assert cephadm_module.cert_mgr.get_key('cephadm-signed_grafana_key', host='host1')
-    assert cephadm_module.cert_mgr.get_key('cephadm-signed_grafana_key', host='host2')
-
-
 @mock.patch('cephadm.migrations.get_cert_issuer_info')
 def test_migrate_grafana_custom_certs(mock_get_cert_issuer_info, cephadm_module: CephadmOrchestrator):
     from datetime import datetime, timezone
@@ -445,6 +427,7 @@ def test_migrate_grafana_custom_certs(mock_get_cert_issuer_info, cephadm_module:
     assert cephadm_module.cert_mgr.get_cert('grafana_ssl_cert', host='host2')
     assert cephadm_module.cert_mgr.get_key('grafana_ssl_key', host='host1')
     assert cephadm_module.cert_mgr.get_key('grafana_ssl_key', host='host2')
+    assert cephadm_module.spec_store._specs['grafana'].certificate_source == CertificateSource.REFERENCE.value
 
 
 def test_migrate_cert_store(cephadm_module: CephadmOrchestrator):

src/pybind/mgr/cephadm/utils.py

@@ -27,17 +27,18 @@ class CephadmNoImage(Enum):
 GATEWAY_TYPES = ['iscsi', 'nfs', 'nvmeof', 'smb']
 MONITORING_STACK_TYPES = ['node-exporter', 'prometheus',
                           'alertmanager', 'grafana', 'loki', 'promtail', 'alloy']
+MGMT_GATEWAY_STACK_TYPES = ['mgmt-gateway', 'oauth2-proxy']
 RESCHEDULE_FROM_OFFLINE_HOSTS_TYPES = ['haproxy', 'nfs']
 
-CEPH_UPGRADE_ORDER = CEPH_TYPES + GATEWAY_TYPES + MONITORING_STACK_TYPES
+CEPH_UPGRADE_ORDER = CEPH_TYPES + GATEWAY_TYPES + MONITORING_STACK_TYPES + MGMT_GATEWAY_STACK_TYPES
 
 # these daemon types use the ceph container image
 CEPH_IMAGE_TYPES = CEPH_TYPES + ['iscsi', 'nfs', 'node-proxy']
 
 # these daemons do not use the ceph image. There are other daemons
 # that also don't use the ceph image, but we only care about those
 # that are part of the upgrade order here
-NON_CEPH_IMAGE_TYPES = MONITORING_STACK_TYPES + ['nvmeof', 'smb']
+NON_CEPH_IMAGE_TYPES = MONITORING_STACK_TYPES + ['nvmeof', 'smb'] + MGMT_GATEWAY_STACK_TYPES
 
 # Used for _run_cephadm used for check-host etc that don't require an --image parameter
 cephadmNoImage = CephadmNoImage.token

src/python-common/ceph/deployment/service_spec.py

@@ -1520,17 +1520,22 @@ def get_port(self) -> List[int]:
         return ports
 
     def validate(self) -> None:
-        super(RGWSpec, self).validate()
 
         if self.ssl:
             if not self.ssl_cert and self.rgw_frontend_ssl_certificate:
                 combined_cert = self.rgw_frontend_ssl_certificate
                 if isinstance(combined_cert, list):
                     combined_cert = '\n'.join(combined_cert)
+                self.certificate_source = CertificateSource.INLINE.value
                 self.ssl_cert, self.ssl_key = parse_combined_pem_file(combined_cert)
+                self.rgw_frontend_ssl_certificate = None
                 if not (self.ssl_cert and self.ssl_key):
                     raise SpecValidationError("Failed to parse rgw_frontend_ssl_certificate field.")
 
+        # This validation is done after adjusting the SSL field so when
+        # RGW Spec is updated with the right fields before validation
+        super(RGWSpec, self).validate()
+
         if self.rgw_realm and not self.rgw_zone:
             raise SpecValidationError(
                     'Cannot add RGW: Realm specified but no zone specified')