Converting the AuthSecret field to a union AuthInfo type #877

arschles · 2017-05-18T23:00:48Z

This change will enable operators to chose different auth schemes for brokers, as the OSB API spec begins to support more in the future.

Fixes #864

pmorie

Looks like what I described in the issue - the validations have to move, of course, and there should be a new validation that if you supply auth info that you supply basic auth (which we will relax as we add other types to AuthInfo).

duglin · 2017-05-19T06:11:15Z

pkg/apis/servicecatalog/types.go

+type BrokerAuthInfo struct {
+	// BasicAuthSecret is a reference to a Secret containing auth information the
+	// catalog should use to authenticate to this Broker using basic auth.
+	BasicAuthSecret *v1.ObjectReference


Is the assumption that we know which one is being used because all others will be nil ?
If so, does that require a loop (or set of if-statements) to find the right one? Could we instead just make AuthInfo in the previous struct a pointer to an Object and dynamically find its type at runtime?

@duglin

Is the assumption that we know which one is being used because all others will be nil ?

Yes, the generic assertion is that if you provide a union type, you must provide one and only one field of that type. Since we only have basic auth right now, that collapses into 'you must provide basic auth'

@duglin in addition to what @pmorie said, only one will be set at once (as the comment above says), so only one will be non-nil at once

pmorie · 2017-05-19T18:30:34Z

pkg/apis/servicecatalog/validation/broker.go

-			allErrs = append(allErrs, field.Invalid(fldPath.Child("authSecret", "namespace"), spec.AuthSecret.Namespace, msg))
+	// TODO: when we start supporting additional auth schemes, this code will have to accommodate
+	// the new schemes
+	basicAuthSecret := spec.AuthInfo.BasicAuthSecret


This is not nil-safe

good call, I'll add an error if basicAuthSecret == nil

pmorie · 2017-05-19T18:35:43Z

pkg/apis/servicecatalog/validation/broker_test.go

-					AuthSecret: &v1.ObjectReference{
-						Namespace: "test-ns",
-						Name:      "test-secret",
+					AuthInfo: servicecatalog.BrokerAuthInfo{


Needs a test case for AuthInfo not nil but empty

note that AuthInfo cannot be nil because it's not a pointer. BasicAuthInfo inside of AuthInfo can be nil, though, and that needs a test.

pmorie · 2017-05-19T18:39:00Z

pkg/controller/controller.go

-	if broker.Spec.AuthSecret == nil {
+	// TODO: when we start supporting additional auth schemes, this code will have to accommodate
+	// the new schemes
+	basicAuthSecret := broker.Spec.AuthInfo.BasicAuthSecret


also not nil-safe

this is nil-safe because AuthInfo is a BrokerAuthInfo, not a *BrokerAuthInfo, and there's a check for a nil basic auth secret (which is a *v1.ObjectReference)

arschles · 2017-05-19T19:59:22Z

@pmorie comments addressed and changes made where appropriate. PTAL

vaikas · 2017-05-24T20:13:04Z

Looks like the integration test failure is for realsies:
[Build & Unit/Integration Tests] I0523 21:04:01.468765 3764 round_trippers.go:398] curl -k -v -XGET -H "Accept: application/json, /" -H "User-Agent: integration.test/v0.0.0 (linux/amd64) kubernetes/$Format" http://localhost:23533/apis/servicecatalog.k8s.io/v1alpha1/serviceclasses?resourceVersion=0
[Build & Unit/Integration Tests] I0523 21:04:01.469412 3764 request.go:991] Response Body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Broker.servicecatalog.k8s.io "test-broker" is invalid: spec.authInfo.basicAuthSecret: Required value: a basic auth secret is required","reason":"Invalid","details":{"name":"test-broker","group":"servicecatalog.k8s.io","kind":"Broker","causes":[{"reason":"FieldValueRequired","message":"Required value: a basic auth secret is required","field":"spec.authInfo.basicAuthSecret"}]},"code":422}
[Build & Unit/Integration Tests] --- FAIL: TestBasicFlows (5.07s)
[Build & Unit/Integration Tests] controller_test.go:267: controller start
[Build & Unit/Integration Tests] controller_test.go:275: informers start
[Build & Unit/Integration Tests] controller_test.go:104: error creating the broker &{{"" ""} {"test-broker" "" "" "" "" "" '\x00' "0001-01-01 00:00:00 +0000 UTC" %!q(*int64=) map[] map[] [] [] ""} {"https://example.com" {"nil"}} {[]}} ("Broker.servicecatalog.k8s.io "test-broker" is invalid: spec.authInfo.basicAuthSecret: Required value: a basic auth secret is required")
[Build & Unit/Integration Tests] framework.go:61: Shutting down server on ports: 23533 and 16738
[Build & Unit/Integration Tests] FAIL
[Build & Unit/Integration Tests] exit status 1
[Build & Unit/Integration Tests] FAIL github.com/kubernetes-incubator/s

pmorie

LGTM; just a couple little nits (sorry), and needs rebase.

pmorie · 2017-06-07T15:42:30Z

pkg/apis/servicecatalog/types.go

-	// catalog should use to authenticate to this Broker.
-	AuthSecret *v1.ObjectReference
+	// AuthInfo contains the data that the service catalog should use to authenticate
+	// with the Broker


missing period

pmorie · 2017-06-07T15:42:58Z

pkg/apis/servicecatalog/v1alpha1/types.go

-	// catalog should use to authenticate to this Broker.
-	AuthSecret *v1.ObjectReference `json:"authSecret,omitempty"`
+	// AuthInfo contains the data that the service catalog should use to authenticate
+	// with the Broker


missing period

pmorie · 2017-06-07T15:44:42Z

pkg/apis/servicecatalog/validation/broker_test.go

@@ -32,6 +32,8 @@ func TestValidateBroker(t *testing.T) {
 		valid  bool
 	}{
 		{
+			// covers the case where there is no AuthInfo field specified. the validator should
+			// ignore the field and still succeed the validation
 			name: "valid broker - no auth secret",


not a feedback on your PR, but I have had a lot of luck with a method that returns a valid resource, and then subtracting / mutating things to create objects for invalid scenarios.

This change will enable operators to chose different auth schemes for brokers, as the OSB API spec begins to support more in the future. Fixes kubernetes-retired#864

pmorie · 2017-06-07T18:37:44Z

LGTM

arschles added the in-progress label May 18, 2017

arschles added this to the 0.1.0 milestone May 18, 2017

arschles requested review from pmorie, krancour and vaikas May 18, 2017 23:00

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2017

pmorie reviewed May 19, 2017

View reviewed changes

duglin reviewed May 19, 2017

View reviewed changes

arschles removed the in-progress label May 19, 2017

arschles self-assigned this May 19, 2017

pmorie reviewed May 19, 2017

View reviewed changes

arschles modified the milestones: 0.8.0, 0.1.0 May 22, 2017

arschles force-pushed the auth-info branch from 515e707 to ad7de2e Compare May 22, 2017 21:58

krancour approved these changes May 23, 2017

View reviewed changes

krancour added the LGTM1 label May 23, 2017

vaikas added the LGTM2 label May 23, 2017

vaikas modified the milestones: 0.8.0, 0.1.0 May 24, 2017

arschles force-pushed the auth-info branch from 76633e6 to 83d0ed0 Compare June 5, 2017 21:34

pmorie reviewed Jun 7, 2017

View reviewed changes

Converting the AuthSecret field to a union AuthInfo type

27f7ddc

This change will enable operators to chose different auth schemes for brokers, as the OSB API spec begins to support more in the future. Fixes kubernetes-retired#864

arschles force-pushed the auth-info branch from 83d0ed0 to 27f7ddc Compare June 7, 2017 18:25

arschles added 3 commits June 7, 2017 11:29

fixing test compile issues

eae84c4

adding grammar to godoc

78c22ae

adding grammar to open api description

5cb2294

pmorie added the LGTM3 label Jun 7, 2017

arschles added awaiting-ci and removed awaiting-ci labels Jun 7, 2017

pmorie merged commit 066159d into kubernetes-retired:master Jun 7, 2017

arschles deleted the auth-info branch June 7, 2017 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting the AuthSecret field to a union AuthInfo type #877

Converting the AuthSecret field to a union AuthInfo type #877

arschles commented May 18, 2017 •

edited

Loading

pmorie left a comment

duglin May 19, 2017

pmorie May 19, 2017

arschles May 19, 2017

pmorie May 19, 2017

arschles May 19, 2017

pmorie May 19, 2017

arschles May 19, 2017

pmorie May 19, 2017

arschles May 19, 2017

arschles commented May 19, 2017

vaikas commented May 24, 2017

pmorie left a comment

pmorie Jun 7, 2017

pmorie Jun 7, 2017

pmorie Jun 7, 2017

pmorie commented Jun 7, 2017

Converting the AuthSecret field to a union AuthInfo type #877

Converting the AuthSecret field to a union AuthInfo type #877

Conversation

arschles commented May 18, 2017 • edited Loading

pmorie 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

arschles commented May 19, 2017

vaikas commented May 24, 2017

pmorie 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

pmorie commented Jun 7, 2017

arschles commented May 18, 2017 •

edited

Loading