snappy-hwe-snaps

Merge ~morphis/snappy-hwe-snaps/+git/wifi-ap:f/write-config-atomically into ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master

  1. Git
  2. lp:~morphis/snappy-hwe-snaps/+git/wifi-ap
  3. f/write-config-atomically
  4. Merge into master
Proposed by Simon Fels
Status: Merged
Approved by: Konrad Zapałowicz
Approved revision: 49bb0599f4a05b84d8cfb0c08f28f624c101bf66
Merged at revision: ccf9818ff798f1ecd07f600d13fe3d72d3b62c28
Proposed branch: ~morphis/snappy-hwe-snaps/+git/wifi-ap:f/write-config-atomically
Merge into: ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master
Diff against target: 144 lines (+49/-10)
4 files modified
cmd/service/api.go (+18/-8)
cmd/service/api_test.go (+2/-2)
spread.yaml (+10/-0)
tests/regression/lp-1694486/task.yaml (+19/-0)
Reviewer Review Type Date Requested Status
System Enablement Bot continuous-integration Approve
Konrad Zapałowicz (community) code Approve
Review via email: mp+324997@code.launchpad.net

Description of the change

Fix LP #1694486

This is fix for https://bugs.launchpad.net/snappy-hwe-snaps/+bug/1694486 where we fail to write the new configuration out atomically when an invalid configuration item was tried to change. In that case the existing configuration was reset back to the snap defaults.

To post a comment you must log in.
Revision history for this message
Konrad Zapałowicz (kzapalowicz) wrote :

Type in 1st commit msg, needs amending: "cmd/service: atomitcally ";

Expected the 3rd commit to touch only tests but it also makes modifications in api itself. Can this be squashed with 1st commit?

review: Needs Fixing (code)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :

PASSED: Continuous integration, rev:c99dc8bf44bac3d035016ce58df14abd88a9b7bd
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1561/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1881
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1469/console
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-test-snap/2261
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1458/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1561/rebuild

review: Approve (continuous-integration)
Revision history for this message
Konrad Zapałowicz (kzapalowicz) wrote :

ack

review: Approve (code)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :

FAILED: Continuous integration, rev:225e38ffd3d2276628cca74fcb5aafe5337fc5c8
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1562/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1882/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1470/console
    FAILURE: https://jenkins.canonical.com/system-enablement/job/generic-test-snap/2262/console
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1459/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1562/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :

PASSED: Continuous integration, rev:49bb0599f4a05b84d8cfb0c08f28f624c101bf66
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1564/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-build-snap-worker/1884
    None: https://jenkins.canonical.com/system-enablement/job/generic-update-snap-mp/1474/console
    SUCCESS: https://jenkins.canonical.com/system-enablement/job/generic-test-snap/2265
    None: https://jenkins.canonical.com/system-enablement/job/generic-cleanup-snap/1462/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/system-enablement/job/generic-build-snap/1564/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cmd/service/api.go b/cmd/service/api.go
