Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bearer auth documentation #460

Merged
merged 4 commits into from
Oct 2, 2017

Conversation

shawn-hurley
Copy link
Contributor

Describe what this PR does and why we need it:
Follow on PR for #445 for the documentation
Changes proposed in this pull request

  • Update the auth documentation to document both how to set up auth, and how it is implemented.
  • Update various other docs to include updates.

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
N/A
Which issue this PR fixes (This will close that issue when PR gets merged)
N/A

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

good enough

docs/auth.md Outdated
**Note: When using OpenShift 3.6 the only option for authentication is Basic Auth. Basic Auth must be enabled to true.**

### Basic Auth
The below section will focus on the imlementation of basic auth.
Copy link
Contributor

Choose a reason for hiding this comment

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

TYPO: imlementation -> implementation

Typically the broker is configured via a
[ConfigMap](https://docs.openshift.com/container-platform/3.5/dev_guide/configmaps.html) in a deployment template.
##### Deployment template
Typically the broker is configured via a [ConfigMap](https://docs.openshift.com/container-platform/3.5/dev_guide/configmaps.html) in a deployment template.
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed in this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stopped the new line. It was creating a new line at via a I put them on the same line. I thought it looked really odd.

@@ -123,7 +124,7 @@ spec:
name: asb-auth-secret
```

Starting wtih v0.0.17 of the service catalog the broker resource configuration changes.
Starting with v0.0.17 of the service catalog the broker resource configuration changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good typo catch.

The below section will focus on the bearer token auth.

#### Configuration
By default, if no authentication is specified the broker will use bearer token auth. The bearer token authentication will use delegated auth from the [kubernetes apiserver](https://github.com/kubernetes/apiserver) library.
Copy link
Contributor

Choose a reason for hiding this comment

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

😭

docs/auth.md Outdated
#### Configuration
By default, if no authentication is specified the broker will use bearer token auth. The bearer token authentication will use delegated auth from the [kubernetes apiserver](https://github.com/kubernetes/apiserver) library.

The configuration is to grant access, through [kubernetes RBAC](https://kubernetes.io/docs/admin/authorization/rbac/) roles and role-bindings, to the url prefix. The broker has added a configuration option `cluster_url` to specify the url_prefx. This value will default to `ansible-service-broker`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd wrap the line better but the important one is 👍
TYPO: url_prefx -> url_prefix

docs/auth.md Outdated
```

#### Deployment template and secrets
Here is an example of creating a secret that the service catalog can use, from the above role that will grant access. From the [deployment template](https://github.com/openshift/ansible-service-broker/blob/master/templates/deploy-ansible-service-broker.template.yaml#L224-L248):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a hard time parsing the first sentence.

Here is an example of creating a secret that the service catalog can use, from the above role that will grant access.

the last part feels confusing: from the above role that will grant access

See if you can reword that a little more clearly. Right now I'm struggling coming up with a better way to phrase it though.

## Developer section

### Auth design
### Basic Auth design

The authentication system is built with a set of interfaces to allow for easily
adding new methods of authentication. The 3 core interfaces are: Provider,
Copy link
Contributor

Choose a reason for hiding this comment

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

SIGH

@jmrodri
Copy link
Contributor

jmrodri commented Sep 29, 2017

Excellent job failing travis with a document change :) @rthallisey is this the same CI problem we've been seeing lately?

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

Just asking the links be locked down to current commit sha.

docs/auth.md Outdated

Bearer auth is using the [kubernetes apiserver](https://github.com/kubernetes/apiserver) to do delegated authentication and authorization. Kubernetes team will keep this library up to date.

The first thing that you need to do when setting up the generic api server is to tell it the cert, key, port, and address to listen on. You are going to use the [`SecureServingOptions`](https://github.com/kubernetes/apiserver/blob/master/pkg/server/options/serving.go#L38) to set these values.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should use a permalink for the kubernetes link.

Copy link
Member

Choose a reason for hiding this comment

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

I see a handful of other references to master, I think they should all be permalinks.

Source: have read a doc that pointed to master and it was no longer the line that they had intended in the doc.

@jmrodri jmrodri merged commit 34f643e into openshift:master Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants