add support for custom project domain in user identity. by acontes · Pull Request #165 · jenkinsci/openstack-cloud-plugin

acontes · 2017-09-06T10:12:02Z

The changes add the ability to connect to openstack instances having
the project's domain name different from the domain name.
It introduces an optional format for the user identity field while
remaining compatible with previous configurations of the
plugin.

olivergondza · 2017-09-07T07:45:24Z

I must admit I am confused by all the OS domains. So the domain we already have is for the domain user belongs to and the project-domain is the one the project belongs to and they can be different in general? With the latter being optional and used just to disambiguate colliding project names between project domains?

Can you please link relevant documentation to the field help to avoid the confusion?

acontes · 2017-09-07T09:03:15Z

In Openstack, both projects (tenant) and users must belong to a domain.
In a 'simple' configuration, projects and users are created within the same domain but nothing enforces it. For the sake of some security rules or different authentication mechanisms, projects and users can be created in different domains.

The fact that projects and users must be linked to a domain is documented in the Openstack documentation. Nothing specifies that they have to share the same domain.

In the code of the pull-request, I have decided to make the project's domain optional to maintain compatibility with the current implementation.

Guillaumichaud · 2017-09-07T16:30:51Z

src/main/resources/jenkins/plugins/openstack/compute/JCloudsCloud/help-identity.html

  <ul>
  <li>For v2 authentication use syntax <tt>TENANT_NAME:USER_NAME</tt></li>
-  <li>For v3 authentication use syntax <tt>PROJECT_NAME:USER_NAME:DOMAIN_NAME</tt></li>
+  <li>For v3 authentication use syntax <tt>[PROJECT_DOMAIN\]PROJECT_NAME:USER_NAME:DOMAIN_NAME</tt> PROJECT_DOMAIN is optional, use only when DOMAIN_NAME and PROJECT_DOMAIN differs</li>


acontes · 2017-09-08T05:04:15Z

typo fixed, thank you @Guillaumichaud for the review

olivergondza · 2017-09-08T06:51:41Z

Right, that was my understanding too. Is there a specific reason to use backslash as a separator and not a :? Also, for user we use user:domain ordering and for project it would be domain:project. How about PROJECT_NAME:PROJECT_DOMAIN:USER_NAME:DOMAIN_NAME?

acontes · 2017-09-08T08:55:59Z

fine by me, I will update the PR accordingly.

pjdarton · 2017-09-08T12:29:34Z

I think that all this foo:bar:optional stuff is just highlighting some tech-debt that we really ought to stuff all this in its own multi-field class. Not as part of this PR but I think we ought to do it...

When it becomes difficult to explain everything in the online help text, that indicates that it's overdue for being refactored ;-)

olivergondza · 2017-09-08T12:59:30Z

@pjdarton, certainly. Though, I hesitate to do it because of all the ajax method signatures getting nasty. IIRC, one of yours that would be painfully affected.

pjdarton · 2017-09-08T21:10:12Z

Yes, the doCheck.../doFill... methods will get a lot more arguments but it's only us that'll see that - the end user will get a much cleaner experience.
It's certainly not something to be done while we've got some large PRs awaiting merger ;-)

acontes · 2017-09-11T08:34:28Z

the updated version simplifies the identity string (as so the documentation) as the domain name for the project is now mandatory.
The migration to the new v3 schema is performed in the readResolve method.

acontes · 2017-09-22T08:18:54Z

Hello there, any comment/remark on the latest version ?
Is it fine on your side ?

acontes · 2017-10-10T12:57:36Z

PR updated to the latest version of master.

malice00 · 2017-10-25T13:04:10Z

I just tried these changes and they solved my authentication issues! I hope this can get merged and released soon?

olivergondza · 2017-10-26T06:12:49Z

I agree with @pjdarton, that this is best refactored into (at least) 2 fields for project and username.

Guillaumichaud · 2017-10-28T09:07:16Z

@acontes Was the "Unresolveable build extension" error expected? :)

acontes · 2017-11-04T21:11:20Z

rather than just adding a second field, I updated the code to use Jenkins' credentials and create two implementations ( one for v2 and one for v3 ). Adding a second field has the same impact on the API but is far less extensible.
As for the previous version of the pull-request, the migration from the former schema is performed in the readResolve method.

olivergondza

Great idea! I have added few minor comment on implementations side.

Is there some test that explicitly verifies the migration is performed correctly? IMO it is enough to extend existing "migration" tests to check the credentials object.

olivergondza · 2017-11-05T09:00:58Z

src/main/java/jenkins/plugins/openstack/compute/internal/Openstack.java

        ) throws FormValidation {
-            final String fingerprint = Util.getDigestOf(endPointUrl + '\n' + identity + '\n' + credential + '\n' + region);
+            if (auth == null) {
+                throw new RuntimeException("authentication is null");


Why not keeping the parameters @Nonnull then?

olivergondza · 2017-11-05T09:02:33Z

src/main/resources/jenkins/plugins/openstack/compute/JCloudsCloud/help-credentialId.html

@@ -0,0 +1,3 @@
+<div>
+    The credential to start the machine.


This is the credential to talk to openstack.

olivergondza · 2017-11-05T09:04:08Z

...in/resources/jenkins/plugins/openstack/compute/auth/AbstractOpenstackCredential/config.jelly

+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
+	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+</j:jelly>


Do we need a config.jelly for the abstract class if it is overridden by all implementations?

olivergondza · 2017-11-05T09:05:49Z

...main/resources/jenkins/plugins/openstack/compute/auth/OpenstackCredentialv2/help-tenant.html

@@ -0,0 +1,3 @@
+<div>
+    The tenant associated to the user to start the machine.


I find the "to start the machine" part in auth helps a bit confusing - it is needed for any interaction with openstack. I guess we can leave it out.

olivergondza · 2017-11-05T09:07:32Z

...in/resources/jenkins/plugins/openstack/compute/auth/OpenstackCredentialv3/help-username.html

@@ -0,0 +1,3 @@
+<div>
+    User name of the user to start the machine.


"to start the machine" is extremely confusing here.

olivergondza · 2017-11-05T09:14:00Z

src/main/java/jenkins/plugins/openstack/compute/JCloudsCloud.java

 public class JCloudsCloud extends Cloud implements SlaveOptions.Holder {

    private static final Logger LOGGER = Logger.getLogger(JCloudsCloud.class.getName());
+    public String credentialId;


olivergondza · 2017-11-05T09:14:46Z

src/main/java/jenkins/plugins/openstack/compute/JCloudsCloud.java

-    public final @Nonnull String identity;
-    public final @Nonnull Secret credential;
+
+    public /*final*/ @Nonnull String identity;


This one should be @Deprecated as well, right?

... and transient.

olivergondza · 2017-11-05T09:15:51Z

src/main/java/jenkins/plugins/openstack/compute/JCloudsCloud.java

+                    OpenstackCredentials.add(migratedOpenstackCredential);
+                    OpenstackCredentials.save();
+                } catch (IOException e) {
+                    e.printStackTrace();


Send to logger, instead.

The changes add the ability to connect to openstack instances having the project's domain name different from the domain name by changing the format of the identity string. The identity string using the new format are converted into the new one using when the plugin is loaded.

acontes · 2017-11-09T15:06:34Z

revised version of the PR to address all the comments.

Credential migration is verified in a new test.
@nonnull is back, not sure why I changed that initially
reworded the help on the fields

add support for custom project domain in user identity.

olivergondza · 2017-11-21T14:52:02Z

Merged, thanks!

twillouer · 2017-11-21T15:43:19Z

👍

Guillaumichaud reviewed Sep 7, 2017

View reviewed changes

acontes force-pushed the custom_project_domainname branch from b0c2194 to 3528f21 Compare September 8, 2017 05:01

olivergondza closed this Sep 8, 2017

olivergondza reopened this Sep 8, 2017

acontes force-pushed the custom_project_domainname branch from 3528f21 to 3464964 Compare September 11, 2017 08:29

olivergondza self-requested a review September 22, 2017 11:49

acontes force-pushed the custom_project_domainname branch from 3464964 to 90edaee Compare October 7, 2017 08:13

acontes force-pushed the custom_project_domainname branch from 90edaee to b38fa20 Compare November 4, 2017 21:06

acontes force-pushed the custom_project_domainname branch from b38fa20 to 06d85c0 Compare November 4, 2017 21:23

olivergondza suggested changes Nov 5, 2017

View reviewed changes

acontes force-pushed the custom_project_domainname branch from 06d85c0 to fd0f931 Compare November 9, 2017 14:47

olivergondza approved these changes Nov 10, 2017

View reviewed changes

olivergondza self-assigned this Nov 10, 2017

olivergondza mentioned this pull request Nov 20, 2017

Can't specify a tenant name with ":" #183

Closed

olivergondza merged commit fd0f931 into jenkinsci:master Nov 21, 2017

olivergondza added a commit that referenced this pull request Nov 21, 2017

Merge pull request #165 from acontes/custom_project_domainname

ae11ca9

add support for custom project domain in user identity.

balous mentioned this pull request Jan 11, 2018

Unable to retrieve image list #185

Closed

		@@ -0,0 +1,3 @@
		<div>
		The tenant associated to the user to start the machine.

		@@ -0,0 +1,3 @@
		<div>
		User name of the user to start the machine.

Uh oh!

Conversation

acontes commented Sep 6, 2017

Uh oh!

olivergondza commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acontes commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acontes commented Sep 8, 2017

Uh oh!

olivergondza commented Sep 8, 2017

Uh oh!

acontes commented Sep 8, 2017

Uh oh!

pjdarton commented Sep 8, 2017

Uh oh!

olivergondza commented Sep 8, 2017

Uh oh!

pjdarton commented Sep 8, 2017

Uh oh!

acontes commented Sep 11, 2017

Uh oh!

acontes commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acontes commented Oct 10, 2017

Uh oh!

malice00 commented Oct 25, 2017

Uh oh!

olivergondza commented Oct 26, 2017

Uh oh!

Guillaumichaud commented Oct 28, 2017

Uh oh!

acontes commented Nov 4, 2017

Uh oh!

olivergondza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acontes commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivergondza commented Nov 21, 2017

Uh oh!

twillouer commented Nov 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

olivergondza commented Sep 7, 2017 •

edited

Loading

acontes commented Sep 7, 2017 •

edited

Loading

acontes commented Sep 22, 2017 •

edited

Loading

acontes commented Nov 9, 2017 •

edited

Loading