Some refactoring of validation #3093

CecileRobertMichon · 2018-05-25T23:19:03Z

What this PR does / why we need it: This PR is the first step towards increasing unit test coverage in validate.go. It separates validations in validate.go in functions that can be unit tested more easily. Functionality should be untouched as the PR mostly aims to move things around (as opposed to changing the validation itself). This refactor also makes it easier to identify gaps in validation. Those will be addressed in multiple follow up PRs.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

CecileRobertMichon · 2018-05-31T21:47:24Z

pkg/api/agentPoolOnlyApi/v20170831/validate.go

@@ -166,17 +167,3 @@ func validateVNET(a *Properties) error {

 	return nil
 }
-
-// GetVNETSubnetIDComponents extract subscription, resourcegroup, vnetname, subnetname from the vnetSubnetID


duplicate code --> moved in common place

CecileRobertMichon · 2018-05-31T21:47:44Z

pkg/api/agentPoolOnlyApi/v20180331/validate.go

@@ -301,17 +301,3 @@ func isCustomVNET(a []*AgentPoolProfile) bool {

 	return false
 }
-
-// GetVNETSubnetIDComponents extract subscription, resourcegroup, vnetname, subnetname from the vnetSubnetID
-func GetVNETSubnetIDComponents(vnetSubnetID string) (string, string, string, string, error) {


duplicate code --> moved in common place

CecileRobertMichon · 2018-05-31T21:47:53Z

pkg/api/agentPoolOnlyApi/vlabs/validate.go

@@ -156,17 +157,3 @@ func validateVNET(a *Properties) error {

 	return nil
 }
-
-// GetVNETSubnetIDComponents extract subscription, resourcegroup, vnetname, subnetname from the vnetSubnetID
-func GetVNETSubnetIDComponents(vnetSubnetID string) (string, string, string, string, error) {


duplicate code --> moved in common place

CecileRobertMichon · 2018-05-31T21:48:31Z

pkg/api/common/net.go

@@ -37,3 +40,17 @@ func IP4BroadcastAddress(n *net.IPNet) net.IP {
 	}
 	return last
 }
+
+// GetVNETSubnetIDComponents extract subscription, resourcegroup, vnetname, subnetname from the vnetSubnetID
+func GetVNETSubnetIDComponents(vnetSubnetID string) (string, string, string, string, error) {


moved duplicated function here

CecileRobertMichon · 2018-05-31T21:48:49Z

pkg/api/common/net_test.go

@@ -48,3 +56,29 @@ func Test_IP4BroadcastAddress(t *testing.T) {
 		}
 	}
 }
+
+func Test_GetVNETSubnetIDComponents(t *testing.T) {


new unit test 🎉

CecileRobertMichon · 2018-05-31T21:49:03Z

pkg/api/v20170701/validate.go

@@ -275,17 +275,3 @@ func validateVNET(a *Properties) error {
 	}
 	return nil
 }
-
-// GetVNETSubnetIDComponents extract subscription, resourcegroup, vnetname, subnetname from the vnetSubnetID
-func GetVNETSubnetIDComponents(vnetSubnetID string) (string, string, string, string, error) {


duplicate code --> moved in common place

CecileRobertMichon · 2018-06-02T00:33:53Z

pkg/api/common/net.go

+	}
+	submatches := re.FindStringSubmatch(vnetSubnetID)
+	if len(submatches) != 4 {
+		return "", "", "", "", fmt.Errorf("unable to find 4 submatches")


changed the return here because return "", "", "", "", err would return a nil error

CecileRobertMichon · 2018-06-02T00:55:22Z

pkg/api/common/net.go

+	}
+	submatches := re.FindStringSubmatch(vnetSubnetID)
+	if len(submatches) != 5 {
+		return "", "", "", "", fmt.Errorf("unable to match regexp")


changed from return "", "", "", "", err because it was returning a nil error and if len(submatches) != 5 from if len(submatches) != 4 because we expect 5 matches, not 4 (the first one is the entire string).

This was a bug in all APIs except agentpoolonly v20180331 (the function was fixed in that one but not the others --> yay duplicate code) : validateVNET would never fail on invalid vnetID because it would find 5 submatches != 4 and exit with nil error.

Curious: what is the fifth regex substring match that we're not doing anything with?

Sorry, you answered the question above.

Unless that regex package is weird, we probably have a sub-optimal expression, as we should be able to formulate a regex result that includes only that matched patterns and not a throwaway match of the entire string as well.

I tested on https://regex-golang.appspot.com/assets/html/index.html and got 4 submatches... I wonder if it's the single quotes

Hmm so this is because that's how the go regexp FindStringSubmatch function works https://golang.org/pkg/regexp/#Regexp.FindStringSubmatch: FindSubmatch returns a slice of slices holding the text of the leftmost match of the regular expression in b and the matches, if any, of its subexpressions, as defined by the 'Submatch' descriptions in the package comment. submatches[0] is the "leftmost match of the regular expression"

CecileRobertMichon · 2018-06-02T00:59:35Z

pkg/api/vlabs/validate.go

@@ -94,23 +94,54 @@ func init() {
 	labelKeyRegex = regexp.MustCompile(labelKeyFormat)
 }

-func isValidEtcdVersion(etcdVersion string) error {


moved function to bottom of file

CecileRobertMichon · 2018-06-02T00:59:53Z

pkg/api/vlabs/validate.go

-	// "" is a valid etcdVersion that maps to DefaultEtcdVersion
-	if etcdVersion == "" {
-		return nil
+// Validate implements APIObject


CecileRobertMichon · 2018-06-02T01:07:25Z

pkg/api/vlabs/validate.go

 			if version == "" {
-				return fmt.Errorf("the following user supplied OrchestratorProfile configuration is not supported: OrchestratorType: %s, OrchestratorRelease: %s, OrchestratorVersion: %s. Please check supported Release or Version for this build of acs-engine", o.OrchestratorType, o.OrchestratorRelease, o.OrchestratorVersion)
+				return fmt.Errorf("the following OrchestratorProfile configuration is not supported: OrchestratorType: \"%s\", OrchestratorRelease: \"%s\", OrchestratorVersion: \"%s\". Please use one of the following versions: %v", o.OrchestratorType, o.OrchestratorRelease, o.OrchestratorVersion, common.GetAllSupportedKubernetesVersions())


return a list of supported versions to the user

CecileRobertMichon · 2018-06-04T20:45:05Z

pkg/api/vlabs/validate_test.go

@@ -29,103 +29,127 @@ const (

 func Test_OrchestratorProfile_Validate(t *testing.T) {


The changes in this unit test file are meant to make the current unit tests work with the refactoring of the validate.go file. A PR for improving and increasing unit test coverage of validation functions will follow.

CecileRobertMichon · 2018-06-04T21:51:58Z

/test all

codecov · 2018-06-04T22:37:51Z

Codecov Report

Merging #3093 into master will increase coverage by 0.25%.
The diff coverage is 52.03%.

@@            Coverage Diff             @@
##           master    #3093      +/-   ##
==========================================
+ Coverage   52.36%   52.61%   +0.25%     
==========================================
  Files         103      103              
  Lines       15426    15413      -13     
==========================================
+ Hits         8078     8110      +32     
+ Misses       6621     6566      -55     
- Partials      727      737      +10

CecileRobertMichon · 2018-06-04T22:52:00Z

unit test will pass once #3165 is merged

jackfrancis

Please remove the openshift translation files. Otherwise looks great!

jackfrancis · 2018-06-08T15:50:24Z

pkg/api/common/net.go

+	}
+	submatches := re.FindStringSubmatch(vnetSubnetID)
+	if len(submatches) != 5 {
+		return "", "", "", "", fmt.Errorf("unable to match regexp")


jackfrancis · 2018-06-08T15:51:39Z

pkg/api/common/net.go

+	}
+	submatches := re.FindStringSubmatch(vnetSubnetID)
+	if len(submatches) != 5 {
+		return "", "", "", "", fmt.Errorf("unable to match regexp")


Curious: what is the fifth regex substring match that we're not doing anything with?

jackfrancis · 2018-06-08T15:53:29Z

pkg/api/common/net.go

+	}
+	submatches := re.FindStringSubmatch(vnetSubnetID)
+	if len(submatches) != 5 {
+		return "", "", "", "", fmt.Errorf("unable to match regexp")


Sorry, you answered the question above.

Unless that regex package is weird, we probably have a sub-optimal expression, as we should be able to formulate a regex result that includes only that matched patterns and not a throwaway match of the entire string as well.

jackfrancis · 2018-06-08T18:39:56Z

/approve
/lgtm

jackfrancis · 2018-06-08T19:35:54Z

/approve
/lgtm

acs-bot · 2018-06-08T19:36:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon,jackfrancis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ghost assigned CecileRobertMichon May 25, 2018

ghost added the in progress label May 25, 2018

acs-bot added the size/L label May 25, 2018

CecileRobertMichon added needs rebase do-not-merge/work-in-progress labels May 25, 2018

CecileRobertMichon force-pushed the refactor-validate branch from 589fef8 to 1d6e48b Compare May 31, 2018 00:33

acs-bot added size/XL and removed size/L labels May 31, 2018

CecileRobertMichon removed the needs rebase label May 31, 2018

acs-bot added size/XXL and removed do-not-merge/work-in-progress size/XL labels May 31, 2018

CecileRobertMichon commented May 31, 2018

View reviewed changes

CecileRobertMichon force-pushed the refactor-validate branch from 660e6c5 to 889d3d3 Compare May 31, 2018 21:54

CecileRobertMichon commented Jun 2, 2018

View reviewed changes

CecileRobertMichon commented Jun 4, 2018

View reviewed changes

CecileRobertMichon mentioned this pull request Jun 4, 2018

Sort k8s versions for better error message readability #3165

Merged

3 tasks

CecileRobertMichon added the needs rebase label Jun 4, 2018

CecileRobertMichon added 10 commits June 7, 2018 16:59

fix nil error return

9dd6c18

s/4/5

09cec50

get versions instead of map

a14d7af

fixed validate unit tests based on new functions

dc47fb6

fix test nil errors

069e0a5

re-add master profile storage validation

6720627

managed disks

b7e32b8

more unit test fixes

72eed07

remove alpha sort

c8ed66e

fix merge with semver

2247a26

CecileRobertMichon force-pushed the refactor-validate branch from a130888 to 2247a26 Compare June 8, 2018 00:02

acs-bot added the approved label Jun 8, 2018

CecileRobertMichon added do-not-merge/hold and removed needs rebase labels Jun 8, 2018

jackfrancis suggested changes Jun 8, 2018

View reviewed changes

undo translation files

31545d9

acs-bot assigned jackfrancis Jun 8, 2018

acs-bot added the lgtm label Jun 8, 2018

jackfrancis approved these changes Jun 8, 2018

View reviewed changes

return the right message for version not supported on windows

ed90b0a

acs-bot removed the lgtm label Jun 8, 2018

CecileRobertMichon added 2 commits June 8, 2018 12:15

etcd version error message format

8ac9a93

fix vnetSubnetID error message

ea6ac6d

acs-bot added the lgtm label Jun 8, 2018

CecileRobertMichon removed the do-not-merge/hold label Jun 8, 2018

CecileRobertMichon merged commit 132003a into Azure:master Jun 8, 2018

ghost removed the in progress label Jun 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some refactoring of validation #3093

Some refactoring of validation #3093

CecileRobertMichon commented May 25, 2018 •

edited

Loading

CecileRobertMichon May 31, 2018

CecileRobertMichon May 31, 2018

CecileRobertMichon May 31, 2018

CecileRobertMichon May 31, 2018

CecileRobertMichon May 31, 2018

CecileRobertMichon May 31, 2018

CecileRobertMichon Jun 2, 2018 •

edited

Loading

CecileRobertMichon Jun 2, 2018 •

edited

Loading

jackfrancis Jun 8, 2018

jackfrancis Jun 8, 2018

jackfrancis Jun 8, 2018

CecileRobertMichon Jun 8, 2018 •

edited

Loading

CecileRobertMichon Jun 8, 2018 •

edited

Loading

CecileRobertMichon Jun 2, 2018

CecileRobertMichon Jun 2, 2018

CecileRobertMichon Jun 2, 2018

CecileRobertMichon Jun 4, 2018 •

edited

Loading

CecileRobertMichon commented Jun 4, 2018

codecov bot commented Jun 4, 2018 •

edited

Loading

CecileRobertMichon commented Jun 4, 2018

jackfrancis left a comment

jackfrancis Jun 8, 2018

jackfrancis Jun 8, 2018

jackfrancis Jun 8, 2018

jackfrancis commented Jun 8, 2018

jackfrancis commented Jun 8, 2018

acs-bot commented Jun 8, 2018

		@@ -29,103 +29,127 @@ const (

		func Test_OrchestratorProfile_Validate(t *testing.T) {

Some refactoring of validation #3093

Some refactoring of validation #3093

Conversation

CecileRobertMichon commented May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon commented Jun 4, 2018

codecov bot commented Jun 4, 2018 • edited Loading

Codecov Report

CecileRobertMichon commented Jun 4, 2018

jackfrancis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Jun 8, 2018

jackfrancis commented Jun 8, 2018

acs-bot commented Jun 8, 2018

CecileRobertMichon commented May 25, 2018 •

edited

Loading

CecileRobertMichon Jun 2, 2018 •

edited

Loading

CecileRobertMichon Jun 2, 2018 •

edited

Loading

CecileRobertMichon Jun 8, 2018 •

edited

Loading

CecileRobertMichon Jun 8, 2018 •

edited

Loading

CecileRobertMichon Jun 4, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading