666902 - Changes to sreg to start using the server-core 2.0

Status: RESOLVED FIXED

(Cloud Services :: Server: Registration, defect)

Description

[Toby Elliott [:telliott]]

Attachment: Updates sreg to use 2.0 server-core

Rebuilding sreg to use 2.0 server-core. The version here has 1 problem - delete's aren't being passed through in the test suite; maybe this'll actually work for tarek, since it's currently working for him and not for me. However, the rest is looking pretty good.

Comment 1

[Toby Elliott [:telliott]]

Attachment: Updates sreg to use 2.0 server-core

Forgot that I'd commented out some tests to figure out the delete problem

Comment 2

[Toby Elliott [:telliott]]

Attachment: Updates sreg to use 2.0 server-core (v3)

A few minor cleanups, and fixed a typo

Comment 3

[Rob Miller [:rmiller]]

Comment on attachment 541733 [details] [diff] [review]
Updates sreg to use 2.0 server-core (v3)

A few comments, but no show-stoppers.  Comments are in the splinter review UI.

Comment 4

[Rob Miller [:rmiller]]

Comment on attachment 541733 [details] [diff] [review]
Updates sreg to use 2.0 server-core (v3)

Review of attachment 541733 [details] [diff] [review]:
-----------------------------------------------------------------

There are possible some underlying server-core concerns that are hinted at by this code, but there's nothing so egregious that I'd block the merge, yet.  Definitely want to have some more conversations about how this all works, though.

::: syncsreg/controller.py
@@ +64,3 @@
>  
> +        try:
> +            self.auth = load_and_configure(app.config, 'auth')

As pointed out by telliott, it seems a bit weird to be loading app level configuration at controller initialization time.  Shouldn't this be loaded at the app level?

@@ +85,4 @@
>  
>      def user_node(self, request):
>          """Returns the storage node root for the user"""
> +        if not self.nodes:

`not x` isn't the same as `x is None`, it's probably safer to use the latter here.

@@ +93,5 @@
> +        if not request.user.get('userid'):
> +            return HTTPNotFound()
> +
> +        if request.user.get('syncNode'):
> +            return json_response(request.user.get('syncNode'))

Hrm... so you have to call `get_user_info` to trigger side effects which then makes data available through the request.user object?  Smells a bit funny to me.

@@ +145,5 @@
>          user = request.config.get('smtp.user')
>          password = request.config.get('smtp.password')
>          subject = _('Resetting your %s password' % product)
> +        res, msg = send_email(sender, request.user['mail'], subject,
> +                              body, host, port, user, password)

This is a bit tangential, but `send_email` is synchronous.  Do we want it to be?

::: syncsreg/tests/test_user.py
@@ +233,4 @@
>  
>      def test_delete_user(self):
>          # unknown user raises a 404
> +        #self.app.delete('/__xx__', status=404)

Do we need to keep this commented code?

Comment 5

[Tarek Ziadé (:tarek)]

Comment on attachment 541733 [details] [diff] [review]
Updates sreg to use 2.0 server-core (v3)

+1 w/ Rob's review and:

>diff -r c38cef9ebd04 Makefile
...
> 
>-    def _get_user_info(self, request):
>-        username = request.sync_info['username']
>-        user_id = self.auth.get_user_id(username)
>-        if user_id is None:
>-            raise HTTPNotFound()
>-        return username, user_id
>+        try:
>+            self.auth = load_and_configure(app.config, 'auth')
>+        except Exception:
>+            logger.error(traceback.format_exc())
>+            logger.error("unable to load auth. Problem?")

I assume this is a typo ? should be self.auth = None, no ?

>+            self.reset = None
>+
>+        try:
>+            self.reset = load_and_configure(app.config, 'reset_codes')
>+        except Exception:
>+            logger.error(traceback.format_exc())
>+            logger.error("unable to load reset codes. Problem?")
>+            self.reset = None
>+
>+        try:
>+            self.nodes = load_and_configure(app.config, 'nodes')
>+        except Exception:

Shouldn't we log the TB here as well, so we don't hide another issue ?

>+            logger.error("No node assignment loaded")
>+            self.nodes = None
>+        self.fallback_node = app.config.get('nodes.fallback_node')
> 
>     def user_node(self, request):
>         """Returns the storage node root for the user"""
>-        username, user_id = self._get_user_info(request)
>+        if not self.nodes:
>+            return json_response('null')
>+
>+        #see if they already have a node assigned
>+        self.auth.get_user_info(request.user, ['syncNode'])
>+        if not request.user.get('userid'):
>+            return HTTPNotFound()

syncNode => syncnode ?

>+
>+        if request.user.get('syncNode'):
>+            return json_response(request.user.get('syncNode'))
>+
>         try:
>-            location = self.auth.get_user_node(user_id)
>-        except (NodeAttributionError, BackendError):
>-            # this happens when the back end failed to get a node
>-            return json_response(None)
>+            new_node = self.nodes.get_best_node(request.user)
>+            if new_node is None:
>+                if self.fallback_node:
>+                    new_node = self.fallback_node
>+                else:
>+                    return json_response('null')
> 
>-        if location is None:
>-            fallback = self.app.config.get('auth.fallback_node')
>-            if fallback is not None:
>-                location = fallback
>+            if self.auth.admin_update_field(request.user, 'syncNode',
>+                                            new_node):
>+                return json_response(new_node)
>+        except Exception:

Logging the TB ?

>+                raise HTTPServiceUnavailable()

...

Are we still testing the falback node ? not sure why the whole test is stripped here:

>-    def test_fallback_node(self):
>-        if self.distant:
>-            return
>-        app = get_app(self.app)
>-        proxy = app.config['auth.fallback_node'] = 'http://myhappy/proxy/'
>-        url = '/%s/node/weave' % self.user_name
>-        res = self.app.get(url)
>-        self.assertEqual(res.json, proxy)
>-
>-        del app.config['auth.fallback_node']
>-        res = self.app.get(url)
>-        self.assertEqual(res.json, None)
>+            self.auth.delete_user(user, 'x' * 9)

Comment 6

[Toby Elliott [:telliott]]

(In reply to comment #5)
> Comment on attachment 541733 [details] [diff] [review] [review]
> Updates sreg to use 2.0 server-core (v3)
> 
> +1 w/ Rob's review and:
> 
> >diff -r c38cef9ebd04 Makefile

> 
> I assume this is a typo ? should be self.auth = None, no ?
> 
> >+            self.reset = None

Good catch.


> >+        try:
> >+            self.nodes = load_and_configure(app.config, 'nodes')
> >+        except Exception:
> 
> Shouldn't we log the TB here as well, so we don't hide another issue ?
> 

I think that would be reasonable, though it'll be a bit weird, as nodes isn't actually required.


> 
> syncNode => syncnode ?
> 

No, it's CamelCase in LDAP, so we've just sort of gone from there.


> 
> Are we still testing the falback node ? not sure why the whole test is
> stripped here:
> 
> >-    def test_fallback_node(self):
> >-        if self.distant:
> >-            return
> >-        app = get_app(self.app)
> >-        proxy = app.config['auth.fallback_node'] = 'http://myhappy/proxy/'
> >-        url = '/%s/node/weave' % self.user_name
> >-        res = self.app.get(url)
> >-        self.assertEqual(res.json, proxy)
> >-
> >-        del app.config['auth.fallback_node']
> >-        res = self.app.get(url)
> >-        self.assertEqual(res.json, None)
> >+            self.auth.delete_user(user, 'x' * 9)

Fallback node is part of node-assignment now, so should probably be tested there (and you're right, it's missing there right now).

Comment 7

[Tarek Ziadé (:tarek)]

Comment on attachment 541733 [details] [diff] [review]
Updates sreg to use 2.0 server-core (v3)

I am r+ assuming that the mentioned issues are addressed (test removed + a few fixes)

Comment 8

[Toby Elliott [:telliott]]

Fallback node test migration is covered in Bug 673311

Comment 9

[Toby Elliott [:telliott]]

All applied in http://hg.mozilla.org/services/server-sreg/rev/65d9ed2e9a7b

I've decided that the fallback node does belong here and updated Bug 673311 to reflect that.

Comment 10

[Tarek Ziadé (:tarek)]

The push brakes the building see bug 675875

Comment 11

[Toby Elliott [:telliott]]

Fixed the underlying bug, so this can be closed again.