gcp: verify project services are enabled before install #3773

SujanaN08 · 2020-06-18T14:49:44Z

xref: https://issues.redhat.com/browse/CORS-1254

To verify the list of services that are enabled before a cluster is installed we are making a call to
https://serviceusage.googleapis.com/v1/{parent=*/*}/services via the gcp client.go.
The link to the documentaion is here
In platformpermscheck.go we get the list of services and check if all the required services are enabled , else it returns an error.
The gcp validation.go checks for the list of services code.

patrickdillon · 2020-06-18T15:42:57Z

/assign

SujanaN08 · 2020-06-18T21:25:53Z

/assign @abhinavdahiya

abhinavdahiya · 2020-06-19T01:16:34Z

.idea/.gitignore

@@ -0,0 +1,8 @@
+# Default ignored files


I don't think you intented to add .idea files to the PR.
You can look at https://stackoverflow.com/a/1753078 to add .idea/ to your local git repo so it doesn't show up in unstaged files.

abhinavdahiya · 2020-06-19T01:18:45Z

pkg/asset/installconfig/gcp/validation.go

@@ -133,3 +133,18 @@ func validateMachineNetworksContainIP(fldPath *field.Path, networks []types.Mach
 	}
 	return field.ErrorList{field.Invalid(fldPath, subnetName, fmt.Sprintf("subnet CIDR range start %s is outside of the specified machine networks", ip))}
 }
+
+func CheckForServicesToBeEnabled(projectServices []string) error {


https://blog.golang.org/godoc

provide good info on how you should document functions are that public API of a package.

abhinavdahiya · 2020-06-19T01:20:49Z

pkg/asset/installconfig/gcp/validation.go

@@ -133,3 +133,18 @@ func validateMachineNetworksContainIP(fldPath *field.Path, networks []types.Mach
 	}
 	return field.ErrorList{field.Invalid(fldPath, subnetName, fmt.Sprintf("subnet CIDR range start %s is outside of the specified machine networks", ip))}
 }
+
+func CheckForServicesToBeEnabled(projectServices []string) error {


Suggested change

func CheckForServicesToBeEnabled(projectServices []string) error {

func ValidateEnabledServices(services []string) error {

remember in Go, the package name is usually part of the call site, so keeping that in mind gcp.ValidateEnabledServices is much closer describing what the function does.

abhinavdahiya · 2020-06-19T01:22:44Z

pkg/asset/installconfig/gcp/validation.go

@@ -133,3 +133,18 @@ func validateMachineNetworksContainIP(fldPath *field.Path, networks []types.Mach
 	}
 	return field.ErrorList{field.Invalid(fldPath, subnetName, fmt.Sprintf("subnet CIDR range start %s is outside of the specified machine networks", ip))}
 }
+
+func CheckForServicesToBeEnabled(projectServices []string) error {
+	services := map[string]string{"compute.googleapis.com": "enabled", "cloudapis.googleapis.com": "enabled",


try using https://godoc.org/k8s.io/apimachinery/pkg/util/sets

that package provides an easy sets.NewString(<list of string values>) to easily run a check Has(<service>)

abhinavdahiya · 2020-06-19T01:25:54Z

pkg/asset/installconfig/gcp/validation.go

+		if _, ok := services[service]; ok {
+			return errors.New(service + "is not enabled")
+		}


If we were using sets,

requiredServices Set; enabledServices Set; if remaining := requiredServices.Difference(enabledService); remainging.Len() > 0 { return error that remaining.List() services are not enabled but required. }

abhinavdahiya · 2020-06-19T01:26:24Z

pkg/asset/installconfig/gcp/validation_test.go

+
+

extra lines

abhinavdahiya · 2020-06-19T01:26:50Z

pkg/asset/installconfig/platformpermscheck.go

 	"github.com/openshift/installer/pkg/types/aws"
 	"github.com/openshift/installer/pkg/types/azure"
 	"github.com/openshift/installer/pkg/types/baremetal"
-	"github.com/openshift/installer/pkg/types/gcp"
+	gcp "github.com/openshift/installer/pkg/types/gcp"


Suggested change

gcp "github.com/openshift/installer/pkg/types/gcp"

"github.com/openshift/installer/pkg/types/gcp"

should also just work.. ?

abhinavdahiya · 2020-06-19T01:27:30Z

pkg/asset/installconfig/platformpermscheck.go

+		}
+		services, err := client.GetEnabledServices(ctx, ic.Config.GCP.ProjectID)
+		if err != nil {
+			return err


Suggested change

return err

return errors.Wrap(err, "failed to enabled services for the project")

abhinavdahiya · 2020-06-19T01:28:22Z

pkg/asset/installconfig/platformpermscheck.go

+		}
+		err = gcpconfig.CheckForServicesToBeEnabled(services)
+		if err != nil {
+			return err


Suggested change

return err

return errors.Wrap(err, "validating required services")

abhinavdahiya · 2020-06-19T01:32:25Z

I think you need to run gofmt and golint on the code, you can run ./hack/go-fmt.sh && ./hack/go-lint.sh
For more info look the failing jobs go-fmt and go-lint
Also I think your idea editor should also have a way to run these at save.

Secondly, using the new services client requires updating the vendor, so run go mod tidy && go mod vendor and add that as a separate commit. there should be changes in go.mod go.sum and vendor/ i think

abhinavdahiya · 2020-06-19T01:32:39Z

/test e2e-gcp

abhinavdahiya · 2020-06-19T01:37:19Z

pkg/asset/installconfig/gcp/validation.go

@@ -133,3 +134,25 @@ func validateMachineNetworksContainIP(fldPath *field.Path, networks []types.Mach
 	}
 	return field.ErrorList{field.Invalid(fldPath, subnetName, fmt.Sprintf("subnet CIDR range start %s is outside of the specified machine networks", ip))}
 }
+
+func CheckForServicesToBeEnabled(projectServices []string) error {
+	services := sets.NewString("compute.googleapis.com", "cloudapis.googleapis.com",


Suggested change

services := sets.NewString("compute.googleapis.com", "cloudapis.googleapis.com",

services := sets.NewString("compute.googleapis.com", "cloudapis.googleapis.com",

abhinavdahiya · 2020-06-19T01:39:24Z

Also maybe something to look into will be, if we get Unauthorized error when listing the services enabled for the project, we should just skip the check and continue as warning.

abhinavdahiya · 2020-06-19T01:41:07Z

Also currently the commits have changes across multiple one for the same file, try to collect them into logical groups. keep the vendor update separate, the rest can stay as one.

patrickdillon

this is a good start. left some suggestions.

patrickdillon · 2020-06-19T01:33:30Z

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/coreos/etcd v3.3.18+incompatible // indirect
 	github.com/coreos/go-systemd v0.0.0 // indirect
 	github.com/coreos/ignition v0.35.0
+	github.com/davecgh/go-spew v1.1.1


Spew probably crept in while you were debugging but should not be added to the repo.

patrickdillon · 2020-06-19T01:52:53Z

pkg/asset/installconfig/gcp/validation.go

@@ -133,3 +134,25 @@ func validateMachineNetworksContainIP(fldPath *field.Path, networks []types.Mach
 	}
 	return field.ErrorList{field.Invalid(fldPath, subnetName, fmt.Sprintf("subnet CIDR range start %s is outside of the specified machine networks", ip))}
 }
+
+func CheckForServicesToBeEnabled(projectServices []string) error {
+	services := sets.NewString("compute.googleapis.com", "cloudapis.googleapis.com",


I think these are hard to read. Try listing them out on individual lines.

patrickdillon · 2020-06-19T02:00:52Z

pkg/asset/installconfig/gcp/validation.go

+	var count int = 0
+	for _, service := range projectServices {
+		if services.Has(service) {
+			count++


this approach is reasonable and works.

To me, a more intuitive approach would be to make a set of the project's enabled services and iterate through the required services, checking if each is enabled.

This would have the added benefit that if a required service was not found, you could append the missing service to a slice then include the slice of all missing services in an error message to the user, so they could see which service needs to be enabled.

Also in the same area for improvement #3773 (comment)

patrickdillon · 2020-06-19T02:05:48Z

pkg/asset/installconfig/platformpermscheck.go

+		if err != nil {
+			return err
+		}
+		services, err := client.GetEnabledServices(ctx, "projects/"+ic.Config.GCP.ProjectID)


rather than calling this here in platformpermscheck, I would pass the client to your validation function and get the services there.

patrickdillon · 2020-06-19T02:27:24Z

pkg/asset/installconfig/platformpermscheck.go

+		if err != nil {
+			return err
+		}
+		services, err := client.GetEnabledServices(ctx, "projects/"+ic.Config.GCP.ProjectID)


It was good that you figured out the API issue here.

A few things:

concatenating strings within a function's parameters is hard to read. you should move this a variable, probably named parent

Also you could probably do that string formatting within GetEnabledServices, so it is still just taking the project ID and then is transformed into parent in the function. Also you should probably use Sprintf instead of concatentation

The idea of parent is new to me in the Google API and I don't think I have seen examples in our codebase but I do see some context is provided in the docs I would leave a brief comment that would provide some context of why we need to prepend "projects/" before projectID

openshift-bot · 2020-07-03T04:08:47Z

/retest

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

openshift-bot · 2020-07-03T04:47:48Z

/retest

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

openshift-bot · 2020-07-03T06:06:33Z

/retest

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

openshift-bot · 2020-07-03T11:56:46Z

/retest

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

openshift-bot · 2020-07-03T13:40:51Z

/retest

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

openshift-bot · 2020-07-03T13:53:48Z

/retest

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

openshift-bot · 2020-07-03T14:45:46Z

/retest

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

openshift-bot · 2020-07-03T15:26:07Z

/retest

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

openshift-bot · 2020-07-03T16:03:52Z

/retest

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

openshift-bot · 2020-07-03T17:21:44Z

/retest

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

openshift-bot · 2020-07-03T17:34:49Z

/retest

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

openshift-bot · 2020-07-03T17:47:54Z

/retest

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

openshift-bot · 2020-07-03T18:52:51Z

/retest

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

openshift-bot · 2020-07-03T19:18:51Z

/retest

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

openshift-bot · 2020-07-03T19:44:45Z

/retest

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

openshift-bot · 2020-07-03T21:54:46Z

/retest

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

openshift-bot · 2020-07-03T22:07:48Z

/retest

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

openshift-bot · 2020-07-03T23:39:21Z

/retest

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

openshift-bot · 2020-07-03T23:51:47Z

/retest

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

openshift-bot · 2020-07-04T00:43:48Z

/retest

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

openshift-bot · 2020-07-04T00:56:52Z

/retest

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

openshift-bot · 2020-07-04T02:14:45Z

/retest

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

openshift-bot · 2020-07-04T02:40:44Z

/retest

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

abhinavdahiya · 2020-07-04T03:13:22Z

/override ci/prow/e2e-aws

Previous e2e AWS failed on of adm must gather flake, e2e gcp is already green

openshift-ci-robot · 2020-07-04T03:13:58Z

@abhinavdahiya: Overrode contexts on behalf of abhinavdahiya: ci/prow/e2e-aws

In response to this:

/override ci/prow/e2e-aws

Previous e2e AWS failed on of adm must gather flake, e2e gcp is already green

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-07-04T03:36:42Z

/override ci/prow/e2e-aws

openshift-ci-robot · 2020-07-04T03:37:18Z

@abhinavdahiya: Overrode contexts on behalf of abhinavdahiya: ci/prow/e2e-aws

In response to this:

/override ci/prow/e2e-aws

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-bot · 2020-07-04T03:45:43Z

/retest

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

openshift-ci-robot · 2020-07-04T04:45:07Z

@SujanaN08: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-openstack-upi	`1550fd6`	link	`/test e2e-openstack-upi`
ci/prow/e2e-metal-ipi	`1550fd6`	link	`/test e2e-metal-ipi`
ci/prow/e2e-ovirt	`ebfe8b1`	link	`/test e2e-ovirt`
ci/prow/e2e-openstack	`ebfe8b1`	link	`/test e2e-openstack`
ci/prow/e2e-aws-scaleup-rhel7	`ebfe8b1`	link	`/test e2e-aws-scaleup-rhel7`

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.

openshift-ci-robot requested review from jstuever and mtnbikenc June 18, 2020 14:50

SujanaN08 marked this pull request as draft June 18, 2020 14:50

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 Jun 18, 2020

openshift-ci-robot assigned patrickdillon Jun 18, 2020

SujanaN08 changed the title ~~Cors 1254~~ Cors 1254 - Verify project services are enabled before install Jun 18, 2020

SujanaN08 marked this pull request as ready for review June 18, 2020 21:25

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 Jun 18, 2020

openshift-ci-robot assigned abhinavdahiya Jun 18, 2020

abhinavdahiya changed the title ~~Cors 1254 - Verify project services are enabled before install~~ gcp: verify project services are enabled before install Jun 19, 2020

abhinavdahiya reviewed Jun 19, 2020

View reviewed changes

pkg/asset/installconfig/gcp/validation_test.go Outdated

Comment on lines 234 to 263

Copy link

Contributor

abhinavdahiya Jun 19, 2020

Choose a reason for hiding this comment

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

extra lines

abhinavdahiya reviewed Jun 19, 2020

View reviewed changes

patrickdillon reviewed Jun 19, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020

SujanaN08 force-pushed the CORS-1254 branch 2 times, most recently from 4b2e8e8 to 16cae1e Compare June 24, 2020 14:29

openshift-merge-robot merged commit c7d9de6 into openshift:master Jul 4, 2020

	func CheckForServicesToBeEnabled(projectServices []string) error {
	func ValidateEnabledServices(services []string) error {

	gcp "github.com/openshift/installer/pkg/types/gcp"
	"github.com/openshift/installer/pkg/types/gcp"

	return err
	return errors.Wrap(err, "failed to enabled services for the project")

	return err
	return errors.Wrap(err, "validating required services")

	services := sets.NewString("compute.googleapis.com", "cloudapis.googleapis.com",
	services := sets.NewString("compute.googleapis.com", "cloudapis.googleapis.com",

gcp: verify project services are enabled before install #3773

gcp: verify project services are enabled before install #3773

Conversation

SujanaN08 commented Jun 18, 2020 • edited by abhinavdahiya

patrickdillon commented Jun 18, 2020

SujanaN08 commented Jun 18, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Jun 19, 2020 • edited

abhinavdahiya commented Jun 19, 2020

Choose a reason for hiding this comment

abhinavdahiya commented Jun 19, 2020

abhinavdahiya commented Jun 19, 2020

patrickdillon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 3, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

abhinavdahiya commented Jul 4, 2020

openshift-ci-robot commented Jul 4, 2020

abhinavdahiya commented Jul 4, 2020

openshift-ci-robot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-ci-robot commented Jul 4, 2020

SujanaN08 commented Jun 18, 2020 •

edited by abhinavdahiya

abhinavdahiya commented Jun 19, 2020 •

edited