Bug 1826414: Added validation for GCP Project ID access #3484

rna-afk · 2020-04-20T21:25:59Z

GCP: Fix error message for Invalid Project ID entry

Fixes the error message that occurs after the region selection when an
invalid project ID is entered. Solved by picking up the list of projects
that the user has in their GCP account and offers a select option on the
project entry to select the one that they want. Reduces the chance of
picking an invalid project ID and avoids the error message.

Sample Output:

? SSH Public Key /home/***/.ssh/id_rsa.pub
? Platform gcp
INFO Credentials loaded from file "/home/***/.gcp/osServiceAccount.json" 
? Project ID  [Use arrows to move, enter to select, type to filter, ? for more help]
  OpenShift Dev - Installer (openshift-dev-installer)
> OpenShift GCE Devel (openshift-gce-devel)
  OpenShift GCE Devel - CI Jobs (openshift-gce-devel-ci)
  OpenShift Perf and Scalability (openshift-perfscale)
  OpenShift RHCOS devel (openshift-rhcos-devel)

patrickdillon

This is off to a good start. This only adds validation for the TUI wizard. Validation also should be added for the install-config. That can be done by adding to the Validate function here: https://github.com/openshift/installer/blob/master/pkg/asset/installconfig/gcp/validation.go#L15

You should also add a test for a valid and invalid project.

The commit message begins with script: Not sure what that refers to.

patrickdillon · 2020-04-20T22:53:09Z

pkg/asset/installconfig/gcp/client.go

 	return svc, nil
 }
+
+//GetListOfProjects gets the list of project names and ids associated with the current user.


nit: there should be a space between // and the first word.

patrickdillon · 2020-04-20T22:59:27Z

pkg/asset/installconfig/gcp/client.go

 }
+
+//GetListOfProjects gets the list of project names and ids associated with the current user.
+func (c *Client) GetListOfProjects(ctx context.Context) ([]string, []string, error) {


I think it's better for this to return a map or other data structure. I don't think it's good to rely on corresponding indices between two slices--what if you sort one?

patrickdillon · 2020-04-20T23:01:18Z

pkg/asset/installconfig/gcp/gcp.go

+		ssn: ssn,
+	}
+
+	var listOfCombo []string


A better name would be something like projectChoices. Don't begin variable names with types or pseudo types like list; combo is vague.

patrickdillon · 2020-04-20T23:03:45Z

pkg/asset/installconfig/gcp/gcp.go

+			Prompt: &survey.Select{
+				Message: "Project",
+				Help:    "The GCP project to be used for installation.",
+				Default: fmt.Sprintf("%s", defaultValue),


This is strange... Does this just evaluate to defaultValue so why usefmt.Sprintf?

patrickdillon · 2020-04-20T23:04:56Z

pkg/asset/installconfig/gcp/gcp.go

+		listOfCombo = append(listOfCombo, listOfNames[i]+" ("+v+")")
+	}
+
+	defaultValue := listOfNames[index] + " (" + defaultProject + ")"


but you should be using fmt.Sprintf here.

patrickdillon · 2020-04-20T23:19:32Z

pkg/asset/installconfig/gcp/gcp.go

+		if v == defaultProject {
+			index = i
+		}
+		listOfCombo = append(listOfCombo, listOfNames[i]+" ("+v+")")


the string formatting here should use fmt.Sprintf. Also I think it's more readable to have the string formatting in its own line and assigned to a variable, rather than as an arg to append.

patrickdillon · 2020-04-20T23:24:55Z

pkg/asset/installconfig/gcp/gcp.go

+				Default: fmt.Sprintf("%s", defaultValue),
+				Options: listOfCombo,
 			},
+			Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error {


I think survey already limits choices to Options so I don't think this extra validator is needed.

patrickdillon · 2020-04-20T23:27:12Z

pkg/asset/installconfig/gcp/gcp.go

+	selectedProject = strings.SplitN(selectedProject, " (", 2)[1]
+	selectedProject = selectedProject[:len(selectedProject)-1]


Rather than all of this string manipulation, I think it's better when you create options to also create a map with option as key and the id as value.

patrickdillon · 2020-04-20T23:33:19Z

pkg/asset/installconfig/gcp/gcp.go

+				Message: "Project",
+				Help:    "The GCP project to be used for installation.",
+				Default: fmt.Sprintf("%s", defaultValue),
+				Options: listOfCombo,


I think it is good for the values passed to Options to be sorted.

rna-afk · 2020-04-21T16:08:22Z

/bugzilla refresh

rna-afk · 2020-04-21T16:22:48Z

/bugzilla refresh

openshift-ci-robot · 2020-04-21T16:27:18Z

@rna-afk: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

/bugzilla refresh

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.

rna-afk · 2020-04-21T16:31:59Z

/bugzilla refresh

openshift-ci-robot · 2020-04-21T16:43:53Z

@rna-afk: An error was encountered adding this pull request to the external tracker bugs for bug 1826414 on the Bugzilla server at https://bugzilla.redhat.com:

JSONRPC error 32000: There was an error reported for a GitHub REST call. URL: https://api.github.com/repos/openshift/installer/pulls/3484 Error: 500 Internal Server Error at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 107. at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 107. eval {...} called at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 94 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x5556fe...', 'https://api.github.com/repos/openshift/installer/pulls/3484', 'GET') called at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 58 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x5556fe...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x5556ff459360)') called at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Bug.pm line 298 eval {...} called at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Bug.pm line 298 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x5556ff459360)', 1) called at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/Bug.pm line 121 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x5556ff7bd5a0)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 873 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x5556ff45d870)', 'HASH(0x5556ffd118c0)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x5556ffd118c0)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1170 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x5556ff38a938)') called at /loader/0x5556fb1cd1d8/Bugzilla/Extension/ExternalBugs/WebService.pm line 84 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5556ff36e5c0)') called at (eval 2195) line 1 eval ' $procedure->{code}->($self, @params) ;' called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 220 JSON::RPC::Legacy::Server::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5556ffca7fd8)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 295 Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5556ffca7fd8)') called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 126 JSON::RPC::Legacy::Server::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 70 Bugzilla::WebService::Server::JSONRPC::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/jsonrpc.cgi line 31 ModPerl::ROOT::Bugzilla::ModPerl::ResponseHandler::var_www_html_bugzilla_jsonrpc_2ecgi::handler('Apache2::RequestRec=SCALAR(0x5556ff24ddb0)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 eval {...} called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 ModPerl::RegistryCooker::run('Bugzilla::ModPerl::ResponseHandler=HASH(0x5556ffcceed8)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x5556ffcceed8)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x5556ff24ddb0)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x5556ff24ddb0)') called at (eval 2195) line 0 eval {...} called at (eval 2195) line 0
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

Details

In response to this:

Bug 1826414: Added validation for GCP Project ID & basedomain access

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.

openshift-ci-robot · 2020-04-21T16:44:17Z

@rna-afk: This pull request references Bugzilla bug 1826414, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

/bugzilla refresh

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.

patrickdillon

Left a few more comments. This is in good shape.

Can you update the commit message heading to make it more obvious this relates to GCP?

There are two different conventions generally followed. You can start with something like

pkg/asset/installconfig/gcp: or GCP: but anything that gives more context is fine. Also in general commit messages should be imperative, so Fix error message for Invalid GCP Project ID entry instead of Fixed...

Also we removed the basedomain portion of this card so I was a little thrown initially by the PR title.

patrickdillon · 2020-04-21T17:40:44Z

pkg/asset/installconfig/gcp/client.go

Can we change GetListOfProjects -> GetProjects ? Also if you're changing the comment, can you put in the comment that id is key of map and name is value?

patrickdillon · 2020-04-21T17:42:35Z

pkg/asset/installconfig/gcp/client.go

nit: personally I think just projects is better.

patrickdillon · 2020-04-21T17:45:02Z

pkg/asset/installconfig/gcp/gcp.go

Don't include type in variable name. You could just call this projectIDs, or ids.

patrickdillon · 2020-04-21T17:51:23Z

pkg/asset/installconfig/gcp/gcp.go

I think this line shows that these variable names are confusing.

What about something like:

for id, name := range projects { option := fmt.Sprintf("%s (%s)", name, id) ids[option] = id if option == defaultProject { defaultValue = option } options = append(options, option)

patrickdillon · 2020-04-21T17:55:51Z

pkg/asset/installconfig/gcp/validation.go

error strings like this should start with lowercase letters. Invalid -> invalid. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

patrickdillon · 2020-04-21T17:57:25Z

pkg/asset/installconfig/gcp/validation_test.go

Can you shorten all of this to:

gcpClient.EXPECT().GetListOfProjects(gomock.Any()).Return({"valid-project" : "valid-project"}, nil).AnyTimes()

openshift-ci-robot · 2020-04-21T18:19:23Z

@rna-afk: This pull request references Bugzilla bug 1826414, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1826414: Added validation for GCP Project ID access

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.

patrickdillon · 2020-04-21T19:50:01Z

You should add new test cases in: pkg/asset/installconfig/gcp/validation_test.go

There is an existing test case for invalid project in the context of BYO networking. But your code is more general and catches an error with a minimal gcp platform with an invalid project. So I would recommend a similar test case, which only specifies a valid region and invalid project. You could use the editFunctions removeVPC and removeSubnets to make a minimal install config.

abhinavdahiya · 2020-04-21T23:17:06Z

pkg/asset/installconfig/gcp/gcp.go

Suggested change

return "", err

return "", errors.Wrap(err, "failed to get projects")

abhinavdahiya · 2020-04-21T23:20:31Z

pkg/asset/installconfig/gcp/validation_test.go

try to include the path in the expectedErrMsg, makes it much clearer in terms of what we are testing.

abhinavdahiya · 2020-04-21T23:21:21Z

pkg/asset/installconfig/gcp/validation.go

so this should be an InternalError, because it didn't have anything to do with the project field technically.

pkg/asset/installconfig/gcp/gcp.go

abhinavdahiya · 2020-04-21T23:23:31Z

/test e2e-gcp

openshift-ci-robot · 2020-04-22T13:23:54Z

@rna-afk: This pull request references Bugzilla bug 1826414, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1826414: Added validation for GCP Project ID access

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.

abhinavdahiya · 2020-04-22T16:10:54Z

/approve

openshift-ci-robot · 2020-04-22T16:13:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/asset/installconfig/gcp/OWNERS~~ [abhinavdahiya]

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

patrickdillon · 2020-04-22T17:55:46Z

pkg/asset/installconfig/gcp/validation.go

Abhinav's point in #3484 (comment) was that field.Invalid -> field.InternalError. field.Invalid means the user's input was bad, but in this case there was an error when calling the gcloud API.

You could fix the preexisting code in a separate commit as well.

Abhinav's point in #3484 (comment) was that field.Invalid -> field.InternalError.

I get it now thanks.

You could fix the preexisting code in a separate commit as well.

Got it. Will follow that next time.

You could fix the preexisting code in a separate commit as well.

Got it. Will follow that next time.

No I meant that the earlier code (that I wrote) has the same issue and you could fix it in a separate commit.

Fixes the error message that occurs after the region selection when an invalid project ID is entered. Solved by picking up the list of projects that the user has in their GCP account and offers a select option on the project entry to select the one that they want. Reduces the chance of picking an invalid project ID and avoids the error. CloudResourceManager is used to pick up the list of projects for current user and survey is populated with the values obtained. Generated the gcpclient_generated.go mock file using mockgen and replaced the existing file to add the GetListOfProjects function. Running hack/go-genmock.sh did not work after I changed the interface and ran it.

patrickdillon · 2020-04-22T20:22:44Z

/lgtm

openshift-bot · 2020-04-22T20:29:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-22T20:51:12Z

@rna-afk: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-ovirt	`6035d38`	link	`/test e2e-ovirt`

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.

Details

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.

openshift-bot · 2020-04-22T21:34:50Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-22T22:10:09Z

@rna-afk: All pull requests linked via external trackers have merged: openshift/installer#3484. Bugzilla bug 1826414 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1826414: Added validation for GCP Project ID access

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.

patrickdillon · 2020-04-22T23:11:40Z

Fixes #3044

rna-afk requested review from abhinavdahiya and patrickdillon April 20, 2020 21:25

openshift-ci-robot requested review from jstuever and mtnbikenc April 20, 2020 21:26

rna-afk force-pushed the gcp_project_basenames_validation branch 4 times, most recently from a91b1ed to ea3538b Compare April 20, 2020 22:32

patrickdillon reviewed Apr 20, 2020

View reviewed changes

rna-afk force-pushed the gcp_project_basenames_validation branch 7 times, most recently from 18029ac to 83fa604 Compare April 21, 2020 15:37

rna-afk changed the title ~~Added validation for GCP Project ID & basedomain access~~ Bug 1826414 - Added validation for GCP Project ID & basedomain access Apr 21, 2020

rna-afk changed the title ~~Bug 1826414 - Added validation for GCP Project ID & basedomain access~~ Bug 1826414: Added validation for GCP Project ID & basedomain access Apr 21, 2020

openshift-ci-robot added the bugzilla/low label Apr 21, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 21, 2020

rna-afk force-pushed the gcp_project_basenames_validation branch 2 times, most recently from ef7884b to 1d82772 Compare April 21, 2020 17:35

patrickdillon reviewed Apr 21, 2020

View reviewed changes

rna-afk changed the title ~~Bug 1826414: Added validation for GCP Project ID & basedomain access~~ Bug 1826414: Added validation for GCP Project ID access Apr 21, 2020

rna-afk force-pushed the gcp_project_basenames_validation branch from 1d82772 to c534fcb Compare April 21, 2020 18:25

rna-afk force-pushed the gcp_project_basenames_validation branch from c534fcb to 4e5147c Compare April 21, 2020 20:27

abhinavdahiya reviewed Apr 21, 2020

View reviewed changes

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

rna-afk force-pushed the gcp_project_basenames_validation branch from 4e5147c to efd48f6 Compare April 22, 2020 14:57

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020

patrickdillon reviewed Apr 22, 2020

View reviewed changes

rna-afk force-pushed the gcp_project_basenames_validation branch from efd48f6 to 6035d38 Compare April 22, 2020 18:01

openshift-ci-robot assigned patrickdillon Apr 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020

openshift-merge-robot merged commit bdf803f into openshift:master Apr 22, 2020

patrickdillon mentioned this pull request Apr 22, 2020

Missing user-understandable error message with GOOGLE_CREDENTIALS being unset #3044

Closed

		selectedProject = strings.SplitN(selectedProject, " (", 2)[1]
		selectedProject = selectedProject[:len(selectedProject)-1]

	return "", err
	return "", errors.Wrap(err, "failed to get projects")

Bug 1826414: Added validation for GCP Project ID access #3484

Bug 1826414: Added validation for GCP Project ID access #3484

Uh oh!

Conversation

rna-afk commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rna-afk commented Apr 21, 2020

Uh oh!

rna-afk commented Apr 21, 2020

Uh oh!

openshift-ci-robot commented Apr 21, 2020

Uh oh!

rna-afk commented Apr 21, 2020

Uh oh!

openshift-ci-robot commented Apr 21, 2020

Uh oh!

openshift-ci-robot commented Apr 21, 2020

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Apr 21, 2020

Uh oh!

patrickdillon commented Apr 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhinavdahiya commented Apr 21, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

abhinavdahiya commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rna-afk commented Apr 20, 2020 •

edited

Loading

openshift-ci-robot commented Apr 22, 2020 •

edited

Loading

patrickdillon commented Apr 22, 2020 •

edited

Loading