From 67e05041b4198e4660b91912b576999565552632 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Thu, 29 Jun 2023 16:34:29 +1000 Subject: [PATCH 01/14] add log forwarding section to README --- README.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/README.md b/README.md index f0ab8e42..06129cc3 100644 --- a/README.md +++ b/README.md @@ -403,6 +403,45 @@ $ pebble run --verbose ... ``` + + ## Container usage Pebble works well as a local service manager, but if running Pebble in a separate container, you can use the exec and file management APIs to coordinate with the remote system over the shared unix socket. From ea6f5336cacd8d94d2498fdff657cf55d1c1d47e Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Thu, 29 Jun 2023 16:51:26 +1000 Subject: [PATCH 02/14] remove LogTargets field from service --- internals/plan/plan.go | 50 +---------------- internals/plan/plan_test.go | 107 +++++------------------------------- 2 files changed, 16 insertions(+), 141 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 76fc1feb..d072dbad 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -89,9 +89,6 @@ type Service struct { BackoffFactor OptionalFloat `yaml:"backoff-factor,omitempty"` BackoffLimit OptionalDuration `yaml:"backoff-limit,omitempty"` KillDelay OptionalDuration `yaml:"kill-delay,omitempty"` - - // Log forwarding - LogTargets []string `yaml:"log-targets,omitempty"` } // Copy returns a deep copy of the service. @@ -120,7 +117,6 @@ func (s *Service) Copy() *Service { copied.OnCheckFailure[k] = v } } - copied.LogTargets = append([]string(nil), s.LogTargets...) return &copied } @@ -188,23 +184,6 @@ func (s *Service) Merge(other *Service) { if other.BackoffLimit.IsSet { s.BackoffLimit = other.BackoffLimit } - s.LogTargets = appendUnique(s.LogTargets, other.LogTargets...) -} - -// appendUnique appends into a the elements from b which are not yet present -// and returns the modified slice. -// TODO: move this function into canonical/x-go/strutil -func appendUnique(a []string, b ...string) []string { -Outer: - for _, bn := range b { - for _, an := range a { - if an == bn { - continue Outer - } - } - a = append(a, bn) - } - return a } // Equal returns true when the two services are equal in value. @@ -275,23 +254,8 @@ func CommandString(base, extra []string) string { } // LogsTo returns true if the logs from s should be forwarded to target t. -// This happens if: -// - t.Selection is "opt-out" or empty, and s.LogTargets is empty; or -// - t.Selection is not "disabled", and s.LogTargets contains t. func (s *Service) LogsTo(t *LogTarget) bool { - if t.Selection == DisabledSelection { - return false - } - if len(s.LogTargets) == 0 { - if t.Selection == UnsetSelection || t.Selection == OptOutSelection { - return true - } - } - for _, targetName := range s.LogTargets { - if targetName == t.Name { - return true - } - } + // TODO: implement return false } @@ -813,18 +777,6 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { } } - // Validate service log targets - for serviceName, service := range combined.Services { - for _, targetName := range service.LogTargets { - _, ok := combined.LogTargets[targetName] - if !ok { - return nil, &FormatError{ - Message: fmt.Sprintf(`unknown log target %q for service %q`, targetName, serviceName), - } - } - } - } - // Ensure combined layers don't have cycles. err := combined.checkCycles() if err != nil { diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 2fe9f6e7..1f3b4c86 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -831,15 +831,10 @@ var planTests = []planTest{{ command: foo override: merge startup: enabled - log-targets: - - tgt1 svc2: command: bar override: merge startup: enabled - log-targets: - - tgt1 - - tgt2 log-targets: tgt1: @@ -861,7 +856,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt1"}, }, "svc2": { Name: "svc2", @@ -871,7 +865,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt1", "tgt2"}, }, }, Checks: map[string]*plan.Check{}, @@ -898,15 +891,10 @@ var planTests = []planTest{{ command: foo override: merge startup: enabled - log-targets: - - tgt1 svc2: command: bar override: merge startup: enabled - log-targets: - - tgt1 - - tgt2 log-targets: tgt1: @@ -922,14 +910,10 @@ var planTests = []planTest{{ svc1: command: foo override: merge - log-targets: - - tgt3 svc2: command: bar override: replace startup: enabled - log-targets: - - tgt3 log-targets: tgt1: @@ -949,18 +933,16 @@ var planTests = []planTest{{ Order: 0, Services: map[string]*plan.Service{ "svc1": { - Name: "svc1", - Command: "foo", - Override: plan.MergeOverride, - Startup: plan.StartupEnabled, - LogTargets: []string{"tgt1"}, + Name: "svc1", + Command: "foo", + Override: plan.MergeOverride, + Startup: plan.StartupEnabled, }, "svc2": { - Name: "svc2", - Command: "bar", - Override: plan.MergeOverride, - Startup: plan.StartupEnabled, - LogTargets: []string{"tgt1", "tgt2"}, + Name: "svc2", + Command: "bar", + Override: plan.MergeOverride, + Startup: plan.StartupEnabled, }, }, Checks: map[string]*plan.Check{}, @@ -983,17 +965,15 @@ var planTests = []planTest{{ Order: 1, Services: map[string]*plan.Service{ "svc1": { - Name: "svc1", - Command: "foo", - Override: plan.MergeOverride, - LogTargets: []string{"tgt3"}, + Name: "svc1", + Command: "foo", + Override: plan.MergeOverride, }, "svc2": { - Name: "svc2", - Command: "bar", - Override: plan.ReplaceOverride, - Startup: plan.StartupEnabled, - LogTargets: []string{"tgt3"}, + Name: "svc2", + Command: "bar", + Override: plan.ReplaceOverride, + Startup: plan.StartupEnabled, }, }, Checks: map[string]*plan.Check{}, @@ -1027,7 +1007,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt1", "tgt3"}, }, "svc2": { Name: "svc2", @@ -1037,7 +1016,6 @@ var planTests = []planTest{{ BackoffDelay: plan.OptionalDuration{Value: defaultBackoffDelay}, BackoffFactor: plan.OptionalFloat{Value: defaultBackoffFactor}, BackoffLimit: plan.OptionalDuration{Value: defaultBackoffLimit}, - LogTargets: []string{"tgt3"}, }, }, Checks: map[string]*plan.Check{}, @@ -1101,22 +1079,6 @@ var planTests = []planTest{{ override: merge selection: foobar `}, -}, { - summary: "Service specifies unknown log target", - error: `unknown log target "tgt2" for service "svc1"`, - input: []string{` - services: - svc1: - command: foo - override: merge - log-targets: - - tgt2 - log-targets: - tgt1: - type: loki - location: http://10.1.77.196:3100/loki/api/v1/push - override: merge -`}, }} func (s *S) TestParseLayer(c *C) { @@ -1461,42 +1423,3 @@ func (s *S) TestParseCommand(c *C) { c.Assert(extra, DeepEquals, test.cmdArgs) } } - -func (s *S) TestSelectTargets(c *C) { - logTargets := []*plan.LogTarget{ - {Name: "unset", Selection: plan.UnsetSelection}, - {Name: "optout", Selection: plan.OptOutSelection}, - {Name: "optin", Selection: plan.OptInSelection}, - {Name: "disabled", Selection: plan.DisabledSelection}, - } - services := []*plan.Service{ - {Name: "svc1", LogTargets: nil}, - {Name: "svc2", LogTargets: []string{}}, - {Name: "svc3", LogTargets: []string{"unset"}}, - {Name: "svc4", LogTargets: []string{"optout"}}, - {Name: "svc5", LogTargets: []string{"optin"}}, - {Name: "svc6", LogTargets: []string{"disabled"}}, - {Name: "svc7", LogTargets: []string{"unset", "optin", "disabled"}}, - } - - // Use pointers to bools so the test will fail if we forget to set a value - t, f := true, false - expected := map[string]map[string]*bool{ - "svc1": {"unset": &t, "optout": &t, "optin": &f, "disabled": &f}, - "svc2": {"unset": &t, "optout": &t, "optin": &f, "disabled": &f}, - "svc3": {"unset": &t, "optout": &f, "optin": &f, "disabled": &f}, - "svc4": {"unset": &f, "optout": &t, "optin": &f, "disabled": &f}, - "svc5": {"unset": &f, "optout": &f, "optin": &t, "disabled": &f}, - "svc6": {"unset": &f, "optout": &f, "optin": &f, "disabled": &f}, - "svc7": {"unset": &t, "optout": &f, "optin": &t, "disabled": &f}, - } - - for _, service := range services { - for _, target := range logTargets { - exp := expected[service.Name][target.Name] - c.Assert(exp, NotNil, Commentf("no expected value defined for %s.LogsTo(%s)", service.Name, target.Name)) - c.Check(service.LogsTo(target), Equals, *exp, - Commentf("unexpected value for %s.LogsTo(%s)", service.Name, target.Name)) - } - } -} From 9c5c5b0cce14ebdb484483d7ba483d5a4bf5b173 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 11:00:28 +1000 Subject: [PATCH 03/14] remove Selection field from LogTarget --- internals/plan/plan.go | 34 +++++----------------------- internals/plan/plan_test.go | 44 +++++++++++++------------------------ 2 files changed, 20 insertions(+), 58 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index d072dbad..a69ee605 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -477,11 +477,10 @@ func (c *ExecCheck) Merge(other *ExecCheck) { // LogTarget specifies a remote server to forward logs to. type LogTarget struct { - Name string `yaml:"-"` - Type LogTargetType `yaml:"type"` - Location string `yaml:"location"` - Selection Selection `yaml:"selection,omitempty"` - Override Override `yaml:"override,omitempty"` + Name string `yaml:"-"` + Type LogTargetType `yaml:"type"` + Location string `yaml:"location"` + Override Override `yaml:"override,omitempty"` } // LogTargetType defines the protocol to use to forward logs. @@ -493,16 +492,6 @@ const ( UnsetLogTarget LogTargetType = "" ) -// Selection describes which services' logs will be forwarded to this target. -type Selection string - -const ( - OptOutSelection Selection = "opt-out" - OptInSelection Selection = "opt-in" - DisabledSelection Selection = "disabled" - UnsetSelection Selection = "" -) - // Copy returns a deep copy of the log target configuration. func (t *LogTarget) Copy() *LogTarget { copied := *t @@ -517,9 +506,6 @@ func (t *LogTarget) Merge(other *LogTarget) { if other.Location != "" { t.Location = other.Location } - if other.Selection != "" { - t.Selection = other.Selection - } } // FormatError is the error returned when a layer has a format error, such as @@ -760,17 +746,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { } } - switch target.Selection { - case OptOutSelection, OptInSelection, DisabledSelection, UnsetSelection: - // valid, continue - default: - return nil, &FormatError{ - Message: fmt.Sprintf(`log target %q has invalid selection %q, must be %q, %q or %q`, - name, target.Selection, OptOutSelection, OptInSelection, DisabledSelection), - } - } - - if target.Location == "" && target.Selection != DisabledSelection { + if target.Location == "" { return nil, &FormatError{ Message: fmt.Sprintf(`plan must define "location" for log target %q`, name), } diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 1f3b4c86..5f13de82 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -918,11 +918,10 @@ var planTests = []planTest{{ log-targets: tgt1: override: merge - selection: opt-in tgt2: type: syslog + location: foo override: replace - selection: disabled tgt3: type: loki location: http://10.1.77.206:3100/loki/api/v1/push @@ -979,15 +978,14 @@ var planTests = []planTest{{ Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{ "tgt1": { - Name: "tgt1", - Override: plan.MergeOverride, - Selection: plan.OptInSelection, + Name: "tgt1", + Override: plan.MergeOverride, }, "tgt2": { - Name: "tgt2", - Type: plan.SyslogTarget, - Override: plan.ReplaceOverride, - Selection: plan.DisabledSelection, + Name: "tgt2", + Type: plan.SyslogTarget, + Location: "foo", + Override: plan.ReplaceOverride, }, "tgt3": { Name: "tgt3", @@ -1021,17 +1019,16 @@ var planTests = []planTest{{ Checks: map[string]*plan.Check{}, LogTargets: map[string]*plan.LogTarget{ "tgt1": { - Name: "tgt1", - Type: plan.LokiTarget, - Location: "http://10.1.77.196:3100/loki/api/v1/push", - Override: plan.MergeOverride, - Selection: plan.OptInSelection, + Name: "tgt1", + Type: plan.LokiTarget, + Location: "http://10.1.77.196:3100/loki/api/v1/push", + Override: plan.MergeOverride, }, "tgt2": { - Name: "tgt2", - Type: plan.SyslogTarget, - Override: plan.ReplaceOverride, - Selection: plan.DisabledSelection, + Name: "tgt2", + Type: plan.SyslogTarget, + Location: "foo", + Override: plan.ReplaceOverride, }, "tgt3": { Name: "tgt3", @@ -1068,17 +1065,6 @@ var planTests = []planTest{{ location: http://10.1.77.196:3100/loki/api/v1/push override: merge `}, -}, { - summary: "Invalid selection for log target", - error: `log target "tgt1" has invalid selection "foobar", must be "opt-out", "opt-in" or "disabled"`, - input: []string{` - log-targets: - tgt1: - type: loki - location: http://10.1.77.196:3100/loki/api/v1/push - override: merge - selection: foobar -`}, }} func (s *S) TestParseLayer(c *C) { From 0504a05654bab5e504252784c113bda5175cdb0b Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 11:07:11 +1000 Subject: [PATCH 04/14] add Services field to LogTarget --- internals/plan/plan.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index a69ee605..678eba98 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -255,7 +255,21 @@ func CommandString(base, extra []string) string { // LogsTo returns true if the logs from s should be forwarded to target t. func (s *Service) LogsTo(t *LogTarget) bool { - // TODO: implement + // Iterate backwards through t.Services until we find something matching + // s.Name. + for i := len(t.Services) - 1; i >= 0; i++ { + switch t.Services[i] { + case s.Name: + return true + case ("-" + s.Name): + return false + case "all": + return true + case "-all": + return false + } + } + // Nothing matching the service name, so it was not specified. return false } @@ -480,6 +494,7 @@ type LogTarget struct { Name string `yaml:"-"` Type LogTargetType `yaml:"type"` Location string `yaml:"location"` + Services []string `yaml:"services"` Override Override `yaml:"override,omitempty"` } @@ -495,6 +510,7 @@ const ( // Copy returns a deep copy of the log target configuration. func (t *LogTarget) Copy() *LogTarget { copied := *t + copied.Services = append([]string(nil), t.Services...) return &copied } @@ -506,6 +522,7 @@ func (t *LogTarget) Merge(other *LogTarget) { if other.Location != "" { t.Location = other.Location } + t.Services = append(t.Services, other.Services...) } // FormatError is the error returned when a layer has a format error, such as From f2d72dd099055ae2c0aee2754b1030bfde55daa1 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 11:20:51 +1000 Subject: [PATCH 05/14] validate LogTargets.Services --- internals/plan/plan.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 678eba98..5597dd65 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -522,6 +522,8 @@ func (t *LogTarget) Merge(other *LogTarget) { if other.Location != "" { t.Location = other.Location } + // TODO: reduce log targets? + // e.g. [..., all, -svc1] -> [all, -svc1] t.Services = append(t.Services, other.Services...) } @@ -763,7 +765,21 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { } } - if target.Location == "" { + // Validate service names specified in log target + for _, serviceName := range target.Services { + if serviceName == "all" { + continue + } + if _, ok := combined.Services[serviceName]; ok { + continue + } + return nil, &FormatError{ + Message: fmt.Sprintf(`log target %q specifies unknown service %q`, + target.Name, serviceName), + } + } + + if target.Location == "" && len(target.Services) > 0 { return nil, &FormatError{ Message: fmt.Sprintf(`plan must define "location" for log target %q`, name), } From c13a51b107372a832d3dec6b0bc2c3e1c4a7b24c Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 12:49:09 +1000 Subject: [PATCH 06/14] add "services" field to plan tests - fix service name validation to handle `-` --- internals/plan/plan.go | 6 +++++- internals/plan/plan_test.go | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 5597dd65..829990ff 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -767,12 +767,16 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { // Validate service names specified in log target for _, serviceName := range target.Services { - if serviceName == "all" { + if serviceName == "all" || serviceName == "-all" { continue } if _, ok := combined.Services[serviceName]; ok { continue } + serviceName = strings.TrimPrefix(serviceName, "-") + if _, ok := combined.Services[serviceName]; ok { + continue + } return nil, &FormatError{ Message: fmt.Sprintf(`log target %q specifies unknown service %q`, target.Name, serviceName), diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 5f13de82..b279e0cf 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -840,10 +840,12 @@ var planTests = []planTest{{ tgt1: type: loki location: http://10.1.77.196:3100/loki/api/v1/push + services: [all] override: merge tgt2: type: syslog location: udp://0.0.0.0:514 + services: [svc2] override: merge `}, result: &plan.Layer{ @@ -873,12 +875,14 @@ var planTests = []planTest{{ Name: "tgt1", Type: plan.LokiTarget, Location: "http://10.1.77.196:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, Location: "udp://0.0.0.0:514", + Services: []string{"svc2"}, Override: plan.MergeOverride, }, }, @@ -900,10 +904,12 @@ var planTests = []planTest{{ tgt1: type: loki location: http://10.1.77.196:3100/loki/api/v1/push + services: [all] override: merge tgt2: type: syslog location: udp://0.0.0.0:514 + services: [svc2] override: merge `, ` services: @@ -917,14 +923,16 @@ var planTests = []planTest{{ log-targets: tgt1: + services: [-all, svc1] override: merge tgt2: type: syslog - location: foo + services: [] override: replace tgt3: type: loki location: http://10.1.77.206:3100/loki/api/v1/push + services: [all] override: merge `}, layers: []*plan.Layer{{ @@ -950,12 +958,14 @@ var planTests = []planTest{{ Name: "tgt1", Type: plan.LokiTarget, Location: "http://10.1.77.196:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, Location: "udp://0.0.0.0:514", + Services: []string{"svc2"}, Override: plan.MergeOverride, }, }, @@ -979,18 +989,20 @@ var planTests = []planTest{{ LogTargets: map[string]*plan.LogTarget{ "tgt1": { Name: "tgt1", + Services: []string{"-all", "svc1"}, Override: plan.MergeOverride, }, "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, - Location: "foo", + Services: []string{}, Override: plan.ReplaceOverride, }, "tgt3": { Name: "tgt3", Type: plan.LokiTarget, Location: "http://10.1.77.206:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, }, @@ -1022,18 +1034,19 @@ var planTests = []planTest{{ Name: "tgt1", Type: plan.LokiTarget, Location: "http://10.1.77.196:3100/loki/api/v1/push", + Services: []string{"all", "-all", "svc1"}, Override: plan.MergeOverride, }, "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, - Location: "foo", Override: plan.ReplaceOverride, }, "tgt3": { Name: "tgt3", Type: plan.LokiTarget, Location: "http://10.1.77.206:3100/loki/api/v1/push", + Services: []string{"all"}, Override: plan.MergeOverride, }, }, @@ -1054,6 +1067,7 @@ var planTests = []planTest{{ log-targets: tgt1: type: loki + services: [all] override: merge `}}, { summary: "Unsupported log target type", @@ -1065,6 +1079,17 @@ var planTests = []planTest{{ location: http://10.1.77.196:3100/loki/api/v1/push override: merge `}, +}, { + summary: "Log target specifies invalid service", + error: `log target "tgt1" specifies unknown service "nonexistent"`, + input: []string{` + log-targets: + tgt1: + type: loki + location: http://10.1.77.196:3100/loki/api/v1/push + services: [nonexistent] + override: merge +`}, }} func (s *S) TestParseLayer(c *C) { @@ -1409,3 +1434,5 @@ func (s *S) TestParseCommand(c *C) { c.Assert(extra, DeepEquals, test.cmdArgs) } } + +func (s *S) TestLogsTo(c *C) {} From 1f6b175f91356bd6163be859ab223647c856d169 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 13:08:52 +1000 Subject: [PATCH 07/14] add deprecated service names --- internals/plan/plan.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 829990ff..85d3ed65 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -26,6 +26,7 @@ import ( "strings" "time" + "github.com/canonical/pebble/internals/logger" "github.com/canonical/x-go/strutil/shlex" "gopkg.in/yaml.v3" @@ -928,6 +929,10 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { Message: fmt.Sprintf("cannot use reserved service name %q", name), } } + // Deprecated service names + if name == "all" || name == "default" || name == "none" { + logger.Noticef("WARNING: using %q as service name is deprecated", name) + } if service == nil { return nil, &FormatError{ Message: fmt.Sprintf("service object cannot be null for service %q", name), From 75bfeeb18d75106e37f8c2583f2bf3d989902d01 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 13:33:16 +1000 Subject: [PATCH 08/14] write LogsTo test - catch iteration mistake in LogsTo - modify plan override test, to test changing location/type --- internals/plan/plan.go | 2 +- internals/plan/plan_test.go | 117 +++++++++++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 11 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 85d3ed65..f1d322b0 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -258,7 +258,7 @@ func CommandString(base, extra []string) string { func (s *Service) LogsTo(t *LogTarget) bool { // Iterate backwards through t.Services until we find something matching // s.Name. - for i := len(t.Services) - 1; i >= 0; i++ { + for i := len(t.Services) - 1; i >= 0; i-- { switch t.Services[i] { case s.Name: return true diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index b279e0cf..7dedd564 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -911,6 +911,11 @@ var planTests = []planTest{{ location: udp://0.0.0.0:514 services: [svc2] override: merge + tgt3: + type: loki + location: http://10.1.77.206:3100/loki/api/v1/push + services: [all] + override: merge `, ` services: svc1: @@ -930,9 +935,9 @@ var planTests = []planTest{{ services: [] override: replace tgt3: - type: loki - location: http://10.1.77.206:3100/loki/api/v1/push - services: [all] + type: syslog + location: udp://0.0.0.0:514 + services: [-svc1] override: merge `}, layers: []*plan.Layer{{ @@ -968,6 +973,13 @@ var planTests = []planTest{{ Services: []string{"svc2"}, Override: plan.MergeOverride, }, + "tgt3": { + Name: "tgt3", + Type: plan.LokiTarget, + Location: "http://10.1.77.206:3100/loki/api/v1/push", + Services: []string{"all"}, + Override: plan.MergeOverride, + }, }, }, { Label: "layer-1", @@ -1000,9 +1012,9 @@ var planTests = []planTest{{ }, "tgt3": { Name: "tgt3", - Type: plan.LokiTarget, - Location: "http://10.1.77.206:3100/loki/api/v1/push", - Services: []string{"all"}, + Type: plan.SyslogTarget, + Location: "udp://0.0.0.0:514", + Services: []string{"-svc1"}, Override: plan.MergeOverride, }, }, @@ -1044,9 +1056,9 @@ var planTests = []planTest{{ }, "tgt3": { Name: "tgt3", - Type: plan.LokiTarget, - Location: "http://10.1.77.206:3100/loki/api/v1/push", - Services: []string{"all"}, + Type: plan.SyslogTarget, + Location: "udp://0.0.0.0:514", + Services: []string{"all", "-svc1"}, Override: plan.MergeOverride, }, }, @@ -1435,4 +1447,89 @@ func (s *S) TestParseCommand(c *C) { } } -func (s *S) TestLogsTo(c *C) {} +func (s *S) TestLogsTo(c *C) { + tests := []struct { + services []string + logsTo map[string]bool + }{{ + services: nil, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + }, + }, { + services: []string{}, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + }, + }, { + services: []string{"all"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": true, + }, + }, { + services: []string{"svc1"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": false, + }, + }, { + services: []string{"svc1", "svc2"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": true, + "svc3": false, + }, + }, { + services: []string{"all", "-svc2"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": false, + "svc3": true, + }, + }, { + services: []string{"svc1", "svc2", "-svc1", "all"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": true, + "svc3": true, + }, + }, { + services: []string{"svc1", "svc2", "-all"}, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + "svc3": false, + }, + }, { + services: []string{"all", "-all"}, + logsTo: map[string]bool{ + "svc1": false, + "svc2": false, + "svc3": false, + }, + }, { + services: []string{"svc1", "svc2", "-all", "svc3", "svc1", "-svc3"}, + logsTo: map[string]bool{ + "svc1": true, + "svc2": false, + "svc3": false, + }, + }} + + for _, test := range tests { + target := &plan.LogTarget{ + Services: test.services, + } + + for serviceName, shouldLogTo := range test.logsTo { + service := &plan.Service{ + Name: serviceName, + } + c.Check(service.LogsTo(target), Equals, shouldLogTo, + Commentf("matching service %q against 'services: %v'", serviceName, test.services)) + } + } +} From 062524e2f87607be1cce0cc4382d918ba1d4aea2 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 14:02:08 +1000 Subject: [PATCH 09/14] clean up comments, ready for review --- internals/plan/plan.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index f1d322b0..6816e1cb 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -523,8 +523,6 @@ func (t *LogTarget) Merge(other *LogTarget) { if other.Location != "" { t.Location = other.Location } - // TODO: reduce log targets? - // e.g. [..., all, -svc1] -> [all, -svc1] t.Services = append(t.Services, other.Services...) } @@ -774,6 +772,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { if _, ok := combined.Services[serviceName]; ok { continue } + // Check if this is `-svc` serviceName = strings.TrimPrefix(serviceName, "-") if _, ok := combined.Services[serviceName]; ok { continue From 439165ffbd8ae9f0c464b5ac9425e7df73c3a658 Mon Sep 17 00:00:00 2001 From: Jordan Mitchell Barrett <90195985+barrettj12@users.noreply.github.com> Date: Fri, 30 Jun 2023 14:35:37 +1000 Subject: [PATCH 10/14] elaborate on README example (thx @benhoyt) Co-authored-by: Ben Hoyt --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 06129cc3..5a6d2696 100644 --- a/README.md +++ b/README.md @@ -439,7 +439,7 @@ my-target: services: [-svc1] override: merge ``` -then in the merged layer, `my-target` will collect logs from only `svc2`. You can also use `-all` to remove all services from the list. +then in the merged layer, the `services` list will be merged to `[svc1, svc2, -svc1]`, which evaluates left to right as simply `[svc2]`. So `my-target` will collect logs from only `svc2`. You can also use `-all` to remove all services from the list. --> ## Container usage From 1d979147f288b37cae955cf66b71fcad02d83b5b Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 14:43:06 +1000 Subject: [PATCH 11/14] disallow services starting with "-" - consolidate svc / -svc validation in CombineLayers --- internals/plan/plan.go | 10 ++++++---- internals/plan/plan_test.go | 9 +++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 6816e1cb..62151568 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -769,10 +769,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { if serviceName == "all" || serviceName == "-all" { continue } - if _, ok := combined.Services[serviceName]; ok { - continue - } - // Check if this is `-svc` + // This could be `-svc` - try to trim a preceding serviceName = strings.TrimPrefix(serviceName, "-") if _, ok := combined.Services[serviceName]; ok { continue @@ -932,6 +929,11 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { if name == "all" || name == "default" || name == "none" { logger.Noticef("WARNING: using %q as service name is deprecated", name) } + if strings.HasPrefix(name, "-") { + return nil, &FormatError{ + Message: fmt.Sprintf(`cannot use service name %q: starting with "-" not allowed`, name), + } + } if service == nil { return nil, &FormatError{ Message: fmt.Sprintf("service object cannot be null for service %q", name), diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 7dedd564..9b0f4a7e 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -1102,6 +1102,15 @@ var planTests = []planTest{{ services: [nonexistent] override: merge `}, +}, { + summary: `Service name can't start with "-"`, + error: `cannot use service name "-svc1": starting with "-" not allowed`, + input: []string{` + services: + -svc1: + command: foo + override: merge +`}, }} func (s *S) TestParseLayer(c *C) { From bc85a358d0d30be7ec7bd632c1f12efc171b8d62 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 14:58:04 +1000 Subject: [PATCH 12/14] fix logger import order --- internals/plan/plan.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index 62151568..c47e3215 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -26,10 +26,10 @@ import ( "strings" "time" - "github.com/canonical/pebble/internals/logger" "github.com/canonical/x-go/strutil/shlex" "gopkg.in/yaml.v3" + "github.com/canonical/pebble/internals/logger" "github.com/canonical/pebble/internals/osutil" ) From 89012244d7c764106f97d3a596471a4e247fcd73 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Fri, 30 Jun 2023 16:51:45 +1000 Subject: [PATCH 13/14] target.Location must always be set --- internals/plan/plan.go | 2 +- internals/plan/plan_test.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index c47e3215..d54adea4 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -780,7 +780,7 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { } } - if target.Location == "" && len(target.Services) > 0 { + if target.Location == "" { return nil, &FormatError{ Message: fmt.Sprintf(`plan must define "location" for log target %q`, name), } diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 9b0f4a7e..1883ece4 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -932,6 +932,7 @@ var planTests = []planTest{{ override: merge tgt2: type: syslog + location: udp://1.2.3.4:514 services: [] override: replace tgt3: @@ -1007,6 +1008,7 @@ var planTests = []planTest{{ "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, + Location: "udp://1.2.3.4:514", Services: []string{}, Override: plan.ReplaceOverride, }, @@ -1052,6 +1054,7 @@ var planTests = []planTest{{ "tgt2": { Name: "tgt2", Type: plan.SyslogTarget, + Location: "udp://1.2.3.4:514", Override: plan.ReplaceOverride, }, "tgt3": { From 576706f52cd87aafccfa0f2b932941ee567096de Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Tue, 4 Jul 2023 09:57:37 +1200 Subject: [PATCH 14/14] address @niemeyer's review comments - simplify log-target.services validation - improve deprecation warning - expand examples in README --- README.md | 19 +++++++++++++++++-- internals/plan/plan.go | 7 +++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 5a6d2696..315e9d08 100644 --- a/README.md +++ b/README.md @@ -426,7 +426,7 @@ log-targets: For each log target, use the `services` key to specify a list of services to collect logs from. In the above example, the `syslog-example` target will collect logs from `svc1` and `svc2`. -Use the special keyword `all` to match all services. In the above example, `loki-example` will collect logs from all services. +Use the special keyword `all` to match all services, including services that might be added in future layers. In the above example, `loki-example` will collect logs from all services. To remove a service from a log target when merging, prefix the service name with a minus `-`. For example, if we have a base layer with ```yaml @@ -439,7 +439,22 @@ my-target: services: [-svc1] override: merge ``` -then in the merged layer, the `services` list will be merged to `[svc1, svc2, -svc1]`, which evaluates left to right as simply `[svc2]`. So `my-target` will collect logs from only `svc2`. You can also use `-all` to remove all services from the list. +then in the merged layer, the `services` list will be merged to `[svc1, svc2, -svc1]`, which evaluates left to right as simply `[svc2]`. So `my-target` will collect logs from only `svc2`. + +You can also use `-all` to remove all services from the list. For example, adding an override layer with +```yaml +my-target: + services: [-all] + override: merge +``` +would remove all services from `my-target`, effectively disabling `my-target`. Meanwhile, adding an override layer with +```yaml +my-target: + services: [-all, svc1] + override: merge +``` +would remove all services and then add `svc1`, so `my-target` would receive logs from only `svc1`. + --> ## Container usage diff --git a/internals/plan/plan.go b/internals/plan/plan.go index d54adea4..22d403c9 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -766,11 +766,10 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { // Validate service names specified in log target for _, serviceName := range target.Services { - if serviceName == "all" || serviceName == "-all" { + serviceName = strings.TrimPrefix(serviceName, "-") + if serviceName == "all" { continue } - // This could be `-svc` - try to trim a preceding - serviceName = strings.TrimPrefix(serviceName, "-") if _, ok := combined.Services[serviceName]; ok { continue } @@ -927,7 +926,7 @@ func ParseLayer(order int, label string, data []byte) (*Layer, error) { } // Deprecated service names if name == "all" || name == "default" || name == "none" { - logger.Noticef("WARNING: using %q as service name is deprecated", name) + logger.Noticef("Using keyword %q as a service name is deprecated", name) } if strings.HasPrefix(name, "-") { return nil, &FormatError{