From d5888bae24963b7e4808267a832024c930fbc1b0 Mon Sep 17 00:00:00 2001
From: Brett Smith <brett.smith@curii.com>
Date: Mon, 20 Feb 2023 10:53:06 -0500
Subject: [PATCH] 19980: Recognize the arv: prefix for all system properties

arvados-cwl-runner currently uses this prefix to record Git information,
and we want to use it for all future system properties.

Arvados-DCO-1.1-Signed-off-by: Brett Smith <brett.smith@curii.com>
---
 sdk/go/arvados/vocabulary.go      | 14 +++++++++--
 sdk/go/arvados/vocabulary_test.go | 39 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/sdk/go/arvados/vocabulary.go b/sdk/go/arvados/vocabulary.go
index 16f80db07d..caab6e8fbe 100644
--- a/sdk/go/arvados/vocabulary.go
+++ b/sdk/go/arvados/vocabulary.go
@@ -10,10 +10,13 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"regexp"
 	"strconv"
 	"strings"
 )
 
+const systemKeyPattern = `^arv:[a-zA-Z]`
+
 type Vocabulary struct {
 	reservedTagKeys map[string]bool          `json:"-"`
 	StrictTags      bool                     `json:"strict_tags"`
@@ -26,7 +29,10 @@ type VocabularyTag struct {
 	Values map[string]VocabularyTagValue `json:"values"`
 }
 
-// Cannot have a constant map in Go, so we have to use a function
+// Cannot have a constant map in Go, so we have to use a function.
+// If you are adding a new system property, it SHOULD match `systemKeyPattern`
+// above, and Check will allow it. This map is for historical exceptions that
+// predate standardizing on this prefix.
 func (v *Vocabulary) systemTagKeys() map[string]bool {
 	return map[string]bool{
 		// Collection keys - set by arvados-cwl-runner
@@ -265,9 +271,13 @@ func (v *Vocabulary) Check(data map[string]interface{}) error {
 	if v == nil {
 		return nil
 	}
+	systemKeyRegexp, err := regexp.Compile(systemKeyPattern)
+	if err != nil {
+		return err
+	}
 	for key, val := range data {
 		// Checks for key validity
-		if v.reservedTagKeys[key] {
+		if systemKeyRegexp.MatchString(key) || v.reservedTagKeys[key] {
 			// Allow reserved keys to be used even if they are not defined in
 			// the vocabulary no matter its strictness.
 			continue
diff --git a/sdk/go/arvados/vocabulary_test.go b/sdk/go/arvados/vocabulary_test.go
index 2025f97b33..c06efda3e0 100644
--- a/sdk/go/arvados/vocabulary_test.go
+++ b/sdk/go/arvados/vocabulary_test.go
@@ -6,6 +6,7 @@ package arvados
 
 import (
 	"encoding/json"
+	"fmt"
 	"regexp"
 	"strings"
 
@@ -301,6 +302,44 @@ func (s *VocabularySuite) TestNewVocabulary(c *check.C) {
 	}
 }
 
+func (s *VocabularySuite) TestValidSystemProperties(c *check.C) {
+	s.testVoc.StrictTags = true
+	properties := map[string]interface{}{
+		"arv:gitBranch": "main",
+		"arv:OK":        true,
+		"arv:cost":      123,
+	}
+	c.Check(s.testVoc.Check(properties), check.IsNil)
+}
+
+func (s *VocabularySuite) TestSystemPropertiesFirstCharacterAlphabetic(c *check.C) {
+	s.testVoc.StrictTags = true
+	properties := map[string]interface{}{"arv:": "value"}
+	c.Check(s.testVoc.Check(properties), check.NotNil)
+	// If we expand the list of allowed characters in the future, these lists
+	// may need adjustment to match.
+	for _, prefix := range []string{" ", ".", "_", "-", "1"} {
+		for _, suffix := range []string{"", "invalid"} {
+			key := fmt.Sprintf("arv:%s%s", prefix, suffix)
+			properties := map[string]interface{}{key: "value"}
+			c.Check(s.testVoc.Check(properties), check.NotNil)
+		}
+	}
+}
+
+func (s *VocabularySuite) TestSystemPropertiesPrefixTypo(c *check.C) {
+	s.testVoc.StrictTags = true
+	for _, key := range []string{
+		"arv :foo",
+		"arvados",
+		"arvados:foo",
+		"Arv:foo",
+	} {
+		properties := map[string]interface{}{key: "value"}
+		c.Check(s.testVoc.Check(properties), check.NotNil)
+	}
+}
+
 func (s *VocabularySuite) TestValidationErrors(c *check.C) {
 	tests := []struct {
 		name       string
-- 
2.30.2