manager: return custom annotation validation errors instead ignore them

rpaas/k8s: fix lint

internal/pkg/rpaas/k8s.go

@@ -264,7 +264,11 @@ func (m *k8sRpaasManager) CreateInstance(ctx context.Context, args CreateArgs) e
 	}
 
 	setDescription(instance, args.Description)
-	setAnnotations(instance, args.Annotations())
+	annotations, err := args.Annotations()
+	if err != nil {
+		return err
+	}
+	setAnnotations(instance, annotations)
 	instance.SetTeamOwner(args.Team)
 	if m.clusterName != "" {
 		instance.SetClusterName(m.clusterName)
@@ -281,12 +285,13 @@ func (m *k8sRpaasManager) CreateInstance(ctx context.Context, args CreateArgs) e
 }
 
 func (m *k8sRpaasManager) UpdateInstance(ctx context.Context, instanceName string, args UpdateInstanceArgs) error {
+	var err error
 	instance, err := m.GetInstance(ctx, instanceName)
 	if err != nil {
 		return err
 	}
 
-	if err := m.validateUpdateInstanceArgs(ctx, instance, args); err != nil {
+	if err = m.validateUpdateInstanceArgs(ctx, instance, args); err != nil {
 		return err
 	}
 
@@ -303,7 +308,11 @@ func (m *k8sRpaasManager) UpdateInstance(ctx context.Context, instanceName strin
 	if m.clusterName != "" {
 		instance.SetClusterName(m.clusterName)
 	}
-	setAnnotations(instance, args.Annotations())
+	annotations, err := args.Annotations()
+	if err != nil {
+		return err
+	}
+	setAnnotations(instance, annotations)
 	setTags(instance, args.Tags)
 	setIP(instance, args.IP())
 	setLoadBalancerName(instance, args.LoadBalancerName())

internal/pkg/rpaas/k8s_test.go

@@ -7,6 +7,7 @@ package rpaas
 import (
 	"context"
 	"crypto/tls"
+	"fmt"
 	"net/http"
 	"regexp"
 	"testing"
@@ -23,6 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	metricsv1beta1 "k8s.io/metrics/pkg/apis/metrics/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -3098,7 +3100,7 @@ func Test_k8sRpaasManager_CreateInstance(t *testing.T) {
 		},
 		{
 			name:        "with custom annotations only set allowed ones",
-			args:        CreateArgs{Name: "r1", Team: "t1", Parameters: map[string]interface{}{"annotations": "{\"my-custom-annotation\": \"my-value\", \"rpaas.extensions.tsuru.io/tags\": \"tag1,tag2\", \"rpaas.extensions.tsuru.io/description\": \"my description\"}"}},
+			args:        CreateArgs{Name: "r1", Team: "t1", Parameters: map[string]interface{}{"annotations": "{\"my-custom-annotation\": \"my-value\"}"}},
 			extraConfig: config.RpaasConfig{ForbiddenAnnotationsPrefixes: []string{"rpaas.extensions.tsuru.io"}},
 			expected: v1alpha1.RpaasInstance{
 				TypeMeta: metav1.TypeMeta{
@@ -3147,6 +3149,12 @@ func Test_k8sRpaasManager_CreateInstance(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:          "with custom annotations and forbidden prefixes",
+			args:          CreateArgs{Name: "r1", Team: "t1", Parameters: map[string]interface{}{"annotations": "{\"my-custom-annotation\": \"my-value\", \"rpaas.extensions.tsuru.io/tags\": \"tag1,tag2\", \"rpaas.extensions.tsuru.io/description\": \"my description\"}"}},
+			extraConfig:   config.RpaasConfig{ForbiddenAnnotationsPrefixes: []string{"rpaas.extensions.tsuru.io"}},
+			expectedError: `annotation "rpaas.extensions.tsuru.io/tags" is not allowed`,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -3310,6 +3318,23 @@ func Test_k8sRpaasManager_UpdateInstance(t *testing.T) {
 				assert.Equal(t, `cannot unset flavor "feature-create-only" as it is a creation only flavor`, err.Error())
 			},
 		},
+		{
+			name:     "when tries to set an invalid annotation format",
+			instance: "instance1",
+			args: UpdateInstanceArgs{
+				Description: "Another description",
+				Plan:        "plan2",
+				Tags:        []string{},
+				Team:        "team-two",
+				Parameters: map[string]interface{}{
+					"annotations": "{\"test.io/_my-custom-annotation\": \"my-value\"}",
+				},
+			},
+			assertion: func(t *testing.T, err error, instance *v1alpha1.RpaasInstance) {
+				require.Error(t, err)
+				assert.Equal(t, fmt.Sprintf(`invalid annotation "test.io/_my-custom-annotation": %v`, validation.IsValidLabelValue("test.io/_my-custom-annotation")), err.Error())
+			},
+		},
 		{
 			name:     "when successfully updating an instance",
 			instance: "instance1",
@@ -3320,7 +3345,7 @@ func Test_k8sRpaasManager_UpdateInstance(t *testing.T) {
 				Team:        "team-two",
 				Parameters: map[string]interface{}{
 					"lb-name":     "my-instance.example",
-					"annotations": "{\"my-custom-annotation\": \"my-value\", \"rpaas.extensions.tsuru.io/tags\": \"tag1,tag2\", \"rpaas.extensions.tsuru.io/description\": \"my description\"}",
+					"annotations": "{\"my-custom-annotation\": \"my-value\"}",
 				},
 			},
 			assertion: func(t *testing.T, err error, instance *v1alpha1.RpaasInstance) {

internal/pkg/rpaas/manager.go

@@ -112,7 +112,7 @@ func (args CreateArgs) PlanOverride() string {
 	return getPlanOverride(args.Parameters, args.Tags)
 }
 
-func (args CreateArgs) Annotations() map[string]string {
+func (args CreateArgs) Annotations() (map[string]string, error) {
 	return getAnnotations(args.Parameters)
 }
 
@@ -140,7 +140,7 @@ func (args UpdateInstanceArgs) PlanOverride() string {
 	return getPlanOverride(args.Parameters, args.Tags)
 }
 
-func (args UpdateInstanceArgs) Annotations() map[string]string {
+func (args UpdateInstanceArgs) Annotations() (map[string]string, error) {
 	return getAnnotations(args.Parameters)
 }
 
@@ -262,46 +262,43 @@ type CertificateData struct {
 	Key         string `json:"key"`
 }
 
-func getAnnotations(params map[string]interface{}) (annotations map[string]string) {
+func getAnnotations(params map[string]interface{}) (map[string]string, error) {
 	p, found := params["annotations"]
 	if !found {
-		return
+		return nil, nil
 	}
 	annotationsStr, ok := p.(string)
 	if !ok {
-		return
+		return nil, fmt.Errorf("invalid annotations: %v", p)
 	}
-
+	annotations := map[string]string{}
 	err := json.Unmarshal([]byte(annotationsStr), &annotations)
 	if err != nil {
-		return
+		return nil, err
 	}
 	forbiddenAnnotationsPrefixes := config.Get().ForbiddenAnnotationsPrefixes
 	for k := range annotations {
 		// check if annotation complies with allowed k8s annotations
 		annotationParts := strings.Split(k, "/")
 		if len(annotationParts) > 1 {
 			if errs := validation.IsDNS1123Subdomain(annotationParts[0]); len(errs) != 0 {
-				delete(annotations, k)
-				continue
+				return nil, fmt.Errorf("invalid annotation %q: %s", k, errs)
 			}
 			if errs := validation.IsValidLabelValue(annotationParts[1]); len(errs) != 0 {
-				delete(annotations, k)
-				continue
+				return nil, fmt.Errorf("invalid annotation %q: %s", k, errs)
 			}
 		} else {
 			if errs := validation.IsValidLabelValue(annotationParts[0]); len(errs) != 0 {
-				delete(annotations, k)
-				continue
+				return nil, fmt.Errorf("invalid annotation %q: %s", k, errs)
 			}
 		}
 		for _, prefix := range forbiddenAnnotationsPrefixes {
 			if strings.HasPrefix(k, prefix) {
-				delete(annotations, k)
+				return nil, fmt.Errorf("annotation %q is not allowed", k)
 			}
 		}
 	}
-	return annotations
+	return annotations, nil
 }
 
 func getFlavors(params map[string]interface{}, tags []string) (flavors []string) {

internal/pkg/rpaas/manager_test.go

@@ -5,11 +5,13 @@
 package rpaas
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/tsuru/rpaas-operator/internal/config"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 func TestCreateArgs_Flavors(t *testing.T) {
@@ -323,29 +325,40 @@ func TestUpdateInstanceArgs_PlanOverride(t *testing.T) {
 }
 func TestUpdateInstanceArgs_Annotations(t *testing.T) {
 	tests := []struct {
-		args UpdateInstanceArgs
-		want map[string]string
+		args          UpdateInstanceArgs
+		want          map[string]string
+		expectedError string
 	}{
 		{},
 		{
 			args: UpdateInstanceArgs{
-				Parameters: map[string]interface{}{"annotations": `{"image": "nginx:alpine", "rpaas.extensions.tsuru.io/test": "not valid", "test.io/valid": "valid"}`},
+				Parameters: map[string]interface{}{"annotations": `{"image": "nginx:alpine", "test.io/valid": "valid"}`},
 			},
 			want: map[string]string{"image": "nginx:alpine", "test.io/valid": "valid"},
 		},
+		{
+			args: UpdateInstanceArgs{
+				Parameters: map[string]interface{}{"annotations": `{"image": "nginx:alpine", "rpaas.extensions.tsuru.io/test": "not valid", "test.io/valid": "valid"}`},
+			},
+			expectedError: "annotation \"rpaas.extensions.tsuru.io/test\" is not allowed",
+		},
 		{
 			args: UpdateInstanceArgs{
 				Parameters: map[string]interface{}{"annotations": `{"invalid_domain.io/test": "test"}`},
 			},
-			want: map[string]string{},
+			expectedError: fmt.Sprintf("invalid annotation \"invalid_domain.io/test\": %s", validation.IsDNS1123Subdomain("invalid_domain.io/test")),
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run("", func(t *testing.T) {
 			err := config.Init()
 			require.NoError(t, err)
-			have := tt.args.Annotations()
+			have, err := tt.args.Annotations()
+			if tt.expectedError != "" {
+				assert.EqualError(t, err, tt.expectedError)
+				return
+			}
 			assert.Equal(t, tt.want, have)
 		})
 	}