2index d1228d7..8ebd649 100644
3--- a/cmd/service/api.go
4+++ b/cmd/service/api.go
5@@ -16,11 +16,14 @@
6 package main
7
8 import (
9+ "bytes"
10 "encoding/json"
11 "fmt"
12 "io/ioutil"
13 "net/http"
14 "os"
15+
16+ "github.com/snapcore/snapd/osutil"
17 )
18
19 var api = []*serviceCommand{
20@@ -53,9 +56,9 @@ func getConfiguration(c *serviceCommand, writer http.ResponseWriter, request *ht
21 }
22
23 func postConfiguration(c *serviceCommand, writer http.ResponseWriter, request *http.Request) {
24- path := getConfigOnPath(os.Getenv("SNAP_DATA"))
25+ configPath := getConfigOnPath(os.Getenv("SNAP_DATA"))
26 config := make(map[string]interface{})
27- if readConfiguration([]string{path}, config) != nil {
28+ if readConfiguration([]string{configPath}, config) != nil {
29 resp := makeErrorResponse(http.StatusInternalServerError,
30 "Failed to read existing configuration file", "internal-error")
31 sendHTTPResponse(writer, resp)
32@@ -68,13 +71,11 @@ func postConfiguration(c *serviceCommand, writer http.ResponseWriter, request *h
33 return
34 }
35
36- file, err := os.Create(path)
37- if err != nil {
38- resp := makeErrorResponse(http.StatusInternalServerError, "Can't write configuration file", "internal-error")
39+ if request.Body == nil {
40+ resp := makeErrorResponse(http.StatusInternalServerError, "Error reading the request body", "internal-error")
41 sendHTTPResponse(writer, resp)
42 return
43 }
44- defer file.Close()
45
46 body, err := ioutil.ReadAll(request.Body)
47 if err != nil {
48@@ -84,7 +85,7 @@ func postConfiguration(c *serviceCommand, writer http.ResponseWriter, request *h
49 }
50
51 var items map[string]interface{}
52- if err = json.Unmarshal(body, &items); err != nil {
53+ if err := json.Unmarshal(body, &items); err != nil {
54 resp := makeErrorResponse(http.StatusInternalServerError, "Malformed request", "internal-error")
55 sendHTTPResponse(writer, resp)
56 return
57@@ -100,10 +101,19 @@ func postConfiguration(c *serviceCommand, writer http.ResponseWriter, request *h
58 config[key] = value
59 }
60
61+ var b bytes.Buffer
62 for key, value := range config {
63 key = convertKeyToStorageFormat(key)
64 value = escapeTextForShell(value)
65- file.WriteString(fmt.Sprintf("%s=%s\n", key, value))
66+ b.Write([]byte(fmt.Sprintf("%s=%s\n", key, value)))
67+ }
68+
69+ // Always use an atomic write for the configuration file to ensure
70+ // it's state is always persistent and kept in error cases.
71+ if err := osutil.AtomicWriteFile(configPath, b.Bytes(), 0644, osutil.AtomicWriteFlags(0)); err != nil {
72+ resp := makeErrorResponse(http.StatusInternalServerError, "Can't write configuration file", "internal-error")
73+ sendHTTPResponse(writer, resp)
74+ return
75 }
76
77 if err := restartAccessPoint(c); err != nil {
78diff --git a/cmd/service/api_test.go b/cmd/service/api_test.go
79index be7149d..29ac81b 100644
80--- a/cmd/service/api_test.go
81+++ b/cmd/service/api_test.go
82@@ -126,7 +126,7 @@ func (s *S) TestNoDefaultConfiguration(c *check.C) {
83 os.Setenv("SNAP", oldsnap)
84 }
85
86-func (s *S) TestWriteError(c *check.C) {
87+func (s *S) TestNoRequestBody(c *check.C) {
88 // Test a non writable path:
89 os.Setenv("SNAP_DATA", "/nodir")
90
91@@ -157,7 +157,7 @@ func (s *S) TestWriteError(c *check.C) {
92 c.Assert(resp.StatusCode, check.Equals, http.StatusInternalServerError)
93 c.Assert(resp.Type, check.Equals, "error")
94 c.Assert(resp.Result["kind"], check.Equals, "internal-error")
95- c.Assert(resp.Result["message"], check.Equals, "Can't write configuration file")
96+ c.Assert(resp.Result["message"], check.Equals, "Error reading the request body")
97 }
98
99 func (s *S) TestInvalidJSON(c *check.C) {
100diff --git a/spread.yaml b/spread.yaml
101index 7836458..3a743c0 100644
102--- a/spread.yaml
103+++ b/spread.yaml
104@@ -63,4 +63,14 @@ suites:
105 prepare-each: |
106 . $TESTSLIB/prepare-each.sh
107 restore-each: |
108+ . $TESTSLIB/restore-each.sh
109+ tests/regression/:
110+ summary: Regression tests for the wifi-ap snap
111+ systems:
112+ - ubuntu-core-16
113+ prepare: |
114+ . $TESTSLIB/prepare.sh
115+ prepare-each: |
116+ . $TESTSLIB/prepare-each.sh
117+ restore-each: |
118 . $TESTSLIB/restore-each.sh
119diff --git a/tests/regression/lp-1694486/task.yaml b/tests/regression/lp-1694486/task.yaml
120new file mode 100644
121index 0000000..0241050
122--- /dev/null
123+++ b/tests/regression/lp-1694486/task.yaml
124@@ -0,0 +1,19 @@
125+summary: Verify AP configuration is written atomically
126+
127+details: |
128+ Previous versions of the wifi-ap snap didn't write out the state
129+ of the AP configuration file atomically so that we ended in a
130+ state without any of the previously set configuration items when
131+ an error occured.
132+
133+ See https://bugs.launchpad.net/snappy-hwe-snaps/+bug/1694486 for
134+ more details.
135+
136+execute: |
137+ wifi-ap.config set wifi.ssid=Foo
138+ test "$(wifi-ap.config get wifi.ssid)" = Foo
139+ ! wifi-ap.config set ssid=Bar
140+ # Before LP #1694486 got fixed the value for the wifi.ssid item has
141+ # been reset to the default value now, which is 'Ubuntu', because
142+ # we supplied an invalid configuration key 'ssid' above.
143+ test "$(wifi-ap.config get wifi.ssid)" = Foo
144\ No newline at end of file

Subscribers

People subscribed via source and target branches

to all changes: