Merge lp:~dimitern/juju-core/lp-1308146-1.18-backport into lp:juju-core/1.18

[Dimiter Naydenov]

Fixed lp:1308146 - backported from github/trunk

See http://pad.lv/1308146 - this fixes the issue by backporting
the already landed fix from github.com/juju/juju/ trunk - for
more info, see https://github.com/juju/juju/pull/786.

Fixed lp:1308146 - backported from github/trunk

See http://pad.lv/1308146 - this fixes the issue by backporting
the already landed fix from github.com/juju/juju/ trunk - for
more info, see https://github.com/juju/juju/pull/786.

EDIT: Fixed base from lp:juju-core to lp:juju-core/1.18
(lbox propose somehow managed to get it wrong)

[Dimiter Naydenov (dimitern)]

Reviewers: mp+235115_code.launchpad.net,

Message:
Please take a look.

Description:
Fixed lp:1308146 - backported from github/trunk

See http://pad.lv/1308146 - this fixes the issue by backporting
the already landed fix from github.com/juju/juju/ trunk - for
more info, see https://github.com/juju/juju/pull/786.

https://code.launchpad.net/~dimitern/juju-core/lp-1308146-1.18-backport/+merge/235115

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/138640043/

Affected files (+85, -12 lines):
   A [revision details]
   M agent/mongo/mongo.go
   M environs/cloudinit/cloudinit_test.go
   M environs/instances/instancetype.go
   M environs/instances/instancetype_test.go
   M environs/testing/tools.go
   M provider/ec2/image_test.go
   M scripts/win-installer/setup.iss
   M state/charm.go
   M state/state.go
   M state/state_test.go
   M version/version.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140513040856-lm051mdy3n916km6
+New revision:
<email address hidden>

Index: state/state_test.go
=== modified file 'state/state_test.go'
--- state/state_test.go 2014-03-27 12:53:54 +0000
+++ state/state_test.go 2014-09-18 12:52:26 +0000
@@ -6,6 +6,7 @@
  import (
   "fmt"
   "net/url"
+ "path/filepath"
   "sort"
   "strconv"
   "time"
@@ -340,6 +341,50 @@
   c.Assert(sch.BundleSha256(), gc.Equals, "missing")
  }

+func (s *StateSuite) TestUpdateUploadedCharmEscapesSpecialCharsInConfig(c
*gc.C) {
+ // Make sure when we have mongodb special characters like "$" and
+ // "." in the name of any charm config option, we do proper
+ // escaping before storing them and unescaping after loading. See
+ // also http://pad.lv/1308146.
+
+ // Clone the dummy charm and change the config.
+ configWithProblematicKeys := []byte(`
+options:
+ $bad.key: {default: bad, description: bad, type: string}
+ not.ok.key: {description: not ok, type: int}
+ valid-key: {description: all good, type: boolean}
+ still$bad.: {description: not good, type: float}
+ $.$: {description: awful, type: string}
+ ...: {description: oh boy, type: int}
+ just$: {description: no no, type: float}
+`[1:])
+ chDir := testing.Charms.ClonedDirPath(c.MkDir(), "dummy")
+ err := utils.AtomicWriteFile(
+ filepath.Join(chDir, "config.yaml"),
+ configWithProblematicKeys,
+ 0666,
+ )
+ c.Assert(err, gc.IsNil)
+ ch, err := charm.ReadDir(chDir)
+ c.Assert(err, gc.IsNil)
+ missingCurl := charm.MustParseURL("local:quantal/missing-1")
+ bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
+ c.Assert(err, gc.IsNil)
+
+ _, err = s.State.PrepareLocalCharmUpload(missingCurl)
+ c.Assert(err, gc.IsNil)
+ sch, err := s.State.UpdateUploadedCharm(ch, missingCurl,
bundleURL, "missing")
+ c.Assert(err, gc.IsNil)
+ c.Assert(sch.URL(), gc.DeepEquals, missingCurl)
+ c.Assert(sch.Revision(), gc.Equals, missingCurl.Revision)
+ c.Assert(sch.IsUploaded(), jc.IsTrue)
+ c.Assert(sch.IsPlaceholder(), jc.IsFalse)
+ c.Assert(sch.Meta(), gc.DeepEquals, ch.Meta()...

[Martin Packman (gz)]

Looks good to me.

review: Approve

=== modified file 'state/charm.go'
--- state/charm.go	2014-01-29 20:57:31 +0000
+++ state/charm.go	2014-09-18 12:58:15 +0000
@@ -27,6 +27,17 @@
 }
 
 func newCharm(st *State, cdoc *charmDoc) (*Charm, error) {
+	// Because we probably just read the doc from state, make sure we
+	// unescape any config option names for "$" and ".". See
+	// http://pad.lv/1308146
+	if cdoc != nil && cdoc.Config != nil {
+		unescapedConfig := charm.NewConfig()
+		for optionName, option := range cdoc.Config.Options {
+			unescapedName := unescapeReplacer.Replace(optionName)
+			unescapedConfig.Options[unescapedName] = option
+		}
+		cdoc.Config = unescapedConfig
+	}
 	return &Charm{st: st, doc: *cdoc}, nil
 }
 

=== modified file 'state/state.go'
--- state/state.go	2014-03-26 18:37:50 +0000
+++ state/state.go	2014-09-18 12:58:15 +0000
@@ -855,9 +855,18 @@
 func (st *State) updateCharmDoc(
 	ch charm.Charm, curl *charm.URL, bundleURL *url.URL, bundleSha256 string, preReq interface{}) (*Charm, error) {
 
+	// Make sure we escape any "$" and "." in config option names
+	// first. See http://pad.lv/1308146.
+	cfg := ch.Config()
+	escapedConfig := charm.NewConfig()
+	for optionName, option := range cfg.Options {
+		escapedName := escapeReplacer.Replace(optionName)
+		escapedConfig.Options[escapedName] = option
+	}
+
 	updateFields := bson.D{{"$set", bson.D{
 		{"meta", ch.Meta()},
-		{"config", ch.Config()},
+		{"config", escapedConfig},
 		{"bundleurl", bundleURL},
 		{"bundlesha256", bundleSha256},
 		{"pendingupload", false},

=== modified file 'state/state_test.go'
--- state/state_test.go	2014-03-27 12:53:54 +0000
+++ state/state_test.go	2014-09-18 12:58:15 +0000
@@ -6,6 +6,7 @@
 import (
 	"fmt"
 	"net/url"
+	"path/filepath"
 	"sort"
 	"strconv"
 	"time"
@@ -340,6 +341,50 @@
 	c.Assert(sch.BundleSha256(), gc.Equals, "missing")
 }
 
+func (s *StateSuite) TestUpdateUploadedCharmEscapesSpecialCharsInConfig(c *gc.C) {
+	// Make sure when we have mongodb special characters like "$" and
+	// "." in the name of any charm config option, we do proper
+	// escaping before storing them and unescaping after loading. See
+	// also http://pad.lv/1308146.
+
+	// Clone the dummy charm and change the config.
+	configWithProblematicKeys := []byte(`
+options:
+  $bad.key: {default: bad, description: bad, type: string}
+  not.ok.key: {description: not ok, type: int}
+  valid-key: {description: all good, type: boolean}
+  still$bad.: {description: not good, type: float}
+  $.$: {description: awful, type: string}
+  ...: {description: oh boy, type: int}
+  just$: {description: no no, type: float}
+`[1:])
+	chDir := testing.Charms.ClonedDirPath(c.MkDir(), "dummy")
+	err := utils.AtomicWriteFile(
+		filepath.Join(chDir, "config.yaml"),
+		configWithProblematicKeys,
+		0666,
+	)
+	c.Assert(err, gc.IsNil)
+	ch, err := charm.ReadDir(chDir)
+	c.Assert(err, gc.IsNil)
+	missingCurl := charm.MustParseURL("local:quantal/missing-1")
+	bundleURL, err := url.Parse("http://bundles.testing.invalid/dummy-1")
+	c.Assert(err, gc.IsNil)
+
+	_, err = s.State.PrepareLocalCharmUpload(missingCurl)
+	c.Assert(err, gc.IsNil)
+	sch, err := s.State.UpdateUploadedCharm(ch, missingCurl, bundleURL, "missing")
+	c.Assert(err, gc.IsNil)
+	c.Assert(sch.URL(), gc.DeepEquals, missingCurl)
+	c.Assert(sch.Revision(), gc.Equals, missingCurl.Revision)
+	c.Assert(sch.IsUploaded(), jc.IsTrue)
+	c.Assert(sch.IsPlaceholder(), jc.IsFalse)
+	c.Assert(sch.Meta(), gc.DeepEquals, ch.Meta())
+	c.Assert(sch.Config(), gc.DeepEquals, ch.Config())
+	c.Assert(sch.BundleURL(), gc.DeepEquals, bundleURL)
+	c.Assert(sch.BundleSha256(), gc.Equals, "missing")
+}
+
 func (s *StateSuite) assertPlaceholderCharmExists(c *gc.C, curl *charm.URL) {
 	// Find charm directly and verify only the charm URL and
 	// Placeholder are set.