Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ingress annotation validation earlier in k8s lifecycle #1270

Merged
merged 9 commits into from
Dec 8, 2020
Merged

Ingress annotation validation earlier in k8s lifecycle #1270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f4abacc
Ingress annotation validation earlier in k8s lifecycle
Dec 7, 2020
8de2ac5
Add a comment describing how annotation validations are run
Dec 7, 2020
1e79a56
Correct output messages from parsing helper tests
Dec 8, 2020
0a5c8e2
Sort ingress annotation validation errors alphabetically
Dec 8, 2020
d3e6b92
Refactor validateIngressAnnotations func
Dec 8, 2020
d16827a
Change unexported functions back to private
Dec 8, 2020
9d2ee77
Add earlier validation of nginx.com/slow-start annotation
Dec 8, 2020
1925a4b
Separate out nginx and nginx plus validation
Dec 8, 2020
255f29c
Fix golint issues in parsing_helpers.go
Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 13 additions & 25 deletions internal/configs/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,41 +93,29 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool
if err != nil {
glog.Error(err)
}
if isPlus {
mikestephen marked this conversation as resolved.
Show resolved Hide resolved
cfgParams.HealthCheckEnabled = healthCheckEnabled
} else {
glog.Warning("Annotation 'nginx.com/health-checks' requires NGINX Plus")
}
cfgParams.HealthCheckEnabled = healthCheckEnabled
}

if cfgParams.HealthCheckEnabled {
if healthCheckMandatory, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory", ingEx.Ingress); exists {
if err != nil {
glog.Error(err)
}
cfgParams.HealthCheckMandatory = healthCheckMandatory
if healthCheckMandatory, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory", ingEx.Ingress); exists {
if err != nil {
glog.Error(err)
}
cfgParams.HealthCheckMandatory = healthCheckMandatory
}

if cfgParams.HealthCheckMandatory {
if healthCheckQueue, exists, err := GetMapKeyAsInt64(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory-queue", ingEx.Ingress); exists {
if err != nil {
glog.Error(err)
}
cfgParams.HealthCheckMandatoryQueue = healthCheckQueue
if healthCheckQueue, exists, err := GetMapKeyAsInt64(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory-queue", ingEx.Ingress); exists {
if err != nil {
glog.Error(err)
}
cfgParams.HealthCheckMandatoryQueue = healthCheckQueue
}

if slowStart, exists := ingEx.Ingress.Annotations["nginx.com/slow-start"]; exists {
if parsedSlowStart, err := ParseTime(slowStart); err != nil {
glog.Errorf("Ingress %s/%s: Invalid value nginx.org/slow-start: got %q: %v", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName(), slowStart, err)
} else {
if isPlus {
cfgParams.SlowStart = parsedSlowStart
} else {
glog.Warning("Annotation 'nginx.com/slow-start' requires NGINX Plus")
}
parsedSlowStart, err := ParseTime(slowStart)
if err != nil {
glog.Error(err)
}
cfgParams.SlowStart = parsedSlowStart
}

if serverTokens, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/server-tokens", ingEx.Ingress); exists {
Expand Down
72 changes: 55 additions & 17 deletions internal/configs/parsing_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type apiObject interface {
// GetMapKeyAsBool searches the map for the given key and parses the key as bool.
func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool, bool, error) {
if str, exists := m[key]; exists {
b, err := strconv.ParseBool(str)
b, err := ParseBool(str)
if err != nil {
return false, exists, fmt.Errorf("%s %v/%v '%s' contains invalid bool: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
}
Expand All @@ -35,7 +35,7 @@ func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool,
// GetMapKeyAsInt tries to find and parse a key in a map as int.
func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int, bool, error) {
if str, exists := m[key]; exists {
i, err := strconv.Atoi(str)
i, err := ParseInt(str)
if err != nil {
return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
}
Expand All @@ -49,7 +49,7 @@ func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int, bo
// GetMapKeyAsInt64 tries to find and parse a key in a map as int64.
func GetMapKeyAsInt64(m map[string]string, key string, context apiObject) (int64, bool, error) {
if str, exists := m[key]; exists {
i, err := strconv.ParseInt(str, 10, 64)
i, err := ParseInt64(str)
if err != nil {
return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
}
Expand All @@ -63,7 +63,7 @@ func GetMapKeyAsInt64(m map[string]string, key string, context apiObject) (int64
// GetMapKeyAsUint64 tries to find and parse a key in a map as uint64.
func GetMapKeyAsUint64(m map[string]string, key string, context apiObject, nonZero bool) (uint64, bool, error) {
if str, exists := m[key]; exists {
i, err := strconv.ParseUint(str, 10, 64)
i, err := ParseUint64(str)
if err != nil {
return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid uint64: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err)
}
Expand Down Expand Up @@ -162,31 +162,69 @@ func validateHashLBMethod(method string) (string, error) {
return "", fmt.Errorf("Invalid load balancing method: %q", method)
}

// http://nginx.org/en/docs/syntax.html
var validTimeSuffixes = []string{
"ms",
"s",
"m",
"h",
"d",
"w",
"M",
"y",
// ParseBool ensures that the string value is a valid bool
func ParseBool(s string) (bool, error) {
mikestephen marked this conversation as resolved.
Show resolved Hide resolved
return strconv.ParseBool(s)
}

var durationEscaped = strings.Join(validTimeSuffixes, "|")
var validNginxTime = regexp.MustCompile(`^([0-9]+([` + durationEscaped + `]?){0,1} *)+$`)
// ParseInt ensures that the string value is a valid int
func ParseInt(s string) (int, error) {
return strconv.Atoi(s)
}

// ParseInt64 ensures that the string value is a valid int64
func ParseInt64(s string) (int64, error) {
return strconv.ParseInt(s, 10, 64)
}

// ParseUint64 ensures that the string value is a valid uint64
func ParseUint64(s string) (uint64, error) {
return strconv.ParseUint(s, 10, 64)
}

// timeRegexp http://nginx.org/en/docs/syntax.html
var timeRegexp = regexp.MustCompile(`^([0-9]+([ms|s|m|h|d|w|M|y]?){0,1} *)+$`)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building the regexp from the slice of time units didn't seem to be worth the effort.


// ParseTime ensures that the string value in the annotation is a valid time.
func ParseTime(s string) (string, error) {
s = strings.TrimSpace(s)

if validNginxTime.MatchString(s) {
if timeRegexp.MatchString(s) {
return s, nil
}
return "", errors.New("Invalid time string")
}

// OffsetFmt http://nginx.org/en/docs/syntax.html
const OffsetFmt = `\d+[kKmMgG]?`

var offsetRegexp = regexp.MustCompile("^" + OffsetFmt + "$")

// ParseOffset ensures that the string value is a valid offset
func ParseOffset(s string) (string, error) {
s = strings.TrimSpace(s)

if offsetRegexp.MatchString(s) {
return s, nil
}
return "", errors.New("Invalid offset string")
}

// SizeFmt http://nginx.org/en/docs/syntax.html
const SizeFmt = `\d+[kKmM]?`

var sizeRegexp = regexp.MustCompile("^" + SizeFmt + "$")

// ParseSize ensures that the string value is a valid size
func ParseSize(s string) (string, error) {
s = strings.TrimSpace(s)

if sizeRegexp.MatchString(s) {
return s, nil
}
return "", errors.New("Invalid size string")
}

var threshEx = regexp.MustCompile(`high=([1-9]|[1-9][0-9]|100) low=([1-9]|[1-9][0-9]|100)\b`)
var threshExR = regexp.MustCompile(`low=([1-9]|[1-9][0-9]|100) high=([1-9]|[1-9][0-9]|100)\b`)

Expand Down
189 changes: 189 additions & 0 deletions internal/configs/parsing_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,46 @@ func TestParseTime(t *testing.T) {
}
}

func TestParseOffset(t *testing.T) {
mikestephen marked this conversation as resolved.
Show resolved Hide resolved
var testsWithValidInput = []string{"1", "2k", "2K", "3m", "3M", "4g", "4G"}
var invalidInput = []string{"-1", "", "blah"}
for _, test := range testsWithValidInput {
result, err := ParseOffset(test)
if err != nil {
t.Errorf("TestParseOffset(%q) returned an error for valid input", test)
}
if test != result {
t.Errorf("TestParseOffset(%q) returned %q expected %q", test, result, test)
}
}
for _, test := range invalidInput {
result, err := ParseOffset(test)
if err == nil {
t.Errorf("TestParseOffset(%q) didn't return error. Returned: %q", test, result)
}
}
}

func TestParseSize(t *testing.T) {
var testsWithValidInput = []string{"1", "2k", "2K", "3m", "3M"}
var invalidInput = []string{"-1", "", "blah", "4g", "4G"}
for _, test := range testsWithValidInput {
result, err := ParseSize(test)
if err != nil {
t.Errorf("TestParseSize(%q) returned an error for valid input", test)
}
if test != result {
t.Errorf("TestParseSize(%q) returned %q expected %q", test, result, test)
}
}
for _, test := range invalidInput {
result, err := ParseSize(test)
if err == nil {
t.Errorf("TestParseSize(%q) didn't return error. Returned: %q", test, result)
}
}
}

func TestVerifyThresholds(t *testing.T) {
validInput := []string{
"high=3 low=1",
Expand Down Expand Up @@ -416,3 +456,152 @@ func TestVerifyThresholds(t *testing.T) {
}
}
}

func TestParseBool(t *testing.T) {
var testsWithValidInput = []struct {
input string
expected bool
}{
{"0", false},
{"1", true},
{"true", true},
{"false", false},
}

var invalidInput = []string{
"",
"blablah",
"-100",
"-1",
}

for _, test := range testsWithValidInput {
result, err := ParseBool(test.input)
if err != nil {
t.Errorf("TestParseBool(%q) returned an error for valid input", test.input)
}

if result != test.expected {
t.Errorf("TestParseBool(%q) returned %t expected %t", test.input, result, test.expected)
}
}

for _, input := range invalidInput {
_, err := ParseBool(input)
if err == nil {
t.Errorf("TestParseBool(%q) does not return an error for invalid input", input)
}
}
}

func TestParseInt(t *testing.T) {
var testsWithValidInput = []struct {
input string
expected int
}{
{"0", 0},
{"1", 1},
{"-100", -100},
{"123456789", 123456789},
}

var invalidInput = []string{
"",
"blablah",
"10000000000000000000000000000000000000000000000000000000000000000",
"1,000",
}

for _, test := range testsWithValidInput {
result, err := ParseInt(test.input)
if err != nil {
t.Errorf("TestParseInt(%q) returned an error for valid input", test.input)
}

if result != test.expected {
t.Errorf("TestParseInt(%q) returned %d expected %d", test.input, result, test.expected)
}
}

for _, input := range invalidInput {
_, err := ParseInt(input)
if err == nil {
t.Errorf("TestParseInt(%q) does not return an error for invalid input", input)
}
}
}

func TestParseInt64(t *testing.T) {
var testsWithValidInput = []struct {
input string
expected int64
}{
{"0", 0},
{"1", 1},
{"-100", -100},
{"123456789", 123456789},
}

var invalidInput = []string{
"",
"blablah",
"10000000000000000000000000000000000000000000000000000000000000000",
"1,000",
}

for _, test := range testsWithValidInput {
result, err := ParseInt64(test.input)
if err != nil {
t.Errorf("TestParseInt64(%q) returned an error for valid input", test.input)
}

if result != test.expected {
t.Errorf("TestParseInt64(%q) returned %d expected %d", test.input, result, test.expected)
}
}

for _, input := range invalidInput {
_, err := ParseInt64(input)
if err == nil {
t.Errorf("TestParseInt64(%q) does not return an error for invalid input", input)
}
}
}

func TestParseUint64(t *testing.T) {
var testsWithValidInput = []struct {
input string
expected uint64
}{
{"0", 0},
{"1", 1},
{"100", 100},
{"123456789", 123456789},
}

var invalidInput = []string{
"",
"blablah",
"10000000000000000000000000000000000000000000000000000000000000000",
"1,000",
"-1023",
}

for _, test := range testsWithValidInput {
result, err := ParseUint64(test.input)
if err != nil {
t.Errorf("TestParseUint64(%q) returned an error for valid input", test.input)
}

if result != test.expected {
t.Errorf("TestParseUint64(%q) returned %d expected %d", test.input, result, test.expected)
}
}

for _, input := range invalidInput {
_, err := ParseUint64(input)
if err == nil {
t.Errorf("TestParseUint64(%q) does not return an error for invalid input", input)
}
}
}
5 changes: 4 additions & 1 deletion internal/k8s/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ type Configuration struct {
appPolicyReferenceChecker *appProtectResourceReferenceChecker
appLogConfReferenceChecker *appProtectResourceReferenceChecker

isPlus bool

lock sync.RWMutex
}

Expand All @@ -325,6 +327,7 @@ func NewConfiguration(hasCorrectIngressClass func(interface{}) bool, isPlus bool
policyReferenceChecker: newPolicyReferenceChecker(),
appPolicyReferenceChecker: newAppProtectResourceReferenceChecker(configs.AppProtectPolicyAnnotation),
appLogConfReferenceChecker: newAppProtectResourceReferenceChecker(configs.AppProtectLogConfAnnotation),
isPlus: isPlus,
}
}

Expand All @@ -339,7 +342,7 @@ func (c *Configuration) AddOrUpdateIngress(ing *networking.Ingress) ([]ResourceC
if !c.hasCorrectIngressClass(ing) {
delete(c.ingresses, key)
} else {
validationError = validateIngress(ing).ToAggregate()
validationError = validateIngress(ing, c.isPlus).ToAggregate()
if validationError != nil {
delete(c.ingresses, key)
} else {
Expand Down
Loading