[JENKINS-38370] Start defining APIs that are for the master JVM only #2556
Conversation
stephenc
added
the
needs-more-reviews
|
@jenkinsci/code-reviewers @reviewbybees |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.TYPE) | ||
| @Documented | ||
| public @interface MasterJVMOnly { |
There was a problem hiding this comment.
Maybe just "Master only"? Otherwise one may think it's OK to use classes within Master Node remote invocation calls. It may be safe, but I doubt it's what we want to advertise.
There was a problem hiding this comment.
🐛 It's also wrong location since plugin devs won't be unable do declare the annotation without updating the core.
There was a problem hiding this comment.
If that part of core functionality - then right.
|
|
||
| /** | ||
| * Annotation to mark classes which should not be made available to agent JVMs | ||
| */ |
| @@ -29,6 +29,7 @@ | |||
| * @since 1.494 | |||
| */ | |||
| @Extension @Symbol("location") | |||
There was a problem hiding this comment.
I'd suggest that all Extensions should be implicitly annotated in such way with an opt-out option
There was a problem hiding this comment.
👍 for the approach, but IMHO it should be in https://github.com/kohsuke/access-modifier in order to allow declaring annotation in plugins without Jenkins core dependency update. Hence 🐛 for the PR in the current state
Remoting is de-facto a part of Jenkins core for plugins right now. That's why I propose https://github.com/kohsuke/access-modifier, which can be easily bumped in plugin parent POM |
|
Or maybe new lib-remoting-annotations, but it may be overkill even though it's more reasonable if we consider remoting as a standalone project (but we actually don't) |
|
Access-modifiers is for annotations that are checked at compile time. This is not a check we can do at compile time (unless we mandated plugin authors two split their code int a remote-safe jar dependency and the master-side jar... which would force a similar split on Jenkins core)... so that seems like a very poor fit to me Remoting does not have the concept of a master... and there are things other than Jenkins that use remoting (few and far between, but still) as such, baking the concept of Master and Agent into the remoting API seems wrong (e.g. close source we have the master-to-master remoting channel in which case baking the restriction into remoting seems wrong. That leaves the option of a new library Jar... for what is basically just one annotation... certainly seems like overkill... it will likely be at least 18-24 months before we can even consider turning enforcement on by default (on say 10% of masters)... as such I consider core the perfect fit. |
|
@stephenc I don't ask to introduce support for master/agent into remoting, but to define there annotation to mark some class as "Never send on channel". one way or the other doesn't matter |
|
How about |
|
@olivergondza +1 I didn't know this one, seems like it has been designed for this exact usage |
|
+1 for |
It does, it just does not enforce restrictions itself. See |
|
|
AFAICT it is currently applied to all channels unless you opt out, which core does not seem to do. |
Right, we need to either make public static ClassFilter compose(final ClassFilter... delegates) {
return new ClassFilter() {
@Override
protected boolean isBlacklisted(String name) {
for (ClassFilter delegate : delegates) {
if (delegate.isBlacklisted(name)) {
return true;
}
}
return false;
}
@Override
protected boolean isBlacklisted(Class c) {
for (ClassFilter delegate : delegates) {
if (delegate.isBlacklisted(c)) {
return true;
}
}
return false;
}
};
} |
| // | ||
| // @Override | ||
| // protected boolean isBlacklisted(String name) { | ||
| // return blacklist.contains(name) || delegate.isBlacklisted(name); |
There was a problem hiding this comment.
Actually you might be able to work around this without a remoting bump:
if (blacklist.contains(name)) {
return true;
}
delegate.check(name);
return false;
|
OnMaster is an interface whereas we really want an annotation as you cannot remove an interface from a class signature without breaking binary compatibility |
|
As a marker interface removing would have no realistic impact on binary compatibility. |
|
If everyone is happy with the market interface, fine by me... but the annotation seems a better fit to my mind |
|
I'm not sure |
|
On some level it sounds like anti- |
|
@ydubreuil there is no enforcement yet... only market interface where IMHO an annotation may be a better alignment (though the interface is available now... so we have the choice: available now but imperfect vs available in the future but perhaps a better fit) |
|
@KostyaSha not quite. This is not about banning instances from being serialised to an agent rather about banning the class from even being loaded on the remote agent. I am less convinced that a findbugs rule would catch such issues... for sure we can spot "quick wins" in classes that extend the In part, this is why I suspect it may be some effort to turn on enforcement, but an important effort none the less |
|
@ndeloof iirc I had linkage errors when a marker interface was removed... i'll try some tests again to see if that was something else... (An interface may actually be a good fit as a child class should never be remote safe if the parent is not remote safe, so the only question is what breaks if we remove a marker interface) |
|
The only situation I can imagine removing the marker interface can cause problems (as there are no methods or fields to depend on) is when someone refers to an object using the marker interface and after the update it will no longer implement that interface. This is something we should document and, if we feel strongly, we can even enforce it (plugins must not use the type outside of |
|
Won't enforcing this result in similar issues as the 2.4 null instance problem? |
|
@daniel-beck well yeah, if we turned it on now... what I want to do is turn on enforcement for After 12-24 months we turn it on for say 10% of installations... (have usage stats report installations state of the AB test) if that stays at 10% then we can ramp it up to 50% and switch over to 100% enforcement... when the usage stats report a good % actually using the enforcement we can decide it safe to remove the disable enforcement option. The current question is really:
|
Maybe enable on dev versions and disable for LTS? |
Yeah I was thinking the same thing. |
Well, IMHO it requires some additional clarification on the Website, etc. Even after the migration to the "Weekly" term we do not explicitly say that its users act as testers. But generally I support the idea. |
stephenc
force-pushed
the
master-jvm-only
branch
from
1823cdf to
1640b90
Compare
|
Ok, so I have reworked this PR to now use the The only bit I feel may be "controversial" is making One option we could do is introduce an |
If you want introduce something, why not annotation then? |
|
@KostyaSha said:
I am not sure we need to introduce something. My belief is that we should just ban the loading of |
| @@ -323,7 +324,7 @@ | |||
| @ExportedBean | |||
| public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLevelItemGroup, StaplerProxy, StaplerFallback, | |||
| ModifiableViewGroup, AccessControlled, DescriptorByNameOwner, | |||
| ModelObjectWithContextMenu, ModelObjectWithChildren { | |||
| ModelObjectWithContextMenu, ModelObjectWithChildren, OnMaster { | |||
There was a problem hiding this comment.
Node implements it already.
There was a problem hiding this comment.
Well, it never hurts to make explicit for a better Javadoc visiblity
| @@ -16,4 +16,36 @@ | |||
| * @since 1.475 | |||
| */ | |||
| public interface OnMaster { | |||
| // TODO uncomment once we can have a delegating ClassFilter, also add SystemProperty to toggle feature | |||
There was a problem hiding this comment.
As noted in an earlier revision:
if (blacklist.contains(name)) {
return true;
}
delegate.check(name);
return false;There was a problem hiding this comment.
Would be great to have references to the issue in this TODO
|
I notice that But in fact including it would break new code, never mind compatibility. Consider this snippet in jenkinsci/workflow-support-plugin#15, which is designed to run on an agent. It does not transfer a So perhaps we need to scale back the ambition here, from banning classes which we think have no legitimate use on an agent (but might, if the code is written carefully), to classes which we know will actively cause harm—such as types in the Servlet API, the original motivation for JENKINS-26677. |
There was a problem hiding this comment.
🐝 for the cuent scope, but I agree with @jglick that other classes could be annotated as well.
Regarding extension points, I'm not 100% sure it's going to fly in general. I'd bug since ProcessKiller is being executed on slaves (https://github.com/jenkinsci/cygwin-process-killer-plugin/blob/master/src/main/java/com/synopsys/arc/jenkinsci/plugins/cygwinprocesskiller/CygwinProcessKiller.java).
| @@ -323,7 +324,7 @@ | |||
| @ExportedBean | |||
| public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLevelItemGroup, StaplerProxy, StaplerFallback, | |||
| ModifiableViewGroup, AccessControlled, DescriptorByNameOwner, | |||
| ModelObjectWithContextMenu, ModelObjectWithChildren { | |||
| ModelObjectWithContextMenu, ModelObjectWithChildren, OnMaster { | |||
There was a problem hiding this comment.
Well, it never hurts to make explicit for a better Javadoc visiblity
|
UPD: *regarding banning ALL extension points |
|
I think that an annotation would be better than an interface since iirc an annotation is more clearly marked in the javadoc than interfaces are, especially when something is implementing a lot of them or it is far down in the inheritance tree. |
|
@reviewbybees done |
|
@rsandell Well, the interface already exists. Seems wasteful to deprecate it and replace with an annotation that does the same thing. |
oleg-nenashev
added
ready-for-merge
|
Marked as ready-for-merge since there is no negative feedback. |
|
👍 |
|
@oleg-nenashev I believe you may have merged this too soon.
The real concern is with |
|
@stephenc Well, I just lost the context about this ExtensionPoint conversation. The PR has not been labeled properly, hence I've merged it after addressing the process requirements. But labeling as WiP is a responsibility of the PR requester, so I'll need to send you the Sad Panda's head in order to share it with you :( Anyway, I agree that we need to revert a particular annotation for Extension points ASAP. |
|
@oleg-nenashev well you removed the labels in the first place
So that sad panda is all yours |
|
@oleg-nenashev in future, if the author adds a "needs-review" label, perhaps check with the author before removing it |
|
It was not WiP because we still had not established whether to change it or whether it was fully baked... the outcome of the review would have been a decision on whether it is WIP or ready for merge |
|
well. For such cases we can use the "on-hold" label now. "needs-review" and all other such guys are not documented, hence I agree that may be misreadings. I'll keep the Sad Panda's head for myself since unlikely Irish customs accepts such parcels. |
See JENKINS-38370
This PR only introduces the annotation. Commented code in the annotation shows one potential path to implementing the enforcement, but that path is not currently available due to the current API contract of
ClassFilter(additionally I need to check ifClassFilterapplies only to classes coming from the agent and not to classes received by the agent)I have identified a couple of classes that are clearly
MasterJVMOnlythere are likely more such classes in core... plugins will also want to use this annotation, e.g.CredentialsProvideris intended to beMasterJVMOnly