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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-47736] Introduced ClassFilter.setDefault #208

Merged
merged 8 commits into from
Jan 9, 2018
Merged

[JENKINS-47736] Introduced ClassFilter.setDefault #208

merged 8 commits into from
Jan 9, 2018

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 30, 2017

JENKINS-47736

@reviewbybees

@jglick jglick mentioned this pull request Oct 30, 2017
Merged
@oleg-nenashev
Copy link
Member

@jglick It requires the JEP review, right? What is the current state of this process?

@jglick
Copy link
Member Author

jglick commented Nov 27, 2017

I am not really sure.

@jglick
Copy link
Member Author

jglick commented Nov 28, 2017

@oleg-nenashev FYI

…
00:11:50.494 [linux] Testing factory JNLP4-plaintext
00:37:30.425 [linux] Cancelling nested steps due to timeout
…

oleg-nenashev
oleg-nenashev previously requested changes Dec 19, 2017
View reviewed changes
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.

All the changes introduced here are reasonable even without JEP-200. The most interesting part of this JEP is changing the default behavior in the core. Blacklist filters just move the de-facto existing restrictions from the core.

Assuming the documentation gets polished a bit, I see no strict need to wait till JEP-200 is fully approved. So just a minor change requirement regarding the documentation.

src/main/java/hudson/remoting/ClassFilter.java Outdated
public static final String FILE_OVERRIDE_LOCATION_PROPERTY = "hudson.remoting.ClassFilter.DEFAULTS_OVERRIDE_LOCATION";

private static final Logger LOGGER = Logger.getLogger(ClassFilter.class.getName());

protected boolean isBlacklisted(String name) {
public boolean isBlacklisted(String name) {
Copy link
Member

Choose a reason for hiding this comment

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

Some Javadoc here would be helpful

src/main/java/hudson/remoting/ClassFilter.java Outdated
return false;
}

protected boolean isBlacklisted(Class c) {
public boolean isBlacklisted(Class c) {
Copy link
Member

Choose a reason for hiding this comment

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

same

src/main/java/hudson/remoting/ClassFilter.java
@@ -15,6 +15,7 @@
import java.util.regex.PatternSyntaxException;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

/**
* Restricts what classes can be received through remoting.
Copy link
Member

Choose a reason for hiding this comment

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

IIUC with JEP-200 the same ClassFilter will be also applied to XStream. I recommend documenting it here

Copy link
Member Author

Choose a reason for hiding this comment

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

It already was applied to XStream, but yes it should be documented anyway.

src/main/java/hudson/remoting/ClassFilter.java Outdated
/**
* Changes the effective value of {@link #DEFAULT}.
* @param filter a new default to set; may or may not delegate to {@link STANDARD}
*/
Copy link
Member

Choose a reason for hiding this comment

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

@since TODO

oleg-nenashev
oleg-nenashev approved these changes Dec 22, 2017
View reviewed changes
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.

🐝 . As discussed in JEP-200, I will probably merge it ahead Remoting 3.16 even if the JEP is not approved. This part just enhances API and does not break anything.

@jglick
Copy link
Member Author

jglick commented Dec 22, 2017

It mostly just adds clearer APIs while leaving the deprecated appendDefaultFilter with the same behavior. My concern would be a few additions to the default blacklist. SignedObject is already being added by core, but Method and json-lib would be behavioral changes if integrated. In particular this last change would cause a functional regression until jenkinsci/dockerhub-notification-plugin#16 is released. CC @rsandell who seems to be active in this plugin.

Since there is no advantage to releasing the new API ahead of the core PR which uses it, I would still suggest doing them together, but up to you ultimately. @reviewbybees done anyway

@oleg-nenashev
Copy link
Member

Agreed, the plugins need to be released ahead at least

@oleg-nenashev
Copy link
Member

I have merged the PR with the current master and deployed the new snapshot: remoting-3.16-20171228.162243-1

@rsandell
Copy link
Member

rsandell commented Jan 4, 2018

dockerhub-notification-2.2.1 released today.

@jglick
Copy link
Member Author

jglick commented Jan 8, 2018

Saw that, thanks @rsandell.

@oleg-nenashev
Copy link
Member

Windows build is unstable due to the INFRA issue AFAICT:

[INFO] --- maven-jar-plugin:3.0.2:jar (default-jar) @ remoting ---

[INFO] Downloading: https://repo.maven.apache.org/maven2/org/apache/maven/maven-archiver/3.1.1/maven-archiver-3.1.1.pom

Jan 09, 2018 9:40:02 AM org.apache.maven.wagon.providers.http.httpclient.impl.execchain.RetryExec execute

INFO: I/O exception (java.net.SocketException) caught when processing request to {s}->https://repo.maven.apache.org:443: Connection reset

Jan 09, 2018 9:40:02 AM org.apache.maven.wagon.providers.http.httpclient.impl.execchain.RetryExec execute

INFO: Retrying request to {s}->https://repo.maven.apache.org:443

Jan 09, 2018 9:40:21 AM org.apache.maven.wagon.providers.http.httpclient.impl.execchain.RetryExec execute

INFO: I/O exception (java.net.SocketException) caught when processing request to {s}->https://repo.maven.apache.org:443: Connection reset

Jan 09, 2018 9:40:21 AM org.apache.maven.wagon.providers.http.httpclient.impl.execchain.RetryExec execute

INFO: Retrying request to {s}->https://repo.maven.apache.org:443

Jan 09, 2018 9:40:40 AM org.apache.maven.wagon.providers.http.httpclient.impl.execchain.RetryExec execute

INFO: I/O exception (java.net.SocketException) caught when processing request to {s}->https://repo.maven.apache.org:443: Connection reset

Jan 09, 2018 9:40:40 AM org.apache.maven.wagon.providers.http.httpclient.impl.execchain.RetryExec execute

INFO: Retrying request to {s}->https://repo.maven.apache.org:443

Windows tests are green, so I do not care much: [windows] [WARNING] Tests run: 515, Failures: 0, Errors: 0, Skipped: 6

@oleg-nenashev
Copy link
Member

JEP-200 has been accepted. 🚢 🇮🇹

@oleg-nenashev oleg-nenashev merged commit 1fda115 into jenkinsci:master Jan 9, 2018
@jglick jglick deleted the whitelist-JENKINS-47736 branch January 9, 2018 19:48
oleg-nenashev added a commit to oleg-nenashev/remoting that referenced this pull request Jan 10, 2018
@oleg-nenashev
Changelog: Fix the reference to jenkinsci#208
aa91e33
oleg-nenashev added a commit that referenced this pull request Jan 10, 2018
@oleg-nenashev
Changelog and Javadoc update towards 2.16 (#246)
46e873d 
* Changelog: Noting 3.16

* Javadoc: Add @SInCE to the ClassFilter#setDefault() meethod

* Changelog: We already know the target Jenkins weekly release

* Changelog: Fix the reference to #208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jglick @oleg-nenashev @rsandell