Added support for unscoped access, implemented caching mechanism to reduce API calls to Nova #1276
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
99cc751 to
9299e18
Compare
9df513e to
68b2da9
Compare
No need to specify project per instace if you want them all. [openstack] collect service catalog for unscoped instances
[openstack] fix typo
[openstack][test] adding some unscoped authentication test cases [openstack] tag is project_name - renaming
464ffd0 to
b7f9d06
Compare
* add metric about backoff * Small fixes * Remove yolo mode * Cleaner if statement * Fix test on argument position * fix'
…ghtly [openstack] watch out for references - copy list
07d482e to
5406ba7
Compare
* Creates a caching system and calls the /servers/details endpoint directly
…1512) * Only set the changes-since once on run * Remove overall project_name tag, let individual servers do it * Add lots of debug logs * Gaurantees non active servers are removed from cache * Resilient double checking of active server * Final cleanup of code/comments
4 tasks
ofek
reviewed
May 9, 2018
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| @@ -16,7 +16,7 @@ is available in our contribution guidelines. | |||
|
|
|||
| ### Versioning | |||
|
|
|||
| - [ ] Bumped the check version in `manifest.json` | |||
| - [ ] Bumped the check version in `manifest.json` (or update manifest version to 1.0.0 and drop check version) | |||
There was a problem hiding this comment.
Let's not say this as we can only do that for ported checks.
There was a problem hiding this comment.
Agreed, I think this got pulled in during some git magic to rebase things. I'll get rid of it. 🙇
ofek
requested changes
May 9, 2018
| auth_url = urljoin(keystone_server_url, "{0}/auth/tokens".format(DEFAULT_KEYSTONE_API_VERSION)) | ||
| headers = {'Content-Type': 'application/json'} | ||
|
|
||
| resp = requests.post(auth_url, headers=headers, data=json.dumps(payload), verify=ssl_verify, timeout=DEFAULT_API_REQUEST_TIMEOUT, proxies=proxy) |
There was a problem hiding this comment.
It looks like you can only do that if you are using requests 2.14+ http://docs.python-requests.org/en/master/community/updates/#id20 Currently in testing on an Agent with a lower version doing this results in a 400 response code.
| or not user.get('name')\ | ||
| or not user.get('password')\ | ||
| or not user.get("domain")\ | ||
| or not user.get("domain").get("id"): |
There was a problem hiding this comment.
Could we make this
if not (
user and
user.get('name') and
user.get('password') and
user.get("domain") and
user.get("domain").get("id")
):
| raise IncompleteAuthScope() | ||
|
|
||
| if auth_scope['project'].get('name'): | ||
| # We need to add a domain scope to avoid name clashes. Search for one. If not raise IncompleteConfig |
There was a problem hiding this comment.
Should this say we raise IncompleteAuthScope?
| auth_scope['project']['domain']['name'] = auth_scope['project']['domain'].pop('id') | ||
| else: | ||
| elif auth_scope: | ||
| auth_scope['project']['name'] = auth_scope['project'].pop('id') |
There was a problem hiding this comment.
Let's make this
if auth_scope:
if 'domain' in auth_scope['project']:
auth_scope['project']['domain']['name'] = auth_scope['project']['domain'].pop('id')
else:
auth_scope['project']['name'] = auth_scope['project'].pop('id')
| if not keystone_server_url: | ||
| raise IncompleteConfig() | ||
|
|
||
| ssl_verify = init_config.get("ssl_verify", False) |
There was a problem hiding this comment.
Shouldn't this default to True?
| project_auth_token = token_resp.headers.get('X-Subject-Token') | ||
| except (requests.exceptions.HTTPError, requests.exceptions.Timeout, requests.exceptions.ConnectionError) as e: | ||
| # TODO: Raise something | ||
| pass |
| headers = {'Content-Type': 'application/json'} | ||
| auth_url = urljoin(keystone_server_url, "{0}/auth/tokens".format(DEFAULT_KEYSTONE_API_VERSION)) | ||
|
|
||
| resp = requests.post(auth_url, headers=headers, data=json.dumps(payload), verify=ssl_verify, timeout=DEFAULT_API_REQUEST_TIMEOUT, proxies=proxy) |
There was a problem hiding this comment.
Same comment as above about getting a 400 from the request
| try: | ||
| resp = requests.get(url, headers=headers, verify=self._ssl_verify, params=params, | ||
| timeout=DEFAULT_API_REQUEST_TIMEOUT, proxies=self.proxy_config) | ||
| resp.raise_for_status() | ||
| except requests.exceptions.HTTPError: | ||
| except requests.HTTPError as e: |
There was a problem hiding this comment.
requests.exceptions.HTTPError was correct
ofek
previously approved these changes
May 11, 2018
|
@truthbk As this was originally the work of your PR, do you have any concerns with this being merged as-is? Tests have been added with regards to the caching feature and I think all is well here from my side. |
ofek
approved these changes
May 23, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
This PR aims to enable unscoped access to multiple projects a user may authenticate to via a single configuration instance. As such a user shall only provide the set of credentials which we will then use to collect all authorized projects, create scopes for each individual project, and then iterate the projects collecting metrics and tagging them accordingly.
This PR also implements a caching mechanism to hold information about server details between check runs, things like project_name, server_id, etc. This was we can reduce the size of the API call on each check run and uses the
changes-sincefilter on the/servers/detailsendpoint to only pull in newly ACTIVE servers into the cache. We remove servers when we hit an exception trying to access thediagnosticsendpoint on that server because that is an indication that its DOWN.We also cache the project name to remove a redundant API call of mapping tenant id to project name multiple times in the check.
Motivation
Added flexibility, ease for maintenance, shorten collection time and reduce stress on the openstack suite of APIs.
Testing Guidelines
An overview on testing
is available in our contribution guidelines.
Versioning
manifest.jsondatadog_checks/{integration}/__init__.pyCHANGELOG.md. Please useUnreleasedas the date in the titlefor the new section.
Additional Notes
Refactoring of the project scopes is in order (and in progress)