Added support for policy data deletion #447
Conversation
✅ Deploy Preview for opal-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Hi @maurice-freitag ,
Thanks for that great, much needed contribution !
I do have couple of concerns - detailed in my comments.
LMK if you think you can fix those :)
| @@ -17,7 +17,9 @@ class DataSourceEntry(BaseModel): | |||
| """ | |||
|
|
|||
| # How to obtain the data | |||
| url: str = Field(..., description="Url source to query for data") | |||
| url: Optional[str] = Field( | |||
| "", description="Url source to query for data. Optional with save_method=DELETE" | |||
There was a problem hiding this comment.
or if static data is embedded in entry
| @@ -72,6 +74,25 @@ def publish_data_updates(self, update: DataUpdate): | |||
| """ | |||
| all_topic_combos = set() | |||
|
|
|||
| invalidEntries: List[DataSourceEntry] = [] | |||
There was a problem hiding this comment.
Hmm... validation is good although I'm not sure that's the right spot.
Clients can get entries also from external_source_url.
I think the best way would be to make those validations in DataSourceEntry's own initialization (and catching the potential exceptions in the proper places)
| # get path to store the URL data (default mode (None) is as "" - i.e. as all the data at root) | ||
| policy_store_path = "" if entry is None else entry.dst_path | ||
| # always default to PUT | ||
| method = "PUT" if entry is None else entry.save_method |
There was a problem hiding this comment.
You already have "PUT" as the default method in set_policy_data,
So it might be better to not pass it at all when it's none and let each policy store (there would be other stores in OPAL soon) have its own defaults.
| if len(entries) > 0: | ||
| logger.info("Fetching policy data", urls=repr(urls)) | ||
| urls = [(entry.url, entry.config, entry.data) | ||
| for entry in entries if entry.save_method.upper() != "DELETE"] |
There was a problem hiding this comment.
I don't really like giving "DELETE" a special care here.
url can be None and handler would just not fetch anything, So I feel like being explicit here overall makes the code less readable.
| reports.append(report) | ||
| # re-raise so the context manager will be aware of the failure | ||
| raise | ||
| if entry.save_method.upper() == "DELETE": |
There was a problem hiding this comment.
Again, I don't think I like the special handling of "DELETE".
I don't think it makes sense to split it to different methods here - PolicyStoreClient might need to have different implementations for different methods but that's not the concern of this part.
I know this method have become too big and messy over time, but we should fix it in some other way (or possibly in another PR, I thought about separating all the fetching & http status codes handling to another method called enrich_entries which fetches urls, and could do other stuff in the future).
Moreover, as a result of your separation of handle_delete_data from handle_new_data:
- path normalization isn't happening for "DELETE".
- reports are not collected for "DELETE".
- reports collected for other entries are not reported.
In short - I think you should basically let this code handle DELETE entries like any other entry (maybe should make DataFetcher.handle_url not log an error when both url & data are None, or if you have the time implement that enrich_entries which proxies handle_urls but not letting through entries of that kind)
|
Just figured out we have a similar but old and unmerged PR by @orishavit: #314 |
|
@maurice-freitag are you still on to finish this PR ? |
Yes, I'll probably get to it this week, however I suspect #314 might do the trick. I'll have a look at it and abandon this PR if appropriate. |
|
@orweis @roekatz #314 looks a lot more elegant to me. I'm unsure about the JSON Patch stuff, I would have imagined it to be a lot more complicated than it looks. I haven't tested it but @orishavit 's solution looks preferable to mine (sadly 😅) . |
Fixes Issue
Closes #430, partially closes #46
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers