Merge pull request #7288 from concourse/issue/7284

containerd: split long property values into multiple chunks

testflight/regression_test.go

@@ -0,0 +1,18 @@
+package testflight_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/gexec"
+)
+
+var _ = Describe("Regression tests", func() {
+	Describe("issue 7282", func() {
+		It("does not error when resources emit long metadata strings", func() {
+			setAndUnpausePipeline("fixtures/long-metadata.yml")
+
+			watch := fly("trigger-job", "-j", inPipeline("job"), "-w")
+			Expect(watch).To(gexec.Exit(0))
+		})
+	})
+})

worker/runtime/backend.go

@@ -237,7 +237,11 @@ func (b *GardenBackend) createContainer(ctx context.Context, gdnSpec garden.Cont
 
 	oci.Mounts = append(oci.Mounts, netMounts...)
 
-	return b.client.NewContainer(ctx, gdnSpec.Handle, gdnSpec.Properties, oci)
+	labels, err := propertiesToLabels(gdnSpec.Properties)
+	if err != nil {
+		return nil, fmt.Errorf("convert properties to labels: %w", err)
+	}
+	return b.client.NewContainer(ctx, gdnSpec.Handle, labels, oci)
 }
 
 func (b *GardenBackend) startTask(ctx context.Context, cont containerd.Container) error {
@@ -308,19 +312,19 @@ func (b *GardenBackend) Destroy(handle string) error {
 // Containers lists all containers filtered by properties (which are ANDed
 // together).
 //
-func (b *GardenBackend) Containers(properties garden.Properties) (containers []garden.Container, err error) {
+func (b *GardenBackend) Containers(properties garden.Properties) ([]garden.Container, error) {
 	filters, err := propertiesToFilterList(properties)
 	if err != nil {
-		return
+		return nil, err
 	}
 
 	res, err := b.client.Containers(context.Background(), filters...)
 	if err != nil {
 		err = fmt.Errorf("list containers: %w", err)
-		return
+		return nil, err
 	}
 
-	containers = make([]garden.Container, len(res))
+	containers := make([]garden.Container, len(res))
 	for i, containerdContainer := range res {
 		containers[i] = NewContainer(
 			containerdContainer,
@@ -329,7 +333,7 @@ func (b *GardenBackend) Containers(properties garden.Properties) (containers []g
 		)
 	}
 
-	return
+	return containers, nil
 }
 
 // Lookup returns the container with the specified handle.

worker/runtime/backend_test.go

@@ -297,7 +297,7 @@ func (s *BackendSuite) TestContainersWithProperProperties() {
 	s.Equal(1, s.client.ContainersCallCount())
 
 	_, labelSet := s.client.ContainersArgsForCall(0)
-	s.ElementsMatch([]string{"labels.foo==bar", "labels.caz==zaz"}, labelSet)
+	s.ElementsMatch([]string{"labels.foo.0==bar", "labels.caz.0==zaz"}, labelSet)
 }
 
 func (s *BackendSuite) TestContainersConversion() {

worker/runtime/container.go

@@ -197,7 +197,7 @@ func (c *Container) Properties() (garden.Properties, error) {
 		return garden.Properties{}, fmt.Errorf("labels retrieval: %w", err)
 	}
 
-	return labels, nil
+	return labelsToProperties(labels), nil
 }
 
 // Property returns the value of the property with the specified name.
@@ -219,11 +219,11 @@ func (c *Container) Property(name string) (string, error) {
 // Set a named property on a container to a specified value.
 //
 func (c *Container) SetProperty(name string, value string) error {
-	labelSet := map[string]string{
-		name: value,
+	labelSet, err := propertiesToLabels(garden.Properties{name: value})
+	if err != nil {
+		return err
 	}
-
-	_, err := c.container.SetLabels(context.Background(), labelSet)
+	_, err = c.container.SetLabels(context.Background(), labelSet)
 	if err != nil {
 		return fmt.Errorf("set label: %w", err)
 	}

worker/runtime/container_test.go

@@ -337,7 +337,7 @@ func (s *ContainerSuite) TestSetGraceTimeSetLabelsSucceeds() {
 	s.NoError(err)
 
 	expectedLabelSet := map[string]string{
-		"garden.grace-time": "1234",
+		"garden.grace-time.0": "1234",
 	}
 	_, labelSet := s.containerdContainer.SetLabelsArgsForCall(0)
 	s.Equal(expectedLabelSet, labelSet)
@@ -358,7 +358,7 @@ func (s *ContainerSuite) TestPropertyNotFound() {
 
 func (s *ContainerSuite) TestPropertyReturnsValue() {
 	properties := garden.Properties{
-		"any": "some-value",
+		"any.0": "some-value",
 	}
 	s.containerdContainer.LabelsReturns(properties, nil)
 	result, err := s.container.Property("any")

worker/runtime/integration/integration_test.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"syscall"
 	"testing/iotest"
@@ -910,6 +911,63 @@ func (s *IntegrationSuite) TestRequestTimeoutZero() {
 	}()
 }
 
+// TestPropertiesGetChunked tests that we are able to store arbitrarily long
+// properties, getting around containerd's label length restriction.
+//
+func (s *IntegrationSuite) TestPropertiesGetChunked() {
+	handle := uuid()
+
+	longString := ""
+	for i := 0; i < 10000; i++ {
+		longString += strconv.Itoa(i)
+	}
+
+	properties := garden.Properties{
+		"long1": longString,
+		// Concourse may try to set an empty value property on a container.
+		// This just gets ignored (i.e. subsequent calls to
+		// container.Properties() won't include it)
+		"empty": "",
+	}
+
+	container, err := s.gardenBackend.Create(garden.ContainerSpec{
+		Handle:     handle,
+		RootFSPath: "raw://" + s.rootfs,
+		Privileged: true,
+		Properties: properties,
+	})
+	s.NoError(err)
+
+	containers, err := s.gardenBackend.Containers(garden.Properties{
+		"long1": longString,
+	})
+	s.NoError(err)
+
+	s.Len(containers, 1)
+
+	err = container.SetProperty("long2", longString)
+	s.NoError(err)
+
+	containers, err = s.gardenBackend.Containers(garden.Properties{
+		"long1": longString,
+		"long2": longString,
+	})
+	s.NoError(err)
+	s.Len(containers, 1)
+
+	err = container.SetProperty(longString, "foo")
+	s.Error(err)
+	s.Regexp("property.*too long", err.Error())
+
+	properties, err = container.Properties()
+	s.NoError(err)
+
+	s.Equal(garden.Properties{
+		"long1": longString,
+		"long2": longString,
+	}, properties)
+}
+
 func (s *IntegrationSuite) TestNetworkMountsAreRemoved() {
 	// Using custom backend, clean up BeforeTest() stuff
 	s.gardenBackend.Stop()

worker/runtime/properties.go

@@ -2,10 +2,112 @@ package runtime
 
 import (
 	"fmt"
+	"strconv"
+	"strings"
 
 	"code.cloudfoundry.org/garden"
 )
 
+// propertiesToLabels converts a set of properties to a set of labels,
+// splitting up the property values into multiple labels if they would exceed
+// containerd's restriction on the length of the key+value for a single label.
+//
+// each label key is of the form: `key.SEQUENCE_NUMBER`, where SEQUENCE_NUMBER
+// starts from 0 and counts up. for instance, a property `key: ...` may be
+// stored in multiple labels like so:
+//
+// key.0: first chunk of value...
+// key.1: ...second chunk of value...
+// ...
+// key.n: ...last chunk of value
+//
+func propertiesToLabels(properties garden.Properties) (map[string]string, error) {
+	// Hard restriction on the total length of key + value imposed by
+	// containerd on a per-label basis.
+	const maxLabelLen = 4096
+
+	// Restrict the key length to no more than half the label length.
+	// This ratio is arbitrary, but helps ensure that:
+	// 1. The key + sequence number suffix cannot exceed maxLabelLen, and
+	// 2. We can fit a reasonable amount of data from the value in each chunk
+	const maxKeyLen = maxLabelLen / 2
+
+	labelSet := map[string]string{}
+	for key, value := range properties {
+		sequenceNum := 0
+		if len(key) > maxKeyLen {
+			return nil, fmt.Errorf("property name %q is too long", key[:32]+"...")
+		}
+		for {
+			chunkKey := key + "." + strconv.Itoa(sequenceNum)
+			valueLen := maxLabelLen - len(chunkKey)
+			if valueLen > len(value) {
+				valueLen = len(value)
+			}
+
+			labelSet[chunkKey] = value[:valueLen]
+			value = value[valueLen:]
+
+			if len(value) == 0 {
+				break
+			}
+
+			sequenceNum++
+		}
+	}
+	return labelSet, nil
+}
+
+// labelsToProperties is the inverse of propertiesToLabels. It combines all of
+// the related labels into a single property by concatenating them together in
+// order.
+//
+// Any labels that aren't of the correct format (i.e. key.n) will be ignored.
+//
+func labelsToProperties(labels map[string]string) garden.Properties {
+	properties := garden.Properties{}
+	for len(labels) > 0 {
+		var key string
+		// Pick an arbitrary chunk from the labels
+		for k := range labels {
+			key = k
+			break
+		}
+
+		chunkSequenceStart := strings.LastIndexByte(key, '.')
+		if chunkSequenceStart < 0 {
+			// Not a properly formatted chunk. Just ignore.
+			delete(labels, key)
+			continue
+		}
+
+		propertyName := key[:chunkSequenceStart]
+
+		var property strings.Builder
+		for sequenceNum := 0; ; sequenceNum++ {
+			chunkKey := propertyName + "." + strconv.Itoa(sequenceNum)
+			chunkValue, ok := labels[chunkKey]
+			if !ok {
+				break
+			}
+			delete(labels, chunkKey)
+			property.WriteString(chunkValue)
+		}
+
+		if property.Len() == 0 {
+			// External components may add labels to containers that contain
+			// '.' but aren't chunked properties. If we encounter a label that
+			// has no chunks, just ignore it.
+			delete(labels, key)
+			continue
+		}
+
+		properties[propertyName] = property.String()
+	}
+
+	return properties
+}
+
 // propertiesToFilterList converts a set of garden properties to a list of
 // filters as expected by containerd.
 //
@@ -21,19 +123,26 @@ import (
 //           |      key
 //           what
 //
-func propertiesToFilterList(properties garden.Properties) (filters []string, err error) {
-	filters = make([]string, len(properties))
-
-	idx := 0
+// note that the key in this case represents the label key, which is not the
+// same as the property key - refer to propertiesToLabels.
+//
+func propertiesToFilterList(properties garden.Properties) ([]string, error) {
 	for k, v := range properties {
 		if k == "" || v == "" {
-			err = fmt.Errorf("key or value must not be empty")
-			return
+			return nil, fmt.Errorf("key or value must not be empty")
 		}
-
-		filters[idx] = "labels." + k + "==" + v
-		idx++
 	}
 
-	return
+	labels, err := propertiesToLabels(properties)
+	if err != nil {
+		return nil, err
+	}
+
+	filters := make([]string, 0, len(labels))
+
+	for k, v := range labels {
+		filters = append(filters, "labels."+k+"=="+v)
+	}
+
+	return filters, nil
 }