diff --git a/internal/pkg/rpaas/k8s.go b/internal/pkg/rpaas/k8s.go index 66ebeef4e..808daa4a6 100644 --- a/internal/pkg/rpaas/k8s.go +++ b/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) @@ -303,7 +307,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()) diff --git a/internal/pkg/rpaas/k8s_test.go b/internal/pkg/rpaas/k8s_test.go index 77a73aeb4..53cf6bf15 100644 --- a/internal/pkg/rpaas/k8s_test.go +++ b/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) { diff --git a/internal/pkg/rpaas/manager.go b/internal/pkg/rpaas/manager.go index 40e84c8ca..774f03771 100644 --- a/internal/pkg/rpaas/manager.go +++ b/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,19 +262,19 @@ 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 { @@ -282,26 +282,23 @@ func getAnnotations(params map[string]interface{}) (annotations map[string]strin 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) { diff --git a/internal/pkg/rpaas/manager_test.go b/internal/pkg/rpaas/manager_test.go index 8cb5413bb..b7ac9e01a 100644 --- a/internal/pkg/rpaas/manager_test.go +++ b/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,21 +325,28 @@ 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")), }, } @@ -345,7 +354,11 @@ func TestUpdateInstanceArgs_Annotations(t *testing.T) { 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) }) }