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

[JEP-230] [JENKINS-55582] Convert instance-identity to a detached plugin #6585

Merged
merged 20 commits into from
Jun 22, 2022

Conversation

jglick
Copy link
Member

@jglick jglick commented May 19, 2022

See JEP-230, especially as of jenkinsci/jep#389. Continues work done in #5304 and #6570.

Sanity testing: start Jenkins from scratch; accept default plugins; check that /instance-identity/ endpoint works; create an inbound TCP agent (need to first enable TCP port) and connect; run a Pipeline build on it. Disable instance-identity despite stern warnings and restart—the agent will not work, and there are also various errors from sshd and github plugins. Enable it again and restart—back to normal. Enable SSHD and use the CLI in -ssh mode and via ssh.

Proposed changelog entries

  • The instance-identity module has been converted to a detached plugin.

Proposed upgrade guidelines

Normally the new plugin will be automatically installed. If you use an “as-code” installation mechanism like a text file with a list of plugins, you may need to add instance-identity (version 3.1 or later) in order to restore this functionality. In particular, inbound agents using TCP (but not WebSocket) transport require this plugin to be installed. There may be other cases, which can be identified by use of APIs in the jenkins.model.identity package rather than directly accessing org.jenkinsci.main.modules.instance_identity.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

jglick added a commit to jenkinsci/jep that referenced this pull request May 19, 2022
war/pom.xml Outdated Show resolved Hide resolved
@jglick jglick requested a review from jtnord May 19, 2022 18:03
@@ -256,11 +256,6 @@ THE SOFTWARE.
<artifactId>remoting</artifactId>
<version>${remoting.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.modules</groupId>
<artifactId>instance-identity</artifactId>
Copy link
Member Author

Choose a reason for hiding this comment

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

Once this is released, bom-weekly can get it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though if I understand correctly from jenkinsci/bom#671 jenkinsci/bom#570 (comment) jenkinsci/bom#681, it cannot be added until this PR is in the oldest supported line, so that will be a while. In the meantime the handful of plugins using this dep can specify 3.1.

@@ -423,7 +419,7 @@ THE SOFTWARE.
<artifactItem>
<groupId>org.jenkins-ci.modules</groupId>
<artifactId>sshd</artifactId>
<version>3.0.3</version>
<version>3.236.ved5e1b_cb_50b_2</version>
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -399,7 +395,7 @@ THE SOFTWARE.
<artifactItem>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>bouncycastle-api</artifactId>
<version>2.25</version>
<version>2.26</version>
Copy link
Member Author

Choose a reason for hiding this comment

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

dep of current instance-identity

timja pushed a commit to jenkinsci/jep that referenced this pull request May 19, 2022
* JEP-230 edits

* Linking to jenkinsci/jenkins#6585

* Treating JEP-230 as accepted
Other than `instance-identity` itself, known to be used from:
* `support-core`
* `pipeline-restful-api`
* `operations-center-server` (CloudBees CI)
* `devoptics` (CloudBees, but not CI)
@daniel-beck
Copy link
Member

// Send a request to /instance-identity/ at the resource root URL and check whether it is this Jenkins
try {
URLConnection urlConnection = new URL(resourceRootUrlString + "instance-identity/").openConnection();
if (urlConnection instanceof HttpURLConnection) {
HttpURLConnection httpURLConnection = (HttpURLConnection) urlConnection;
int responseCode = httpURLConnection.getResponseCode();
if (responseCode == 200) {
String identityHeader = urlConnection.getHeaderField("X-Instance-Identity");
if (identityHeader == null) {
return FormValidation.warning(Messages.ResourceDomainConfiguration_NotJenkins());
}
// URL points to a Jenkins instance
RSAPublicKey publicKey = InstanceIdentityProvider.RSA.getPublicKey();
if (publicKey != null) {
String identity = Base64.getEncoder().encodeToString(publicKey.getEncoded());
if (identity.equals(identityHeader)) {
return FormValidation.ok(Messages.ResourceDomainConfiguration_ThisJenkins());
}
return FormValidation.warning(Messages.ResourceDomainConfiguration_OtherJenkins());
} // the current instance has no public key
return FormValidation.warning(Messages.ResourceDomainConfiguration_SomeJenkins());
}
// response is error
String responseMessage = httpURLConnection.getResponseMessage();
if (responseCode == 404) {
String responseBody = String.join("", IOUtils.readLines(httpURLConnection.getErrorStream(), StandardCharsets.UTF_8));
if (responseMessage.contains(ERROR_RESPONSE) || responseBody.contains(ERROR_RESPONSE)) {
return FormValidation.ok(Messages.ResourceDomainConfiguration_ResourceResponse());
}
}
return FormValidation.error(Messages.ResourceDomainConfiguration_FailedIdentityCheck(responseCode, responseMessage));
}
return FormValidation.error(Messages.ResourceDomainConfiguration_Invalid()); // unlikely to ever be hit
} catch (MalformedURLException ex) {
// Not expected to be hit
LOGGER.log(Level.FINE, "MalformedURLException occurred during instance identity check for " + resourceRootUrlString, ex);
return FormValidation.error(Messages.ResourceDomainConfiguration_Exception(ex.getMessage()));
} catch (IOException ex) {
LOGGER.log(Level.FINE, "IOException occurred during instance identity check for " + resourceRootUrlString, ex);
return FormValidation.warning(Messages.ResourceDomainConfiguration_IOException(ex.getMessage()));
}
}
will need an alternative approach for detecting whether "there" is "here".

@jglick
Copy link
Member Author

jglick commented May 19, 2022

will need an alternative approach for detecting whether "there" is "here"

Not going to bother with that. If you avoid installing instance-identity (which is very unlikely in GUI mode) then you would just not get this bit of form validation.


javax-activation-api 2.330 1.2.0-2
javax-mail-api 2.330 1.6.2-5

# JENKINS-55582
instance-identity 2.348 3.1
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: split point may need to be updated just before merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

(have been trying to keep this up to date when merging in master, but whoever finally merges this PR should double-check)

@jglick jglick marked this pull request as ready for review June 2, 2022 19:18
@jglick jglick requested a review from a team June 2, 2022 19:18
@oleg-nenashev oleg-nenashev added upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Jun 11, 2022
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

+1 for pushing it forward

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 11, 2022
@timja
Copy link
Member

timja commented Jun 11, 2022

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@jglick
Copy link
Member Author

jglick commented Jun 13, 2022

Whoever is merging, please bear in mind #6585 (comment)

@timja timja merged commit bcc2efb into jenkinsci:master Jun 22, 2022
@jglick jglick deleted the instance-identity-JEP-230 branch June 22, 2022 23:16
basil added a commit to basil/jenkins that referenced this pull request Jun 24, 2022
…lugin (jenkinsci#6585)

Co-authored-by: Basil Crow <me@basilcrow.com>
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
@timja
Copy link
Member

timja commented Jun 24, 2022

Caused ATH failure: jenkinsci/acceptance-test-harness#796

@basil
Copy link
Member

basil commented Dec 5, 2022

Likely cause of JENKINS-70206.

jtnord added a commit that referenced this pull request Jun 5, 2024
Whilst the test was correct that RJR would load detached plugins it was
actually relying on it to do so to load instance-idenentity so that JNLP
(sic) agents would work.

if RJR is updated then this assumption no longer holds and the test
would time out waiting for the JNLP agent to connect.

instance-identity is already in the test classpath - and we are not
testing that this functionality works with no plugins or a limited set,
so no longer remove any test scoped plugin from the test.

ammends #6585
@daniel-beck
Copy link
Member

Caused (sort of)/revealed JENKINS-75145.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants