Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block extensions disallowed by policy #3259
Block extensions disallowed by policy #3259
Changes from 33 commits
c2cc2c6
151081d
edec2af
a37508f
b0da554
a4f5cab
699b9ba
86de0c5
c3e9b89
65d7034
95f247a
3b18519
63da127
471cd59
8ea989b
83f6ff0
b037e41
ba3869c
dfcc158
fe07ffa
a31bdcf
5198cf8
4a0a4ef
daa8017
bacc425
3319916
8c31798
32ef5c1
f0895b7
0c9f1c7
fc2de23
c3aac0f
be42640
7069a0b
8231234
ce5cf20
e222aa7
5abbfc6
6abd3f8
9fa38ea
7ebbd2b
1e6a82d
8cd64d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should probably use get_agent_conf_file_path() instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to change this to use __report_policy_error() or __handle_and_report_ext_handler_error() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to handle extensions_disabled scenario in this PR, I think we should remove the logic to create handler status and extension status here and call __report_policy_error instead.
Although, if we do that, I think we should rename __report_policy_error to something like '__report_ext_disallowed_error' and update the comments in that function to indicate it can be called for either extension disallowed by policy OR extension processing disabled via config.
We should also rename the policy_error_map to be generic to extensions disabled too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this to call __handle_ext_disallowed_error( ), and set a terminal error code from _EXT_DISALLOWED_ERROR_MAP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this code be set to something terminal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're handling extensions_disabled in this PR, then we should use the policy_error_map to determine error code, but rename the map to be generic to any disallowed ext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, extensions disabled and extensions not allowed by policy should use the same method to report status, so report_policy_error will need a little refactoring to allow for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does an extension get removed from disallowed_ext_handlers? The ExtHandlersHandler is instantiated only once on agent init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appending to the list in a method named "report_policy_error" does not feel right (reporting should not change the state of the object). rename to handle_policy_error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this to "handle_ext_disallowed_error"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment pointing out that we are intentionally reporting the error at the handler and status level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like "We report the same error at both the handler status and extension status level." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, what i was trying to point is that reporting the error both at the handler and status level is not needed (or should not be needed). e.g. install errors are reported at the handler level, while single-config errors are reported at the status level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment still needs addressing. we need to explain why
# Set handler status for all extensions (with and without settings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments, but might be missing this piece of the explanation - is this what you were thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the above comment