asset/installconfig: improve validation of user inputs #711

staebler · 2018-11-21T04:14:23Z

For most of the validation, bespoke code has been removed in lieu of calling code that already exists and is more widely used.

Base Domain: Use kubernetes validation of rfc-1123 sub-domains, with the addition that the base domain can end with a dot. This change removes the ability to use uppercase letters in the base domain.
Cluster Name: Use kubernetes validation of rfc-1123 sub-domains. This change removes the restriction that each label in the cluster name is at most 63 characters long.
Pull Secret: Validate that (1) there is an "auths" field, (2) the "auths" value has fields, and (3) each field in the "auth" value has an "auth" or a "credsStore" field.
SSH Public Key: Use ParseAuthorizedKey from golang.org/x/crypto/ssh to validate that the provided ssh key can be parsed.

Validation has been added to the InstallConfig asset to enforce that the install-config.yml file loaded is valid. Some fields have been modified to make this easier. For example, the CIDR fields that were strings had their types changed to ipnet.IPNet.

Fixes https://jira.coreos.com/browse/CORS-850
Fixes https://jira.coreos.com/browse/CORS-941
Fixes #910

abhinavdahiya · 2018-11-21T21:24:20Z

pkg/validate/validate_test.go

@@ -249,51 +122,6 @@ func TestEmail(t *testing.T) {
 		})
 	}
 }
-func TestCIDRsDontOverlap(t *testing.T) {


Might be useful to validate cluster and service cidrs do not overlap.

I'll add some validation to install-config.yml.

@staebler any progress on this?

The implementation for install-config.yml validation is done and add to this PR. I have a bunch of unit tests that I need to create for it still, so I have marked this PR as WIP while I finish those.

abhinavdahiya · 2018-11-28T19:47:36Z

pkg/ipnet/ipnet.go

+
+// ParseCIDRMust parses a CIDR from its string representation. If the parse fails,
+// the function will panic.
+func ParseCIDRMust(s string) *IPNet {


nit: MustParseCIDR

crawford

A few of the commit titles also need to be shorted slightly. I would leave pkg/ off of the scope since we know where the assets live (the scope doesn't need to be a path).

pkg/asset/installconfig/aws/aws.go

staebler · 2018-12-06T02:01:15Z

/retest

eparis · 2018-12-11T22:15:33Z

https://jira.coreos.com/browse/CORS-941

abhinavdahiya · 2018-12-13T22:45:27Z

@staebler commits 1971857 .. 5bddc88 are /lgtm

I am a little concerned with addition of gomock and gomock generated files.

How do we regenerate them?
How do we ensure they are always up to date (), new test in ci / or go test ./... is going to catch that?

pkg/asset/installconfig/filefetcher_generated_test.go

staebler · 2018-12-14T18:41:33Z

@abhinavdahiya Some documentation around running unit tests and re-generating mock files has been added to CONTRIBUTING.md.

abhinavdahiya · 2018-12-14T19:04:20Z

/lgtm

abhinavdahiya · 2018-12-14T22:22:14Z

/lgtm

wking · 2018-12-14T22:27:51Z

CONTRIBUTING.md

+
+Some unit tests use mocks that are generated with gomock. If the underlying interface for a mock changes, then the mock will need to be regenerated. Use the following to regenerate all of the mocks under the pkg package.
+
+```


nit: this and the following fenced block should use:

```sh

Because they're shell excerpts. But we can fix that in follow-up work.

wking · 2018-12-14T22:38:40Z

pkg/types/validation/installconfig_test.go

+			valid: false,
+		},
+		{
+			name: "missing worker machine pool",


Should this be allowed? We can create worker pools as a day-2 operation, can't we?

currently our install fails if we do not have workers. So this is required for now.

I remember it failing at one point without workers defined in the install config. That may have been resolved since I originally wrote it. I'll dig into it further, and if we succeed without workers defined, then I will file a follow-up PR.

wking · 2018-12-14T22:41:00Z

pkg/types/validation/installconfig_test.go

+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.Machines = append(c.Machines, types.MachinePool{
+					Name: "other",


I think we want to support this. See here.

hive team is going to be making that change soon, they can change the tests.
also right now we have nothing that supports adding these random pools.

Use kubernetes validation of rfc-1123 sub-domains, with the addition that the base domain can end with a dot. This change removes the ability to use uppercase letters in the base domain. https://jira.coreos.com/browse/CORS-850

Validate that (1) there is an "auths" field, (2) the "auths" value has fields, and (3) each field in the "auth" value has an "auth" or a "credsStore" field. https://jira.coreos.com/browse/CORS-850

Use ParseAuthorizedKey from golang.org/x/crypto/ssh to validate that the provided ssh key can be parsed. https://jira.coreos.com/browse/CORS-850

The validation functions use golang.org/x/crypto/ssh to validate the ssh key provided. https://jira.coreos.com/browse/CORS-850

Move the uri validation from pkg/asset/installconfig/libvirt to pkg/validation for consistency with other validation functions. https://jira.coreos.com/browse/CORS-850

Much of the code that made of the CIDR validation was there to give a custom description of what made the CIDR provided invalid. This has been removed in favor of using the error returned by the golang library's ParseCIDR function. Additionally, the code to determine whether two CIDRs overlap was unnecessary complex because it did not take advantage of the fact that CIDRs only overlap if one is a subset of the other. https://jira.coreos.com/browse/CORS-850

When an InstallConfig asset is loaded from an on-disk file, validate that the fields of the InstallConfig have appropriate values. This uses all the same validations that are down on the user-provided assets upon which the InstalConfig asset depends as well as addition validations on other fields. #711

The emailaddress asset is no longer used as of commit e7aaedb.

Remove validation functions that are no longer being used. https://jira.coreos.com/browse/CORS-850

https://jira.coreos.com/browse/CORS-850

Add gomock to use to mock asset.FileFetcher in unit tests that load assets. https://jira.coreos.com/browse/CORS-850

…nfig.yml Move the fetching of openstack platform values to types/openstack/validation so that it can be used to validate values in the install-config.yml as well. https://jira.coreos.com/browse/CORS-888

wking · 2018-12-15T00:21:21Z

looks like this needs another rebase

staebler · 2018-12-15T00:24:08Z

looks like this needs another rebase

Rebased to fix conflicts with #654.

cc @abhinavdahiya @wking

abhinavdahiya · 2018-12-15T00:49:26Z

/lgtm

openshift-ci-robot · 2018-12-15T00:49:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

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~~ [abhinavdahiya,staebler]

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

abhinavdahiya · 2018-12-15T01:33:54Z

/retest

openshift-ci-robot · 2018-12-15T03:05:31Z

@staebler: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-libvirt	`af073f9`	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2018-12-15T03:19:12Z

Router timed out.

/retest

Covering 5813f61 (Merge pull request openshift#745 from wking/s3-vpc-endpoint, 2018-12-13) through 874876e (Merge pull request openshift#711 from staebler/improve_input_validation, 2018-12-14).

Catching up with openshift/installer@b2d6fa40 (validate: simplify CIDR validation, 2018-11-27, openshift/installer#711).

And rename to 'Regions', since the target package is already about validation. ValidRegions was added to the aws package in b2d6fa4 (validate: simplify CIDR validation, 2018-11-27, openshift#711), but it's just used for validation and it isn't a type defintion.

Catching up with openshift/installer@b2d6fa40 (validate: simplify CIDR validation, 2018-11-27, openshift/installer#711).

Catching up with openshift/installer@b2d6fa40 (validate: simplify CIDR validation, 2018-11-27, openshift/installer#711). We can't convert the v1alpha1 type to an *IPNet, because it doesn't have DeepCopyInto methods [1]: go vet ./pkg/... ./cmd/... ./contrib/... # github.com/openshift/hive/pkg/apis/hive/v1alpha1 pkg/apis/hive/v1alpha1/zz_generated.deepcopy.go:87:8: (*in).DeepCopyInto undefined (type *ipnet.IPNet has no field or method DeepCopyInto) We used to define those methods in the installer, but stopped in openshift/installer@ca6f6195 (Revert "pkg/ipnet: Add DeepCopy and DeepCopyInto for IPNet", 2018-09-20, openshift/installer#295). [1]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_hive/144/pull-ci-openshift-hive-master-unit/253/build-log.txt

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 21, 2018

openshift-ci-robot requested review from flaper87 and tomassedovic November 21, 2018 04:14

abhinavdahiya reviewed Nov 21, 2018

View reviewed changes

staebler changed the title ~~pkg/asset/installconfig: improve validation of user inputs~~ WIP: pkg/asset/installconfig: improve validation of user inputs Nov 28, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2018

abhinavdahiya reviewed Nov 28, 2018

View reviewed changes

crawford suggested changes Nov 28, 2018

View reviewed changes

pkg/asset/installconfig/aws/aws.go Outdated Show resolved Hide resolved

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 3, 2018

staebler changed the title ~~WIP: pkg/asset/installconfig: improve validation of user inputs~~ asset/installconfig: improve validation of user inputs Dec 4, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2018

abhinavdahiya reviewed Dec 14, 2018

View reviewed changes

pkg/asset/installconfig/filefetcher_generated_test.go Outdated Show resolved Hide resolved

staebler mentioned this pull request Dec 14, 2018

No validation of config in create ignition-configs #910

Closed

openshift-ci-robot assigned abhinavdahiya Dec 14, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2018

staebler mentioned this pull request Dec 14, 2018

lack of feedback when using invalid pull secret #901

Closed

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2018

wking reviewed Dec 14, 2018

View reviewed changes

validate: improve base domain validation

3badd35

Use kubernetes validation of rfc-1123 sub-domains, with the addition that the base domain can end with a dot. This change removes the ability to use uppercase letters in the base domain. https://jira.coreos.com/browse/CORS-850

staebler added 13 commits December 14, 2018 19:17

validate: improve image pull secret validation

1df7704

Validate that (1) there is an "auths" field, (2) the "auths" value has fields, and (3) each field in the "auth" value has an "auth" or a "credsStore" field. https://jira.coreos.com/browse/CORS-850

validate: improve ssh public key validation

d2c528f

Use ParseAuthorizedKey from golang.org/x/crypto/ssh to validate that the provided ssh key can be parsed. https://jira.coreos.com/browse/CORS-850

vendor: golang.org/x/crypto ssh sub-package

ef9282d

The validation functions use golang.org/x/crypto/ssh to validate the ssh key provided. https://jira.coreos.com/browse/CORS-850

asset/installconfig: move uri validation to validation package

d813360

Move the uri validation from pkg/asset/installconfig/libvirt to pkg/validation for consistency with other validation functions. https://jira.coreos.com/browse/CORS-850

asset/installconfig: remove emailaddress asset

4ccebbf

The emailaddress asset is no longer used as of commit e7aaedb.

validate: remove unused functions

c793af1

Remove validation functions that are no longer being used. https://jira.coreos.com/browse/CORS-850

types: unit tests for InstallConfig validation

da09a30

https://jira.coreos.com/browse/CORS-850

asset/installconfig: unit tests for InstallConfig.Load

cfc5eac

https://jira.coreos.com/browse/CORS-850

vendor: add github.com/golang/mock/gomock

e5bb73e

Add gomock to use to mock asset.FileFetcher in unit tests that load assets. https://jira.coreos.com/browse/CORS-850

asset/installconfig: validate openstack platform values in install-co…

0ce4e26

…nfig.yml Move the fetching of openstack platform values to types/openstack/validation so that it can be used to validate values in the install-config.yml as well. https://jira.coreos.com/browse/CORS-888

.: describe how to run unit tests in CONTRIBUTING.md

355cad8

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2018

openshift-merge-robot merged commit 874876e into openshift:master Dec 15, 2018

wking mentioned this pull request Dec 15, 2018

CHANGELOG: Document changes since 5813f611 #917

Merged

wking added a commit to wking/hive that referenced this pull request Dec 19, 2018

pkg/apis/hive/v1alpha1: Make VPCCIDRBlock an *IPNet

2a66f36

Catching up with openshift/installer@b2d6fa40 (validate: simplify CIDR validation, 2018-11-27, openshift/installer#711).

wking mentioned this pull request Dec 19, 2018

pkg/types/aws/validation: Move ValidRegions into this package #942

Merged

wking added a commit to wking/hive that referenced this pull request Dec 19, 2018

pkg/apis/hive/v1alpha1: Make VPCCIDRBlock an *IPNet

02ff8fa

Catching up with openshift/installer@b2d6fa40 (validate: simplify CIDR validation, 2018-11-27, openshift/installer#711).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asset/installconfig: improve validation of user inputs #711

asset/installconfig: improve validation of user inputs #711

staebler commented Nov 21, 2018 •

edited by eparis

Loading

abhinavdahiya Nov 21, 2018

staebler Nov 22, 2018

abhinavdahiya Nov 27, 2018

staebler Nov 28, 2018

abhinavdahiya Nov 28, 2018

crawford left a comment

staebler commented Dec 6, 2018

eparis commented Dec 11, 2018

abhinavdahiya commented Dec 13, 2018 •

edited

Loading

staebler commented Dec 14, 2018

abhinavdahiya commented Dec 14, 2018

abhinavdahiya commented Dec 14, 2018

wking Dec 14, 2018

wking Dec 14, 2018

abhinavdahiya Dec 14, 2018 •

edited

Loading

staebler Dec 14, 2018

wking Dec 14, 2018

abhinavdahiya Dec 14, 2018 •

edited

Loading

wking commented Dec 15, 2018

staebler commented Dec 15, 2018

abhinavdahiya commented Dec 15, 2018

openshift-ci-robot commented Dec 15, 2018

abhinavdahiya commented Dec 15, 2018

openshift-ci-robot commented Dec 15, 2018 •

edited

Loading

wking commented Dec 15, 2018


		Some unit tests use mocks that are generated with gomock. If the underlying interface for a mock changes, then the mock will need to be regenerated. Use the following to regenerate all of the mocks under the pkg package.

		```

asset/installconfig: improve validation of user inputs #711

asset/installconfig: improve validation of user inputs #711

Conversation

staebler commented Nov 21, 2018 • edited by eparis 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

crawford left a comment

Choose a reason for hiding this comment

staebler commented Dec 6, 2018

eparis commented Dec 11, 2018

abhinavdahiya commented Dec 13, 2018 • edited Loading

staebler commented Dec 14, 2018

abhinavdahiya commented Dec 14, 2018

abhinavdahiya commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

wking commented Dec 15, 2018

staebler commented Dec 15, 2018

abhinavdahiya commented Dec 15, 2018

openshift-ci-robot commented Dec 15, 2018

abhinavdahiya commented Dec 15, 2018

openshift-ci-robot commented Dec 15, 2018 • edited Loading

wking commented Dec 15, 2018

staebler commented Nov 21, 2018 •

edited by eparis

Loading

abhinavdahiya commented Dec 13, 2018 •

edited

Loading

abhinavdahiya Dec 14, 2018 •

edited

Loading

abhinavdahiya Dec 14, 2018 •

edited

Loading

openshift-ci-robot commented Dec 15, 2018 •

edited

Loading