Merge lp:~rvb/maas/bug-1062607 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Cope with doubles slashes being removed by apache when editing an interface.

This branch fixes the url machinery so that it will cope with the fact that apache transforms multiple slashes in a url into a single one. That pattern appears in the url that maas generates when one edits an interface with an empty name.

= Notes =

Maybe one can find a better way to express the fact that the new url matchers should match an empty interface "(?P<interface>[\w\-]{0})"…?

Note that I've tested in a production instance (behind apache), that this actually fixes bug 1062607.

[Gavin Panella (allenap)]

Include a prefix, like "@" or "if=", then the regex can be "if=(?P<interface>[\w\-]{0})" ?

[Jeroen T. Vermeulen (jtv)]

I don't see how this can work: "(?P<interface>[\w\-]{0})" will only match a string of length zero. If you want zero or more instances, that's actually easier: "(?P<interface>[\w\-]*)"

I could be missing something, of course. But if not, this suggests a missing test.

review: Needs Fixing

[Raphaël Badin (rvb)]

> I don't see how this can work: "(?P<interface>[\w\-]{0})" will only match a
> string of length zero.

Indeed, that's exactly the intention, I want it to match 'interface' so that the view will be called with interface=''.

> If you want zero or more instances, that's actually
> easier: "(?P<interface>[\w\-]*)"

Yeah, but the problem is the '/'.

Here is the url we have for the 'eth0' interface:
http://localhost:8080/MAAS/clusters/24449ff8-3540-4dfa-b434-ab79cc550401/interfaces/eth0/edit/

Here is the url we have for the '' interface:
http://localhost:8080/MAAS/clusters/24449ff8-3540-4dfa-b434-ab79cc550401/interfaces//edit/
The problem is that apache transforms it into
http://localhost:8080/MAAS/clusters/24449ff8-3540-4dfa-b434-ab79cc550401/interfaces/edit/
That's the url that the new regex matches.

[Raphaël Badin (rvb)]

> Include a prefix, like "@" or "if=", then the regex can be
> "if=(?P<interface>[\w\-]{0})" ?

That's actually a bit more complicated than the solution I have here I think. What I like about my solution is that is adds two completely new urls that we could remove once bug 1063881 is fixed and the new regex are easy to read.

[Jeroen T. Vermeulen (jtv)]

I think I see what you're saying now. But then why not just "(?P<interface>)" to get the empty string there?

[Raphaël Badin (rvb)]

> I think I see what you're saying now. But then why not just "(?P<interface>)"
> to get the empty string there?

Because that would match stuff like:
http://localhost:8080/MAAS/clusters/24449ff8-3540-4dfa-b434-ab79cc550401/interfaces/blahedit/

And I don't want that :)

I just want to match:

http://localhost:8080/MAAS/clusters/24449ff8-3540-4dfa-b434-ab79cc550401/interfaces/edit/

[Jeroen T. Vermeulen (jtv)]

I don't see what you mean... (?P<interface>) matches only the empty string, nothing else.

[Raphaël Badin (rvb)]

> I don't see what you mean... (?P<interface>) matches only the empty string,
> nothing else.

Ah, you're right, I've updated the branch.

[Jeroen T. Vermeulen (jtv)]

OK

review: Approve

=== modified file 'src/maasserver/tests/test_views_settings_clusters.py'
--- src/maasserver/tests/test_views_settings_clusters.py	2012-10-03 07:57:55 +0000
+++ src/maasserver/tests/test_views_settings_clusters.py	2012-10-08 18:01:35 +0000
@@ -29,11 +29,14 @@
     reload_object,
     )
 from maasserver.testing.factory import factory
-from maasserver.testing.testcase import (
-    AdminLoggedInTestCase,
-    )
+from maasserver.testing.testcase import AdminLoggedInTestCase
 from maastesting.matchers import ContainsAll
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+    AllMatch,
+    Contains,
+    Equals,
+    MatchesStructure,
+    )
 
 
 class ClusterListingTest(AdminLoggedInTestCase):
@@ -142,3 +145,32 @@
         self.assertThat(
             reload_object(interface),
             MatchesStructure.byEquality(**data))
+
+
+# XXX: rvb 2012-10-08 bug=1063881: apache transforms '//' into '/' in
+# the urls it passes around and this happens when an interface has an empty
+# name.
+class ClusterInterfaceDoubleSlashBugTest(AdminLoggedInTestCase):
+
+    def test_edit_delete_empty_cluster_interface_when_slash_removed(self):
+        nodegroup = factory.make_node_group()
+        interface = factory.make_node_group_interface(
+            nodegroup=nodegroup, interface='',
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        edit_link = reverse(
+            'cluster-interface-edit',
+            args=[nodegroup.uuid, interface.interface])
+        delete_link = reverse(
+            'cluster-interface-delete',
+            args=[nodegroup.uuid, interface.interface])
+        links = [edit_link, delete_link]
+        # Just make sure that the urls contains '//'.  If this is not
+        # true anymore, because we've refactored the urls, this test can
+        # problably be removed.
+        self.assertThat(links, AllMatch(Contains('//')))
+        # Simulate what apache (when used as a frontend) does to the
+        # urls.
+        new_links = [link.replace('//', '/') for link in links]
+        response_statuses = [
+            self.client.get(link).status_code for link in new_links]
+        self.assertThat(response_statuses, AllMatch(Equals(httplib.OK)))

=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py	2012-10-08 08:28:23 +0000
+++ src/maasserver/urls.py	2012-10-08 18:01:35 +0000
@@ -141,6 +141,17 @@
         r'^clusters/(?P<uuid>[\w\-]+)/interfaces/(?P<interface>[\w\-]*)/'
         'delete/$',
         ClusterInterfaceDelete.as_view(), name='cluster-interface-delete'),
+    # XXX: rvb 2012-10-08 bug=1063881:
+    # These two urls are only here to cope with the fact that an interface
+    # can have an empty name, thus leading to urls containing the
+    # pattern '//' that is then reduced by apache into '/'.
+    adminurl(
+        r'^clusters/(?P<uuid>[\w\-]+)/interfaces/(?P<interface>)'
+        'edit/$', ClusterInterfaceEdit.as_view()),
+    adminurl(
+        r'^clusters/(?P<uuid>[\w\-]+)/interfaces/(?P<interface>)'
+        'delete/$', ClusterInterfaceDelete.as_view()),
+    # /XXX
     adminurl(r'^settings/$', settings, name='settings'),
     adminurl(
         r'^settings/archives/add/$', settings_add_archive,