Skip to content

Commit

Permalink
gateway: simplify defaults for policy file config (#3941)
Browse files Browse the repository at this point in the history
The interpretation of the configured `ip_routing_policy_file`
configuration option was:
- if the value is identical to the default file path and the file does
  not exist, ignore it and return a default routing policy
- otherwise, read the file at the specified location (and fail if it
  doesn't exist)

Not only does this seem a bit convoluted, but it can also fail in
interesting ways; when the user does not want to specify an
`ip_routing_policy_file` and there is an error stat-ing the file at the
default location (e.g. no permission to read directory, error while
accessing network file system, etc.), the SIG would refuse to start.

Simplified to only handle the empty value as special default.
The consequence of this is that the configuration files now _must_
specify the `ip_routing_policy` file path if it should be loaded.

Also, removed the default for the `traffic_policy_file` option, making
this option mandatory in the configuration file.

Finally, removed the unused `dispatcher` configuration entry.
  • Loading branch information
matzf authored Dec 4, 2020
1 parent cc7ec7b commit c861333
Show file tree
Hide file tree
Showing 5 changed files with 12 additions and 60 deletions.
5 changes: 0 additions & 5 deletions go/pkg/gateway/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ type Gateway struct {
CtrlAddr string `toml:"ctrl_addr,omitempty"`
// Data plane address, for frames.
DataAddr string `toml:"data_addr,omitempty"`
// SCION dispatcher path.
Dispatcher string `toml:"dispatcher,omitempty"`
}

func (cfg *Gateway) Validate() error {
Expand All @@ -58,9 +56,6 @@ func (cfg *Gateway) Validate() error {
if cfg.TrafficPolicy == "" {
cfg.TrafficPolicy = DefaultSessionPoliciesFile
}
if cfg.IPRoutingPolicy == "" {
cfg.IPRoutingPolicy = DefaultIPRoutingPolicyFile
}
cfg.CtrlAddr = DefaultAddress(cfg.CtrlAddr, defaultCtrlPort)
cfg.DataAddr = DefaultAddress(cfg.DataAddr, defaultDataPort)
return nil
Expand Down
7 changes: 2 additions & 5 deletions go/pkg/gateway/config/configtest/configtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,14 @@ import (
"github.com/scionproto/scion/go/pkg/gateway/config"
)

func InitGateway(cfg *config.Gateway) {
cfg.Dispatcher = "garbage"
}
func InitGateway(cfg *config.Gateway) {}

func CheckGateway(t *testing.T, cfg *config.Gateway) {
assert.Equal(t, "gateway", cfg.ID)
assert.Equal(t, config.DefaultSessionPoliciesFile, cfg.TrafficPolicy)
assert.Equal(t, config.DefaultIPRoutingPolicyFile, cfg.IPRoutingPolicy)
assert.Empty(t, cfg.IPRoutingPolicy)
assert.Equal(t, config.DefaultCtrlAddr, cfg.CtrlAddr)
assert.Equal(t, config.DefaultDataAddr, cfg.DataAddr)
assert.Empty(t, cfg.Dispatcher)
}

func InitTunnel(cfg *config.Tunnel) {}
Expand Down
24 changes: 4 additions & 20 deletions go/pkg/gateway/config/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
package config

import (
"os"

"github.com/scionproto/scion/go/lib/log"
"github.com/scionproto/scion/go/lib/serrors"
"github.com/scionproto/scion/go/pkg/gateway/control"
Expand All @@ -29,7 +27,6 @@ const (
// FIXME(lukedirtwalker): cleanup traffic policy and use "session.policy"
// instead.
DefaultSessionPoliciesFile = "/share/conf/traffic.policy"
DefaultIPRoutingPolicyFile = "/share/conf/ip_routing.policy"
)

// Publisher publishes new configurations.
Expand Down Expand Up @@ -71,9 +68,6 @@ func (l *Loader) validate() error {
if l.SessionPoliciesFile == "" {
return serrors.New("SessionPoliciesFile must be set")
}
if l.RoutingPolicyFile == "" {
return serrors.New("RoutingPolicyFile must be set")
}
if l.Publisher == nil {
return serrors.New("Publisher must be set")
}
Expand Down Expand Up @@ -120,21 +114,11 @@ func (l *Loader) loadFiles() (control.SessionPolicies, *routing.Policy, error) {
}

func (l *Loader) loadRoutingPolicy() (*routing.Policy, error) {
path := l.RoutingPolicyFile
if path == DefaultIPRoutingPolicyFile {
// for a default file that doesn't exist return a default routing policy
// that rejects everything.
_, err := os.Stat(path)
switch {
case err == nil:
case os.IsNotExist(err):
return &routing.Policy{DefaultAction: routing.Reject}, nil
default:
return nil, serrors.WrapStr("accessing default routing policy file", err,
"file", path)
}
if l.RoutingPolicyFile == "" {
// return a default routing policy that rejects everything.
return &routing.Policy{DefaultAction: routing.Reject}, nil
}
rp, err := routing.LoadPolicy(path)
rp, err := routing.LoadPolicy(l.RoutingPolicyFile)
if err != nil {
return nil, err
}
Expand Down
17 changes: 1 addition & 16 deletions go/pkg/gateway/config/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,21 +117,6 @@ func TestLoaderRun(t *testing.T) {
}()
xtest.AssertReadReturnsBefore(t, doneCh, time.Second)
},
"missing routing policy file fails": func(t *testing.T, ctrl *gomock.Controller) {
loader := &config.Loader{
SessionPoliciesFile: "session.policy",
Publisher: mock_config.NewMockPublisher(ctrl),
Trigger: make(chan struct{}),
SessionPolicyParser: mock_control.NewMockSessionPolicyParser(ctrl),
}
doneCh := make(chan struct{})
go func() {
defer close(doneCh)
err := loader.Run()
assert.Error(t, err)
}()
xtest.AssertReadReturnsBefore(t, doneCh, time.Second)
},
"close before run immediately returns": func(t *testing.T, ctrl *gomock.Controller) {
loader := &config.Loader{
SessionPoliciesFile: "session.policy",
Expand Down Expand Up @@ -232,7 +217,7 @@ func TestLoaderRun(t *testing.T) {
logger.EXPECT().Info(gomock.Any(), gomock.Any())
loader := &config.Loader{
SessionPoliciesFile: spFile,
RoutingPolicyFile: config.DefaultIPRoutingPolicyFile,
RoutingPolicyFile: "",
Publisher: publisher,
Trigger: trigger,
SessionPolicyParser: parser,
Expand Down
19 changes: 5 additions & 14 deletions go/pkg/gateway/config/sample.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,18 @@ const gatewaySample = `
# ID of the gateway (default "gateway")
id = "gateway"
# The traffic policy file. This file is required to exist. If empty, the gateway
# attempts to read from the default location.
# (default "/share/conf/traffic.policy")
# The traffic policy file. If not set or empty, the gateway attempts to read the
# policy from the default location. If set, the gateway will read the policy from
# the specified location. If the file does not exist, the gateway will exit with
# an error.
# (default "/share/conf/traffic.policy")
traffic_policy_file = "/share/conf/traffic.policy"
# The IP routing policy file. If not set or empty, the gateway attempts to read
# the policy from the default location. If set, the gateway will read the policy
# from the specified location. If the file is specified but does not exist, the
# gateway will exit with an error. It the file is not specified and no file in the
# default location exists, a default policy that rejects everything is used.
# (default "/share/conf/ip_routing.policy")
ip_routing_policy_file = "/share/conf/ip_routing.policy"
# The IP routing policy file. If set, the gateway will read the policy
# from the specified location. It no file is specified, a default policy
# that rejects all IP prefix announcements is used.
# (default "")
ip_routing_policy_file = ""
# The bind address for control messages. If the host part of the address is
# empty, the gateway infers the address based on the route to the control
Expand All @@ -56,9 +50,6 @@ ctrl_addr = ":30256"
#
# (default ":30056")
data_addr = ":30056"
# Custom SCION dispatcher path (default "")
dispatcher = ""
`

const tunnelSample = `
Expand Down

0 comments on commit c861333

Please sign in to comment.