From 00134ea903391ae1307f82fa09c11fde6d2cf3a7 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Sat, 27 May 2023 18:52:33 +0200 Subject: [PATCH] fix(appset): allow cluster urls to be matched (#13715) * fix: allow cluster urls to be matched Related to #13646, and after discussion with @crenshaw-dev, it turns out that matching on cluster urls is not possible. This is due to the fact that the implementation of `LabelSelectorAsSelector` from `k8s.io/apimachinery` validates that a label value is no longer than 63 characters, and validates that it's alphanumeric. In order to work around that, we'll create our own implementation of `LabelSelectorAsSelector`. This implementation has been copied verbatim, with the difference that in `isValidLabelValue`, we first check if the label value is a valid url. If it is not, we proceed with the label checks as with the original implementation. Apart from that, the only other differences are making as much as possible to be package-private; the intent is to only make `Matches` and `LabelSelectorAsSelector` available from outside the package. Signed-off-by: Blake Pettersson * chore: drop all label value restrictions We want to be more flexible in what we accept in post-selectors, mainly that we want to allow other values than only server urls. For this, we will drop all restrictions that a typical "label value" would typically have. Signed-off-by: Blake Pettersson --------- Signed-off-by: Blake Pettersson --- .../generators/generator_spec_processor.go | 6 +- .../generator_spec_processor_test.go | 183 +++++++++++- applicationset/utils/selector.go | 261 ++++++++++++++++++ 3 files changed, 446 insertions(+), 4 deletions(-) create mode 100644 applicationset/utils/selector.go diff --git a/applicationset/generators/generator_spec_processor.go b/applicationset/generators/generator_spec_processor.go index 419cc2dde9702..09dbccc2d077a 100644 --- a/applicationset/generators/generator_spec_processor.go +++ b/applicationset/generators/generator_spec_processor.go @@ -6,7 +6,6 @@ import ( "github.com/argoproj/argo-cd/v2/applicationset/utils" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" @@ -26,7 +25,10 @@ type TransformResult struct { // Transform a spec generator to list of paramSets and a template func Transform(requestedGenerator argoprojiov1alpha1.ApplicationSetGenerator, allGenerators map[string]Generator, baseTemplate argoprojiov1alpha1.ApplicationSetTemplate, appSet *argoprojiov1alpha1.ApplicationSet, genParams map[string]interface{}) ([]TransformResult, error) { - selector, err := metav1.LabelSelectorAsSelector(requestedGenerator.Selector) + // This is a custom version of the `LabelSelectorAsSelector` that is in k8s.io/apimachinery. This has been copied + // verbatim from that package, with the difference that we do not have any restrictions on label values. This is done + // so that, among other things, we can match on cluster urls. + selector, err := utils.LabelSelectorAsSelector(requestedGenerator.Selector) if err != nil { return nil, fmt.Errorf("error parsing label selector: %w", err) } diff --git a/applicationset/generators/generator_spec_processor_test.go b/applicationset/generators/generator_spec_processor_test.go index f9d8fb415292a..cc03f141f8dab 100644 --- a/applicationset/generators/generator_spec_processor_test.go +++ b/applicationset/generators/generator_spec_processor_test.go @@ -94,8 +94,160 @@ func TestMatchValues(t *testing.T) { } } -func emptyTemplate() argoprojiov1alpha1.ApplicationSetTemplate { - return argoprojiov1alpha1.ApplicationSetTemplate{ +func TestMatchValuesGoTemplate(t *testing.T) { + testCases := []struct { + name string + elements []apiextensionsv1.JSON + selector *metav1.LabelSelector + expected []map[string]interface{} + }{ + { + name: "no filter", + elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "cluster","url": "url"}`)}}, + selector: &metav1.LabelSelector{}, + expected: []map[string]interface{}{{"cluster": "cluster", "url": "url"}}, + }, + { + name: "nil", + elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "cluster","url": "url"}`)}}, + selector: nil, + expected: []map[string]interface{}{{"cluster": "cluster", "url": "url"}}, + }, + { + name: "values.foo should be foo but is ignore element", + elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "cluster","url": "url","values":{"foo":"bar"}}`)}}, + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "values.foo": "foo", + }, + }, + expected: []map[string]interface{}{}, + }, + { + name: "values.foo should be bar", + elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "cluster","url": "url","values":{"foo":"bar"}}`)}}, + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "values.foo": "bar", + }, + }, + expected: []map[string]interface{}{{"cluster": "cluster", "url": "url", "values": map[string]interface{}{"foo": "bar"}}}, + }, + { + name: "values.0 should be bar", + elements: []apiextensionsv1.JSON{{Raw: []byte(`{"cluster": "cluster","url": "url","values":["bar"]}`)}}, + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "values.0": "bar", + }, + }, + expected: []map[string]interface{}{{"cluster": "cluster", "url": "url", "values": []interface{}{"bar"}}}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + var listGenerator = NewListGenerator() + var data = map[string]Generator{ + "List": listGenerator, + } + + applicationSetInfo := argov1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set", + }, + Spec: argov1alpha1.ApplicationSetSpec{ + GoTemplate: true, + }, + } + + results, err := Transform(argov1alpha1.ApplicationSetGenerator{ + Selector: testCase.selector, + List: &argov1alpha1.ListGenerator{ + Elements: testCase.elements, + Template: emptyTemplate(), + }}, + data, + emptyTemplate(), + &applicationSetInfo, nil) + + assert.NoError(t, err) + assert.ElementsMatch(t, testCase.expected, results[0].Params) + }) + } +} + +func TestTransForm(t *testing.T) { + testCases := []struct { + name string + selector *metav1.LabelSelector + expected []map[string]interface{} + }{ + { + name: "server filter", + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"server": "https://production-01.example.com"}, + }, + expected: []map[string]interface{}{{ + "metadata.annotations.foo.argoproj.io": "production", + "metadata.labels.argocd.argoproj.io/secret-type": "cluster", + "metadata.labels.environment": "production", + "metadata.labels.org": "bar", + "name": "production_01/west", + "nameNormalized": "production-01-west", + "server": "https://production-01.example.com", + }}, + }, + { + name: "server filter with long url", + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"server": "https://some-really-long-url-that-will-exceed-63-characters.com"}, + }, + expected: []map[string]interface{}{{ + "metadata.annotations.foo.argoproj.io": "production", + "metadata.labels.argocd.argoproj.io/secret-type": "cluster", + "metadata.labels.environment": "production", + "metadata.labels.org": "bar", + "name": "some-really-long-server-url", + "nameNormalized": "some-really-long-server-url", + "server": "https://some-really-long-url-that-will-exceed-63-characters.com", + }}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + testGenerators := map[string]Generator{ + "Clusters": getMockClusterGenerator(), + } + + applicationSetInfo := argov1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set", + }, + Spec: argov1alpha1.ApplicationSetSpec{}, + } + + results, err := Transform( + argov1alpha1.ApplicationSetGenerator{ + Selector: testCase.selector, + Clusters: &argov1alpha1.ClusterGenerator{ + Selector: metav1.LabelSelector{}, + Template: argov1alpha1.ApplicationSetTemplate{}, + Values: nil, + }}, + testGenerators, + emptyTemplate(), + &applicationSetInfo, nil) + + assert.NoError(t, err) + assert.ElementsMatch(t, testCase.expected, results[0].Params) + }) + } +} + +func emptyTemplate() argov1alpha1.ApplicationSetTemplate { + return argov1alpha1.ApplicationSetTemplate{ Spec: argov1alpha1.ApplicationSpec{ Project: "project", }, @@ -152,8 +304,35 @@ func getMockClusterGenerator() Generator { }, Type: corev1.SecretType("Opaque"), }, + &corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + Kind: "Secret", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "some-really-long-server-url", + Namespace: "namespace", + Labels: map[string]string{ + "argocd.argoproj.io/secret-type": "cluster", + "environment": "production", + "org": "bar", + }, + Annotations: map[string]string{ + "foo.argoproj.io": "production", + }, + }, + Data: map[string][]byte{ + "config": []byte("{}"), + "name": []byte("some-really-long-server-url"), + "server": []byte("https://some-really-long-url-that-will-exceed-63-characters.com"), + }, + Type: corev1.SecretType("Opaque"), + }, } runtimeClusters := []runtime.Object{} + for _, clientCluster := range clusters { + runtimeClusters = append(runtimeClusters, clientCluster) + } appClientset := kubefake.NewSimpleClientset(runtimeClusters...) fakeClient := fake.NewClientBuilder().WithObjects(clusters...).Build() diff --git a/applicationset/utils/selector.go b/applicationset/utils/selector.go new file mode 100644 index 0000000000000..53db73a5b3a48 --- /dev/null +++ b/applicationset/utils/selector.go @@ -0,0 +1,261 @@ +package utils + +import ( + "fmt" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/klog/v2" + "sort" + "strconv" + "strings" +) + +var ( + unaryOperators = []string{ + string(selection.Exists), string(selection.DoesNotExist), + } + binaryOperators = []string{ + string(selection.In), string(selection.NotIn), + string(selection.Equals), string(selection.DoubleEquals), string(selection.NotEquals), + string(selection.GreaterThan), string(selection.LessThan), + } + validRequirementOperators = append(binaryOperators, unaryOperators...) +) + +// Selector represents a label selector. +type Selector interface { + // Matches returns true if this selector matches the given set of labels. + Matches(labels.Labels) bool + + // Add adds requirements to the Selector + Add(r ...Requirement) Selector +} + +type internalSelector []Requirement + +// ByKey sorts requirements by key to obtain deterministic parser +type ByKey []Requirement + +func (a ByKey) Len() int { return len(a) } + +func (a ByKey) Swap(i, j int) { a[i], a[j] = a[j], a[i] } + +func (a ByKey) Less(i, j int) bool { return a[i].key < a[j].key } + +// Matches for a internalSelector returns true if all +// its Requirements match the input Labels. If any +// Requirement does not match, false is returned. +func (s internalSelector) Matches(l labels.Labels) bool { + for ix := range s { + if matches := s[ix].Matches(l); !matches { + return false + } + } + return true +} + +// Add adds requirements to the selector. It copies the current selector returning a new one +func (s internalSelector) Add(reqs ...Requirement) Selector { + ret := make(internalSelector, 0, len(s)+len(reqs)) + ret = append(ret, s...) + ret = append(ret, reqs...) + sort.Sort(ByKey(ret)) + return ret +} + +type nothingSelector struct{} + +func (n nothingSelector) Matches(l labels.Labels) bool { + return false +} + +func (n nothingSelector) Add(r ...Requirement) Selector { + return n +} + +// Nothing returns a selector that matches no labels +func nothing() Selector { + return nothingSelector{} +} + +// Everything returns a selector that matches all labels. +func everything() Selector { + return internalSelector{} +} + +// LabelSelectorAsSelector converts the LabelSelector api type into a struct that implements +// labels.Selector +// Note: This function should be kept in sync with the selector methods in pkg/labels/selector.go +func LabelSelectorAsSelector(ps *v1.LabelSelector) (Selector, error) { + if ps == nil { + return nothing(), nil + } + if len(ps.MatchLabels)+len(ps.MatchExpressions) == 0 { + return everything(), nil + } + requirements := make([]Requirement, 0, len(ps.MatchLabels)+len(ps.MatchExpressions)) + for k, v := range ps.MatchLabels { + r, err := newRequirement(k, selection.Equals, []string{v}) + if err != nil { + return nil, err + } + requirements = append(requirements, *r) + } + for _, expr := range ps.MatchExpressions { + var op selection.Operator + switch expr.Operator { + case v1.LabelSelectorOpIn: + op = selection.In + case v1.LabelSelectorOpNotIn: + op = selection.NotIn + case v1.LabelSelectorOpExists: + op = selection.Exists + case v1.LabelSelectorOpDoesNotExist: + op = selection.DoesNotExist + default: + return nil, fmt.Errorf("%q is not a valid pod selector operator", expr.Operator) + } + r, err := newRequirement(expr.Key, op, append([]string(nil), expr.Values...)) + if err != nil { + return nil, err + } + requirements = append(requirements, *r) + } + selector := newSelector() + selector = selector.Add(requirements...) + return selector, nil +} + +// NewSelector returns a nil selector +func newSelector() Selector { + return internalSelector(nil) +} + +func validateLabelKey(k string, path *field.Path) *field.Error { + if errs := validation.IsQualifiedName(k); len(errs) != 0 { + return field.Invalid(path, k, strings.Join(errs, "; ")) + } + return nil +} + +// NewRequirement is the constructor for a Requirement. +// If any of these rules is violated, an error is returned: +// (1) The operator can only be In, NotIn, Equals, DoubleEquals, Gt, Lt, NotEquals, Exists, or DoesNotExist. +// (2) If the operator is In or NotIn, the values set must be non-empty. +// (3) If the operator is Equals, DoubleEquals, or NotEquals, the values set must contain one value. +// (4) If the operator is Exists or DoesNotExist, the value set must be empty. +// (5) If the operator is Gt or Lt, the values set must contain only one value, which will be interpreted as an integer. +// (6) The key is invalid due to its length, or sequence +// +// of characters. See validateLabelKey for more details. +// +// The empty string is a valid value in the input values set. +// Returned error, if not nil, is guaranteed to be an aggregated field.ErrorList +func newRequirement(key string, op selection.Operator, vals []string, opts ...field.PathOption) (*Requirement, error) { + var allErrs field.ErrorList + path := field.ToPath(opts...) + if err := validateLabelKey(key, path.Child("key")); err != nil { + allErrs = append(allErrs, err) + } + + valuePath := path.Child("values") + switch op { + case selection.In, selection.NotIn: + if len(vals) == 0 { + allErrs = append(allErrs, field.Invalid(valuePath, vals, "for 'in', 'notin' operators, values set can't be empty")) + } + case selection.Equals, selection.DoubleEquals, selection.NotEquals: + if len(vals) != 1 { + allErrs = append(allErrs, field.Invalid(valuePath, vals, "exact-match compatibility requires one single value")) + } + case selection.Exists, selection.DoesNotExist: + if len(vals) != 0 { + allErrs = append(allErrs, field.Invalid(valuePath, vals, "values set must be empty for exists and does not exist")) + } + case selection.GreaterThan, selection.LessThan: + if len(vals) != 1 { + allErrs = append(allErrs, field.Invalid(valuePath, vals, "for 'Gt', 'Lt' operators, exactly one value is required")) + } + for i := range vals { + if _, err := strconv.ParseInt(vals[i], 10, 64); err != nil { + allErrs = append(allErrs, field.Invalid(valuePath.Index(i), vals[i], "for 'Gt', 'Lt' operators, the value must be an integer")) + } + } + default: + allErrs = append(allErrs, field.NotSupported(path.Child("operator"), op, validRequirementOperators)) + } + + return &Requirement{key: key, operator: op, strValues: vals}, allErrs.ToAggregate() +} + +// Requirement contains values, a key, and an operator that relates the key and values. +// The zero value of Requirement is invalid. +// Requirement implements both set based match and exact match +// Requirement should be initialized via NewRequirement constructor for creating a valid Requirement. +// +k8s:deepcopy-gen=true +type Requirement struct { + key string + operator selection.Operator + // In the majority of cases we have at most one value here. + // It is generally faster to operate on a single-element slice + // than on a single-element map, so we have a slice here. + strValues []string +} + +func (r *Requirement) hasValue(value string) bool { + for i := range r.strValues { + if r.strValues[i] == value { + return true + } + } + return false +} + +func (r *Requirement) Matches(ls labels.Labels) bool { + switch r.operator { + case selection.In, selection.Equals, selection.DoubleEquals: + if !ls.Has(r.key) { + return false + } + return r.hasValue(ls.Get(r.key)) + case selection.NotIn, selection.NotEquals: + if !ls.Has(r.key) { + return true + } + return !r.hasValue(ls.Get(r.key)) + case selection.Exists: + return ls.Has(r.key) + case selection.DoesNotExist: + return !ls.Has(r.key) + case selection.GreaterThan, selection.LessThan: + if !ls.Has(r.key) { + return false + } + lsValue, err := strconv.ParseInt(ls.Get(r.key), 10, 64) + if err != nil { + klog.V(10).Infof("ParseInt failed for value %+v in label %+v, %+v", ls.Get(r.key), ls, err) + return false + } + + // There should be only one strValue in r.strValues, and can be converted to an integer. + if len(r.strValues) != 1 { + klog.V(10).Infof("Invalid values count %+v of requirement %#v, for 'Gt', 'Lt' operators, exactly one value is required", len(r.strValues), r) + return false + } + + var rValue int64 + for i := range r.strValues { + rValue, err = strconv.ParseInt(r.strValues[i], 10, 64) + if err != nil { + klog.V(10).Infof("ParseInt failed for value %+v in requirement %#v, for 'Gt', 'Lt' operators, the value must be an integer", r.strValues[i], r) + return false + } + } + return (r.operator == selection.GreaterThan && lsValue > rValue) || (r.operator == selection.LessThan && lsValue < rValue) + default: + return false + } +}