[JENKINS-41730] - Add option to ignore the X-Jenkins-Agent-Protocols header #146

oleg-nenashev · 2017-02-05T15:38:48Z

In particular cases the X-Jenkins-Agent-Protocols response header may change (e.g. due to the improperly configured proxy). This change adds diagnostics for such case and also adds a system property, which allows working around the issue by sacrificing the performance.

Add the org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver.ignoreJenkinsAgentProtocolsHeader property, which disables the cache
Print warning in the case if the list of supported protocols is empty
Add documentation

https://issues.jenkins-ci.org/browse/JENKINS-41730

@reviewbybees, esp. @stephenc

…header + improve diagnostics - [x] Add the `org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver.ignoreJenkinsAgentProtocolsHeader` property, which disables the cache - [x] Print warning in the case if the list of supported protocols is empty - [x] Add documentation

stephenc · 2017-02-05T16:11:21Z

🐛 if the server says do not try JNLP2 then do not try JNLP2 as there could be a MiM. Better to have the system property take the list of protocols rather than being a boolean

That way for systems needing the assurance of JNLP4 they can keep that assurance

oleg-nenashev · 2017-02-06T11:41:16Z

@stephenc OK, will update. Though I feel it causes some fun with order of the protocols. It should be listed at least

stephenc · 2017-02-06T11:53:10Z

causes some fun with order of the protocols

Just say it is the set of protocols to try. the order of protocol attempts is hard-coded

@stephenc

…dpointResolver.protocolNamesToTry according to the feedback from @stephenc

daniel-beck · 2017-02-07T10:16:59Z

src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java

+
+                    if (agentProtocolNames.isEmpty()) {
+                        LOGGER.log(Level.WARNING, "Received the empty list of supported protocols from the server. " +
+                                "All protocols are disabled on the master side OR the 'X-Jenkins-Agent-Protocols' header is corrupted (JENKINS-41730). " +


So the problem is that the header is set, but empty? Who the hell does that!?

Shouldn't this also apply to the case that the header isn't set, or do we handle that better anyway?

daniel-beck

Previous comment still applies, but for a mostly diagnostic system property, should be fine.

daniel-beck · 2017-02-07T10:56:30Z

src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java

+                                "to define the supported protocols.");
+                    }
+
+                    LOGGER.log(Level.INFO, "Remoting server accepts the following protocols: {0}", agentProtocolNames);


Should this not be in an else block?

yes, I agree

Were you going to address this?

oleg-nenashev · 2017-02-07T11:29:33Z

@daniel-beck. "No header" is being already handled, because it is a valid case when new agent connects to the master with old core version. Regarding the header replacement, this is a HTTPS wrapper being used by one of CB customers. Cannot say more here, but you can find the support ticket. I have confirmed that my Nginx-powered HTTPS wrapper returns headers correctly. So it is likely a configuration issue. But workaround is reasonable in ang case IMHO (e.g. when a custom agent admin wants to enforce JNLP4)

…

On Feb 7, 2017 11:17 AM, "Daniel Beck" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/jenkinsci/remoting/engine/ JnlpAgentEndpointResolver.java <#146 (review)> : > @@ -173,7 +184,31 @@ public JnlpAgentEndpoint resolve() throws IOException { } } } + + if (agentProtocolNames.isEmpty()) { + LOGGER.log(Level.WARNING, "Received the empty list of supported protocols from the server. " + + "All protocols are disabled on the master side OR the 'X-Jenkins-Agent-Protocols' header is corrupted (JENKINS-41730). " + So the problem is that the header is set, but empty? Who the hell does that!? Shouldn't this also apply to the case that the header isn't set, or do we handle that better anyway? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#146 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC3IoNNlnri-XNeUuJPfo5RpSvKjh7L3ks5raESbgaJpZM4L3h2C> .

daniel-beck · 2017-02-07T11:31:32Z

Regarding the header replacement, this is a HTTPS wrapper

Right, just wondered where we draw the line between what's reasonable to support, and what's just too crazy…

oleg-nenashev · 2017-02-09T16:34:53Z

@stephenc ping, wdyt?

stephenc

LGTM but looks like you still have changes that DB requested

oleg-nenashev · 2017-02-09T20:29:57Z

Yes, but there was no sense to fix it before the major blocker got solved

oleg-nenashev · 2017-02-10T17:09:36Z

May be a candidate for backporting

… empty

oleg-nenashev · 2017-02-13T10:50:00Z

I doubt we really want to backport it.

oleg-nenashev · 2017-02-13T11:53:26Z

@reviewbybees done

https://github.com/jenkinsci/remoting/edit/master/CHANGELOG.md Fixed issues: * [JENKINS-40710](https://issues.jenkins-ci.org/browse/JENKINS-40710) - Match headers case-insensitively in `JnlpAgentEndpointResolver` in order to be compliant with HTTP2 lower-case headers. ([PR jenkinsci#139](jenkinsci/remoting#139), [PR jenkinsci#140](jenkinsci/remoting#140)) * [JENKINS-41513](https://issues.jenkins-ci.org/browse/JENKINS-41513) - Prevent `NullPointerException` in `JnlpAgentEndpointResolver` when receiving a header with `null` name. ([PR jenkinsci#140](jenkinsci/remoting#140)) * [JENKINS-41852](https://issues.jenkins-ci.org/browse/JENKINS-41852) - Fix exported object pinning logic to prevent release due to the integer overflow. ([PR jenkinsci#148](jenkinsci/remoting#148)) Improvements: * [JENKINS-41730](https://issues.jenkins-ci.org/browse/JENKINS-41730) - Add the new `org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver.ignoreJenkinsAgentProtocolsHeader` property, which allows specifying a custom list of supported protocols instead of the one returned by the Jenkins master. ([PR jenkinsci#146](jenkinsci/remoting#146)) * Print the Filesystem Jar Cache directory location in the error message when this cache directory is not writable. ([PR jenkinsci#143](jenkinsci/remoting#143)) * Replace `MimicException` with the older `ProxyException` when serializing non-serializable exceptions thrown by the remote code. ([PR jenkinsci#141](jenkinsci/remoting#141)) * Use OID of the `ClassLoaderProxy` in error message when the proxy cannot be located in the export table. ([PR jenkinsci#147](jenkinsci/remoting#147))

https://github.com/jenkinsci/remoting/edit/master/CHANGELOG.md Fixed issues: * [JENKINS-40710](https://issues.jenkins-ci.org/browse/JENKINS-40710) - Match headers case-insensitively in `JnlpAgentEndpointResolver` in order to be compliant with HTTP2 lower-case headers. ([PR #139](jenkinsci/remoting#139), [PR #140](jenkinsci/remoting#140)) * [JENKINS-41513](https://issues.jenkins-ci.org/browse/JENKINS-41513) - Prevent `NullPointerException` in `JnlpAgentEndpointResolver` when receiving a header with `null` name. ([PR #140](jenkinsci/remoting#140)) * [JENKINS-41852](https://issues.jenkins-ci.org/browse/JENKINS-41852) - Fix exported object pinning logic to prevent release due to the integer overflow. ([PR #148](jenkinsci/remoting#148)) Improvements: * [JENKINS-41730](https://issues.jenkins-ci.org/browse/JENKINS-41730) - Add the new `org.jenkinsci.remoting.engine.JnlpAgentEndpointResolver.ignoreJenkinsAgentProtocolsHeader` property, which allows specifying a custom list of supported protocols instead of the one returned by the Jenkins master. ([PR #146](jenkinsci/remoting#146)) * Print the Filesystem Jar Cache directory location in the error message when this cache directory is not writable. ([PR #143](jenkinsci/remoting#143)) * Replace `MimicException` with the older `ProxyException` when serializing non-serializable exceptions thrown by the remote code. ([PR #141](jenkinsci/remoting#141)) * Use OID of the `ClassLoaderProxy` in error message when the proxy cannot be located in the export table. ([PR #147](jenkinsci/remoting#147)) (cherry picked from commit 815da8a)

[JENKINS-41730] - Switch to org.jenkinsci.remoting.engine.JnlpAgentEn…

7d7237f

…dpointResolver.protocolNamesToTry according to the feedback from @stephenc

daniel-beck reviewed Feb 7, 2017

View reviewed changes

daniel-beck approved these changes Feb 7, 2017

View reviewed changes

stephenc reviewed Feb 9, 2017

View reviewed changes

oleg-nenashev added the backporting-candidate label Feb 10, 2017

oleg-nenashev added 2 commits February 13, 2017 13:38

[JENKINS-41730] - Pront the list of supported protocols iff it is not…

bc7a1a1

… empty

[JENKINS-41730] - Get rid of unnecessary line breaks

5177e40

oleg-nenashev removed the backporting-candidate label Feb 13, 2017

stephenc approved these changes Feb 13, 2017

View reviewed changes

daniel-beck approved these changes Feb 13, 2017

View reviewed changes

oleg-nenashev merged commit 612cc27 into jenkinsci:master Feb 13, 2017

oleg-nenashev added a commit that referenced this pull request Feb 16, 2017

Changelog: Noting #146

6806524

oleg-nenashev mentioned this pull request Feb 17, 2017

[JENKINS-40710, JENKINS-41513, JENKINS-41852, JENKINS-41730] - Update remoting to 3.5 jenkinsci/jenkins#2751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-41730] - Add option to ignore the X-Jenkins-Agent-Protocols header #146

[JENKINS-41730] - Add option to ignore the X-Jenkins-Agent-Protocols header #146

oleg-nenashev commented Feb 5, 2017

stephenc commented Feb 5, 2017

oleg-nenashev commented Feb 6, 2017

stephenc commented Feb 6, 2017

daniel-beck Feb 7, 2017

daniel-beck left a comment

daniel-beck Feb 7, 2017

oleg-nenashev Feb 8, 2017

stephenc Feb 9, 2017

oleg-nenashev commented Feb 7, 2017 via email

daniel-beck commented Feb 7, 2017

oleg-nenashev commented Feb 9, 2017

stephenc left a comment

oleg-nenashev commented Feb 9, 2017

oleg-nenashev commented Feb 10, 2017

oleg-nenashev commented Feb 13, 2017

oleg-nenashev commented Feb 13, 2017

[JENKINS-41730] - Add option to ignore the X-Jenkins-Agent-Protocols header #146

[JENKINS-41730] - Add option to ignore the X-Jenkins-Agent-Protocols header #146

Conversation

oleg-nenashev commented Feb 5, 2017

stephenc commented Feb 5, 2017

oleg-nenashev commented Feb 6, 2017

stephenc commented Feb 6, 2017

daniel-beck Feb 7, 2017

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

daniel-beck Feb 7, 2017

Choose a reason for hiding this comment

oleg-nenashev Feb 8, 2017

Choose a reason for hiding this comment

stephenc Feb 9, 2017

Choose a reason for hiding this comment

oleg-nenashev commented Feb 7, 2017 via email

daniel-beck commented Feb 7, 2017

oleg-nenashev commented Feb 9, 2017

stephenc left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Feb 9, 2017

oleg-nenashev commented Feb 10, 2017

oleg-nenashev commented Feb 13, 2017

oleg-nenashev commented Feb 13, 2017