[JENKINS-57570] - Add support for JCasC using SecretBytes #48

stephenashank · 2019-04-05T05:25:50Z

I am pulling this out separately before putting my changes on top of #19, this is a separate branch. The reason that the commits from the earlier PR include me is because I first rebased onto master, but I plan to merge the changes there first before merging my additional changes.

I've converted most fields to be set through @DataboundSetters, with the exception of emailAddress from the P12 config because it is final.

I had to rename jsonKeyFile/p12KeyFile (the String field) to prevJsonKeyFile/prevP12KeyFile, and use getters and setters for jsonKeyFile (the FileItem that is used in the UI). Stapler will only call the @DataboundSetter if there is a corresponding accessor with the correct type.

Given the amount that needed to be updated in terms of dependencies, I think this might warrant a major version increase, interesed in feedback on that.

ikedam

Sorry but I couldn’t do a good review as I’m not a maintainer of this plugin and don’t know much about JCaaC.
I added some comments about general Jenkins issues and coding styles.

This is hard to review as I can’t see changes from #19 . I highly recommend you to create another review to see changes from #19.

ikedam · 2019-04-05T13:41:37Z

pom.xml

@@ -110,6 +110,7 @@
 	    <format>html</format>
 	    <format>xml</format>
 	  </formats>
+    <check/>


looks an inconsistent indentation.

I wouldn't worry too much about formatting nits in this PR. We're about to make the code formatting automatic across all GCP supported plugins.

ikedam · 2019-04-05T13:42:22Z

pom.xml

+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>plain-credentials</artifactId>
+      <version>1.5</version>


Do you really need this?

Yes, this is required to be able to configure the credentials, at least during the CasC tests. I will see if I can restrict the scope to tests as well without breaking manual configuration with the Google Robot Private Key credentials

It sounds like a bug of JCasC.
It’s strange that plugins require dependencies that aren’t necessary for plugin features.
Please consider to file a Jira ticket for JCasC.

These dependencies are needed because JCasC does not do plugin installation.
You should scope these dependencies as either optional or test.
That should be handled by Jenkins Core or inside the POM.
See https://issues.jenkins-ci.org/browse/JENKINS-53767

JCasC uses GitHub issues.

Should not be needed if the credential plugin had direct JCasC support

I discovered that I was able to scope to tests by also marking optional as true. This is so that hpi does not include it, like this example from the parent plugin pom here.

The reason is that the hpi plugin includes test dependencies by default: https://jenkinsci.github.io/maven-hpi-plugin/run-mojo.html#dependencyResolution

ikedam · 2019-04-05T13:42:40Z

pom.xml

+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>ssh-credentials</artifactId>
+      <version>1.13</version>


Do you really need this?

This is not strictly necessary but when I run mvn hpi:run without this the configuration as code support plugin fails to load and leaves an error in the terminal. I don't know why that happens as everything CasC specific should remain scoped to testing. I'll see what I can do to suppress that warning without including this.

These dependencies are needed because JCasC does not do plugin installation.
You should scope these dependencies as either optional or test.
That should be handled by Jenkins Core or inside the POM.
See https://issues.jenkins-ci.org/browse/JENKINS-53767

this shouldn't be needed anymore on latest JCasc

ikedam · 2019-04-05T13:46:39Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

    if (jsonKeyFile != null && jsonKeyFile.getSize() > 0) {
      try {
        JsonKey jsonKey = JsonKey.load(new JacksonFactory(),
-                jsonKeyFile.getInputStream());
+            jsonKeyFile.getInputStream());


I recommend you to use indentations in the same way as original codes.

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

src/main/java/com/google/jenkins/plugins/credentials/oauth/P12ServiceAccountConfig.java

stephenashank · 2019-04-05T17:16:41Z

If you look at the changes tab you should be able to select only the commits I added for review.

stephenashank · 2019-04-05T18:10:51Z

src/test/resources/com/google/jenkins/plugins/credentials/oauth/test-key.json

+  "type": "service_account",
+  "project_id": "jenkins-g-oauth-plugin",
+  "private_key_id": "b5bde820c66ffc456deeeca6c7a36b70f937f053",
+  "private_key": "-----BEGIN PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCkQqPYbTVxB8Hw\nNSpfIJ5k+8B5Y5Ly5jW+hirOXwSOjJ38Baw1qQPADWc2U6GkKhZmyQscAJgIvFVW\nvfNrDDQgKolVr4GGf09m9vYlusnCPqFksL8L/qd92wi0sYMs5dNtm/OfW2zat1fD\nIXtCm9zglkwZKhln3p01E3HB3x1IDtZoZsIqxm+ImXsvFwADMt8uTB1Hq1n4YuFm\nFu2Um8CPHr59eV7REAEQc948AciYXGeIJTxUrWLXLJqk5zZgktZmWoShMW3UKl/k\nP8vNoQOsuR6sJ/gcld7EHScQtU22+FNrFEuE6RHO8nwjN+QqNjh0MR7DQxZoHMAw\n1IE83usxAgMBAAECggEAA2TA9pGibrDbHRocfRDJeo5E2dhe+SlDKT0waqk5IoC0\nEPiLiCP5paJvp4ZNcu/8XKUdt2qDBmQtf0bA0SbhHhyCEAAZuC4BzyxkcoXA3kqo\n6JgDtgaNwcDp/A5L5qkWLjl2rF9JMpDiek/Jfuge/wQjGctLCtOx/wTLsmNCCAkf\nTVJh9JjFH59stPJG8j4yjx0L+EKdwQyTKTahXrRm4rccjDwhvA7T7RSqmbjhDu2+\nX9ljP0Uch/udLYGEt+SnpcVJuJTJ+ivmG8zvKX5YPndC6hfY13eMXxtRMYuYQOuf\nA0rnnhDLMQPh5fg2eaXRNI7N4sNLmgYsp/mGWsftmQKBgQDn7rnlEBncJHJWRJB/\nlUZ5toGDE4nO+N9kkje2K2Zlyn5gwOmnakaFyVTbCjzSwyatOTxWyAsqVLLb8dXQ\n59m+A3YZ3QYNYggN8WgkHnt1EWB56EbEUFX0XaWccYBC+4G37V44FXeblVzLQIfF\n+uiJMqp2hsYEmgsn35jzyWVXWQKBgQC1TjSfUa4ZQs0AuTy0A55Mpmk1l4ZK4Ddx\n/vfQMbTnrReVZ7C/nSvoDy48hMzEmMbuMiivk7jnkFvx7M6LiNPhEqqoJ5Y5mBca\nNvnnN4TckuJdAtknygBogGGs13ypt1KaA4FetMT4IWqsoFI+g6Z7a+7pxnHHdmjb\nGk5J8juPmQKBgDd3Tvup2xVbngBli00HrQAElnp7XLSjrgEGOs6VGHr1bz3CRN6l\nutHZ8TIlA6C/zOsWSgjS9GCeOtwAvMql99vKRh8vTXg73oM/HVGt/IZlrnXZB7uB\nHt0+3BFKz4q1TTNoT+UHtT/++18cQpwlQiE1fbC00HxfPpW9kn2Sx2qZAoGAVays\nDzEw3P0FFLdz6PjgwAXPJ6T/r7g+Wx8KCZbDjsrrnw1Np25lBhbOWYjDno83Se3n\n7fgXY02DNVIa1DMHNI92l1mFkpe9KwUZmFpS7Ux3rU4gQb4h7T/laCC35xca8G6B\nnrg7b6mS2Bo2YxYhAKejUVMWBxR8PjUzE5xk6tkCgYB9ZqRD0cyCIphOdzAM9l1P\nDIZtesjraXDKZEiWJJq0yOE5qCnqF0pAVFSZF3pVJ0gnurzjm5tPx6xRS8qjbu1X\n2dux06e7uBQ1qVddP9buf3LmDDTDcVcBYs4dATAy+D8bpdNgnQgx0koo0bKNQSr6\npSpvb/7PiYTR7Lqmjn3RxA==\n-----END PRIVATE KEY-----\n",


Note: this key file and the p12 key are well formed but not usable for authentication

However, after seeing your comment on the compute engine plugin I'll switch to using mocks here

craigdbarber · 2019-04-10T17:03:12Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

@@ -53,36 +63,109 @@
  private static final long serialVersionUID = 6818111194672325387L;
  private static final Logger LOGGER =
      Logger.getLogger(JsonServiceAccountConfig.class.getSimpleName());
-  private String jsonKeyFile;
+  @CheckForNull


We'll want to migrate away from using FindBugs as it's no longer maintained. I've created a new issue for this, could you please add a TODO here referencing it: #49

Additionally, this class duplicates much of the data/logic contained in this class: https://github.com/googleapis/google-auth-library-java/blob/master/oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java
I've created a new issue tracking de-duplication work with google-auth-library-java project, could you please add a TODO here referencing it: #50

You can update the parent pom to get SpotBugs for free 😄
No need to switch annotation since SpotBugs have un-deprecated the FindBugs once

https://github.com/jenkinsci/plugin-pom/blob/master/CHANGELOG.md#336

@Casz Thank you for this, turns out this actually has been building with spotbugs since I already upgraded the parent pom for CasC compatibility.

@craigatgoogle Are you satisfied with that resolution? Added TODO from #50 but not #49 for now.

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

craigdbarber · 2019-04-10T17:07:24Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

+    }
+  }
+
+  /**@param jsonKeyFileUpload uploaded json key file */


This Javadoc header is improperly formatted and is missing a method description. Additionally capitalization and punctuation need correcting.

I think for the most part getters and setters are self documenting and the parameter description clears up what is being set. I'm struggling to come up with a description to add here that is both not redundant and not exposing unnecessary details about the implementation. What are your thoughts?

craigdbarber · 2019-04-10T17:09:04Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

+    if (filename == null && prevJsonKeyFile != null) {
+      this.filename = extractFilename(prevJsonKeyFile);
+      this.secretJsonKey = getSecretBytesFromFile(prevJsonKeyFile);
+    }


What is the behavior if filename and secretJsonKey are still null at this point? Is it worth logging this case and/or throwing an exception for this case?

If both are still null, then they would first see a log message from the call to setUploadJsonKeyFile. With both null, the credentials created wouldn't be usable so I think at the very least adding a log message here makes sense. I've purposefully avoided using exceptions in the databound setters to avoid the demon jenkins page, but for this constructor it may make sense to throw an exception instead because it is not called directly in the UI.

jetersen · 2019-04-10T17:30:02Z

linking: jenkinsci/configuration-as-code-plugin#809

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

src/test/resources/com/google/jenkins/plugins/credentials/oauth/test-key.json

craigdbarber · 2019-04-15T20:48:19Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

-    File jsonKeyFile = KeyUtils.createKeyFile("key", ".json");
-    KeyUtils.writeKeyToFileEncoded(jsonKey.toPrettyString(), jsonKeyFile);
-    return jsonKeyFile.getAbsolutePath();
+  /** @param filename The json key file name. */


s/json/JSON here and throughout.

craigdbarber · 2019-04-15T20:49:37Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java

      } catch (IOException ignored) {
      }
    }
    return jsonKey;
  }

  /**
-   * descriptor for .json service account authentication
+   * Descriptor for .json service account authentication.


s/.json/JSON

craigdbarber · 2019-04-15T20:50:15Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/P12ServiceAccountConfig.java

- * provides authentication mechanism for a service account by setting a service
- * account email address and .p12 private key file
+ * Provides authentication mechanism for a service account by setting a service
+ * account email address and .p12 private key file.


Here and throughout.

craigdbarber

A couple more minor nits.
LGTM otherwise. Great work!

…r p12 keys.

…ted resources.

… this is based on master and not develop.

…C tests as optional so that they are not included when using hpi to package or run.

…witching to the version that uses spotbugs instead of findbugs is required to use CasC.

…zation and punctuation in javadoc.

* Use DataboundSetter throughout except for final field emailAddress for p12 keys. * Update dependencies required for using JCasC plugin in tests. * Remove unneeded imports. * Add test to verify Configuration as Code behavior, along with associated resources. * Temporarily use old format for license header for checkStyle, because this is based on master and not develop. * Preserve backwards compatibility of json keys. * Preverse compatibility of P12 keys. * Avoid using real keys in any form during JCasC tests. * Address checkstyle failures. * Fix indentation misalignment. Mark dependencies only required for CasC tests as optional so that they are not included when using hpi to package or run. * Remove findbugs dependency. Uses spotbugs from the parent pom since switching to the version that uses spotbugs instead of findbugs is required to use CasC. * Add eof new line to test-key.json * Add missing javadoc on JsonServiceAccountConfig. Use correct capitalization and punctuation in javadoc. * Add TODO tracking issue jenkinsci#50 for deduplication. * Fix secret bytes to account for newline in file. * Incorporate PR feedback. * Fix typos in yml comments.

* Use DataboundSetter throughout except for final field emailAddress for p12 keys. * Update dependencies required for using JCasC plugin in tests. * Remove unneeded imports. * Add test to verify Configuration as Code behavior, along with associated resources. * Temporarily use old format for license header for checkStyle, because this is based on master and not develop. * Preserve backwards compatibility of json keys. * Preverse compatibility of P12 keys. * Avoid using real keys in any form during JCasC tests. * Address checkstyle failures. * Fix indentation misalignment. Mark dependencies only required for CasC tests as optional so that they are not included when using hpi to package or run. * Remove findbugs dependency. Uses spotbugs from the parent pom since switching to the version that uses spotbugs instead of findbugs is required to use CasC. * Add eof new line to test-key.json * Add missing javadoc on JsonServiceAccountConfig. Use correct capitalization and punctuation in javadoc. * Add TODO tracking issue #50 for deduplication. * Fix secret bytes to account for newline in file. * Incorporate PR feedback. * Fix typos in yml comments.

jetersen · 2019-04-26T00:26:11Z

pom.xml

+    </dependency>
+    <dependency>
+      <groupId>io.jenkins.configuration-as-code</groupId>
+      <artifactId>configuration-as-code-support</artifactId>


Hopefully, you can get rid of this one soon:
jenkinsci/configuration-as-code-plugin#862
jenkinsci/configuration-as-code-plugin#863

oleg-nenashev · 2019-05-20T22:06:54Z

Created https://issues.jenkins-ci.org/browse/JENKINS-57570 so that it gets displayed properly on the dashboard

stephenashank requested review from craigdbarber and ikedam April 5, 2019 05:26

ikedam suggested changes Apr 5, 2019

View reviewed changes

stephenashank commented Apr 5, 2019

View reviewed changes

craigdbarber reviewed Apr 10, 2019

View reviewed changes

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java Outdated Show resolved Hide resolved

craigdbarber reviewed Apr 10, 2019

View reviewed changes

src/main/java/com/google/jenkins/plugins/credentials/oauth/JsonServiceAccountConfig.java Outdated Show resolved Hide resolved

craigdbarber reviewed Apr 10, 2019

View reviewed changes

src/test/resources/com/google/jenkins/plugins/credentials/oauth/test-key.json Outdated Show resolved Hide resolved

craigdbarber reviewed Apr 15, 2019

View reviewed changes

craigdbarber approved these changes Apr 15, 2019

View reviewed changes

Stephen Shank added 14 commits April 15, 2019 14:18

Use DataboundSetter throughout except for final field emailAddress fo…

3b54f03

…r p12 keys.

Update dependencies required for using JCasC plugin in tests.

17d8275

Remove unneeded imports.

6e4af90

Add test to verify Configuration as Code behavior, along with associa…

d7c5901

…ted resources.

Temporarily use old format for license header for checkStyle, because…

d20b6aa

… this is based on master and not develop.

Preserve backwards compatibility of json keys.

e5c75d5

Preverse compatibility of P12 keys.

5aed408

Avoid using real keys in any form during JCasC tests.

e803dbd

Address checkstyle failures.

0138cbb

Fix indentation misalignment. Mark dependencies only required for Cas…

da22605

…C tests as optional so that they are not included when using hpi to package or run.

Remove findbugs dependency. Uses spotbugs from the parent pom since s…

f8de1ae

…witching to the version that uses spotbugs instead of findbugs is required to use CasC.

Add eof new line to test-key.json

4993643

Add missing javadoc on JsonServiceAccountConfig. Use correct capitali…

c02b562

…zation and punctuation in javadoc.

Add TODO tracking issue jenkinsci#50 for deduplication.

6b24344

Stephen Shank added 3 commits April 15, 2019 14:18

Fix secret bytes to account for newline in file.

5972c00

Incorporate PR feedback.

dd9bc3c

Fix typos in yml comments.

db6e79f

stephenashank force-pushed the feature/jenkinscasc branch from 8af72c1 to db6e79f Compare April 15, 2019 21:19

stephenashank changed the title ~~DO NOT MERGE: Add support for JCasC using SecretBytes~~ Add support for JCasC using SecretBytes Apr 15, 2019

stephenashank merged commit d69e66f into jenkinsci:master Apr 15, 2019

jetersen reviewed Apr 26, 2019

View reviewed changes

oleg-nenashev changed the title ~~Add support for JCasC using SecretBytes~~ [JENKINS-57570] - Add support for JCasC using SecretBytes May 20, 2019

orrc mentioned this pull request May 17, 2020

[JENKINS-61809] Include JCasC instructions in README jenkinsci/google-play-android-publisher-plugin#29

Merged

[JENKINS-57570] - Add support for JCasC using SecretBytes #48

[JENKINS-57570] - Add support for JCasC using SecretBytes #48

Conversation

stephenashank commented Apr 5, 2019

ikedam 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

jetersen Apr 6, 2019 • edited Loading

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

stephenashank commented Apr 5, 2019

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

jetersen Apr 10, 2019 • edited Loading

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

stephenashank Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen commented Apr 10, 2019

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

craigdbarber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented May 20, 2019

jetersen Apr 6, 2019 •

edited

Loading

jetersen Apr 10, 2019 •

edited

Loading

stephenashank Apr 10, 2019 •

edited

Loading