Skip to content
This repository has been archived by the owner on Feb 4, 2019. It is now read-only.

Documentation about Openstack Keystone v2/v3 usage #214

Closed
wants to merge 1 commit into from
Closed

Documentation about Openstack Keystone v2/v3 usage #214

wants to merge 1 commit into from

Conversation

axel3rd
Copy link
Contributor

@axel3rd axel3rd commented May 7, 2018

See JCLOUDS-1414, documentation about Openstack keystone v2/v3 usage

@jclouds-commentator
Copy link

@jclouds-commentator
Copy link

@jclouds-commentator
Copy link

@jclouds-commentator
Copy link

@axel3rd
Copy link
Contributor Author

axel3rd commented May 7, 2018

Some tries about code & list typo, the last looks fine: Openstack Keystone v2-v3 authentication

Copy link
Contributor

@andreaturli andreaturli left a comment

Choose a reason for hiding this comment

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

thanks @axel3rd it is super useful!

I've left some comments mainly on the snippets: please address them and edit the document accordingly, i.e. if you replace Some.class with NovaApi.class make sure you do it all over the places.

Thanks

* On v2: *tenant*, *user*, *password*.
* On v3: a *project* (new name for *tenant*), an authentication *domain* for this *project*, a *user*, an authentication *domain* for this *user* (the two domains can be different).

