Changes to sreg to start using the server-core 2.0

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: telliott, Assigned: telliott)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 541656 [details] [diff] [review]
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.
Attachment #541656 - Flags: review?(tarek)
(Assignee)

Comment 1

7 years ago
Created attachment 541659 [details] [diff] [review]
Updates sreg to use 2.0 server-core

Forgot that I'd commented out some tests to figure out the delete problem
Assignee: nobody → telliott
Attachment #541656 - Attachment is obsolete: true
Attachment #541659 - Flags: review?(tarek)
Attachment #541656 - Flags: review?(tarek)
(Assignee)

Comment 2

7 years ago
Created attachment 541733 [details] [diff] [review]
Updates sreg to use 2.0 server-core (v3)

A few minor cleanups, and fixed a typo
Attachment #541659 - Attachment is obsolete: true
Attachment #541733 - Flags: review?(tarek)
Attachment #541659 - Flags: review?(tarek)
(Assignee)

Updated

7 years ago
Attachment #541733 - Flags: review?(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.
Attachment #541733 - Flags: review?(rmiller) → review+
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 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)
(Assignee)

Comment 6

7 years ago
(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 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)
Attachment #541733 - Flags: review?(tarek) → review+
(Assignee)

Comment 8

7 years ago
Fallback node test migration is covered in Bug 673311
(Assignee)

Comment 9

7 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
The push brakes the building see bug 675875
Blocks: 675875
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 11

7 years ago
Fixed the underlying bug, so this can be closed again.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.