handle non-boolean values in handlermanifest #3208
Conversation
nagworld9
requested review from
narrieta,
ZhidongPeng and
maddieford
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3208 +/- ##
===========================================
+ Coverage 71.97% 72.32% +0.34%
===========================================
Files 103 114 +11
Lines 15692 16978 +1286
Branches 2486 2452 -34
===========================================
+ Hits 11295 12280 +985
- Misses 3881 4132 +251
- Partials 516 566 +50 ☔ View full report in Codecov by Sentry. |
azurelinuxagent/ga/exthandlers.py
Outdated
| """ | ||
| for key in ['reportHeartbeat', 'continueOnUpdateFailure', 'supportsMultipleExtensions']: | ||
| value = self.data['handlerManifest'].get(key, False) | ||
| if not isinstance(value, bool): |
There was a problem hiding this comment.
I think this expresses the intention of the check a little better:
value = self.data['handlerManifest'].get(key)
if value is not None and not isinstance(value, bool):
azurelinuxagent/ga/exthandlers.py
Outdated
| value = self.data['handlerManifest'].get(key, False) | ||
| if not isinstance(value, bool): | ||
| msg =("In the handler manifest: '{0}' has a non-boolean value [{1}] for boolean type. Please change it to a boolean value. " | ||
| "For backward compatibility, 'true' (case insensitive) is accepted, and other values default to False.").format(key, value) |
There was a problem hiding this comment.
I would not advertise this, since we do not want to encourage its use
There was a problem hiding this comment.
you mean this part "For backward compatibility, 'true' (case insensitive) is accepted, and other values default to False.
There was a problem hiding this comment.
Yup, we don't want to encourage the use of strings as booleans. We need to supported for extensions that already do it, but we should not let other extensions know that we do.
azurelinuxagent/ga/exthandlers.py
Outdated
| add_event(name=ext_name, is_success=False, message=msg, op=WALAEventOperation.ExtensionHandlerManifest, log_event=False) | ||
|
|
||
| @staticmethod | ||
| def _parse_boolean_value(value, default_val=False): |
There was a problem hiding this comment.
let's not have a default for default_val; in this case, forcing the caller to explicitly specify the default value makes the code more readable
Description
We learned recently that some extensions are sending string value "true" or "false" for boolean flags in handlermanifest. This led to wrong matching in the agent. For ex: string "false" matched as true since we assume they are boolean value and condition not flag became as flag has value or not instead flag has true or false
Addressing this pr with reporting warning plus default to false if they send non-boolean value except "true". For backward compatibility, 'true' (case insensitive) is accepted.
Issue #
PR information
Quality of Code and Contribution Guidelines