snap,wrappers: add support for socket activation #3916

Merged
merged 18 commits into from Nov 14, 2017

Conversation

Projects
None yet
8 participants
Contributor

albertodonato commented Sep 14, 2017

Add support for socket activation for snap services.

This allows a new sockets section to be specified in the app to define sockets that can activate the service.
If sockets are defined there, the service is not started immediately, but at the first request.
The current implementation supports the ListenStream and SocketMode systemd directives.

An example of config is the following:

apps:  
  myapp:
    command: mydaemon
    daemon: simple
    sockets:
      sock1:
        listen-stream: 8080
      sock2:
        listen-stream: $SNAP_COMMON/sock2.socket
        socket-mode: 0660

See https://forum.snapcraft.io/t/socket-activation-support/2050 for the discussion on the feature

Contributor

albertodonato commented Sep 14, 2017

Note that snapcraft requires the following change to support the sockets stanza: https://github.com/albertodonato/snapcraft/tree/sockets-config

codecov-io commented Sep 14, 2017

Codecov Report

Merging #3916 into master will increase coverage by 0.03%.
The diff coverage is 82.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3916      +/-   ##
==========================================
+ Coverage   75.72%   75.76%   +0.03%     
==========================================
  Files         437      437              
  Lines       37869    38052     +183     
==========================================
+ Hits        28678    28830     +152     
- Misses       7190     7213      +23     
- Partials     2001     2009       +8
Impacted Files Coverage Δ
snap/info_snap_yaml.go 94.21% <100%> (+0.18%) ⬆️
snap/info.go 80.75% <100%> (ø) ⬆️
snap/validate.go 96.9% <100%> (+1.41%) ⬆️
wrappers/services.go 76.53% <68.86%> (-3.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9429119...b77fdfe. Read the comment docs.

Thanks for the contribution!

Here are some notes about the overall idea and the code details:

@@ -409,6 +409,12 @@ type SlotInfo struct {
Apps map[string]*AppInfo
}
+// SocketInfo provides information on application sockets.
+type SocketInfo struct {
@niemeyer

niemeyer Sep 18, 2017

Contributor

The syntax used is a bit unclear to me. What does it mean to have multiple socket names pointing to such a structure, when for each socket file we need a corresponding service file, and we only have a single service file for each application listed?

Also, we can have multiple ListenStreams for a single socket file, which apparently conflicts with this schema?

I'll ignore this issue for the rest of the review, but we need to understand how these things will work before moving on.

@albertodonato

albertodonato Sep 19, 2017

Contributor

You don't need to have a .service file for each .socket file, if the .socket defines Service=<service name>. This allows having multipel sockets activating one service and controlling them individually.

I'm not sure if you can define multiple ListenStreams in a single file, but even if it possible, is having them separate gives more control (for instance the service itself could disable some sockets under certain conditions)

@niemeyer

niemeyer Sep 19, 2017

Contributor

Yes, multiple ListenStreams are supported, and snapd itself uses that.

That said, you are right. Allowing multiple files makes it more flexible, so +1.

snap/info.go
@@ -409,6 +409,12 @@ type SlotInfo struct {
Apps map[string]*AppInfo
}
+// SocketInfo provides information on application sockets.
+type SocketInfo struct {
+ ListenStream string
@niemeyer

niemeyer Sep 18, 2017

Contributor

The "listen-stream" option must be documented and implemented as supporting both ints and strings, so people can use 8080 as in systemd without being surprised that this breaks down. For Go, there's no need to handle it specially as it will transparently unmarshal the string properly. Python (snapcraft) may need a conditional to handle both cases properly, though. Let's please have a small test ensuring ints work too.

snap/info.go
+// SocketInfo provides information on application sockets.
+type SocketInfo struct {
+ ListenStream string
+ SocketMode string
@niemeyer

niemeyer Sep 18, 2017

Contributor

This should probably be uint32, which is the underlying type of os.FileMode. But definitely an integer, even if a different type, and the content in the yaml file should be an integer too. Octal notation also works in yaml.

snap/info.go
-func (app *AppInfo) ServiceSocketFile() string {
- return filepath.Join(dirs.SnapServicesDir, app.SecurityTag()+".socket")
+// ServiceSocketFiles returns the list of systemd socket file paths for the daemon app.
+func (app *AppInfo) ServiceSocketFiles() []string {
@niemeyer

niemeyer Sep 18, 2017

Contributor

We probably don't need that one. We can easily iterate as:

for name, socket := range app.Sockets {
    fmt.Println(name, socket.File())
}

See below as well.

snap/info.go
+}
+
+// SocketFile returns the systemd socket file path for the specified app socket.
+func (app *AppInfo) SocketFile(name string) string {
@niemeyer

niemeyer Sep 18, 2017

Contributor

This one should be in SocketInfo itself, as a File method. There's an established convention in this specific file that the child types link back to the parent types so that we can use them without having to provide a trail of parents every time. For example, the first field in AppInfo is Snap, and same for PlugInfo, SlotInfo, and HookInfo. So following this strategy we want an App *AppInfo as the first field in SocketInfo so we can hand it around and have methods on it without having to provide the context they're under.

snap/info_snap_yaml.go
@@ -90,6 +92,11 @@ type layoutYaml struct {
Symlink string `yaml:"symlink,omitempty"`
}
+type socketsYaml struct {
+ ListenStream string `yaml:"listen-stream,omitempty"`
+ SocketMode string `yaml:"socket-mode,omitempty"`
@niemeyer

niemeyer Sep 18, 2017

Contributor

Same comment about typing as above.

snap/info_snap_yaml.go
+ for name, data := range yApp.Sockets {
+ var socketMode string
+ if data.SocketMode != "" {
+ if _, err := strconv.ParseUint(data.SocketMode, 8, 32); err != nil {
@niemeyer

niemeyer Sep 18, 2017

Contributor

Details here will change as a consequence.

wrappers/services_test.go
+ c.Check(
+ strings.Contains(
+ string(content1),
+ "[Socket]\nService=snap.hello-snap.svc1.service\nListenStream=sock1.socket\nSocketMode=0666\n\n"),
@niemeyer

niemeyer Sep 18, 2017

Contributor

Is that supported? The manual seems to require a slash as first char to handle it as a path.

@albertodonato

albertodonato Sep 19, 2017

Contributor

Ah yeah, that was just to validate template output. I'll change it to be more realistic

@niemeyer niemeyer changed the title from add support for socket activation in apps to snap,wrappers: add support for socket activation Sep 18, 2017

Thanks for the changes!

Another round:

@@ -409,6 +409,12 @@ type SlotInfo struct {
Apps map[string]*AppInfo
}
+// SocketInfo provides information on application sockets.
+type SocketInfo struct {
@niemeyer

niemeyer Sep 18, 2017

Contributor

The syntax used is a bit unclear to me. What does it mean to have multiple socket names pointing to such a structure, when for each socket file we need a corresponding service file, and we only have a single service file for each application listed?

Also, we can have multiple ListenStreams for a single socket file, which apparently conflicts with this schema?

I'll ignore this issue for the rest of the review, but we need to understand how these things will work before moving on.

@albertodonato

albertodonato Sep 19, 2017

Contributor

You don't need to have a .service file for each .socket file, if the .socket defines Service=<service name>. This allows having multipel sockets activating one service and controlling them individually.

I'm not sure if you can define multiple ListenStreams in a single file, but even if it possible, is having them separate gives more control (for instance the service itself could disable some sockets under certain conditions)

@niemeyer

niemeyer Sep 19, 2017

Contributor

Yes, multiple ListenStreams are supported, and snapd itself uses that.

That said, you are right. Allowing multiple files makes it more flexible, so +1.

@@ -324,6 +335,14 @@ func setAppsFromSnapYaml(y snapYaml, snap *Info) error {
app.Slots[slotName] = slot
slot.Apps[appName] = app
}
+ for name, data := range yApp.Sockets {
@niemeyer

niemeyer Sep 19, 2017

Contributor

The name needs to be validated as this is being used in multiple security sensitive places, including path names.

We can start reusing the ValidateName which validates snap names.

@albertodonato

albertodonato Sep 22, 2017

Contributor

Done (I actually added ValidateAppSocketName with the same logic, but different error message)

snap/info_snap_yaml_test.go
@@ -1336,21 +1336,65 @@ apps:
post-stop-command: post-stop-cmd
restart-condition: on-abnormal
bus-name: busName
+ sockets:
+ sock1:
+ listen-stream: /tmp/sock1.socket
@niemeyer

niemeyer Sep 19, 2017

Contributor

Unfortunately the fact these files are absolute creates a more serious issue that we'll need to find a solution for. We cannot allow snaps to create arbitrary files in arbitrary locations, which is what this is now doing. Note that systemd is running out of any confinement, and will do whatever it's asked to do.

@zyga

zyga Oct 30, 2017

Contributor

What is worse those names will be created on the host and not in the execution environment. The /tmp/sock1.socket won't exist for the application at runtime.

@albertodonato

albertodonato Oct 30, 2017

Contributor

@zyga that's actually addressed in code, I forgot to fix those paths. Done now

snap/info_test.go
+ sock1:
+ listen-stream: s1.socket
+ sock2:
+ listen-stream: s2.socket
@niemeyer

niemeyer Sep 19, 2017

Contributor

Where is this being used? I can't see any tests leveraging that.

@albertodonato

albertodonato Sep 22, 2017

Contributor

Probably leftover from previous code, dropped.

snap/validate.go
@@ -161,6 +161,13 @@ func validateField(name, cont string, whitelist *regexp.Regexp) error {
return nil
}
+func validateAppSocket(name string, socket *SocketInfo) error {
+ if socket.ListenStream == "" {
+ return fmt.Errorf("app socket '%s' has empty ListenStream", name)
@niemeyer

niemeyer Sep 19, 2017

Contributor

("socket %q must define listen-stream", name)

The name verification mentioned earlier may be performed here.

wrappers/services.go
+ if !systemd.IsTimeout(err) {
+ return err
+ }
+ inter.Notify(fmt.Sprintf("%s refused to stop, killing.", serviceName))
@niemeyer

niemeyer Sep 19, 2017

Contributor

I'm not sure this logic makes sense for sockets. There's nothing to term or kill there. I think we can preserve the original logic here without the closure, and use simpler stops for the sockets.

wrappers/services.go
+
+ for _, socket := range app.Sockets {
+ err := stop(filepath.Base(socket.File()))
+ if err != nil {
@niemeyer

niemeyer Sep 19, 2017

Contributor

Let's not return earlier if we cannot stop the sockets for whatever reason. Instead:

  1. Stop all sockets, collect errors
  2. Stop service using fancier logic
  3. Return error from service itself, if it failed to stop
  4. Return first error in socket stops, if any
wrappers/services.go
@@ -141,6 +169,20 @@ func AddSnapServices(s *snap.Info, inter interacter) (err error) {
return err
}
written = append(written, svcFilePath)
+
+ // Generate systemd socket files if needed
+ socketFiles, err := generateSnapSocketFiles(app)
@niemeyer

niemeyer Sep 19, 2017

Contributor

systemd creates the socket files by itself, so we don't need that.

Unfortunately that plus the earlier point that sockets are actually absolute paths creates a more serious issue that we'll need to find a solution for. We cannot allow snaps to create arbitrary files in arbitrary locations, which is what this is now doing.

wrappers/services.go
@@ -288,3 +342,61 @@ WantedBy={{.ServicesTarget}}
return templateOut.Bytes()
}
+
+func genSocketFile(appInfo *snap.AppInfo, socketName string) []byte {
@niemeyer

niemeyer Sep 19, 2017

Contributor

Let's please call this one genServiceSocketFile, to make it slightly more clear that this is not in fact an actual socket.

wrappers/services.go
+[Socket]
+Service={{.ServiceFileName}}
+ListenStream={{.SocketInfo.ListenStream}}
+{{if .SocketInfo.SocketMode}}SocketMode={{.SocketInfo.SocketMode | printf "%04o"}}{{end}}
@niemeyer

niemeyer Sep 19, 2017

Contributor

This also needs to include FileDescriptorName, which is pretty much the only real reason to have a high-level name for those.

+ return templateOut.Bytes()
+}
+
+func generateSnapSocketFiles(app *snap.AppInfo) (*map[string][]byte, error) {
@niemeyer

niemeyer Sep 19, 2017

Contributor

There's pretty much never a reason to have a pointer to a map in Go.

That said, we can drop this whole function. The code will be simpler if we call genServiceSocketFile in the exact spot where we need it above.

+}
+
+func generateSnapSocketFiles(app *snap.AppInfo) (*map[string][]byte, error) {
+ if err := snap.ValidateApp(app); err != nil {
@niemeyer

niemeyer Sep 19, 2017

Contributor

Way too late for validation. This should be validated right at the entrance, in the same place we validate everything else about those structures. We should never have in memory an invalid value.

@albertodonato

albertodonato Sep 22, 2017

Contributor

I followed the pattern of generateSnapServiceFile, which also calls ValidateApp

@albertodonato

albertodonato Sep 22, 2017

Contributor

I wonder if I should pull the Validate() call out of generateSnapServiceFile() and put it directly in AddSnapServices()

@niemeyer

niemeyer Sep 22, 2017

Contributor

Okay, it's fine to leave it that way then as I may be missing something, but in theory this should be unnecessary in both cases as the app instance here should definitely already have been validated if it is in memory and reached this stage. Ideally we never want instances that are invalid walking around in the process.

Contributor

albertodonato commented Oct 10, 2017

This should be ready for another round of review.

I've implemented review comments as well as

  • validating that listen-stream is either a (valid) port or 127.0.0.1:port, [::]:port or [::1]:port, a path under $SNAP_DATA or $SNAP_COMMON (also allowing the use of these prefixes) or an abstract socket starting with @snap.<snap_name>.
  • require that the network-bind plug is specified if sockets are defined
+ command: svc
+ sockets:
+ sock:
+ listen-stream: 8080
@kyrofa

kyrofa Oct 13, 2017

Member

Would it be worth validating that, in such a case as this, the app has at least the network-bind plug? Otherwise we know right here that it will fail, right?

@albertodonato

albertodonato Oct 13, 2017

Contributor

This is indeed required and validated, see 8d91814

@kyrofa

kyrofa Oct 13, 2017

Member

You're on top of things @albertodonato!

+ app.Sockets[name] = &SocketInfo{
+ App: app,
+ Name: name,
+ ListenStream: data.ListenStream,
@kyrofa

kyrofa Oct 13, 2017

Member

Note that systemd actually supports multiple listen streams, to quote:

These options may be specified more than once, in which case incoming traffic on any of the sockets will trigger service activation, and all listed sockets will be passed to the service, regardless of whether there is incoming traffic on them or not. If the empty string is assigned to any of these options, the list of addresses to listen on is reset, all prior uses of any of these options will have no effect.

Should we actually be supporting a list of listen-streams, then?

@albertodonato

albertodonato Oct 13, 2017

Contributor

This was initially suggested by @niemeyer too, but it was agreed to have a .socket file for each socket the application wants to listen on.

@kyrofa

kyrofa Oct 13, 2017

Member

If I want to declare an app that is activated by multiple sockets, what does that YAML look like, then?

@kyrofa

kyrofa Oct 13, 2017

Member

Oh, multiple socket names I suppose.

@albertodonato

albertodonato Oct 16, 2017

Contributor

Correct, you can use multiple entries

Contributor

niemeyer commented Oct 17, 2017

@albertodonato Heya! Just getting back to this after a few weeks sprinting.. What's the status here? Does it include everything we agree, and are tests supposed to be passing now?

Contributor

albertodonato commented Oct 17, 2017

@niemeyer hi! yes, it should have everything that was discussed around format and validation.

WRT tests, I'm not sure what's failing right now. it seems like there was some issue with CI before

Collaborator

mvo5 commented Oct 18, 2017

The tests failed because: "snap/validate.go:303:11: "activatin" is a misspelling of "activation"" - I fixed this typo, lets see if tests are more happy now :)

Contributor

albertodonato commented Oct 18, 2017

argh, thanks @mvo5

Thanks for the changes.

Probably the last round:

snap/validate.go
+func ValidateAppSocketName(name string) error {
+ valid := validSnapName.MatchString(name)
+ if !valid {
+ return fmt.Errorf("invalid app socket name: %q", name)
@niemeyer

niemeyer Oct 24, 2017

Contributor

s/app socket/socket/

(test too)

snap/validate.go
+ }
+}
+
+func validateAppSocketListenAddressPath(socket *SocketInfo, fieldName string, path string) error {
@niemeyer

niemeyer Oct 24, 2017

Contributor

We must ensure a clean path before we can go forward here, otherwise people might use .. to get out of the specified locations. Checking that path == filepath.Clean(path) should do the job.

snap/validate.go
+ "socket %q has invalid %q: only $SNAP_DATA and $SNAP_COMMON prefixes are allowed", socket.Name, fieldName)
+ }
+
+ if path[0] == '/' {
@niemeyer

niemeyer Oct 24, 2017

Contributor

We really want the variables here instead of the absolute paths, since we're introducing this just now and there's apparently no good reason to hardcode those paths.

snap/validate.go
+ // Socket activation requires the "network-bind" plug
+ if len(app.Sockets) > 0 {
+ if _, ok := app.Plugs["network-bind"]; !ok {
+ return fmt.Errorf(`app with sockets must declare the "network-bind" plug`)
@niemeyer

niemeyer Oct 24, 2017

Contributor

Let's please word it as:

"network-bind" interface plug is required when sockets are used

+ Sockets: map[string]*SocketInfo{
+ "sock": socket,
+ },
+ }
@niemeyer

niemeyer Oct 24, 2017

Contributor

Can we please have this logic in a function that returns the app, and then used it on every function below? Each function can tune the good base as necessary for its own needs.

@albertodonato

albertodonato Oct 24, 2017

Contributor

definitely nicer, done

+ return err
+ }
+ for path, content := range *socketFiles {
+ os.MkdirAll(filepath.Dir(path), 0755)
@niemeyer

niemeyer Oct 24, 2017

Contributor

Isn't the path generated by systemd?

@albertodonato

albertodonato Oct 24, 2017

Contributor

are you talking about the .socket file or the directory contaning it?
We probably don't need the os.MkdirAll indeed, since /etc/systemd/system should always exist

@chipaca

chipaca Nov 7, 2017

Member

and if it doesn't exist, /etc/systemd is readonly on core

LGTM, thanks for the changes.

We need another review here. Will poke someone today.

@jdstrand If you could have a look at this as well it'd be nice. Security context is that this touches a configuration option of systemd, which is outside the sandbox.

No need to block on @jdstrand's review, though. We still have a few weeks before this will hit stable, so the review at any point before that is fine.

Contributor

albertodonato commented Oct 25, 2017

thanks @niemeyer !

@niemeyer niemeyer requested a review from chipaca Oct 25, 2017

Contributor

niemeyer commented Oct 25, 2017

We'll have a hand from @chipaca. He's recently tweaked quite a few things around that area, so his view will be welcome here.

Please add a spread test for this.

snap/info_snap_yaml_test.go
@@ -1336,21 +1336,65 @@ apps:
post-stop-command: post-stop-cmd
restart-condition: on-abnormal
bus-name: busName
+ sockets:
+ sock1:
+ listen-stream: /tmp/sock1.socket
@niemeyer

niemeyer Sep 19, 2017

Contributor

Unfortunately the fact these files are absolute creates a more serious issue that we'll need to find a solution for. We cannot allow snaps to create arbitrary files in arbitrary locations, which is what this is now doing. Note that systemd is running out of any confinement, and will do whatever it's asked to do.

@zyga

zyga Oct 30, 2017

Contributor

What is worse those names will be created on the host and not in the execution environment. The /tmp/sock1.socket won't exist for the application at runtime.

@albertodonato

albertodonato Oct 30, 2017

Contributor

@zyga that's actually addressed in code, I forgot to fix those paths. Done now

I haven't finished, but i need to stop for now

snap/info_snap_yaml.go
@@ -285,6 +292,10 @@ func setAppsFromSnapYaml(y snapYaml, snap *Info) error {
if len(y.Slots) > 0 || len(yApp.SlotNames) > 0 {
app.Slots = make(map[string]*SlotInfo)
}
+ if len(yApp.Sockets) > 0 {
+ app.Sockets = make(map[string]*SocketInfo)
@chipaca

chipaca Oct 30, 2017

Member

given that you know the size, you might as well make it with the size

snap/validate.go
@@ -71,6 +73,92 @@ func ValidateAlias(alias string) error {
return nil
}
+// ValidateAppSocketName checks if a string ca be used as a name for a socket
+// (for socket activation).
+func ValidateAppSocketName(name string) error {
@chipaca

chipaca Oct 30, 2017

Member

why is this exported?

@albertodonato

albertodonato Nov 6, 2017

Contributor

I noticed most Validate* functions are exported. Also, it makes it possible to unittest the function in isolation

@chipaca

chipaca Nov 7, 2017

Member

I don't think it should be exported unless it's expected that people would call this from outside of this package. For testing, you make private things exported via export_test.go

snap/validate.go
+// ValidateAppSocketName checks if a string ca be used as a name for a socket
+// (for socket activation).
+func ValidateAppSocketName(name string) error {
+ valid := validSnapName.MatchString(name)
@chipaca

chipaca Oct 30, 2017

Member

can you add a comment to the definition of validSnapName to the effect that we're also using it to validate socket names?

snap/validate.go
+}
+
+// ValidateAppSocketListenAddress checks that the value of socket addresses.
+func ValidateAppSocketListenAddress(socket *SocketInfo, fieldName string, address string) error {
@chipaca

chipaca Oct 30, 2017

Member

why is this exported?

snap/validate.go
+func validateAppSocketListenAddressPath(socket *SocketInfo, fieldName string, path string) error {
+ if path != filepath.Clean(path) {
+ return fmt.Errorf(
+ `socket %q has invalid %q: paths must not include "." or ".."`, socket.Name, fieldName)
@chipaca

chipaca Oct 30, 2017

Member

note that /fo//bar/ (and even just /foo/) will fail this check, with an error message that has no connection to the source of the error. How about

if clean := filepath.Clean(path); clean != path {
    return fmt.Errorf("socket %q has invalid %q: %q should be written as %q", socket.Name, fieldName, path, clean)
}

(or something like that)

@chipaca

chipaca Oct 30, 2017

Member

additionally this needs to check that the resulting socket path isn't too long

@albertodonato

albertodonato Nov 6, 2017

Contributor

is the path lenght limit available somewhere in the library?

snap/validate.go
+ `socket %q has invalid %q: paths must not include "." or ".."`, socket.Name, fieldName)
+ }
+
+ if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/")) {
@chipaca

chipaca Oct 30, 2017

Member

Can we also support ${SNAP_DATA} and ${SNAP_COMMON}? (this ties in with my suggestion about os.Expand)

@chipaca

chipaca Oct 30, 2017

Member

was there a reason not to allow things to be relative, so that if I just say socket-stream: potato I get $SNAP_DATA/potato?

@chipaca

chipaca Oct 30, 2017

Member

also: any reason not to support /run/snap.${SNAP}.foo?

@albertodonato

albertodonato Nov 3, 2017

Contributor

I think allowing relative path would be a bit confusing, it's not explicit what's the base path in that case ( $SNAP_DATA vs $SNAP_COMMON or another dir)

@albertodonato

albertodonato Nov 6, 2017

Contributor

Also, the point here is to require to have the path start with $SNAP_DATA or $SNAP_COMMON, so that there are no hardcoded paths (like /var/snap/<mysnap>/common).
I can add ${SNAP_DATA} and ${SNAP_COMMON} to the list of allowed prefixes, but using os.Expand makes the check more complicated and the error message possibly confusing

+
+ if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/")) {
+ return fmt.Errorf(
+ "socket %q has invalid %q: only $SNAP_DATA and $SNAP_COMMON prefixes are allowed", socket.Name, fieldName)
@chipaca

chipaca Oct 30, 2017

Member

if we do choose to support the ${} variants, I'd leave this message as is

snap/validate.go
+func validateAppSocketListenAddressAbstractSocket(socket *SocketInfo, fieldName string, path string) error {
+ prefix := "@snap." + socket.App.Snap.Name()
+ if !strings.HasPrefix(path, prefix) {
+ return fmt.Errorf("socket %q path for %q must be prefixed with %q", socket.Name, fieldName, prefix)
@chipaca

chipaca Oct 30, 2017

Member

this validator as written has a bug, in that the prefix should probably have a delimiter after the snap name (otherwise a snap fo can mess with the sockets of a snap foo).

Also: why? Wouldn't it be better to support it being just @ or somesuch, and we fill in the snap name?

on the one hand we're forcing users to write things that might change without their control (we're wanting to change service names from snap.yadda to yadda.snap, for example), and even the snap name can change between them creating the snap and it getting used.

on the other hand, even if these things were in their control, if we already know how it should start, why force the user to write it? We already know the snap name, the app name, and the socket name; just by saying @ we can infer @snap.${SNAP}.{SOCKET} (or @yadda -> @snap.${SNAP}.yadda).

On the other other hand, this could result in a socket name that's too long, and it doesn't handle that.

@albertodonato

albertodonato Nov 6, 2017

Contributor

Ah, yes, the prefix has a missing . at the end. I'll fix that.

As for the prefix itself, I'd say it's the same reasoning as for the unix path prefix. The user has to know the actual path anyway, since the point of socket activation is to expose your service, so consumers will have to know the path.
Also, for someone reading the snap[craft].yaml, the partial name would be confusing if he doesn't know how it's internally prefixed.

@niemeyer

niemeyer Nov 10, 2017

Contributor

Indeed, plus we don't know if we'll want to keep those restrictions forever. Explicit sounds better here.

snap/validate.go
+ return nil
+}
+
+func validateAppSocketListenAddressNetAddress(socket *SocketInfo, fieldName string, address string) error {
@chipaca

chipaca Oct 30, 2017

Member

I have a problem with the whole validation code as written: it takes me more time to read the function name, and then find it in the sea of other functions that are all similarly named and often a one-line check, than it takes me to read the function itself. Compounding things the functions all take a bunch of arguments, all slightly different, and only some of them are involved in the validation itself; the rest are for generating the error message.

What's the reasoning for doing it this way? It's really hard to follow.

I can understand the need of having the validators separate, but surely it'd be easier and cleaner to have, say, isValidNetAddress(string) bool?

@albertodonato

albertodonato Nov 6, 2017

Contributor

The naming is following the style of other validators in this module. I agree names are quite verbose, and in this specific case the validator is pretty trivial, but isValidNetAddress is not a good name, since it'd need to be isValidNetAddressForListenAddress or something similar (as it's specific to socket activation).

I'm open to better names and I'm fine with inlining all the sub-validators for ListenAddress if that's easier to follow

@chipaca

chipaca Nov 7, 2017

Member

at least for me it would be. I'll ask somebody else to chip in here.

@niemeyer

niemeyer Nov 10, 2017

Contributor

@chipaca is right.. some of these names are a bit over the top. That said, @albertodonato is also right. We have a local convention for these names which would be nice to respect.

Here is a suggested way forward:

old new
ValidateAppSocketName validateSocketName
ValidateAppSocketListenAddress validateSocketAddr
validateAppSocketListenAddressPath validateSocketAddrPath
validateAppSocketListenAddressAbstractSocket validateSocketAddrAbstract
validateAppSocketListenAddressNetAddress validateSocketAddrNet
validateAppSocketListenAddressNetAddressAddress validateSocketAddrNetHost
validateAppSocketListenAddressNetAddressPort validateSocketAddrNetPort
@niemeyer

niemeyer Nov 10, 2017

Contributor

NetHost and NetPort might indeed be inlined, btw.

@albertodonato

albertodonato Nov 12, 2017

Contributor

Renamed as suggested. I didn't inline validateSocketAddrNetHost and validateSocketAddrNetPort, since the latter is used in two places, and I'd leave the first one for symmetry.

snap/validate.go
+}
+
+func validateAppSocketListenAddressNetAddressAddress(socket *SocketInfo, fieldName string, address string) error {
+ for _, validAddress := range []string{"127.0.0.1", "[::1]", "[::]"} {
@chipaca

chipaca Oct 30, 2017

Member

not "localhost"?

@albertodonato

albertodonato Nov 3, 2017

Contributor

Is that supported by systemd? the manpage only mentions IP addresses

@chipaca

chipaca Nov 7, 2017

Member

no! sorry

@@ -149,6 +237,14 @@ func validateField(name, cont string, whitelist *regexp.Regexp) error {
return nil
}
+func validateAppSocket(socket *SocketInfo) error {
@chipaca

chipaca Oct 30, 2017

Member

can you make this one be func (*SocketInfo) Validate() error?

@albertodonato

albertodonato Nov 6, 2017

Contributor

I can, but the whole point of this module is to validate structs. The same reasoning could apply to Info.Validate()

@chipaca

chipaca Nov 7, 2017

Member

yes, and i'll be doing that refactor some day. I guess I can do this one at that point as well.

snap/validate.go
+ return err
+ }
+
+ return ValidateAppSocketListenAddress(socket, "listen-stream", socket.ListenStream)
@chipaca

chipaca Oct 30, 2017

Member

what's the driver for passing "listen-stream" in like this? It's making everything more complex and I'm not seeing the upside.

@albertodonato

albertodonato Nov 3, 2017

Contributor

The field is passed in so that the error message can mention it. The idea is to reuse the code for other socket types (e.g. "listen-datagram")

snap/validate.go
@@ -183,6 +279,21 @@ func ValidateApp(app *AppInfo) error {
return err
}
}
+
+ // Socket activatin requires the "network-bind" plug
@chipaca

chipaca Oct 30, 2017

Member

activation

snap/validate_test.go
+ for _, validAddress := range validListenAddresses {
+ socket.ListenStream = validAddress
+ err := ValidateApp(app)
+ c.Assert(err, IsNil)
@chipaca

chipaca Oct 30, 2017

Member

in general use Check instead of Assert to to check things; Assert is to check for things that make any other tests pointless (e.g. to check an error value is nil before checking the value returned is as expected).

When doing things in a loop like here, use Check and Commentf: e.g. c.Check(err, IsNil, Commentf(validAddress)) to continue testing and have the context of the failing check in the output

+ for _, invalidAddress := range invalidListenAddresses {
+ socket.ListenStream = invalidAddress
+ err := ValidateApp(app)
+ c.Assert(
@chipaca

chipaca Oct 30, 2017

Member

please don't wrap lines like this

+ return &socketFiles, nil
+}
+
+func renderListenStream(socket *snap.SocketInfo) string {
@chipaca

chipaca Oct 30, 2017

Member

any reason not to use os.Expand?

@chipaca

chipaca Oct 30, 2017

Member

also, where are the tests for renderListenStream?

@albertodonato

albertodonato Nov 6, 2017

Contributor

TestAddSnapSocketFiles tests that

this is so close! thank you for working on it.

just a few remaining nits (new and old), and open questions

snap/validate.go
@@ -71,6 +73,92 @@ func ValidateAlias(alias string) error {
return nil
}
+// ValidateAppSocketName checks if a string ca be used as a name for a socket
+// (for socket activation).
+func ValidateAppSocketName(name string) error {
@chipaca

chipaca Oct 30, 2017

Member

why is this exported?

@albertodonato

albertodonato Nov 6, 2017

Contributor

I noticed most Validate* functions are exported. Also, it makes it possible to unittest the function in isolation

@chipaca

chipaca Nov 7, 2017

Member

I don't think it should be exported unless it's expected that people would call this from outside of this package. For testing, you make private things exported via export_test.go

@@ -183,6 +280,21 @@ func ValidateApp(app *AppInfo) error {
return err
}
}
+
+ // Socket activation requires the "network-bind" plug
@chipaca

chipaca Nov 7, 2017

Member

is this a request from security, our own decision wrt policy, or ...?

@albertodonato

albertodonato Nov 7, 2017

Contributor

It came from discussions (not a request from security). I think it makes sense, if an app can't bind the network it shouldn't be able to be socket-activated

@niemeyer

niemeyer Nov 10, 2017

Contributor

To be clear, the issue isn't that it shouldn't be able to. The issue is that it cannot, right?

@chipaca

chipaca Nov 10, 2017

Member

it should be able to: the one doing the binding is systemd, which is not confined by us

@niemeyer

niemeyer Nov 10, 2017

Contributor

Yes, but there are other socket operations that will be performed in the handed-off file descriptor I suppose? Do those generally overlap with the set defined in the network-bind interface?

Another question is the intent of the plug. Right now one can tell whether a snap will be listening for traffic in the local machine based on the plugs. If we don't force that plug to be present, snaps will be able to listen without an obvious hint or the ability to prevent that. That said, the mere presence of the plug in the snap isn't enough to make it conform. We'd need to force it to be connected as well for the activation to take place.

This might be an argument to drop the plug and just let it go. On the other hand, if we don't force its presence today, we'll not be able to change exactly that behavior later, because we'd break compatibility. If we force the plug to be present, we can change that behavior and allow auto-connection to take place, and we'd know for sure that any snaps that ever existed with socket activation will necessarily continue working.

So, on that basis, I'd prefer to merge the code with the enforcement of the plug present as it is now, and if we change our minds later we can drop that logic.

+ return err
+ }
+ for path, content := range *socketFiles {
+ os.MkdirAll(filepath.Dir(path), 0755)
@niemeyer

niemeyer Oct 24, 2017

Contributor

Isn't the path generated by systemd?

@albertodonato

albertodonato Oct 24, 2017

Contributor

are you talking about the .socket file or the directory contaning it?
We probably don't need the os.MkdirAll indeed, since /etc/systemd/system should always exist

@chipaca

chipaca Nov 7, 2017

Member

and if it doesn't exist, /etc/systemd is readonly on core

Contributor

albertodonato commented Nov 12, 2017

I think I've addressed all comments (modulo function inlining, see comment above), it should be ready for another pass. Thanks for the reviews!

albertodonato added some commits Sep 11, 2017

add support for socket activation in apps
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
define SocketMode as os.Filemode
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
add test for listen-stream passed as integer in YAML
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
add Name and File() to SocketInfo
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
valiate socket name
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
rework stopService
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
rename genSocketFile to getServiceSocketName, include FileDescriptorName
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
validate listen-stream
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
require network-bind plug if socket activation is used
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
lints
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
fix tests
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
address review comments
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
only allow $SNAP_COMMON/$SNAP_DATA as listen-stream prefixes
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
fix listen-stream paths in tests
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
add spread test
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
address review comments.
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
make map of known size
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>
Don't export validation functions
Signed-off-by: Alberto Donato <alberto.donato@gmail.com>

@chipaca chipaca merged commit 960abc1 into snapcore:master Nov 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@albertodonato albertodonato deleted the albertodonato:socket-activation branch Nov 14, 2017

I was asked to verify this for inputs since we are generating systemd socket files from yaml. I only reviewed this PR from that perspective and feel that the input validation is sufficient.

I did add several comments for additional tests that should be added. It would be nice if these could come in a subsequent PR.

+ app := createSampleApp()
+ app.Sockets["sock"].SocketMode = 0600
+ c.Check(ValidateApp(app), IsNil)
+}
@jdstrand

jdstrand Nov 14, 2017

Contributor

Having a test for a bad SocketMode would be good too.

@albertodonato

albertodonato Nov 15, 2017

Contributor

There's no specific validation for SocketMode. I'm not sure how to make it an invalid number

@jdstrand

jdstrand Nov 15, 2017

Contributor

This is coming through the yaml so I was hoping to see a test that mocked the yaml and ran it through the yaml parser, and verifying the output in the testsuite. Eg, "abc", 0123456789, etc.

@chipaca

chipaca Nov 15, 2017

Member

should we check it's <= 0777?

@albertodonato

albertodonato Nov 15, 2017

Contributor

@jdstrand the case where the value is not an integer would be handled by the json loader, it's not specific to this validation. Ican add a check that it's <=0777 as @chipaca suggests

+ `socket "sock" has invalid "listen-stream": "\$SNAP/../some.path" should be written as "some.path"`)
+}
+
+func (s *ValidateSuite) TestValidateAppSocketsInvalidListenStreamPathRelative(c *C) {
@jdstrand

jdstrand Nov 14, 2017

Contributor

The name of this test doesn't seem to correspond to the tested data. Ie, there are not relative paths.

+ invalidListenAddresses := []string{
+ "@snap.notmysnap.my.socket",
+ "@some.other.name",
+ "@snap.myappiswrong.foo",
@jdstrand

jdstrand Nov 14, 2017

Contributor

We should also test @snap.myapp, @@snap.myapp.foo and @snap.myapp\0.foo

+ app := createSampleApp()
+ invalidListenAddresses := []string{
+ "10.0.1.1:8080",
+ "[fafa::baba]:8080",
@jdstrand

jdstrand Nov 14, 2017

Contributor

Can you add: 127.0.0.1\0:8080 and 127.0.0.1::8080

+ // a number alone is not a name
+ "0", "123",
+ // identifier must be plain ASCII
+ "日本語", "한글", "ру́сский язы́к",
@jdstrand

jdstrand Nov 14, 2017

Contributor

Adding an embedded NUL would be good too: aa-a\0-b

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