visit2rahul commented on PR #4422: URL: https://github.com/apache/polaris/pull/4422#issuecomment-4540250200
Hi @dimas-b, hope all is well!!! I've pushed the latest comment with revised implementation. Here is the expected catalog user experience with this change: **Create:** If a user provides a base location without an allowed list, our behavior remains consistent - we populate the list for them (i thought about this and I have intentionally landed this way to keep parity with current behavior while creation of catalog). However, explicit allowed lists provided by user (if any) are now stored exactly as provided without silent additions. Furthermore, if a base location falls outside the specified allowed list, the system will now issue a clear error rejection rather than quietly expanding the perimeter. **Update:** We now enforce the "base-must-be-inside-allowed" check on every update without exception. Users updating the storage configuration must explicitly include the allowed list, as silent inheritance from previous states is no longer supported. For updates limited to the base location, we validate against the existing catalog's allowed list. **Backward compatibility / legacy support:** These changes are not retroactive. So, existing i.e. prior / old catalogs with empty allowed lists will continue to function for property-only updates as they did previously. This change does break 1 pattern though that I want you to review: The primary disruption with this implementation is that partial storage-config updates (e.g rotating an IAM role) now require re-sending the complete allowed list. This has turned out to be a significant refactor of many unit tests which I've tried to do in my commiet (the ones previously relied on silent adding); and the CI is running, will see how it goes. I see its quite a significant change though. What do u think about this much of a bigger design change? If we go with other option --> where we keep the silent add while creation, and just make validation stricter at the time of update - the blast radius (for existing unit and integration tests failures) would be lesser - but i personally liked current implementation and above user experience which is more stricter, governed and well managed in general. Let me know your thoughts, on how this approach looks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
