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

[WIP] add how to create HttpApi plugins for network modules #54340

Open
wants to merge 7 commits into
base: devel
from

Conversation

@samccann
Copy link
Contributor

samccann commented Mar 25, 2019

SUMMARY

Create a new developer page for how to create HttpAPI plugins for network modules

Partially addresses #53681

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

docs.ansible.com

ADDITIONAL INFORMATION

@samccann samccann added this to Needs Triage - todo backlog - Docsite work -anything can go here in Network Documentation via automation Mar 25, 2019

@samccann samccann moved this from Needs Triage - todo backlog - Docsite work -anything can go here to Approved Priority work -Q1 and /or 2.8 in Network Documentation Mar 25, 2019

@samccann samccann moved this from Approved Priority work -Q1 and /or 2.8 to In progress in Network Documentation Mar 25, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 25, 2019

Network connection plugins
==========================

Each network connection plugin has a set of implementation plugins which provide a specific implementation of that connection for a particular set of devices.

This comment has been minimized.

Copy link
@kbreit

kbreit Mar 26, 2019

Contributor

I don't like 'implementation plugins'. I appreciate that's the phrase, but it's confusing since 'implementation' is used later. Maybe just say "...has a set of plugins which provide a specification of the connection for a particular set of devices."

Making requests
^^^^^^^^^^^^^^^

The ``httpapi`` connection plugin has a ``send()`` method, but an HttpApi plugin needs a ``send_request(self, data, **message_kwargs)`` method as a higher-level wrapper to ``send()``. This method should prepare requests by adding fixed values like common headers or URL root paths, and may do more complex work such as turning data into formatted payloads, or determining which path or method to request. It may then also unpack responses to be more easily consumed by the caller.

This comment has been minimized.

Copy link
@kbreit

kbreit Mar 26, 2019

Contributor

What's the difference between httpapi and HttpApi?

The second sentence could be cleaned up. Not sure if it's a run-on per ce, but it is too wordy.

Also, this hits one of my pet pieves - using the word 'consume'.

This comment has been minimized.

Copy link
@dagwieers

dagwieers Mar 29, 2019

Member

Yeah, don't capitalize, there's no value IMO, only confusing.

@kbreit

This comment has been minimized.

Copy link
Contributor

kbreit commented Mar 26, 2019

@samccann Overall this looks to be a good start. I left some inline comments but have a few I'm taking out here.

This should have examples in the document. For example, the authentication section should have examples of how these are used. The variables would be dummy variable names, but they'd be obvious to the reader.

Second, I'd like a "Is HttpApi right for you?" section. I keep hearing connection: local will be deprecated, but that's probably a while off. This section should have features it supports as well as features it doesn't support.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 26, 2019

The test ansible-test sanity --test rstcheck [explain] failed with 2 errors:

docs/docsite/rst/dev_guide/developing_modules_network.rst:46:0: (python) unindent does not match any outer indentation level
docs/docsite/rst/dev_guide/developing_modules_network.rst:60:0: (python) unindent does not match any outer indentation level

click here for bot help

@ansibot ansibot added the ci_verified label Mar 26, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 3, 2019

The test ansible-test sanity --test rstcheck [explain] failed with 2 errors:

docs/docsite/rst/dev_guide/developing_modules_network.rst:59:0: (python) unindent does not match any outer indentation level
docs/docsite/rst/dev_guide/developing_modules_network.rst:73:0: (python) unindent does not match any outer indentation level

click here for bot help

@maxamillion

This comment has been minimized.

Copy link
Contributor

maxamillion commented Apr 3, 2019

👍

@ansibot ansibot removed the ci_verified label Apr 3, 2019

@ansibot ansibot added the docsite label Apr 5, 2019

# Fixed headers for requests
headers = {'Content-Type': 'application/json'}
try:
response, response_content = self.connection.send(path, data, method=method, headers=headers)

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 6, 2019

Contributor

This particular example uses a POST method so it will contain body, probably represented by data. What happens with a GET request?

This comment has been minimized.

Copy link
@Qalthos

Qalthos Apr 15, 2019

Contributor

This example defaults to POST, but could just as easily be called with GET (or PATCH, or PUT, or DELETE), The point is that this method should take whatever information it needs from a module or module_utils and turns that into a valid request for the API it follows. That could involve packing data into a JSON payload, or ignoring it if it sees that method is 'GET', or even just passing it through as shown and trusting whatever calls it to give it the right format. What happens in any case is up to the needs of your plugin, which is why it isn't implemented in the base class.

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 15, 2019

Contributor

So this could be a valid implementation? I'm just swagging it, not sure it works...

def send_request(self, data, path, method='POST'):
    if method == 'POST':
        response, response_content = self.connection.send(path, data, method=method, headers=headers)
    elif method == 'GET':
        response, response_content = self.connection.send(path, None, method=method, headers=headers)

This comment has been minimized.

Copy link
@Qalthos

Qalthos Apr 15, 2019

Contributor

I would probably do

if method == 'GET':
    data = None
response, response_content = self.connection.send(path, data, method=method, headers=headers)

Unless you had some other reason to separate the send() calls, but that would work as well.

Authenticating
--------------

By default, all requests will authenticate with HTTP Basic authentication. If a request can return some kind of token to stand in place of HTTP Basic, the ``update_auth(self, response, response_text)`` method should be implemented to inspect responses for such tokens. If the token is meant to be included with the headers of each request, it is sufficient to return a dictionary which will be merged with the computed headers for each request. The default implementation of this method does exactly this for cookies. If the token is used in another way, say in a query string, you should instead save that token to an instance variable, where the ``send_request()`` method (above) can add it to each request

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 6, 2019

Contributor

Based solely on this paragraph, it's not clear to me how to structure calls which use a static header token and don't need any login or logout calls. The paragraph opens with "By default..." so by just reading this paragraph, I'm concerned I will need to disable that.

This comment has been minimized.

Copy link
@Qalthos

Qalthos Apr 15, 2019

Contributor

I'm not entirely following your question. This is the section that deals with not having login or logout calls. If your header token is already known beforehand and there is no other authentication, then you would have to take some other approach not listed here (probably adding the token as a plugin option, and setting self.connection._auth to use the value of that option in __init__ similar to how the login() example does it below)

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 15, 2019

Contributor

I think that makes sense. I'll need to test my implementation out a little more thoroughly to be sure, but it does mostly answer my question. I wouldn't mind seeing a note stating this as such though.

...
# module is your AnsibleModule instance
connection = Connection(module._socket_path)

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 8, 2019

Contributor

I am receiving errors around module._socket_path not being found (don't have the error in front of me). I'd like to see more documentation about configuring _socket_path. I've also noticed not all plugin examples appear to use it (FTD?) so more detail about that may be helpful too.

**************************

Each network connection plugin has a set of plugins which provide a specification of the connection for a particular set of devices. Public methods of these plugins may be called on the connection proxy object from the module just as connection methods can.

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 8, 2019

Contributor

A paragraph explaining how Connection knows about the proper plugin could be helpful to someone newer to the framework. I'll include a proposal.

**************************

Each network connection plugin has a set of plugins which provide a specification of the connection for a particular set of devices. Public methods of these plugins may be called on the connection proxy object from the module just as connection methods can.

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 8, 2019

Contributor
Suggested change
Because the httpapi architecture uses plugins, modules do not refer directly to their httpapi implemention for requests. Instead, they reference `Connection`, as shown below. `Connection` uses the host's `ansible_network_os` value to determine which plugin implementation to execute for a host.

This comment has been minimized.

Copy link
@Qalthos

Qalthos Apr 15, 2019

Contributor

I've gone a slight;y different direction, first to hopefully make clearer that this section does is about module code, and not plugins, and second because this document is not specifically about httpapi, though that is where we are focusing the most attention right now.

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 15, 2019

Contributor

I saw the commit you pushed and it was clear. Thank you!

@@ -0,0 +1,108 @@

This comment has been minimized.

Copy link
@kbreit

kbreit Apr 8, 2019

Contributor

Would it make sense to include a reference example implementation that is bare bones and can be used a good start? The examples here are good, but it doesn't demonstrate everything end-to-end.

@samccann

This comment has been minimized.

Copy link
Contributor Author

samccann commented Apr 8, 2019

@Qalthos - can you take a look at the open questions above?

@kbreit

This comment has been minimized.

Copy link
Contributor

kbreit commented Apr 10, 2019

A section explaining how to specify input parameter names (ex. username) would be useful. I am trying to create a key for api_key but receiving errors that it's not defined.

@samccann samccann moved this from In progress to Waiting review/merge in Network Documentation Apr 10, 2019

@ansibot ansibot added the stale_ci label Apr 15, 2019

@ansibot ansibot removed the stale_ci label Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.