-
Notifications
You must be signed in to change notification settings - Fork 11
BREAKING CHANGE: remove btool and replace it with splunk.rest #441
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
BREAKING CHANGE: remove btool and replace it with splunk.rest #441
Conversation
| # | ||
| """This module provide settings that can be used to disable/switch features.""" | ||
|
|
||
| use_btool = False |
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 think we would require this setting to be passed from add-on developers, or provide a way to them to override this setting if they want to use btool.
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.
The idea here was to make some kind of backdoor. It's not a feature that user can choose which apprach use, he will be forced to use api but in case of any issue on production env, admins will be able to temporary switch this flag.
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.
Can we add a test where we switch this flag to True and then ensure the test suite passes, thus ensuring the same behaviour when some developers make this change in prod or pre-prod env?
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've added some test for that e.g. tests.unit.test_splunkenv.test_splunkd_uri_backdoor
| server_response, server_content = simpleRequest( | ||
| url, | ||
| sessionKey=session_key, | ||
| getargs={"output_mode": "json"}, | ||
| ) | ||
|
|
||
| result = json.loads(server_content.decode()) |
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 will raise an unexpected exception if simpleRequest is not defined (in case we operate outside of Splunk), do you expect splunkenv to be used outside of Splunk's environment?
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.
hmm, I thought as this is splunkenv module maybe we should support it only in the splunk env? I've added some exception to notify user about this fact. I don't think this is actually used outside but if there will be some requests in future to extend it's functionality we can implement some request-based solution. What do you think?
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.
let's update documentation for this module as well and we can live with it
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.
There is already this information: "Additionally, the splunkenv module can only be used in environments where Splunk is installed, as it relies on Splunk-specific methods for making internal calls." Do you want me to extend it?
solnlib/splunkenv.py
Outdated
| conf_name: str, | ||
| app_name: str, | ||
| session_key: Optional[str] = None, | ||
| user: Optional[str] = "nobody", |
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.
user: Optional[str] = None, OR user: str = "nobody",
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
solnlib/splunkenv.py
Outdated
| result = op.join(_splunk_home(), ETC_LEAF) | ||
|
|
||
| return os.path.normpath(result) | ||
| use_btool = bool(use_btool) |
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.
why?
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.
some historical change, removed
| { | ||
| "content": { | ||
| "disabled": 0, | ||
| "enableSSL": 0 |
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.
Shouldn't these two values be 1 as per the setting in tests/unit/data/mock_splunk/etc/apps/splunk_httpinput/local/inputs.conf?
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 is just to test backdoor solution. We are no longer using btool so "valid" config is in the inputs.json, but I set different value here just to verify if use_btool is taking value from .conf.
|
|
||
| [sslConfig] | ||
| enableSplunkdSSL = true | ||
| enableSplunkdSSL = false |
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.
Any reason to set this to false?
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 is just to test backdoor solution. We are no longer using btool so "valid" config is in the server-sslconfig.json, but I set different value here just to verify if use_btool is taking value from .conf.
| "maxSockets": "0", | ||
| "maxThreads": "0", | ||
| "max_view_cache_size": "1000", | ||
| "mgmtHostPort": "127.0.0.1:8089", |
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.
Shouldn't this value be 127.0.0.2:8079 as per tests/unit/data/mock_splunk/etc/system/default/web.conf?
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.
Same situation as above. I've changed value here to verify "use_btool" flag. Basically .conf files from unit/data/mock_splunkn/etc are no longer in use
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.
Can't make comment on the line that was not changed...
Please take a look at on_shared_storage, it does not seem to be used in the project and # See validateSearchHeadPooling() in src/libbundle/ConfSettings.cpp is not correct anymore.
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.
Yes, this should be removed as we are using make_splunkhome_path from splunk
| if not session_key: | ||
| session_key = getSessionKey() |
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 then need to provide session_key at all if that is calculated for us in getSessionKey method from Splunk directly? I remember there was a case where it did not work quite well, I think it's worth describing it here.
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.
Basically the problem is that splunk's getSessionKey or main module have access to session key if it's used in the REST context but if used e.g. in modinput script, those function won't provide session key. I can add some comment line here
solnlib/splunkenv.py
Outdated
| "get_conf_key_value", | ||
| "get_conf_stanza", | ||
| "get_conf_stanzas", | ||
| "_get_conf_stanzas_from_splunk_api", |
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.
Why expose it here?
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.
right, sorry. It shoudn't be expose here. I will make changes
| if not session_key and hasattr(__main__, "___sessionKey"): | ||
| session_key = getattr(__main__, "___sessionKey") |
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.
What is __main__ in this case?
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.
it depends on which module, that is using this splunkenv functions, was started. E.g. for the rest_handlers modules MConfigHandler is setting this ___sessionKey attribute so it's available here. If mod input started it, ___sessionKey will be missing and that's why user will have to provide it
solnlib/splunkenv.py
Outdated
| session_key = getattr(__main__, "___sessionKey") | ||
|
|
||
| if not session_key: | ||
| raise SessionKeyNotFound() |
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.
Any recommendations in this case?
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.
right, we can add information "Session key is missing. If you are using 'splunkenv' module in your TA, please ensure you are providing session_key to it's functions. For more information please see: https://splunk.github.io/addonfactory-solutions-library-python/release_7_0_0/"
artemrys
left a comment
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.
LGTM overall, I did not review every single line but I am fine if we follow up with couple of other PR after it's merged to develop.
|
🎉 This PR is included in version 6.3.0-beta.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 7.0.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Issue number: ADDON-79884
PR Type
What kind of change does this PR introduce?
Summary
Changes
solnlibwill no longer use Splunk'sbtool.btoolwas replaced with configuration endpoint from Splunksplunk.restmodule. Sincesession_keyis required for an authorization,solnlibno longer supportsget_session_key()function. Also functionmake_splunkhome_pathwill now use native Splunk's implementation fromsplunk.clilib.bundle_pathsUser experience
It's a breaking change for the user as functions related to getting stanza from config files will now required
app_nameandsession_keyto work properly.If users uses
get_session_key()fromsolnlibthey will need to replace it with their own implementation.Checklist
introduced new method
_get_conf_stanzas_from_splunk_apiFollowing functions will now require app_name and/or session_key as a arguments to work:
make_splunkhome_path() now uses
splunk.clilib.bundle_paths.make_splunkhome_pathReview
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear