Reject charms with unstorable data #6859

Merged
merged 3 commits into from Jan 24, 2017

Conversation

Projects
None yet
4 participants
Contributor

mjs commented Jan 24, 2017

Previously, charm data was accepted that generated field names which
couldn't be stored correctly in MongoDB. MongoDB doesn't check these in
some insert scenerios but will reject them in later updates and
queries.

Fixing this properly by escaping and unescaping everywhere is too
dangerous given the imminent 2.1 release, so problematic charm data
is now rejected outright. It is unlikely that current charms will contain
metadata with invalid field names.

This PR adds some utility functions to the mongo/utils package.

  1. IsValidFieldName returns whether a field name is valid for use in a
    MongoDB document.
  2. CheckStorable checks if a document - and its subdocuments - uses valid
    field names throughout.

Drive-by: apiserver/application.StoreCharmArchive was not returning the error when a the charm update to state failed.

QA

Deployed some charms and confirmed they still work. Then deployed a charm with invalid charm data to confirm that it is rejected.

$ juju deploy ubuntu
Located charm "cs:ubuntu-10".
Deploying charm "cs:ubuntu-10".
menno@maiwa ~/canonical/repository/xenial 
$ juju deploy postgresql
Located charm "cs:postgresql-114".
Deploying charm "cs:postgresql-114".
menno@maiwa ~/canonical/repository/xenial 
$ juju deploy ./ubuntubad        
ERROR cannot upload charm: invalid charm data: "$foo" is not a valid field name

mjs added some commits Jan 24, 2017

mongo/utils: New utils to check doc field names
IsValidFieldName returns whether a field name is valid for use in a
MongoDB document.

CheckStorable checks if a document - and its subdocuments - uses valid
field names throughout.
state: Reject charms with unstorable data
Previously, charms data was accepted that generated field names which
couldn't be stored correctly in MongoDB. MongoDB doesn't check these in
*some* insert scenerios but will reject them in later updates and
queries.

Fixing this properly by escaping and unescaping everywhere is too
dangerous given the imminent 2.1 release, so bad charm data is now
rejected outright. It is unlikely that current charms will contain
metadata with invalid field names.
apiserver/application: Fix StoreCharmArchive error
StoreCharmArchive wasn't returning an error when the charm database
update failed. It looks like this has been broken for some time.

axw approved these changes Jan 24, 2017

@@ -222,80 +222,6 @@ func (s *ApplicationSuite) TestSetCharmUpdatesBindings(c *gc.C) {
})
}
-func (s *ApplicationSuite) TestSetCharmWithWeirdlyNamedEndpoints(c *gc.C) {
@axw

axw Jan 24, 2017

Member

will we be changing things later to escape/unescape? if yes, skip rather than delete?

@mjs

mjs Jan 24, 2017

Contributor

I think this was the wrong place to test this anyway. The new tests in for the State charm methods seems like a better place for this.

Contributor

mjs commented Jan 24, 2017

$$merge$$

Contributor

jujubot commented Jan 24, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Jan 24, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10090

Owner

jameinel commented Jan 24, 2017

$$merge$$

Owner

jameinel commented Jan 24, 2017

looked like a spurious lxd failure

Contributor

jujubot commented Jan 24, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 715bfce into juju:2.1 Jan 24, 2017

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@mjs mjs deleted the mjs:reject-unstorable-charm-data branch Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment