Skip to content

Commit 003b7e8

Browse files
Jonathan S. Katzjkatz
Jonathan S. Katz
authored and
jkatz
committed
Refactor of label validation function
This moves the function to a common area in the API code and adds some much needed testing to it.
1 parent 5c56a34 commit 003b7e8

File tree

3 files changed

+122
-58
lines changed

3 files changed

+122
-58
lines changed

‎internal/apiserver/common.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
kerrors "k8s.io/apimachinery/pkg/api/errors"
2828
"k8s.io/apimachinery/pkg/api/resource"
2929
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
30+
"k8s.io/apimachinery/pkg/util/validation"
3031
)
3132

3233
const (
@@ -44,6 +45,8 @@ var (
4445
// ErrDBContainerNotFound is an error that indicates that a "database" container
4546
// could not be found in a specific pod
4647
ErrDBContainerNotFound = errors.New("\"database\" container not found in pod")
48+
// ErrLabelInvalid indicates that a label is invalid
49+
ErrLabelInvalid = errors.New("invalid label")
4750
// ErrStandbyNotAllowed contains the error message returned when an API call is not
4851
// permitted because it involves a cluster that is in standby mode
4952
ErrStandbyNotAllowed = errors.New("Action not permitted because standby mode is enabled")
@@ -127,6 +130,60 @@ func ValidateBackrestStorageTypeForCommand(cluster *crv1.Pgcluster, storageTypeS
127130
return nil
128131
}
129132

133+
// ValidateLabel is derived from a legacy method and validates if the input is a
134+
// valid Kubernetes label.
135+
//
136+
// A label is composed of a key and value.
137+
//
138+
// The key can either be a name or have an optional prefix that i
139+
// terminated by a "/", e.g. "prefix/name"
140+
//
141+
// The name must be a valid DNS 1123 value
142+
// THe prefix must be a valid DNS 1123 subdomain
143+
//
144+
// The value can be validated by machinery provided by Kubenretes
145+
//
146+
// Ref: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/
147+
func ValidateLabel(labelStr string) (map[string]string, error) {
148+
labelMap := map[string]string{}
149+
150+
for _, v := range strings.Split(labelStr, ",") {
151+
pair := strings.Split(v, "=")
152+
if len(pair) != 2 {
153+
return labelMap, fmt.Errorf("%w: label format incorrect, requires key=value", ErrLabelInvalid)
154+
}
155+
156+
// first handle the key
157+
keyParts := strings.Split(pair[0], "/")
158+
159+
switch len(keyParts) {
160+
default:
161+
return labelMap, fmt.Errorf("%w: invalid key for "+v, ErrLabelInvalid)
162+
case 2:
163+
if errs := validation.IsDNS1123Subdomain(keyParts[0]); len(errs) > 0 {
164+
return labelMap, fmt.Errorf("%w: invalid key for %s: %s", ErrLabelInvalid, v, strings.Join(errs, ","))
165+
}
166+
167+
if errs := validation.IsDNS1123Label(keyParts[1]); len(errs) > 0 {
168+
return labelMap, fmt.Errorf("%w: invalid key for %s: %s", ErrLabelInvalid, v, strings.Join(errs, ","))
169+
}
170+
case 1:
171+
if errs := validation.IsDNS1123Label(keyParts[0]); len(errs) > 0 {
172+
return labelMap, fmt.Errorf("%w: invalid key for %s: %s", ErrLabelInvalid, v, strings.Join(errs, ","))
173+
}
174+
}
175+
176+
// handle the value
177+
if errs := validation.IsValidLabelValue(pair[1]); len(errs) > 0 {
178+
return labelMap, fmt.Errorf("%w: invalid value for %s: %s", ErrLabelInvalid, v, strings.Join(errs, ","))
179+
}
180+
181+
labelMap[pair[0]] = pair[1]
182+
}
183+
184+
return labelMap, nil
185+
}
186+
130187
// ValidateResourceRequestLimit validates that a Kubernetes Requests/Limit pair
131188
// is valid, both by validating the values are valid quantity values, and then
132189
// by checking that the limit >= request. This also needs to check against the

‎internal/apiserver/common_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ limitations under the License.
1616
*/
1717

1818
import (
19+
"errors"
20+
"fmt"
21+
"strings"
1922
"testing"
2023

2124
crv1 "github.com/crunchydata/postgres-operator/pkg/apis/crunchydata.com/v1"
@@ -164,6 +167,66 @@ func TestValidateBackrestStorageTypeForCommand(t *testing.T) {
164167
})
165168
}
166169

170+
func TestValidateLabel(t *testing.T) {
171+
t.Run("valid", func(t *testing.T) {
172+
inputs := []map[string]string{
173+
map[string]string{"key": "value"},
174+
map[string]string{"example.com/key": "value"},
175+
map[string]string{"key1": "value1", "key2": "value2"},
176+
}
177+
178+
for _, input := range inputs {
179+
labelStr := ""
180+
181+
for k, v := range input {
182+
labelStr += fmt.Sprintf("%s=%s,", k, v)
183+
}
184+
185+
labelStr = strings.Trim(labelStr, ",")
186+
187+
t.Run(labelStr, func(*testing.T) {
188+
labels, err := ValidateLabel(labelStr)
189+
190+
if err != nil {
191+
t.Fatalf("expected no error, got: %s", err.Error())
192+
}
193+
194+
for k := range labels {
195+
if v, ok := input[k]; !(ok || v == labels[k]) {
196+
t.Fatalf("label values do not matched (%s vs. %s)", input[k], labels[k])
197+
}
198+
}
199+
})
200+
}
201+
})
202+
203+
t.Run("invalid", func(t *testing.T) {
204+
inputs := []string{
205+
"key",
206+
"key=value=value",
207+
"key=value,",
208+
"b@d=value",
209+
"b@d-prefix/key=value",
210+
"really/bad/prefix/key=value",
211+
"key=v\\alue",
212+
}
213+
214+
for _, input := range inputs {
215+
t.Run(input, func(t *testing.T) {
216+
_, err := ValidateLabel(input)
217+
218+
if err == nil {
219+
t.Fatalf("expected an invalid input error.")
220+
}
221+
222+
if !errors.Is(err, ErrLabelInvalid) {
223+
t.Fatalf("expected an ErrLabelInvalid error.")
224+
}
225+
})
226+
}
227+
})
228+
}
229+
167230
func TestValidateResourceRequestLimit(t *testing.T) {
168231
t.Run("valid", func(t *testing.T) {
169232
resources := []struct{ request, limit, defaultRequest string }{

‎internal/apiserver/labelservice/labelimpl.go‎

Lines changed: 2 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@ limitations under the License.
1717

1818
import (
1919
"context"
20-
"fmt"
21-
"strings"
2220

2321
"github.com/crunchydata/postgres-operator/internal/apiserver"
2422
"github.com/crunchydata/postgres-operator/internal/config"
@@ -29,7 +27,6 @@ import (
2927
log "github.com/sirupsen/logrus"
3028
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3129
"k8s.io/apimachinery/pkg/types"
32-
"k8s.io/apimachinery/pkg/util/validation"
3330
)
3431

3532
// Label ... 2 forms ...
@@ -50,7 +47,7 @@ func Label(request *msgs.LabelRequest, ns, pgouser string) msgs.LabelResponse {
5047
return resp
5148
}
5249

53-
labelsMap, err = validateLabel(request.LabelCmdLabel)
50+
labelsMap, err = apiserver.ValidateLabel(request.LabelCmdLabel)
5451
if err != nil {
5552
resp.Status.Code = msgs.Error
5653
resp.Status.Msg = err.Error()
@@ -181,59 +178,6 @@ func addLabels(items []crv1.Pgcluster, DryRun bool, LabelCmdLabel string, newLab
181178
}
182179
}
183180

184-
// validateLabel validates if the input is a valid Kubernetes label
185-
//
186-
// A label is composed of a key and value.
187-
//
188-
// The key can either be a name or have an optional prefix that i
189-
// terminated by a "/", e.g. "prefix/name"
190-
//
191-
// The name must be a valid DNS 1123 value
192-
// THe prefix must be a valid DNS 1123 subdomain
193-
//
194-
// The value can be validated by machinery provided by Kubenretes
195-
//
196-
// Ref: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/
197-
func validateLabel(LabelCmdLabel string) (map[string]string, error) {
198-
labelMap := map[string]string{}
199-
200-
for _, v := range strings.Split(LabelCmdLabel, ",") {
201-
pair := strings.Split(v, "=")
202-
if len(pair) != 2 {
203-
return labelMap, fmt.Errorf("label format incorrect, requires key=value")
204-
}
205-
206-
// first handle the key
207-
keyParts := strings.Split(pair[0], "/")
208-
209-
switch len(keyParts) {
210-
default:
211-
return labelMap, fmt.Errorf("invalid key for " + v)
212-
case 2:
213-
if errs := validation.IsDNS1123Subdomain(keyParts[0]); len(errs) > 0 {
214-
return labelMap, fmt.Errorf("invalid key for %s: %s", v, strings.Join(errs, ","))
215-
}
216-
217-
if errs := validation.IsDNS1123Label(keyParts[1]); len(errs) > 0 {
218-
return labelMap, fmt.Errorf("invalid key for %s: %s", v, strings.Join(errs, ","))
219-
}
220-
case 1:
221-
if errs := validation.IsDNS1123Label(keyParts[0]); len(errs) > 0 {
222-
return labelMap, fmt.Errorf("invalid key for %s: %s", v, strings.Join(errs, ","))
223-
}
224-
}
225-
226-
// handle the value
227-
if errs := validation.IsValidLabelValue(pair[1]); len(errs) > 0 {
228-
return labelMap, fmt.Errorf("invalid value for %s: %s", v, strings.Join(errs, ","))
229-
}
230-
231-
labelMap[pair[0]] = pair[1]
232-
}
233-
234-
return labelMap, nil
235-
}
236-
237181
// DeleteLabel ...
238182
// pgo delete label mycluster yourcluster --label=env=prod
239183
// pgo delete label --label=env=prod --selector=group=somegroup
@@ -252,7 +196,7 @@ func DeleteLabel(request *msgs.DeleteLabelRequest, ns string) msgs.LabelResponse
252196
return resp
253197
}
254198

255-
labelsMap, err = validateLabel(request.LabelCmdLabel)
199+
labelsMap, err = apiserver.ValidateLabel(request.LabelCmdLabel)
256200
if err != nil {
257201
resp.Status.Code = msgs.Error
258202
resp.Status.Msg = "labels not formatted correctly"

0 commit comments

Comments
 (0)