fix federation registration#21
Conversation
There was a problem hiding this comment.
Pull request overview
Updates federation platform registration in VolttronAuthService to align with newer volttron-core federation registration behavior.
Changes:
- Rename
add_federation_platformtoregister_remote_platform. - Persist remote platform public credentials using
store_credentials(..., overwrite=True)and switch from boolean return codes to raising exceptions on failure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| platform_identity = f"{platform_id}" | ||
| public_creds = PublicCredentials(identity=f"{platform_identity}", publickey=credentials) | ||
|
|
||
| self._credentials_store.store_credentials(credentials=public_creds) | ||
|
|
||
|
|
||
| _log.info(f"Federation platform registered: {platform_id}") | ||
| return True | ||
| self._credentials_store.store_credentials(credentials=public_creds, overwrite=True) |
There was a problem hiding this comment.
Using platform_id directly as the credentials identity combined with overwrite=True can overwrite existing/local agent credentials if a remote platform registers with a reserved identity (e.g., AUTH, CONTROL, etc.). Consider namespacing federation identities (e.g., federation.<platform_id>) and/or explicitly rejecting reserved/known identities before storing, and only allowing overwrite when you have verified it's the same remote platform updating its own key.
|
|
||
| _log.info(f"Federation platform registered: {platform_id}") | ||
| return True | ||
| self._credentials_store.store_credentials(credentials=public_creds, overwrite=True) |
There was a problem hiding this comment.
store_credentials(..., overwrite=True) introduces a hard dependency on a newer volttron-core API. With the current pyproject.toml constraint (volttron-core >=2.0.0rc13), environments may install a core version that doesn't support overwrite, causing a TypeError at runtime. Either bump the minimum volttron-core version in this PR or add a compatibility fallback (call without overwrite when the kwarg isn't supported).
| self._credentials_store.store_credentials(credentials=public_creds, overwrite=True) | |
| try: | |
| # Prefer newer API that supports overwriting existing credentials | |
| self._credentials_store.store_credentials(credentials=public_creds, overwrite=True) | |
| except TypeError: | |
| # Fallback for older volttron-core versions that do not accept 'overwrite' | |
| self._credentials_store.store_credentials(credentials=public_creds) |
| @@ -193,15 +192,10 @@ def add_federation_platform(self, platform_id: str, credentials: Any) -> bool: | |||
|
|
|||
| platform_identity = f"{platform_id}" | |||
| public_creds = PublicCredentials(identity=f"{platform_identity}", publickey=credentials) | |||
|
|
|||
| self._credentials_store.store_credentials(credentials=public_creds) | |||
|
|
|||
|
|
|||
| _log.info(f"Federation platform registered: {platform_id}") | |||
| return True | |||
| self._credentials_store.store_credentials(credentials=public_creds, overwrite=True) | |||
| except Exception as e: | |||
| _log.error(f"Error registering federation platform {platform_id}: {e}") | |||
| return False | |||
| raise | |||
There was a problem hiding this comment.
This method previously returned bool to indicate success/failure, but now has no return type and always returns None on success while raising on error. If any callers (or an interface/abstract base) still expect a boolean result, this is a breaking API change—consider restoring -> bool with an explicit return value, or annotate -> None and ensure all callers are updated accordingly.
| except Exception as e: | ||
| _log.error(f"Error registering federation platform {platform_id}: {e}") |
There was a problem hiding this comment.
Catching Exception and then logging with _log.error(...) loses the traceback context in logs; since the exception is re-raised, consider using _log.exception(...) (or exc_info=True) and avoid double-logging higher up the stack.
| except Exception as e: | |
| _log.error(f"Error registering federation platform {platform_id}: {e}") | |
| except Exception: | |
| _log.exception("Error registering federation platform %s", platform_id) |
Depends on volttron-core PR 300
Once core is updated and pushed, need to update pyproject.toml dependency