White/Black List Filtering and Multiple Registries Refactor #271

shawn-hurley · 2017-07-13T18:47:07Z

No description provided.

eriknelson · 2017-07-13T20:40:45Z

This is obviously pretty big, I'm going to try to follow this up with a test plan and try to point out the stuff that needs review attention. It's a little misleading in +/- because reorg'd a bunch of files into different packages.

shawn-hurley · 2017-07-13T20:24:07Z

etc/example-config.yaml

@@ -23,3 +29,5 @@ broker:
  launch_apb_on_bind: false
  recovery: true
  output_request: true
+  whitelist: "/etc/ansible-service-broker/whitelist.yaml"


@eriknelson I think this needs to be moved to the registry here.

Ah, missed that one.

shawn-hurley · 2017-07-13T20:25:25Z

pkg/apb/types_test.go

@@ -20,6 +20,7 @@ const SpecImage = "fusor/etherpad-apb"
 const SpecBindable = false
 const SpecAsync = "optional"
 const SpecDescription = "A note taking webapp"
+const SpecRegistryName = "test"


This should be removed missed it in the cleanup.

shawn-hurley · 2017-07-13T20:41:58Z

pkg/registries/registry.go

+	}
+
+	if len(filteredNames) != 0 {
+		r.log.Info("Filtered APBs:")


Some of the images might not be APB's?

It feels very wrong to me for the adapter to return a list of image names for filtering that contain images that are not APBs. I suppose it could happen, and the adapter's LoadSpecs will validate the labels and metadata further, but if GetImageNames is returning non-APBs, something is going awry.

If it feels that wrong then we need to discuss how to change the dockerhub adapter. Currently, the dockerhub adapter will get all the images in an org. This does not know anything about APB's because we can not check the label at this time. We could do something like if the name does not contain "apb" then do not return it. I am having trouble coming up with another solution that does not make another network request. I also think that this might want to be an issue/card that we prioritize for next sprint.

I'm realizing my initial assumption was wrong, but I still think the set of image names that hits the filter should ideally be only APBs. I originally thought the adapter was pushing the query serverside (GET /?name=*-apb) or something to that effect, but that's actually not how its ever worked. It has always fetched every name under an org, and we filtered on our side based on *-apb. It was always a limitation of their API, from the work we did back in CAP days.

My thoughts on an ideal scenario:

Adapters can query a service and get back only APBs

Registry is able to validate this set and confirm that the names are indeed apbs

Vetted set of names hits filter

By this point, we're confident we have a set of images we know we care about, and we load the specs. (Possibly validate additional labels like apb.spec version).

Obviously we need to make some concessions because:

The service a particular adapter is talking to for fetching APB names may or may not support server-side filtering. In Dockerhub's case, it definitely does not.

Since we're strictly dealing with names, we don't have a lot of options for determining whether or not an image is actually an APB. Until now, it has always been -apb suffix.

I'd propose Registry.LoadSpecs to filter any image names that do not match -apb$ before they hit Filter.Run. It's a filter that applies to all registries, so it belongs at the Registry level and not in the adapters. Potential drawback here is possible apbs will be filtered if they don't follow this naming convention, but that has always been the case until now, and to your point, we should put a card on the backlog to think about how we can accomplish this better.

Ok, I will work on this now

+1 when I ask the adapter for the list of images, I expect the list of APBs. I understand if the adapter needs to get ALL of them because of an api limitation but it should filter it down by name first.

shawn-hurley · 2017-07-13T20:43:15Z

pkg/registries/registry.go

+
+	if len(validNames) != 0 {
+		r.log.Debugf("Passing APBs:", r.config.Name)
+		for _, name := range validNames {


What about putting in a goroutine. We could do the whole loop off by itself with little effort and would not block if slice gets big

I don't want this loop's logs to get interlaced with other logs running concurrently. What do you think about building up a single string with line breaks inside a goroutine and outputting once, so you get the full list displayed atomically?

rthallisey

I still need to go through the filter piece. Looking good 👍

rthallisey · 2017-07-13T18:59:48Z

docs/apb-filter-design.md

+Both are optional file paths that are yaml files containing arrays
+of regexes. Broker will construct the set of eligible APBs from this list.
+
+If only whitelist is present, strictest mode of operation. Any APB that does *not*


Can you rephrase this.

rthallisey · 2017-07-13T19:02:13Z

docs/apb-filter-design.md

+If only whitelist is present, strictest mode of operation. Any APB that does *not*
+match on a regex in the whitelist will be filtered.
+
+If only blacklist is present, all discovered APBs will be eligible *unless*


Can you also rephrase this so it's obvious what blacklist is. Something like If only blacklist.yml exists, ...

What if we make a table?

The lists will have the following behavior:

present allowed blocked

only whitelist matches a regex in list ANY APB that does not match

only blacklist ALL APBs that do not match APBs that match a regex in list

both present matches regex in whitelist but NOT in blacklist APBs that match a regex in blacklist

rthallisey · 2017-07-13T19:02:44Z

docs/apb-filter-design.md

+If only blacklist is present, all discovered APBs will be eligible *unless*
+they match on a regex found in the blacklist.
+
+If both are present, same behavior as whitelist, but if a value matches on white


Be specific about what's present and add more detail to this sentence.

rthallisey · 2017-07-13T22:36:58Z

docs/apb-filter-design.md

+If both are present, same behavior as whitelist, but if a value matches on white
+and black, blacklist will take precedence and block the whitelist match.
+
+These can be easily configured by cluster operators via config maps.


Can there be a section on configuration? This gets mentioned out of no where.

This is not intended to be considered operator documentation, I need to follow this up with that. It was mostly scratch design thoughts prior to writing any code.

rthallisey · 2017-07-13T22:40:19Z

docs/apb-filter-design.md

@@ -0,0 +1,80 @@
+# APB Filter Design Doc
+


To me, this feels inbetween a spec and a guide. Should this doc be purely focused on how to use the filter instead of how it works?

This is distinctly a "Spec" or a "Design" doc that we started doing this sprint for the major features. The intent is to have a plan that has been vetted and reviewed before implementation. It was a bit awkward because we were still figuring out how we wanted to accomplish this, but its the same type of document as the one @shawn-hurley sent out for multi-reg and @jmrodri sent out for auth. I think we're trying to settle on posting them as PRs against his repo moving forwards, the auth one being a good reference.

I do still need to document white/blacklist usage in the relevant config doc though.

I'm conflicted because I think the spec should ultimately reflect the implementation as was committed. If we go with the PR and specifications directory approach, we can have a history of the conversation. Seeing how it began and how it ultimately ended.

I was planning on updating my auth spec to reflect the implementation. Then leave it be.

@jmrodri @rthallisey so what action should I take based on this? Does this document need to get updated, or is it okay to leave it reflective of design thoughts before the implementation and update separate official usage documentation for operators?

rthallisey · 2017-07-13T22:58:41Z

pkg/broker/broker.go

+			return nil, err
+		}
+		if err != nil {
+			registryErrors = append(registryErrors, err)


Do you also want to output the error here? Whatever err is never gets pushed to stdout when r.Fail is false.

rthallisey · 2017-07-13T23:03:45Z

pkg/registries/registry.go

+	return specs, len(imageNames), nil
+}
+
+// Fail - should this registry and error cause a failure.


Can you rephrase this comment

rthallisey · 2017-07-13T23:31:06Z

pkg/broker/broker.go


 	return &BootstrapResponse{SpecCount: len(specs), ImageCount: imageCount}, nil
 }

+func addNameAndIDForSpec(specs []*apb.Spec, registryName string) {


I think lint may yell at you for not adding a comment before this function

this is an unexported method, so it wont complain. Do you think there needs to be a comment?

I think a comment would be good. Looking at the title, it wasn't clear to me at first what this function did.

shawn-hurley · 2017-07-14T13:55:45Z

pkg/registries/registry.go

+	return specs, len(imageNames), nil
+}
+
+func registryFilterImagesForAPBs(imageNames []string) []string {


@eriknelson can you make sure this is what you were thinking?

@shawn-hurley perfect, thanks. We should discuss your thoughts more too, think you're right; there's a better way to handle this.

jmrodri

Wow, that was a big one :) All in all great job. Some minor nits with typos or document changes.

jmrodri · 2017-07-14T14:11:31Z

docs/apb-filter-design.md

+
+`whitelist: "/etc/ansible-service-broker/whitelist.yaml"`
+`blacklist: "/etc/ansible-service-broker/blacklist.yaml"`
+


Change to be more readable:

whitelist: "/etc/ansible-service-broker/whitelist.yaml" blacklist: "/etc/ansible-service-broker/blacklist.yaml"

jmrodri · 2017-07-14T14:17:49Z

docs/apb-filter-design.md

+`blacklist: "/etc/ansible-service-broker/blacklist.yaml"`
+
+Both are optional file paths that are yaml files containing arrays
+of regexes. Broker will construct the set of eligible APBs from this list.


Both are yaml files containing arrays of regular expressions. Both file paths are optional. Broker will construct the set of eligible APBs from this list.

This is part of the problem I mentioned to you. These aren't files anymore, they're arrays in the main configuration file on registries. What should I do now that implementation has changed since spec design?

jmrodri · 2017-07-14T14:25:14Z

docs/apb-filter-design.md

+If only whitelist is present, strictest mode of operation. Any APB that does *not*
+match on a regex in the whitelist will be filtered.
+
+If only blacklist is present, all discovered APBs will be eligible *unless*


What if we make a table?

The lists will have the following behavior:

present allowed blocked

only whitelist matches a regex in list ANY APB that does not match

only blacklist ALL APBs that do not match APBs that match a regex in list

both present matches regex in whitelist but NOT in blacklist APBs that match a regex in blacklist

jmrodri · 2017-07-14T14:28:16Z

docs/apb-filter-design.md

+on Registries. Can extend LoadSpecs() method to accept a "filter" object,
+so they can easily filter discovered apbs. Registries should filter from
+total list of discovered apb names before loading specs from metadata. Cuts
+down on unecessary work.


Some typos and formatting :)

NOTE: Broker filtering of APBs should be performed behind LoadSpecs() method on Registries. We can extend LoadSpecs() method to accept a "filter" object, so they can easily filter discovered APBs. Registries should filter from total list of discovered APB names before loading specs from metadata. This will cut down on unnecessary work.

jmrodri · 2017-07-14T14:29:50Z

docs/apb-filter-design.md

+total list of discovered apb names before loading specs from metadata. Cuts
+down on unecessary work.
+
+Loudly log strange corner cases like no specs available due to empty but


jmrodri · 2017-07-14T15:50:57Z

pkg/registries/filter.go

+	"sync"
+)
+
+type filterMode uint8


what is this C? 😆

jmrodri · 2017-07-14T15:54:42Z

pkg/registries/filter.go

+	return reduceChunks(matchChunksChan)
+}
+
+func reduceChunks(chunks <-chan []string) <-chan matchSetT {


cool, feels very CompSci-y

jmrodri · 2017-07-14T15:54:59Z

pkg/registries/filter.go

+	out := make(chan matchSetT)
+	go func() {
+		matchSet := make(matchSetT)
+		// Read and process chunks as they complete until input chan is cloesd


TYPO: cloesd -> closed

jmrodri · 2017-07-14T15:56:21Z

pkg/registries/filter_test.go

+const testBlacklistFile = "blacklist.yaml"
+const testBlacklistOverrideFile = "blacklist_override.yaml"
+
+func testGetRegexFromFile(file string) []string {


Does this actually run? I always thought they had to be TestXYZ. I learned something new today.

Believe it's a utility function, not a test. Think it deserves a name change?

jmrodri · 2017-07-14T16:00:31Z

pkg/registries/registry.go

+	Pass string
+	Org  string
+	Type string
+	Name string


do the optional fields need to be marked omitempty?

We are never marshaling registry to JSON so I don't see a need. does anyone else have thoughts?

eriknelson · 2017-07-14T18:33:18Z

Spoke with @rthallisey and @jmrodri re: apb-filter-design.md. Ultimately, it was supposed to be an implementation proposal that didn't belong in docs/ and fell very much out of date with that actual implementation. Plan is to remove the file since it's entirely inaccurate and follow up this PR with proper usage documentation.

Adding ability to catch ID collison and to warn user which spec is overwritting. fix build issues when refactoring registry to its own package. Adding registry failure bahavior * if all fail, fail the bootstrap * if 1 fails and is marked on fail_on_error then fail the bootstrap\ this does not continue the bootstrap * Adding method that will take an error and determine if the registry should fail refactor so that the dockerhub and rhcc are using the same image data -> spec. * stop pulling down entire image for dockerhub * using regisitry.dockerhub.com to get the manifest which is the same manifest as RHCC * able to combine image data -> spec. Multiple registries that are using the same manifest EP. * Allows us to generate the spec ID. * attempts to make the service name unique by keeping the registry with it. fixing bug with name not valid.

* Tests are failing and need to write more. * need to make dockerhub FetchSpec faster with goroutines

rthallisey

Overall LGTM. Minor nits

rthallisey · 2017-07-14T18:58:34Z

pkg/broker/broker.go


 	return &BootstrapResponse{SpecCount: len(specs), ImageCount: imageCount}, nil
 }

+func addNameAndIDForSpec(specs []*apb.Spec, registryName string) {


I think a comment would be good. Looking at the title, it wasn't clear to me at first what this function did.

rthallisey · 2017-07-14T19:08:48Z

pkg/registries/adapters/rhcc_adapter_test.go

+	config := Configuration{URL: u}
+	adapter := RHCCAdapter{Config: config, Log: log}
+	imageNames, err := adapter.GetImageNames()
+	//specs, num, err := reg.LoadSpecs()


Looks like a left over comment

rthallisey · 2017-07-14T19:22:36Z

pkg/registries/registry.go

+func registryFilterImagesForAPBs(imageNames []string) []string {
+	newNames := []string{}
+	for _, imagesName := range imageNames {
+		if strings.HasSuffix(strings.ToLower(imagesName), "-apb") {


This filters for the *-apb tag in the name. In the past we're we reading labels for an image? Do we document that '-apb' is required per image?

The Dockerhub adapter has always pulled the entire population of image names from the API and processed them broker-side in this way (due to a limitation of the API), then loaded the metadata from a separate endpoint and confirmed the label. We had a discussion about this elsewhere on the PR I'll try to link, but this was the conclusion until we can address this directly.

+1 re: documentation, that's a must.

This should be followed up and documented here https://github.com/fusor/ansible-playbook-bundle imo. @eriknelson @jmrodri @rthallisey thoughts?

rthallisey · 2017-07-14T19:29:13Z

pkg/registries/filter.go

+	filterModeNone
+)
+
+type failedRegexp struct {


Style nit: Make a type.go for all the structs and consts.

I actually am not a huge fan of this, but I could be swayed. @shawn-hurley what's most golang-like, in your opinion?

Kubernetes seems to use the type file. the standard lib does not. a good example is the net/http package. https://golang.org/src/net/http/header.go. Usually, when talking about good style/idioms I side with the standard library.

rthallisey · 2017-07-14T19:40:37Z

@eriknelson ack from me once my comments are addressed

shawn-hurley · 2017-07-14T20:21:32Z

(8) Multiple Registry Adapters configured in one Broker Instance

shawn-hurley · 2017-07-14T20:21:33Z

(8) Multiple Registry Adapters configured in one Broker Instance

…t#271) * initial attempt at multiple registries. Adding ability to catch ID collison and to warn user which spec is overwritting. fix build issues when refactoring registry to its own package. Adding registry failure bahavior * if all fail, fail the bootstrap * if 1 fails and is marked on fail_on_error then fail the bootstrap\ this does not continue the bootstrap * Adding method that will take an error and determine if the registry should fail refactor so that the dockerhub and rhcc are using the same image data -> spec. * stop pulling down entire image for dockerhub * using regisitry.dockerhub.com to get the manifest which is the same manifest as RHCC * able to combine image data -> spec. Multiple registries that are using the same manifest EP. * Allows us to generate the spec ID. * attempts to make the service name unique by keeping the registry with it. fixing bug with name not valid. * Initial filter commit + tests (openshift#261) * fixing util * first pass at refactor * Tests are failing and need to write more. * need to make dockerhub FetchSpec faster with goroutines * fixing tests and fixing vet, lint issues * Adding test cases for registry updating name for get images * fixing name generation for uniqueness and dns compliant. * fixing test cases * Precompiled filter regex * documentation changes for registry config * Addressing PR comments regarding config and registry * rebase error * Adding registry ability to filter out images that do not end in -apb * addressing comments on typos, and documentation * fixing the build upon rebasing master * Addressing pull request comments.

shawn-hurley changed the title ~~Feature/filtered multi reg~~ White/Black list Filtering and Multiple Registries Refactor Jul 13, 2017

shawn-hurley changed the title ~~White/Black list Filtering and Multiple Registries Refactor~~ White/Black List Filtering and Multiple Registries Refactor Jul 13, 2017

shawn-hurley requested review from jmrodri and rthallisey July 13, 2017 18:49

shawn-hurley commented Jul 13, 2017

View reviewed changes

rthallisey suggested changes Jul 13, 2017

View reviewed changes

shawn-hurley force-pushed the feature/filtered-multi-reg branch from 4cf6978 to 3c49e24 Compare July 14, 2017 13:22

shawn-hurley commented Jul 14, 2017

View reviewed changes

jmrodri suggested changes Jul 14, 2017

View reviewed changes

Erik Nelson and others added 15 commits July 14, 2017 14:53

Initial filter commit + tests (#261)

52820a0

fixing util

6466d49

first pass at refactor

6cfda6a

* Tests are failing and need to write more. * need to make dockerhub FetchSpec faster with goroutines

fixing tests and fixing vet, lint issues

0593585

Adding test cases for registry updating name for get images

101fad6

fixing name generation for uniqueness and dns compliant.

0095223

fixing test cases

0682bf2

Precompiled filter regex

09dd2a1

documentation changes for registry config

877c83e

Addressing PR comments regarding config and registry

bcabe5b

rebase error

565b525

Adding registry ability to filter out images that do not end in -apb

accffd8

addressing comments on typos, and documentation

4a385fc

fixing the build upon rebasing master

dd868e4

shawn-hurley force-pushed the feature/filtered-multi-reg branch from 6539b41 to dd868e4 Compare July 14, 2017 18:58

rthallisey reviewed Jul 14, 2017

View reviewed changes

rthallisey approved these changes Jul 14, 2017

View reviewed changes

Addressing pull request comments.

e0bcbff

jmrodri approved these changes Jul 14, 2017

View reviewed changes

eriknelson merged commit 7609e79 into master Jul 14, 2017

eriknelson deleted the feature/filtered-multi-reg branch July 14, 2017 20:14

shawn-hurley mentioned this pull request Jul 14, 2017

Proposal: Multiple Registry’s #256

Closed

eriknelson mentioned this pull request Jul 14, 2017

Filtering documentation #279

Merged

rthallisey mentioned this pull request Jan 25, 2018

If -apb is a required naming convention, should be enforced by apb-tools ansibleplaybookbundle/ansible-playbook-bundle#215

Closed

present	allowed	blocked
only whitelist	matches a regex in list	ANY APB that does not match
only blacklist	ALL APBs that do not match	APBs that match a regex in list
both present	matches regex in whitelist but NOT in blacklist	APBs that match a regex in blacklist


		`whitelist: "/etc/ansible-service-broker/whitelist.yaml"`
		`blacklist: "/etc/ansible-service-broker/blacklist.yaml"`

White/Black List Filtering and Multiple Registries Refactor #271

White/Black List Filtering and Multiple Registries Refactor #271

Conversation

shawn-hurley commented Jul 13, 2017

eriknelson commented Jul 13, 2017

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

shawn-hurley Jul 14, 2017 • edited

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

rthallisey 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

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

eriknelson Jul 14, 2017 • edited

Choose a reason for hiding this comment

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

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

eriknelson commented Jul 14, 2017

rthallisey 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

rthallisey commented Jul 14, 2017

shawn-hurley commented Jul 14, 2017

shawn-hurley commented Jul 14, 2017

shawn-hurley Jul 14, 2017 •

edited

eriknelson Jul 14, 2017 •

edited