Bearer Token Auth via kubernetes Apiserver #445

shawn-hurley · 2017-09-20T15:16:07Z

Describe what this PR does and why we need it:
Sets up ASB with the apiserver kubernetes project.
Changes proposed in this pull request

Serve ASB via apiserver
Set up delegated auth using nonResoureceURLs to control access
Setting servicecatalog.ServiceBroker with bearer token auth from a secret generated for a service account
Will generate certs if no certs are given, therefore removing insecure argument.

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
fusor/catasb#142

Will neeed another PR for Documentation update.

rthallisey · 2017-09-20T17:21:50Z

pkg/clients/kubernetes.go

@@ -24,7 +24,7 @@ import (
 	"errors"

 	logging "github.com/op/go-logging"
-	restclient "k8s.io/client-go/rest"
+	"k8s.io/client-go/rest"



can you move this down with the other k8s.io imports

rthallisey · 2017-09-20T17:24:59Z

pkg/handler/handler.go

 	h := handler{
 		router:       *mux.NewRouter(),
 		broker:       b,
 		log:          log,
 		brokerConfig: brokerConfig,
 	}
+	s := h.router.PathPrefix(prefix).Subrouter()

 	// TODO: Reintroduce router restriction based on API version when settled upstream


This covers the TODO here.

can you explain?

I thought 104 covered this todo. But, I think I'm confusing a Headers and a pathprefix. They aren't related.

rthallisey · 2017-09-20T17:26:11Z

pkg/handler/handler_test.go

@@ -145,14 +145,14 @@ func init() {

 func TestNewHandler(t *testing.T) {
 	testb := MockBroker{Name: "testbroker"}
-	testhandler := NewHandler(testb, log, brokerConfig)
+	testhandler := NewHandler(testb, log, brokerConfig, "")


It might be worth adding a prefix to the test.

rthallisey · 2017-09-20T17:37:47Z

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


 - apiVersion: servicecatalog.k8s.io/v1alpha1
  kind: ${BROKER_KIND}
  metadata:
    name: ansible-service-broker
  spec:
-    url: ${ASB_SCHEME}://asb-1338-ansible-service-broker.${ROUTING_SUFFIX}
+    url: https://asb.ansible-service-broker.svc:1338/ansible-service-broker/


This makes it a requirement to use https. I'm not against that, but then ASB_SCHEME should be removed.

Another way to do this is keep ASB_SCHEME and BROKER_AUTH in place but change BROKER_AUTH to:

value: '{ "bearer": { "secretRef": { "kind": "Secret", "namespace": "ansible-service-broker", "name": "ansible-service-broker" } } }'

You also aren't using BROKER_URL_PREFIX

rthallisey · 2017-09-20T17:40:39Z

pkg/app/app.go

 	"time"

 	kapierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"


Nit: Can you organize this so that imports with a name are together and those without a name are together.

kapierrors "k8s.io/apimachinery/pkg/api/errors" kubeversiontypes "k8s.io/apimachinery/pkg/version" ... "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/wait" ...

go imports organize the imports like this I would prefer to continue to use go imports.

Does anyone else have any thoughts?

It's the style we've been using. I don't mind changing it.

we could name them all and goimports will be fine. The problem with changing it is that @shawn-hurley and myself will flip it back when we save the file again. Not sure if there's a way to configure goimports. And I'd hate to stop using it since it does so much for me automatically.

rthallisey · 2017-09-20T17:49:32Z

pkg/app/app.go

+			clusterURL = a.config.Broker.ClusterURL
+		}
+	} else {
+		clusterURL = ClusterURLPreFix


Nit: Can you change ClusterURLPreFix to something like DefaultClusterURLPrefix

rthallisey · 2017-09-20T17:50:48Z

pkg/app/app.go

+	genericserver.Handler.NonGoRestfulMux.HandlePrefix(fmt.Sprintf("%v/", clusterURL), daHandler)
+	a.log.Notice("Listening on https://%s", genericserver.SecureServingInfo.BindAddress)
+
+	a.log.Notice("Starting apiserver")


Shouldn't we note the Broker is started here? a.log.Notice("Ansible Service Broker Started")

I mean it does not actually start until the next line and could fail. I would prefer to make it a.log.Notice("Ansible Service Broker Starting")? is that agreeable?

rthallisey · 2017-09-20T17:53:57Z

pkg/app/app.go

+	err = genericserver.PrepareRun().Run(wait.NeverStop)
+	a.log.Errorf("unable to wait on run - %v", err)
+
+	//TODO: Add Flag so we can still use the old way of doing this.


I don't this it's necessary to also support the old way.

rthallisey · 2017-09-20T19:13:25Z

pkg/app/args.go

@@ -30,7 +30,6 @@ import (
 type Args struct {
 	ConfigFile string `short:"c" long:"config" description:"Config File" default:"/etc/ansible-service-broker/config.yaml"`
 	Version    bool   `short:"v" long:"version" description:"Print version information"`
-	Insecure   bool   `long:"insecure" description:"Run Ansible Service Broker in insecure mode."`


This is going to require a change in catasb.

I have the branch in my repo and will be posting a PR soon.

rthallisey · 2017-09-20T19:54:49Z

@shawn-hurley I'm going to mess around with this a little by hand and get back to you. I also want to make sure the gate passes on this which will require #446

jmrodri

I've seen most of this code. I'm not a fan of this whole apiserver idea. I find it annoying that in order to integrate with an apiserver that serves basically as a proxy that I have to change the architecture of my application.

This is how folks want it, then so be it. Approval from me of this PR is not an acceptance consider it a ACK under Protest.

rthallisey · 2017-09-20T20:25:46Z

@jmrodri How close is your alternative to working? I'd like to evaluate both options.

jwmatthews · 2017-09-20T20:28:28Z

@rthallisey it's been decided to go with Shawn's approach for delegated auth. This is what the project needs at this point in time.

We are not pursuing the alternative approach.

jmrodri · 2017-09-20T20:40:06Z

@rthallisey like @jwmatthews mentioned it's been decided for us. I've deleted the bearerauth branch I had going.

jmrodri · 2017-09-20T20:40:15Z

@shawn-hurley does basic auth still work? because that's what the current spec supports.

shawn-hurley · 2017-09-21T17:31:53Z

@jmrodri adding the ability that if basic auth if set, then we will use that instead of bearer token auth. This makes sense because if you set both authentication types on the ServiceBroker it will honor basic auth first.

I still think that our default should be bearer token auth.

jwmatthews · 2017-09-21T18:40:21Z

I expect default workflow will want Bearer Token Auth as well

enj · 2017-09-22T19:15:25Z

LGTM

There are some followup items but this is in line with our discussions.

eriknelson

ACK. Not my preferred path forwards but it sounds like this is simply required. Well done @shawn-hurley, no nits from me from my rudimentary skim.

jmrodri · 2017-09-25T02:58:53Z

pkg/app/app.go

 	}
 	daHandler := handler.NewHandler(a.broker, a.log.Logger, a.config.Broker, clusterURL)

 	genericserver.Handler.NonGoRestfulMux.HandlePrefix(fmt.Sprintf("%v/", clusterURL), daHandler)
 	a.log.Notice("Listening on https://%s", genericserver.SecureServingInfo.BindAddress)

-	a.log.Notice("Starting apiserver")
+	a.log.Notice("Ansible Serivce Broker Starting")


TYPO: Serivce -> Service

rthallisey · 2017-09-25T11:50:50Z

Corresponding catasb change: fusor/catasb#142

jwmatthews

ACK, before merging I'd like to coordinate with some demos that are planned for 9/26, worried about merging this in and causing breakage with the template change.

* more stupid changes * broker runs but does not stay up.

* No support for insecure. * if no certs given will generate its own * ignor the folder that the certs get generated too

* Still trying to remove insecure broker code.

* template and catasb are still defaulting for bearer token auth.

* getting gate build working

* apiserver work * updated glide yaml/lock * update vendor to match the new glide values * Create and return genericapiserver from ApiServer method * more stupid changes * broker runs but does not stay up. * add natefinch dep * getting genericapiserver working. * making the initialization a little cleaner. * No support for insecure. * if no certs given will generate its own * ignor the folder that the certs get generated too * Working deploy template and apiserver * addressing pr comments. * Still trying to remove insecure broker code. * Adding ability for basic auth to override the bearer token auth. * template and catasb are still defaulting for bearer token auth. * adding temporary template to be used for a couple of days * fixing gate build for a couple of days * Adding ability to serve at the root url * getting gate build working * fixing build. going back to a single deployment template

shawn-hurley changed the title ~~Apiserver~~ Bearer Token Auth via kubernetes Apiserver Sep 20, 2017

shawn-hurley requested review from jmrodri, jwmatthews, eriknelson and rthallisey September 20, 2017 15:17

shawn-hurley added feature needs-review labels Sep 20, 2017

shawn-hurley force-pushed the apiserver branch from b74e290 to 96f8e1c Compare September 20, 2017 15:24

rthallisey suggested changes Sep 20, 2017

View reviewed changes

shawn-hurley force-pushed the apiserver branch from a93113e to 810a13f Compare September 20, 2017 19:06

rthallisey suggested changes Sep 20, 2017

View reviewed changes

shawn-hurley mentioned this pull request Sep 20, 2017

Bearer Token Auth update fusor/catasb#142

Merged

jmrodri approved these changes Sep 20, 2017

View reviewed changes

shawn-hurley force-pushed the apiserver branch from 810a13f to b341229 Compare September 21, 2017 17:30

shawn-hurley force-pushed the apiserver branch from b341229 to b7b2f31 Compare September 22, 2017 18:48

eriknelson approved these changes Sep 22, 2017

View reviewed changes

jmrodri suggested changes Sep 25, 2017

View reviewed changes

rthallisey approved these changes Sep 25, 2017

View reviewed changes

shawn-hurley force-pushed the apiserver branch from b7b2f31 to dd9086f Compare September 25, 2017 12:55

jmrodri approved these changes Sep 25, 2017

View reviewed changes

jwmatthews approved these changes Sep 25, 2017

View reviewed changes

shawn-hurley removed the needs-review label Sep 25, 2017

shawn-hurley force-pushed the apiserver branch from 330eb31 to 8361c24 Compare September 25, 2017 20:28

jmrodri and others added 13 commits September 26, 2017 10:57

apiserver work

cadc7a6

updated glide yaml/lock

f2a343c

update vendor to match the new glide values

33aa627

add natefinch dep

6a1fa5f

Create and return genericapiserver from ApiServer method

459239f

* more stupid changes * broker runs but does not stay up.

getting genericapiserver working.

7a6dcb3

making the initialization a little cleaner.

9d2d30d

* No support for insecure. * if no certs given will generate its own * ignor the folder that the certs get generated too

Working deploy template and apiserver

0d03446

addressing pr comments.

9105455

* Still trying to remove insecure broker code.

Adding ability for basic auth to override the bearer token auth.

d854de5

* template and catasb are still defaulting for bearer token auth.

adding temporary template to be used for a couple of days

0e6bc80

fixing gate build for a couple of days

ed15d7f

Adding ability to serve at the root url

cb80aca

* getting gate build working

shawn-hurley force-pushed the apiserver branch from 8361c24 to cb80aca Compare September 26, 2017 15:00

fixing build. going back to a single deployment template

899afa2

shawn-hurley merged commit f1a4158 into master Sep 26, 2017

shawn-hurley mentioned this pull request Sep 29, 2017

Bearer auth documentation #460

Merged

rthallisey mentioned this pull request Oct 4, 2017

Connection refused when using HTTPS ansibleplaybookbundle/ansible-playbook-bundle#122

Closed

jmrodri deleted the apiserver branch October 19, 2017 01:31

Bearer Token Auth via kubernetes Apiserver #445

Bearer Token Auth via kubernetes Apiserver #445

shawn-hurley commented Sep 20, 2017 •

edited

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

shawn-hurley Sep 20, 2017

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

shawn-hurley Sep 20, 2017

rthallisey Sep 20, 2017

jmrodri Sep 20, 2017

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

shawn-hurley Sep 20, 2017

rthallisey Sep 20, 2017

rthallisey Sep 20, 2017

shawn-hurley Sep 20, 2017

rthallisey commented Sep 20, 2017

jmrodri left a comment

rthallisey commented Sep 20, 2017

jwmatthews commented Sep 20, 2017

jmrodri commented Sep 20, 2017

jmrodri commented Sep 20, 2017 •

edited

shawn-hurley commented Sep 21, 2017 •

edited

jwmatthews commented Sep 21, 2017

enj commented Sep 22, 2017

eriknelson left a comment •

edited

jmrodri Sep 25, 2017

rthallisey commented Sep 25, 2017 •

edited

jwmatthews left a comment

Bearer Token Auth via kubernetes Apiserver #445

Bearer Token Auth via kubernetes Apiserver #445

Conversation

shawn-hurley commented Sep 20, 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

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

rthallisey commented Sep 20, 2017

jmrodri left a comment

Choose a reason for hiding this comment

rthallisey commented Sep 20, 2017

jwmatthews commented Sep 20, 2017

jmrodri commented Sep 20, 2017

jmrodri commented Sep 20, 2017 • edited

shawn-hurley commented Sep 21, 2017 • edited

jwmatthews commented Sep 21, 2017

enj commented Sep 22, 2017

eriknelson left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Sep 25, 2017 • edited

jwmatthews left a comment

Choose a reason for hiding this comment

shawn-hurley commented Sep 20, 2017 •

edited

jmrodri commented Sep 20, 2017 •

edited

shawn-hurley commented Sep 21, 2017 •

edited

eriknelson left a comment •

edited

rthallisey commented Sep 25, 2017 •

edited