From a3c0ae8b82b1ab3950b51364c093a03057ec8735 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Mon, 1 Apr 2024 10:43:45 +0800 Subject: [PATCH 1/8] feat: make layers with label prefix pebble reserved --- internals/overlord/planstate/manager.go | 8 ++++++-- internals/plan/plan.go | 16 ++++++++++++++++ internals/plan/plan_test.go | 16 ++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go index b7c21348..1127d854 100644 --- a/internals/overlord/planstate/manager.go +++ b/internals/overlord/planstate/manager.go @@ -208,7 +208,7 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error { defer m.planLock.Unlock() newLayer := &plan.Layer{ - // Labels with "pebble-*" prefix are (will be) reserved, see: + // Labels with "pebble-*" prefix are reserved, see: // https://github.com/canonical/pebble/issues/220 Label: "pebble-service-args", Services: make(map[string]*plan.Service), @@ -231,7 +231,11 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error { err := newLayer.Validate() if err != nil { - return err + if _, ok := err.(*plan.ReservedLabelError); ok { + // Labels with "pebble-*" prefix are reserved. + } else { + return err + } } return m.appendLayer(newLayer) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index d4217ba6..f10e38cb 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -549,6 +549,16 @@ func (e *FormatError) Error() string { return e.Message } +// ReservedLabelError is the error returned when a layer label has the reserved +// prefix "pebble-", such as "pebble-foo-bar". +type ReservedLabelError struct { + Message string +} + +func (e *ReservedLabelError) Error() string { + return e.Message +} + // CombineLayers combines the given layers into a single layer, with the later // layers overriding earlier ones. // Neither the individual layers nor the combined layer are validated here - the @@ -819,6 +829,12 @@ func (layer *Layer) Validate() error { } } + if strings.HasPrefix(layer.Label, "pebble-") { + return &ReservedLabelError{ + Message: fmt.Sprintf("cannot use reserved layer label %q", layer.Label), + } + } + return nil } diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index aa6f3f38..1d17a18d 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -1933,3 +1933,19 @@ func (s *S) TestMergeServiceContextOverrides(c *C) { WorkingDir: "/working/dir", }) } + +func (s *S) TestValidateLayerReservedLabel(c *C) { + newLayer := &plan.Layer{ + // Labels with "pebble-*" are reserved, + Label: "pebble-service-args", + Services: make(map[string]*plan.Service), + } + err := newLayer.Validate() + c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-service-args"`) +} + +func (s *S) TestParseLayerReservedLabel(c *C) { + // Validate fails if layer label has prefix "pebble-" + _, err := plan.ParseLayer(0, "pebble-service-args", []byte("{}")) + c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-service-args"`) +} From 120755f084fb1f9c5e137dcbeac9c5446d915e3c Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Mon, 1 Apr 2024 10:56:11 +0800 Subject: [PATCH 2/8] chore: update ut comments --- internals/plan/plan_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 1d17a18d..0e5371c7 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -1936,7 +1936,7 @@ func (s *S) TestMergeServiceContextOverrides(c *C) { func (s *S) TestValidateLayerReservedLabel(c *C) { newLayer := &plan.Layer{ - // Labels with "pebble-*" are reserved, + // Labels with "pebble-*" prefix are reserved, Label: "pebble-service-args", Services: make(map[string]*plan.Service), } @@ -1945,7 +1945,7 @@ func (s *S) TestValidateLayerReservedLabel(c *C) { } func (s *S) TestParseLayerReservedLabel(c *C) { - // Validate fails if layer label has prefix "pebble-" + // Validate fails if layer label has the reserved prefix "pebble-" _, err := plan.ParseLayer(0, "pebble-service-args", []byte("{}")) c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-service-args"`) } From a5705526f98b989d7e6aa3fcdeb7dc926685d256 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Wed, 3 Apr 2024 08:52:15 +0800 Subject: [PATCH 3/8] chore: remove layer validation in SetServiceArgs --- internals/overlord/planstate/manager.go | 10 ++-------- internals/plan/plan.go | 12 +----------- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go index 1127d854..4c6848d2 100644 --- a/internals/overlord/planstate/manager.go +++ b/internals/overlord/planstate/manager.go @@ -229,14 +229,8 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error { } } - err := newLayer.Validate() - if err != nil { - if _, ok := err.(*plan.ReservedLabelError); ok { - // Labels with "pebble-*" prefix are reserved. - } else { - return err - } - } + // We have added validation for layer labels (pebble-* prefix is reserved), + // so after creating the Layer directly, we don't validate it any more. return m.appendLayer(newLayer) } diff --git a/internals/plan/plan.go b/internals/plan/plan.go index f10e38cb..d2e03db1 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -549,16 +549,6 @@ func (e *FormatError) Error() string { return e.Message } -// ReservedLabelError is the error returned when a layer label has the reserved -// prefix "pebble-", such as "pebble-foo-bar". -type ReservedLabelError struct { - Message string -} - -func (e *ReservedLabelError) Error() string { - return e.Message -} - // CombineLayers combines the given layers into a single layer, with the later // layers overriding earlier ones. // Neither the individual layers nor the combined layer are validated here - the @@ -830,7 +820,7 @@ func (layer *Layer) Validate() error { } if strings.HasPrefix(layer.Label, "pebble-") { - return &ReservedLabelError{ + return &FormatError{ Message: fmt.Sprintf("cannot use reserved layer label %q", layer.Label), } } From 485e35351d96183d7242dbd320c8f8fe9d1bbd13 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Wed, 3 Apr 2024 08:55:37 +0800 Subject: [PATCH 4/8] chore: remove layer validation in SetServiceArgs --- internals/overlord/planstate/manager.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go index 4c6848d2..f40a4969 100644 --- a/internals/overlord/planstate/manager.go +++ b/internals/overlord/planstate/manager.go @@ -210,6 +210,8 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error { newLayer := &plan.Layer{ // Labels with "pebble-*" prefix are reserved, see: // https://github.com/canonical/pebble/issues/220 + // Validation for layer labels has been added, so after so after creating the + // Layer directly (not parsing from raw data), it won't need to be validated. Label: "pebble-service-args", Services: make(map[string]*plan.Service), } @@ -229,8 +231,5 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error { } } - // We have added validation for layer labels (pebble-* prefix is reserved), - // so after creating the Layer directly, we don't validate it any more. - return m.appendLayer(newLayer) } From 9599d4a9096e07eddc82ecbcf7d667f04007c283 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Wed, 3 Apr 2024 08:56:10 +0800 Subject: [PATCH 5/8] chore: remove an unnecessary test --- internals/plan/plan_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 0e5371c7..51503c7d 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -1934,16 +1934,6 @@ func (s *S) TestMergeServiceContextOverrides(c *C) { }) } -func (s *S) TestValidateLayerReservedLabel(c *C) { - newLayer := &plan.Layer{ - // Labels with "pebble-*" prefix are reserved, - Label: "pebble-service-args", - Services: make(map[string]*plan.Service), - } - err := newLayer.Validate() - c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-service-args"`) -} - func (s *S) TestParseLayerReservedLabel(c *C) { // Validate fails if layer label has the reserved prefix "pebble-" _, err := plan.ParseLayer(0, "pebble-service-args", []byte("{}")) From 8a0bb486264f72e1baf913a5fe7db53e9f9bc772 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Sat, 6 Apr 2024 14:10:56 +0800 Subject: [PATCH 6/8] Update internals/overlord/planstate/manager.go Co-authored-by: Ben Hoyt --- internals/overlord/planstate/manager.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go index f40a4969..ec23d051 100644 --- a/internals/overlord/planstate/manager.go +++ b/internals/overlord/planstate/manager.go @@ -208,10 +208,9 @@ func (m *PlanManager) SetServiceArgs(serviceArgs map[string][]string) error { defer m.planLock.Unlock() newLayer := &plan.Layer{ - // Labels with "pebble-*" prefix are reserved, see: - // https://github.com/canonical/pebble/issues/220 - // Validation for layer labels has been added, so after so after creating the - // Layer directly (not parsing from raw data), it won't need to be validated. + // Labels with "pebble-*" prefix are reserved for use by Pebble. + // Layer.Validate() ensures this, so skip calling that because we're creating the + // Layer directly, not parsing from user input. Label: "pebble-service-args", Services: make(map[string]*plan.Service), } From 71b57401bd08cdf4e34e4e1137d286aae6f1d569 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Sat, 6 Apr 2024 14:14:07 +0800 Subject: [PATCH 7/8] chore: rework according to code reviews --- internals/plan/plan.go | 12 ++++++------ internals/plan/plan_test.go | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internals/plan/plan.go b/internals/plan/plan.go index d2e03db1..2d5f3bcd 100644 --- a/internals/plan/plan.go +++ b/internals/plan/plan.go @@ -687,6 +687,12 @@ func CombineLayers(layers ...*Layer) (*Layer, error) { // See also Plan.Validate, which does additional checks based on the combined // layers. func (layer *Layer) Validate() error { + if strings.HasPrefix(layer.Label, "pebble-") { + return &FormatError{ + Message: `cannot use reserved label prefix "pebble-"`, + } + } + for name, service := range layer.Services { if name == "" { return &FormatError{ @@ -819,12 +825,6 @@ func (layer *Layer) Validate() error { } } - if strings.HasPrefix(layer.Label, "pebble-") { - return &FormatError{ - Message: fmt.Sprintf("cannot use reserved layer label %q", layer.Label), - } - } - return nil } diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index 51503c7d..f0de0c60 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -1934,8 +1934,8 @@ func (s *S) TestMergeServiceContextOverrides(c *C) { }) } -func (s *S) TestParseLayerReservedLabel(c *C) { +func (s *S) TestPebbleLabelPrefixReserved(c *C) { // Validate fails if layer label has the reserved prefix "pebble-" - _, err := plan.ParseLayer(0, "pebble-service-args", []byte("{}")) - c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-service-args"`) + _, err := plan.ParseLayer(0, "pebble-foo", []byte("{}")) + c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-foo"`) } From 526d4892f9f940ac4042c3446d013d8f220a5503 Mon Sep 17 00:00:00 2001 From: Tiexin Guo Date: Sat, 6 Apr 2024 14:15:43 +0800 Subject: [PATCH 8/8] chore: rework according to code reviews --- internals/plan/plan_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals/plan/plan_test.go b/internals/plan/plan_test.go index f0de0c60..3a99fcdf 100644 --- a/internals/plan/plan_test.go +++ b/internals/plan/plan_test.go @@ -1937,5 +1937,5 @@ func (s *S) TestMergeServiceContextOverrides(c *C) { func (s *S) TestPebbleLabelPrefixReserved(c *C) { // Validate fails if layer label has the reserved prefix "pebble-" _, err := plan.ParseLayer(0, "pebble-foo", []byte("{}")) - c.Check(err, ErrorMatches, `cannot use reserved layer label "pebble-foo"`) + c.Check(err, ErrorMatches, `cannot use reserved label prefix "pebble-"`) }