-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
[JENKINS-48480] Switch deprecated protocols to opt-in. #3188
Conversation
They no longer need to be removed by setup wizard since they won't be enabled in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the effect on upgrades would be from this change.
} | ||
|
||
@Override | ||
public String getName() { | ||
// we only want to force the protocol off for users that have explicitly banned it via system property | ||
// everyone on the A/B test will just have the opt-in flag toggled | ||
// TODO strip all this out and hardcode OptIn==TRUE once JENKINS-36871 is merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2492 FTR
@@ -61,6 +67,14 @@ | |||
|
|||
private ExecutorService pool; | |||
|
|||
@Before | |||
public void setUp() { | |||
jenkins.CLI.get().setEnabled(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary; it is still enabled by default in core, disabled in wizard.
It is already enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will break the upgrading instances with old Remoting versions
@oleg-nenashev Ah, I see what you mean. So to fix this means any protocol that was previously opt-out and switched to opt-in, should actually be enabled on upgrade, unless it was explicitly disabled before. |
@Vlatombe even this is not going to help in general. E.g. configuration-as-code instances with disabled setup wizard will start behaving differently. So such change should be done with a preliminary announcement (maybe as a part of Jenkins 3) |
We have a changelog, should be good enough for something minor like this. |
Which kinds of upgraded instances would be affected? IIUC this would mean that some users of rather old Jenkins versions, predating the config UI that lets you explicitly select protocol versions, upon updating directly to this change would have the old and insecure agent protocols disabled after the upgrade, so if they also had obsolete |
That is also my understanding.
I agree. This is fairly minor, easy to recover from, and the changelog/upgrade guide can cover this. |
I propose to do it after https://issues.jenkins-ci.org/browse/JENKINS-48766 is implemented and released. It would add extra logging and UI info so the update will be more smooth to users |
@oleg-nenashev It is unclear to me how these are related. What's the ETA for that issue? |
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. |
@daniel-beck ETA depends on my employer, cannot commit on it. Regarding the relation between issues, I want Remoting older than 3.0to be deprecated in LTS before we rollout this breaking change. |
Remoting 3.0 went into 2.27, 1.5 years ago. How much longer do you want 2.x block obvious improvements to Jenkins? We should just add changelog notes and stop worrying about every tiny thing. People will read the changelog once their Jenkins breaks, and it'll tell them to update their agents. Problem solved. |
@daniel-beck I am working a lot on support fronts in my company, and I see Remoting 2.x in production almost every second day. So it's not a tiny thing. As a Remoting maintainer, I want to handle it properly. Once I get some promised Remoting maintenance time, I will do that. The question is only whether it lands in the next LTS or in the next next. |
In such case we will be able to merge Vincent's PR in ~2 weeks even if I do not deliver on the Remoting Minimal versions. Does it sound like a deal? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR I have delivered on the minimal version check API in https://issues.jenkins-ci.org/browse/JENKINS-48766 (Jenkins 2.104+). There is a follow-up for the agent side (JENKINS-50095), but IMHO it is not a blocker.
@Vlatombe please bump the remoting.minimum.supported.version
in the root pom.xml to 3.4+ or so. It should be enough to unlobk this story taking the proper announcement.
@oleg-nenashev I added your required change. |
It will be still a major breaking change for Jenkins users, who run old protocol versions. So I anticipate a bunch of tickets in JIRA even if there is a warning notice in changelog/upgrade guidelines. My comments are addressed, and I do not block the merge. If @daniel-beck @dwnusbaum @jeffret-b and other @jenkinsci/code-reviewers are fine with that, please feel free to go forward. |
Let me see if I understand. This change will break the distributed system configuration for some number of Jenkins users. If they know how to fix it, they can do so in a fairly simple fashion. There is an argument that they shouldn't be using these insecure protocols, but their security concerns may be minimal in their environment. There is another argument that these protocols have been deprecated for a long time and we should no longer be concerned with assisting those who won't upgrade. My biggest concern with the proposed change is that the impact of this change isn't sufficiently communicated. Isn't the proposed changelog entry kind of a side-effect of the bigger change? It doesn't provide users who might be impacted with sufficient warning or troubleshooting information. A smaller concern is that affected users would have to make the same corrections every time they upgrade. A one-time correction on upgrade is one thing, but having to do it each time is a greater burden and dissuades people from upgrading. However, given the long deprecation of these protocols that may not be a serious concern. Not long ago there was some heated discussion on one of the Jenkins email lists about how frequently Jenkins upgrades break things. Without better communication, I think this change may hit that stereotype. Or is there better communication on this somewhere that I'm missing? |
SSH Slaves always have latest remoting. Windows agents set up in the last X months/years should have auto-update enabled (I don't know whether it's retroactive, but doubt it). What's else? Manual slave.jar downloads? I still think banner for the weeklies and upgrade guide for LTS should do this. It's less of a breaking change (like JEP-200 or some of our worst security fixes were), just a nontrivial upgrade for some. Also, I bet this 'forced upgrade' will reduce the number of people complaining about remoting being terrible, just because they're on ancient releases lacking tons of robustness and diagnosability improvements 😸 |
I don't think this is a concern. This would only affect people who upgrade from a version prior introduction to Agent protocol selection (<2.16) or who never saved Jenkins global configuration since it got introduced. For those people, re-enabling the deprecated protocols (if they really want it) would be a one-time thing and they would never be bothered again. |
NOTE: I do not commit to provide any kind of issue triage in JIRA if it gets merged in such way. All reports about not working agents will have to be triaged by somebody else. Does anybody commit to do so? |
It sounds like I misunderstood regarding a need to repeat the manual re-configuration step so I withdraw that concern. My remaining concern is that we communicate about this change. Specifically, I think more is occurring with this change than is indicated in the proposed changelog entry. |
We can always send a heads-up blogpost before the release to increase the visibility a bit |
As long as we add a changelog entry, section to the upgrade guide, and/or blog post noting something along the lines of the following then I am fine with the change:
|
I agree with Devin. At a minimum it needs to go into the changelog. It would be better if it could go into one or two of those other places. |
Looks reasonable. The minimized configuration approach for opt in/opt out is a bit annoying here, as everyone will just get these disabled, but it is what we have. It's little different from semi-notable changes we've done in the past. This isn't "Java 8 or Jenkins doesn't start" levels of serious, but rather similar to "Build no longer implies Cancel" or like a number of "major rfe" changes (big blue circles in the changelog). So appropriate regular changelog and LTS upgrade guide entries should do it. |
A question on process with this PR: Does the proposed changelog entry at the top need to be changed to something more like what Devin suggested? Or, is that something that will be changed / added when the changelog gets assembled? (Or forgotten?) |
Updated the changelog entry. FTR, the agent protocol selection has been added in 2.16 (https://issues.jenkins-ci.org/browse/JENKINS-37032). |
I am fine with shipping in the next weekly |
Taking the feedback above, I am going to merge the pull request so that we get more soak testing in the weekly release before it goes to LTS. |
They no longer need to be removed by setup wizard since they won't be
enabled in the first place.
See JENKINS-48480.
Proposed changelog entries
Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@reviewbybees