JClouds provide backward compatibility between keystone v2-v3 ... but you should have following section in mind to fully understand the authentication on your Openstack platform (in addition of blog: [OpenStack Keystone V3 Support](https://jclouds.apache.org/blog/2018/01/16/keystone-v3/)).
Copy link
Contributor

Choose a reason for hiding this comment

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

please use jclouds or Apache jclouds

Copy link
Contributor

Choose a reason for hiding this comment

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

provides instead of provide

This snippet:
{% highlight java %}
final Properties overrides = new Properties();
overrides.put(KeystoneProperties.KEYSTONE_VERSION, "2");
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't KEYSTONE_VERSION=2 the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According the Javadoc the default is 3 ; but I have not verified or really measured the impact to not provide this property ...

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @axel3rd for the quick update

I think from nova code https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/NovaApiMetadata.java#L74 the default keystone version is 3 but the extra property would probably not hurt

@demobox @nacx any preference?

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave it here for clarity. Also as we move forward version 2 will be eventually deprecated and removed, so better encourage users to be explicit about the version being used.

final Properties overrides = new Properties();
overrides.put(KeystoneProperties.KEYSTONE_VERSION, "2");

ContextBuilder.newBuilder(new SomeApiMetadata())
Copy link
Contributor

Choose a reason for hiding this comment

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

why new SomeApiMetadata()? wouldn't be clear with something as generic as openstack-nova

.endpoint("https://host:5000/v2.0")
.credentials("myTenant:foo", "bar")
.overrides(overrides)
.buildApi(Some.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

why Some.class here? I think NovaApi.class is a bit better, as it is now in http://jclouds.apache.org/guides/openstack/#nova, wdyt?

@jclouds-commentator
Copy link

@axel3rd
Copy link
Contributor Author

axel3rd commented May 7, 2018

@andreaturli : Thanks for grammar fix & suggestions. Fixed in Keystone v2-v3 authentication. Except about KeystoneProperties.KEYSTONE_VERSION ... not sure about that.

@andreaturli
Copy link
Contributor

Actually @axel3rd I think we should somehow edit List Servers and Swift: Use Containers paragraphs as well pointing maybe the reader to the authentication section above instead of using

        novaApi = ContextBuilder.newBuilder(provider)
                .endpoint("http://xxx.xxx.xxx.xxx:5000/v2.0/")
                .credentials(identity, credential)
                .modules(modules)
                .buildApi(NovaApi.class);

wdyt?

@axel3rd
Copy link
Contributor Author

axel3rd commented May 7, 2018

pointing maybe the reader to the authentication section above instead of using

I have hesitate to do that ... but the current snippet explicits how to retrieve an API (nova, swift, ...) even if v2 is deprecated (v3 needs more code).
So keystone considerations was writted before (hoping that user read it before trying sample).

Perhaps a consensus could be to add a comment before ContextBuilder like:

// Please refer to 'Keystone v2-v3 authentication' for complete authentication use case

@nacx
Copy link
Member

nacx commented May 7, 2018

One minor comment, but it would help readability if you format the generated json auth example.

@jclouds-commentator
Copy link

@jclouds-commentator
Copy link

@axel3rd
Copy link
Contributor Author

axel3rd commented May 7, 2018

Perhaps a consensus could be to add a comment before ContextBuilder like [...]:
One minor comment, but it would help readability if you format the generated json auth example.

Done (without new-line-comma for json, to reduce lines number). Last proposal: Keystone v2-v3 authentication (and following)

Copy link
Member

@nacx nacx left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.
Since you reference a new keystone property I'd hold this and merge once the changes in Keystone are merged and the new property exists.

@axel3rd
Copy link
Contributor Author

axel3rd commented May 7, 2018

Since you reference a new keystone property I'd hold this and merge once the changes in Keystone are merged and the new property exists.

Yes ... and perhaps some minor change on properties name in progress ...

@andreaturli
Copy link
Contributor

thanks @axel3rd

@jclouds-commentator
Copy link

@axel3rd
Copy link
Contributor Author

axel3rd commented May 7, 2018

Yes ... and perhaps some minor change on properties name in progress ...

Done, 1 line changed:

overrides.put(KeystoneProperties.PROJECT_DOMAIN_NAME, "default"); // Since jclouds v2.2.0 (see PROJECT_DOMAIN_ID as complement)

Linked with jclouds#1204.

@nacx
Copy link
Member

nacx commented May 10, 2018

Changes merged and published to the website. Thanks @axel3rd!

@nacx nacx closed this May 10, 2018
@axel3rd axel3rd deleted the JCLOUDS-1414-OpenstackKeystoneComplements branch May 10, 2018 13:12
@axel3rd
Copy link
Contributor Author

axel3rd commented May 10, 2018

@nacx : As a "new feature", I didn't thought it was could be integrated in v2.1.x, I will be attentive to the roadmap because this PR doc is perhaps not true: : Keystone v2-v3 authentication (v3: Project-scoped)

overrides.put(KeystoneProperties.PROJECT_DOMAIN_NAME, "default"); // Since jclouds v2.2.0 (see PROJECT_DOMAIN_ID as complement)

@nacx
Copy link
Member

nacx commented May 10, 2018

We can change that. As long as the feature does not break backward compat, I see no problem in adding it in the next bugfix release. It will help adoption and facilitate early testing.

@axel3rd
Copy link
Contributor Author

axel3rd commented May 10, 2018

So very valuable 2 characters PR ^^ : #215

@nacx
Copy link
Member

nacx commented May 10, 2018

Often these tiny PRs get more review comments and change requests than thousand lines ones! :)

@@ -92,6 +93,220 @@ There are some differences in terminology between jclouds and OpenStack that sho
</div>
</div>

## <a id="keystone"></a>Keystone v2-v3 authentication

Openstack Keystone (aka: [OpenStack Identity Service](https://docs.openstack.org/keystone/latest/)) has major changes between v2 and v3 (detail. [Identity API v2.0 and v3 History](https://docs.openstack.org/keystone/latest/contributor/http-api.html)).
Copy link
Member

Choose a reason for hiding this comment

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

[minor] Quite a few instances of "Openstack" (lowercase s) rather than "OpenStack" here - is that worth cleaning up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In official documentation, "S" is in uppercase. I can do the cleanup in a way or the other, but we should choose :)


Openstack Keystone (aka: [OpenStack Identity Service](https://docs.openstack.org/keystone/latest/)) has major changes between v2 and v3 (detail. [Identity API v2.0 and v3 History](https://docs.openstack.org/keystone/latest/contributor/http-api.html)).

Basically to login, you should provide:
Copy link
Member

Choose a reason for hiding this comment

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

[minor] Drop "basically"; just "To login, provide:"?

* On v2: *tenant*, *user*, *password*.
* On v3: a *project* (new name for *tenant*), an authentication *domain* for this *project*, a *user*, an authentication *domain* for this *user* (the two domains can be different).

jclouds provides backward compatibility between keystone v2-v3 ... but you should have following section in mind to fully understand the authentication on your Openstack platform (in addition of blog: [OpenStack Keystone V3 Support](https://jclouds.apache.org/blog/2018/01/16/keystone-v3/)).
Copy link
Member

Choose a reason for hiding this comment

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

[minor] "...compatibility between Keystone v2 and v3, but you should keep the following in mind to fully understand authentication against your OpenStack platform:

lots of examples

See also the recent "OpenStack Keystone v3 Support" blog post.

Will produce when authentication needed:

POST https://host:5000/v2.0/tokens HTTP/1.1
{
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular benefit to trying to list the request so exactly here? In my experience, these are the parts of documentation that go stale very quickly.

Can we instead either give the code samples to the user and explain how they can configure logging to see what exactly is being sent (if that is necessary), or describe this a bit more abstractly, e.g. in form of a table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configure logging to see what exactly is sent is simple (jclouds write the property to set to true and SLF4J in debug).

The (little) difficulty in jclouds authentication usage is the option to configure. It depends mainly of OpenStack platform configuration (v2, v3, domains, ...) ... and some manual REST tries on token endpoint could be required to see what is supported by the platform.

So these snippet are to help the user on jclouds usage when it has found its way of auhentication on OpenStack.

@@ -146,6 +361,7 @@ public class JCloudsNova implements Closeable {
String identity = "demo:demo"; // tenantName:userName
String credential = "devstack";

// Please refer to 'Keystone v2-v3 authentication' chapter for complete authentication use case
Copy link
Member

Choose a reason for hiding this comment

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

[minor] "section" rather than "chapter"? Also, should this comment perhaps come before the declaration of String identity?

@axel3rd
Copy link
Contributor Author

axel3rd commented May 11, 2018

@demobox : Thanks for the review. I will do it in a // PR (this one merged) after feedback on my remarks.

@demobox
Copy link
Member

demobox commented May 11, 2018

I will do it in a // PR (this one merged) after feedback on my remarks.

Thanks for taking a look! I should add that I think most of my comments are minor, and it's also fine to leave things as they are unless we feel there's really a good reason for a follow-up PR.

And thanks for helping improve the jclouds documentation, of course!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants