Skip to content
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

newrelic_deployment: updated code to use v2 api #1501

Closed
5 changes: 5 additions & 0 deletions changelogs/fragments/1501_newrelic_v2_api_change.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
major_changes:
- newrelic_deployment - removed newrelic v1 API, added support for v2 API (https://github.com/ansible-collections/community.general/pull/1501).
breaking_changes:
- newrelic_deployment - I(revision) is required in v2 API.
116davinder marked this conversation as resolved.
Show resolved Hide resolved
- newrelic_deployment - check_mode is not supported.
133 changes: 75 additions & 58 deletions plugins/modules/monitoring/newrelic_deployment.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,94 +2,97 @@
# -*- coding: utf-8 -*-

# Copyright 2013 Matt Coddington <[email protected]>
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
# GNU General Public License v3.0+
# (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

from __future__ import absolute_import, division, print_function
__metaclass__ = type


DOCUMENTATION = '''
---
module: newrelic_deployment
author: "Matt Coddington (@mcodd)"
author:
- "Davinder Pal (@116davinder)"
- "Matt Coddington (@mcodd)"
short_description: Notify newrelic about app deployments
description:
- Notify newrelic about app deployments (see https://docs.newrelic.com/docs/apm/new-relic-apm/maintenance/deployment-notifications#api)
- Notify newrelic about app deployments
(U(https://docs.newrelic.com/docs/apm/new-relic-apm/maintenance/record-deployments)).
options:
token:
type: str
description:
- API token, to place in the x-api-key header.
required: true
app_name:
type: str
app_name:
description:
- (one of app_name or application_id are required) The value of app_name in the newrelic.yml file used by the application
- The value of C(app_name) in the C(newrelic.yml) file used by the application.
- Exactly one of I(app_name) or I(application_id) is required.
required: false
application_id:
type: str
application_id:
description:
- (one of app_name or application_id are required) The application id, found in the URL when viewing the application in RPM
- See U(https://rpm.newrelic.com/api/explore/applications/list).
116davinder marked this conversation as resolved.
Show resolved Hide resolved
- Exactly one of I(app_name) or I(application_id) is required.
required: false
changelog:
type: str
changelog:
description:
- A list of changes for this deployment
- A list of changes for this deployment.
required: false
description:
type: str
description:
description:
- Text annotation for the deployment - notes for you
- Text annotation for the deployment - notes for you.
required: false
revision:
type: str
revision:
description:
- A revision number (e.g., git commit SHA)
required: false
user:
- A revision number (for example, git commit SHA).
required: true
116davinder marked this conversation as resolved.
Show resolved Hide resolved
type: str
description:
- The name of the user/process that triggered this deployment
required: false
appname:
116davinder marked this conversation as resolved.
Show resolved Hide resolved
type: str
description:
- Name of the application
- (deprecated) Name of the application
116davinder marked this conversation as resolved.
Show resolved Hide resolved
required: false
environment:
type: str
description:
- The environment for this deployment
- (deprecated) The environment for this deployment.
required: false
validate_certs:
description:
- If C(no), SSL certificates will not be validated. This should only be used
- (deprecated) If C(no), SSL certificates will not be validated. This should only be used
on personally controlled sites using self-signed certificates.
116davinder marked this conversation as resolved.
Show resolved Hide resolved
required: false
default: 'yes'
type: bool

requirements: []
user:
description:
- The name of the user/process that triggered this deployment
required: false
type: str
'''

EXAMPLES = '''
- name: Notify newrelic about an app deployment
community.general.newrelic_deployment:
token: AAAAAA
app_name: myapp
user: ansible deployment
revision: '1.0'
token: XXXXXXXXX
app_name: ansible_app
user: ansible_deployment_user
revision: '1.X'
'''

from ansible.module_utils.basic import AnsibleModule
from ansible.module_utils.urls import fetch_url
from ansible.module_utils.six.moves.urllib.parse import urlencode
import json


# ===========================================
# Module execution.
#


def main():

module = AnsibleModule(
Expand All @@ -99,47 +102,61 @@ def main():
application_id=dict(required=False),
changelog=dict(required=False),
description=dict(required=False),
revision=dict(required=False),
revision=dict(required=True),
user=dict(required=False),
appname=dict(required=False),
116davinder marked this conversation as resolved.
Show resolved Hide resolved
environment=dict(required=False),
validate_certs=dict(default=True, type='bool'),
),
required_one_of=[['app_name', 'application_id']],
supports_check_mode=True
116davinder marked this conversation as resolved.
Show resolved Hide resolved
mutually_exclusive=[['app_name', 'application_id']],
)

# build list of params
params = {}
if module.params["app_name"] and module.params["application_id"]:
module.fail_json(msg="only one of 'app_name' or 'application_id' can be set")

if module.params["app_name"]:
params["app_name"] = module.params["app_name"]
elif module.params["application_id"]:
params["application_id"] = module.params["application_id"]
if module.params['app_name']:
data = 'filter[name]=' + str(module.params['app_name'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a proper encoding function for forms. Since you are using fetch_url, you can simply pass a dictionary and it will take care of encoding automatically (see https://github.com/ansible-collections/community.hrobot/blob/main/plugins/module_utils/failover.py#L64).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, I don't have access to NewRelic so I can't test it.
is this a mandatory change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix a proper method which will encode values correctly. Simple string concatenation is a disaster waiting to happen (in the worst case, a security vulnerability).

Copy link
Author

@116davinder 116davinder Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't modify something which I can't test, module created back in 2018 and was tested and still in use as per my knowledge.
If you say, without modify this it can't be merged than so let it be. I will modify in couple of years once I got access to NewRelic again.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate. In case you don't change this to use urlencode or simply provide a dict, I will not merge this. Sorry.

Copy link
Author

@116davinder 116davinder Dec 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this was so important change, Others/You as Reviewer should have pointed out in my first PR & second PR which was there from 2018 but None.
Now you want this to changed. Hell No!

I will not change any working piece of code which can break the new module implementation. As I don't have access to newrelic to test your enforced changes which means deadend.

Have Fun!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very trivial change to make, which does not affect the whole module. So I see absolutely no need to point out such problems as early as possible in the review process. And if you don't want to make any changes to the code's behavior, well, then I don't see why a review makes sense anyway.

I've now spend quite some time reviewing your code and changes in my free time. If you are not interested in getting this included, then I guess I totally wasted that time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about my time man? do you understand the level of patience from my side ( 2 years ) ?

it's not about trivial change, it's about basic testing, if I can't confirm as maintainer that it will work or not with urlencode/dict then I how i can even make a change, I don't understand that part.

If you have access to NewRelic please make the test with urlencode/dict and suggest the actual code change, I happy to update the code.

Just for your knowledge, half of module will stop working if this data is passed in wrong format as the search api will return none and anyone using app_name as parameter will never work until someone fix it in future.

let's assume your case where it cause some security Issue then it will on NewRelic Side why you/ansible need to worry. NewRelic should not prevent/fix this in there API as a validation and later Ansible Community should fix as part of bugfix process.
Now when I don't have testing resources/credentials, On what base you will merge it even if I change it?

newrelic_api = 'https://api.newrelic.com/v2/applications.json'
headers = {'x-api-key': module.params['token'],
'Content-Type': 'application/x-www-form-urlencoded'}
(resp, info) = fetch_url(module,
newrelic_api,
headers=headers,
data=data,
method='GET')
if info['status'] != 200:
module.fail_json(msg="unable to get application list from newrelic: %s" % info['msg'])
else:
body = json.loads(resp.read())
if body is None:
module.fail_json(msg='No Data for applications')
else:
app_id = body['applications'][0]['id']
if app_id is None:
module.fail_json(msg="App not found in\
116davinder marked this conversation as resolved.
Show resolved Hide resolved
NewRelic Registerd Applications List")
else:
module.fail_json(msg="you must set one of 'app_name' or 'application_id'")

for item in ["changelog", "description", "revision", "user", "appname", "environment"]:
if module.params[item]:
params[item] = module.params[item]

# If we're in check mode, just exit pretending like we succeeded
if module.check_mode:
module.exit_json(changed=True)
app_id = module.params['application_id']

# Send the data to NewRelic
url = "https://rpm.newrelic.com/deployments.xml"
data = urlencode(params)
headers = {
'x-api-key': module.params["token"],
url = 'https://api.newrelic.com/v2/applications/' + str(app_id) \
+ '/deployments.json'
data = {
'deployment': {
'revision': str(module.params['revision']),
'changelog': str(module.params['changelog']),
'description': str(module.params['description']),
'user': str(module.params['user']), }
}
response, info = fetch_url(module, url, data=data, headers=headers)
if info['status'] in (200, 201):

headers = {'x-api-key': module.params['token'],
'Content-Type': 'application/json'}
(response, info) = fetch_url(module, url,
data=module.jsonify(data),
headers=headers, method='POST')
if info['status'] == 201:
module.exit_json(changed=True)
else:
module.fail_json(msg="unable to update newrelic: %s" % info['msg'])
module.fail_json(msg='unable to update newrelic: %s'
% info['msg'])


if __name__ == '__main__':
Expand Down