Bug 1507111 - Add support for a local OpenShift Registry adapter #527

dymurray · 2017-11-01T15:06:23Z

This PR addresses Bug 1507111 to add an adapter for the internal OpenShift registry.

Changes proposed in this pull request

New adapter file
New clusterrolebindings for asb service account to access imagestream resource
More openshift client changes so that we can access the imagestream resource

dymurray · 2017-11-01T20:21:05Z

ansibleplaybookbundle/ansible-playbook-bundle#159

dymurray · 2017-11-02T19:28:16Z

Only thing I'm still working on is a unit test.

Edit: The more I tried to write for this I realized that I can't mock out the openshift client. I will need to revisit writing more extensive tests than just checking the config.

shawn-hurley

Two small things

shawn-hurley · 2017-11-02T20:11:04Z

templates/deploy-ansible-service-broker.template.yaml

@@ -81,6 +81,35 @@ objects:
    verbs: ["get", "post", "put", "patch", "delete"]

 - apiVersion: v1
+  kind: ClusterRole


Can we just add these to the existing cluster role for asb service account?

I actually don't think I can because the other cluster roles are being created under the rbac apiGroup (apiVersion rbac.authorization.k8s.io/v1beta1. I am going to test it to see if there's a way to consolidate them but I thought that would cause roadbumps.

shawn-hurley · 2017-11-02T20:11:54Z

pkg/registries/adapters/local_openshift_adapter.go

+	"strings"
+)
+
+const localOpenShiftName = "openshift-registry"


If the registry name is hardcoded we should make sure that one someone makes a registry they don't have this name for the registry.

For this I was following what was done in the other adapters. For example the dockerhub adapter hardcodes the names to docker.io. But yes I agree.

jmrodri · 2017-11-03T03:13:01Z

Please run make check. Travis failed because the linter failed. The CI actually worked.

jmrodri

First linter, and a few places where we're eating errors.

jmrodri · 2017-11-03T03:28:34Z

pkg/clients/openshift.go

+// ListRegistryImages - List images in internal OpenShift registry
+func (o OpenshiftClient) ListRegistryImages(log *logging.Logger) (images []string, err error) {
+	r := &ImageList{}
+	var imageList []string


Flip lines 371 and 370. I almost didn't see where r was being assigned.

jmrodri · 2017-11-03T03:30:51Z

pkg/clients/openshift.go

+	for _, image := range r.Items {
+		imageList = append(imageList, strings.Split(image.DockerImage, "@")[0])
+	}
+	if err != nil {


This is checking the error return from the imageRestClient? If there's an error from the client how can r be filled out and have items?

I think line 380 error check should be on line 377 BEFORE you loop through r.Items.

jmrodri · 2017-11-03T03:34:08Z

pkg/registries/adapters/local_openshift_adapter.go

+	r.Log.Debug("LocalOpenShiftAdapter::GetImageNames")
+	r.Log.Debug("BundleSpecLabel: %s", BundleSpecLabel)
+
+	openshiftClient, err := clients.Openshift(r.Log)


you are eating the err. If that clients.Openshift failed, line 51 will die a miserable death which might be a panic.

Thanks, fixed

jmrodri · 2017-11-03T03:36:38Z

pkg/registries/adapters/local_openshift_adapter.go

+	r.Log.Debug("LocalOpenShiftAdapter::FetchSpecs")
+	r.Log.Debugf("HERE %v", r.Config.Namespaces)
+	specList := []*apb.Spec{}
+	registryIP, err := r.getServiceIP("docker-registry", "default")


eating another err, you might be able to do something like

var registryIP WHATEVERITRETURNS var err error if registryIP, err := r.getServiceIP("docker-registry", "default"); err != nil { r.logErrorf("Failed to load registry images") return nil, err }

jmrodri · 2017-11-03T03:38:48Z

pkg/registries/adapters/local_openshift_adapter.go

+	specList := []*apb.Spec{}
+	registryIP, err := r.getServiceIP("docker-registry", "default")
+
+	openshiftClient, err := clients.Openshift(r.Log)


another err eaten

jmrodri · 2017-11-03T03:42:35Z

pkg/registries/adapters/local_openshift_adapter.go

+
+func (r LocalOpenShiftAdapter) loadSpec(yamlSpec []byte) (*apb.Spec, error) {
+	r.Log.Debug("LocalOpenShiftAdapter::LoadSpec")
+	var err error


weird to declare a var when you can easily do err := yaml.Unmarshal on line 103.

jmrodri · 2017-11-03T03:47:23Z

pkg/registries/adapters/local_openshift_adapter.go

+	serviceData, err := k8scli.CoreV1().Services(namespace).Get(service, meta_v1.GetOptions{})
+	if err != nil {
+		r.Log.Warningf("Unable to load service '%s' from namespace '%s'", service, namespace)
+		return "", nil


So we got an error but we're returning nil?

dymurray · 2017-11-03T11:36:06Z

@jmrodri @shawn-hurley Thanks for the feedback guys. I think I've addressed everything except for the hardcoded name. Let me know your thoughts on that.

shawn-hurley · 2017-11-03T13:25:32Z

pkg/clients/openshift.go

+	fqImage := FQImage{}
+	var err error
+	r := &ImageList{}
+	err = o.imageRestClient.Get().


Sorry, didn't catch this one before, but should check the error before working with the items.

Thanks, fixed

Updates to getImageList more updates Remove unneeded yaml files Add configuration ability for administrator Added unit test Linting fixes

rthallisey · 2017-11-03T17:23:08Z

pkg/registries/adapters/local_openshift_adapter.go

+
+// RegistryName - Retrieve the registry name
+func (r LocalOpenShiftAdapter) RegistryName() string {
+	return localOpenShiftName


I think the name can be changed of the registry. Think this should be like rhcc: return r.Config.URL.String()

The internal registry uses the api. Name will always be the same.

…nshift#527) * Bug 1507111 - Add local openshift registry adapter Updates to getImageList more updates Remove unneeded yaml files Add configuration ability for administrator Added unit test Linting fixes * Move cluster role declaration to preexisting one * Fix template

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 1, 2017

shawn-hurley added the do-not-merge/work-in-progress label Nov 1, 2017

jmrodri added the work-in-progress label Nov 1, 2017

dymurray force-pushed the ocpregupdate branch from 79d84c7 to d25a255 Compare November 2, 2017 18:57

dymurray changed the title ~~[Do Not Merge] Bug 1507111 - Add support for a local OpenShift Registry adapter~~ Bug 1507111 - Add support for a local OpenShift Registry adapter Nov 2, 2017

dymurray added the needs-review label Nov 2, 2017

dymurray removed do-not-merge/work-in-progress work-in-progress labels Nov 2, 2017

shawn-hurley requested changes Nov 2, 2017

View reviewed changes

jmrodri self-requested a review November 3, 2017 03:11

jmrodri suggested changes Nov 3, 2017

View reviewed changes

shawn-hurley requested changes Nov 3, 2017

View reviewed changes

Bug 1507111 - Add local openshift registry adapter

1205bdf

Updates to getImageList more updates Remove unneeded yaml files Add configuration ability for administrator Added unit test Linting fixes

dymurray force-pushed the ocpregupdate branch from fe508bc to 1205bdf Compare November 3, 2017 13:37

shawn-hurley approved these changes Nov 3, 2017

View reviewed changes

dymurray added 2 commits November 3, 2017 10:16

Move cluster role declaration to preexisting one

c47f25a

Fix template

aa7b767

rthallisey reviewed Nov 3, 2017

View reviewed changes

jmrodri approved these changes Nov 3, 2017

View reviewed changes

jmrodri added bug and removed needs-review labels Nov 3, 2017

rthallisey approved these changes Nov 3, 2017

View reviewed changes

shawn-hurley merged commit e543438 into openshift:master Nov 3, 2017

Bug 1507111 - Add support for a local OpenShift Registry adapter #527

Bug 1507111 - Add support for a local OpenShift Registry adapter #527

dymurray commented Nov 1, 2017

dymurray commented Nov 1, 2017

dymurray commented Nov 2, 2017 •

edited

shawn-hurley left a comment

shawn-hurley Nov 2, 2017

dymurray Nov 3, 2017

shawn-hurley Nov 2, 2017

dymurray Nov 3, 2017

jmrodri commented Nov 3, 2017

jmrodri left a comment

jmrodri Nov 3, 2017

jmrodri Nov 3, 2017

jmrodri Nov 3, 2017

dymurray Nov 3, 2017

jmrodri Nov 3, 2017

jmrodri Nov 3, 2017

jmrodri Nov 3, 2017

jmrodri Nov 3, 2017

dymurray commented Nov 3, 2017

shawn-hurley Nov 3, 2017

dymurray Nov 3, 2017

rthallisey Nov 3, 2017

rthallisey Nov 3, 2017 •

edited

Bug 1507111 - Add support for a local OpenShift Registry adapter #527

Bug 1507111 - Add support for a local OpenShift Registry adapter #527

Conversation

dymurray commented Nov 1, 2017

dymurray commented Nov 1, 2017

dymurray commented Nov 2, 2017 • edited

shawn-hurley 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

jmrodri commented Nov 3, 2017

jmrodri 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey Nov 3, 2017 • edited

Choose a reason for hiding this comment

dymurray commented Nov 2, 2017 •

edited

rthallisey Nov 3, 2017 •

edited