Support YARN endpoints protected by Kerberos/SPNEGO #16

lresende · 2018-09-19T22:27:38Z

There are a few commits here, mostly around Kerberos/SPNEGO and Integration Tests.
Also, bumping the version to 0.2.6 to perform a release with these changes to be used in Jupyter Enterprise Gateway.

Please review, but don't squash/merge.

kevin-bates

This is really great Luciano - thanks! I just had a couple comments.

yarn_api_client/application_master.py

kevin-bates · 2018-09-19T22:49:26Z

yarn_api_client/base.py

-        if self.address is None:
-            raise ConfigurationError('API address is not set')
-        elif self.port is None:
-            raise ConfigurationError('API port is not set')


We should probably add checks against None for address and port at the very end of the constructor to maintain parity.

Also, this is removing a property that allowed the user to get a connection to the particular server. Seems like that needs to be preserved unless we know there are no applications using that property.

Good catch... added it back together with fixes for test cases that were failing

Sorry, not finding where property http_conn was added back in. Please advise.

yarn_api_client/history_server.py

kevin-bates · 2018-09-19T22:50:15Z

yarn_api_client/node_manager.py

    """
-    def __init__(self, address=None, port=8042, timeout=30):
-        self.address, self.port, self.timeout = address, port, timeout
+    def __init__(self, address=None, port=8042, timeout=30, kerberosEnaled=False):


Please fix typo to kerberosEnabled

kevin-bates · 2018-09-19T22:50:39Z

yarn_api_client/resource_manager.py

    """
-    def __init__(self, address=None, port=8088, timeout=30):
-        self.address, self.port, self.timeout = address, port, timeout
+    def __init__(self, address=None, port=8088, timeout=30, kerberosEnaled=False):


Please fix typo to kerberosEnabled

lresende · 2018-09-20T04:54:20Z

There are still some dependency issues with the tests, around the SPNEGO dependency, I will try to figure that out tomorrow.

coveralls · 2018-09-20T15:36:04Z

Coverage decreased (-4.4%) to 91.685% when pulling a21f612 on lresende:security into ad9a807 on toidi:master.

lresende · 2018-09-20T15:36:20Z

Ok, All good with the dependencies, it was a tox configuration issue.

ediskandarov-cur · 2018-09-20T16:10:52Z

setup.py

-    install_requires = install_requires,
+    install_requires = [
+        'requests>=2.7,<3.0',
+        'requests-kerberos==0.12.0',


it's not a good idea to pin requirements here, as it may conflict with project wide requirements
perhaps, it's possible to specify in terms of >=, <=?

ediskandarov-cur · 2018-09-20T16:11:31Z

tests/test_base.py

+import requests
+import requests_mock

+from tests import TestCase


I wish I would use pytest for testing
But fine for now. I may find some time to update tests

ediskandarov-cur · 2018-09-20T16:12:59Z

tests/test_base.py


-                client.request('/ololo', foo='bar')
+    def test_valid_request(self):
+        with requests_mock.mock() as requests_get_mock:


perhaps, use requests_mock as a decorator?

ediskandarov-cur · 2018-09-20T16:13:51Z

tox.ini


 [testenv]
 deps =
+     requests


please keep this list sorted

ediskandarov-cur · 2018-09-20T16:15:03Z

yarn_api_client/application_master.py

    """
-    def __init__(self, address=None, port=8088, timeout=30):
-        self.address, self.port, self.timeout = address, port, timeout
+    def __init__(self, address=None, port=8088, timeout=30, kerberosEnabled=False):


don't mix variable naming styles. Project follows pep8 convention

ediskandarov-cur · 2018-09-20T16:15:38Z

yarn_api_client/base.py

-    from urllib import urlencode
-except ImportError:
-    from urllib.parse import urlencode
+import requests


with requests it becomes so much easies 👍

ediskandarov-cur · 2018-09-20T16:17:44Z

yarn_api_client/base.py

-        if params:
-            path = api_path + '?' + params
+        params = query_args
+        api_endpoint = 'http://{}:{}{}'.format(self.address, self.port, api_path)


I have a doubt about hardcoding http here

enabling https requires some more work throughout the code, we could address that in the future

ediskandarov-cur · 2018-09-20T16:19:09Z

Thanks for the PR!
I'll look again tomorrow morning, as I'm sleepy now

ediskandarov-cur · 2018-09-20T16:19:57Z

.travis.yml

 install:
-  - pip install tox coveralls
+  - pip install --upgrade setuptools pip
+  - pip install --upgrade requests requests_mock requests_kerberos tox coveralls


these deps should be managed by tox

except tox and coveralls

ediskandarov-cur · 2018-09-20T16:21:29Z

itests/integration_test_resource_manager.py

@@ -0,0 +1,82 @@
+# -*- coding: utf-8 -*-


is it possible to make integration tests as a part of CI process?

The integration tests require a running yarn, we could probably add docker, etc... let me create an issue to address that in the near future.

ediskandarov-cur · 2018-09-20T16:21:48Z

itests/integration_test_resource_manager.py

+            appstats = self.resourceManager.cluster_application_statistics()
+            pprint(appstats.data)
+            self.assertIsNotNone(appstats.data['appStatInfo'])
+            # TODO: test arguments


remove TODO

lresende · 2018-09-21T16:52:36Z

@toidi and @kevin-bates All comments have been addressed, and a few issues created around the things that are really general enhancements and not added by these commits. Any other issues, otherwise I would like to add these commits to master and perform a release so I can use this in EG.

kevin-bates

All looks good except I can't find where the http_conn property was restored.

kevin-bates · 2018-09-21T17:30:00Z

yarn_api_client/base.py

-        if self.address is None:
-            raise ConfigurationError('API address is not set')
-        elif self.port is None:
-            raise ConfigurationError('API port is not set')


Sorry, not finding where property http_conn was added back in. Please advise.

lresende · 2018-09-21T19:08:39Z

In BaseYarnAPI there is now

    def _validate_configuration(self):
        if self.address is None:
            raise ConfigurationError('API address is not set')
        elif self.port is None:
            raise ConfigurationError('API port is not set')

and also the validation tests in test/test_base.py are passing:

   def test_http_configuration(self):
        with requests_mock.mock() as requests_get_mock:
            requests_get_mock.get('/ololo', text=json.dumps(BaseYarnAPITestCase.success_response()))

            client = self.get_client()
            client.address = None
            client.port = 80

            with self.assertRaises(ConfigurationError):
                client.request('/ololo')

lresende · 2018-09-21T19:09:18Z

Also note that, because we moved to now use the requests package, we are not using http_connection anymore.

lresende · 2018-09-21T19:17:20Z

And yes, looking from the diffs are a little strange, so I had to go into the dif, find the base.py and show the file from the diff page. You can probably do the same from the commits list as well.

kevin-bates · 2018-09-21T21:04:58Z

I understand we're not using http_connection any more, but http_conn was decorated as a property via @property and I had assumed that that meant it was publicly exposed - therefore a "contract". If that's not the case, then please ignore my comment/concern. I just don't know, nor can we really determine, if that property is in use elsewhere and, if so, that particular client will break after upgrading.

lresende · 2018-09-21T21:24:42Z

Ok, I now understand your question. This is indeed a breaking change, as the HTTP connection is now managed internally by requests package. How about naming the release 0.3.0 and updating the changelog to explicitly describe this breaking change:

0.3.0 Release
    - Add support for YARN endpoints protected by Kerberos/SPNEGO
    - Moved to `requests` package for REST API invocation
    - Remove `http_con` property, as connections are now managed by `requests` package

ediskandarov-cur · 2018-09-22T03:58:26Z

yarn_api_client/resource_manager.py

        loc_args = (
            ('states', states),
-            ('applicationTypes', applicationTypes))
+            ('application_types', application_types))


first item in tuple goes to hadoop api
does it expect ?construct_parameters=X instead of ?applicationTypes=X in query string?

ediskandarov-cur

👍 looks good to me
maybe except rename of application_types query string argument

ediskandarov-cur · 2018-09-22T04:01:11Z

@kevin-bates your concern about http_conn is valid.
On the other hand I didn't have an intention to include it in contract. That property should have been private.

kevin-bates · 2018-09-22T07:04:46Z

@toidi - thanks for the explanation - good to know there wasn't intention to make the property public. I was thinking about taking the version approach that @lresende suggested as well, and I think moving to 0.3.0 is probably the best approach since it covers our bases.

kevin-bates

Based on the final discussion of http_conn, these changes look good to me. Thanks for handling this @lresende and @toidi for your input and guidance.

Integration test that, given a provided YARN ENDPOINT, execute some real scenario test against that server. Note that, if no YARN ENDPOINT is provided, the tests are ignored.

lresende requested a review from kevin-bates September 19, 2018 22:29

lresende added 2 commits September 19, 2018 15:38

Enable building universal distribution

b1ff2c5

Update git configuration files

418ba77

lresende force-pushed the security branch from 62ed482 to c74a29c Compare September 19, 2018 22:38

kevin-bates reviewed Sep 19, 2018

View reviewed changes

lresende force-pushed the security branch 10 times, most recently from 1db7505 to 5f72136 Compare September 20, 2018 04:45

lresende mentioned this pull request Sep 20, 2018

YARN end-point: 'http://localhost:8088/ws/v1/cluster' refused the connection. Is the resource manager running? jupyter-server/enterprise_gateway#441

Closed

lresende force-pushed the security branch 2 times, most recently from 7f3b6e9 to 01d5144 Compare September 20, 2018 15:33

lresende requested a review from ediskandarov-cur September 20, 2018 15:54

ediskandarov-cur reviewed Sep 20, 2018

View reviewed changes

tox.ini Outdated

[testenv]

deps =

requests

Copy link

Collaborator

ediskandarov-cur Sep 20, 2018

Choose a reason for hiding this comment

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

please keep this list sorted

ediskandarov-cur reviewed Sep 20, 2018

View reviewed changes

lresende force-pushed the security branch from 19485b1 to ebf8a47 Compare September 20, 2018 18:08

This was referenced Sep 20, 2018

Fixed encoding bug in python3 #13

Closed

Bug with Python 3: "TypeError: the JSON object must be str, not 'bytes'" #7

Closed

kevin-bates reviewed Sep 21, 2018

View reviewed changes

lresende force-pushed the security branch from ebf8a47 to 51747ab Compare September 21, 2018 19:09

lresende force-pushed the security branch from 51747ab to abf4138 Compare September 21, 2018 19:31

lresende force-pushed the security branch 2 times, most recently from cfc8b30 to 0df6fb7 Compare September 21, 2018 21:21

ediskandarov-cur reviewed Sep 22, 2018

View reviewed changes

ediskandarov-cur approved these changes Sep 22, 2018

View reviewed changes

kevin-bates approved these changes Sep 22, 2018

View reviewed changes

lresende added 3 commits September 22, 2018 04:20

Support YARN endpoints protected by Kerberos/SPNEGO

c04e2fc

Integration tests run against provided YARN ENDPOINT

8ef5cae

Integration test that, given a provided YARN ENDPOINT, execute some real scenario test against that server. Note that, if no YARN ENDPOINT is provided, the tests are ignored.

Bump version in preparation for 0.3.0 release

a21f612

lresende force-pushed the security branch from 0df6fb7 to a21f612 Compare September 22, 2018 11:27

lresende merged commit e017dcf into gateway-experiments:master Sep 22, 2018

lresende deleted the security branch September 22, 2018 11:32

Support YARN endpoints protected by Kerberos/SPNEGO #16

Support YARN endpoints protected by Kerberos/SPNEGO #16

Uh oh!

Conversation

lresende commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevin-bates left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lresende commented Sep 20, 2018

Uh oh!

coveralls commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lresende commented Sep 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ediskandarov-cur commented Sep 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lresende commented Sep 21, 2018

Uh oh!

kevin-bates left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lresende commented Sep 21, 2018

Uh oh!

lresende commented Sep 21, 2018

Uh oh!

lresende commented Sep 21, 2018

Uh oh!

kevin-bates commented Sep 21, 2018

Uh oh!

lresende commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ediskandarov-cur left a comment

Choose a reason for hiding this comment

Uh oh!

ediskandarov-cur commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

lresende commented Sep 19, 2018 •

edited

Loading

coveralls commented Sep 20, 2018 •

edited

Loading

ediskandarov-cur commented Sep 22, 2018 •

edited

Loading