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

Removing configurability of Jenkins.agentProtocols #9903

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 22, 2024

I ran into a case where a CloudBees CI customer was missing a proprietary agent protocol (see #8621). It seems that this was quite inadvertent: they had in the past exported a JCasC bundle that included something like

jenkins:
  agentProtocols:
  - Ping
  - JNLP4-connect

despite never having touched this configuration section, and without paying attention to this line had committed it to SCM, where it prevented new protocols from appearing! (You can find lots of similar examples, some outdated, by searching.)

The history here is long and tortured (#2010, #2950, #3188, #4387, #7451, etc.) but essentially there was a period of Jenkins development in which new TCP (agent) protocols were being actively developed, and a rather complex opt-in-and-out system was devised to allow administrators to pick which protocol(s) to allow and have that choice be partially preserved across upgrades. But for years now, there has only been JNLP4-connect, and there is no meaningful choice: if you do not wish to allow inbound TCP agents, just disable the port.

The entire protocol selection system seems like technical debt now. This PR just deletes it all and treats AgentProtocol like any other headless extension: whichever protocols are registered by core or plugins are allowed. If a plugin developer wishes to conditionally roll out a protocol behind a feature flag, they can conditionally return null from getName. If an administrator has some weird reason to disable one protocol (but allow others) they could filter it out, but this is not something we would advertise as a supported controller customization.

Testing done

The protocols section in Configure Security is gone.

Proposed changelog entries

  • Stop allowing configuration of the list of agent protocols.

Proposed upgrade guidelines

The list of agent protocols is no longer configurable, either through the GUI or JCasC. (Normally there will be two protocols, one for inbound TCP agents, and a ping protocol used to test connectivity.) The agentProtocols section of a JCasC bundle will now be ignored and should be deleted. If you do not wish to allow inbound TCP agents, disable the port rather than the protocol.

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@jglick jglick requested a review from Vlatombe October 22, 2024 17:34
Comment on lines -23 to -25
JnlpSlaveAgentProtocol.displayName=Inbound TCP Agent Protocol/1 (deprecated, unencrypted)
JnlpSlaveAgentProtocol2.displayName=Inbound TCP Agent Protocol/2 (deprecated, unencrypted)
JnlpSlaveAgentProtocol3.displayName=Inbound TCP Agent Protocol/3 (deprecated, basic encryption)
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently forgotten in some earlier cleanup.

JnlpSlaveAgentProtocol2.displayName=Inbound TCP Agent Protocol/2 (deprecated, unencrypted)
JnlpSlaveAgentProtocol3.displayName=Inbound TCP Agent Protocol/3 (deprecated, basic encryption)
JnlpSlaveAgentProtocol4.displayName=Inbound TCP Agent Protocol/4 (TLS encryption)
DeprecatedAgentProtocolMonitor.displayName=Deprecated Agent Protocol Monitor
Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto.

@timja timja added 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 labels Oct 22, 2024
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

LGTM

@Vlatombe
Copy link
Member

/label ready-for-merge


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

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 23, 2024
@timja timja merged commit 7a16302 into jenkinsci:master Oct 24, 2024
15 checks passed
@jglick jglick deleted the agentProtocols branch October 24, 2024 17:43
MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Oct 27, 2024
There may be automated tests in the plugin BOM that depend on the
agent protocols configuration.  Run the tests to confirm that they are
unaffected by:

* jenkinsci/jenkins#9903
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Oct 27, 2024

This causes test failures in the plugin BOM. The configuration as code plugin shows several test failures with this change. Other plugins may also show failures. I've started a plugin BOM run to check other plugins:

Pull requests that need to be included in the plugin BOM run are:

@jglick
Copy link
Member Author

jglick commented Oct 28, 2024

Sorry, I forgot to mark those two ready for review after this merged. Did that now. CC @timja & @Dohbedoh as likely maintainers.

@MarkEWaite
Copy link
Contributor

Thanks @jglick ! The incremental builds of those plugins passed the plugin BOM tests in

I would love to have a release of those two plugins at about the same time as 2.483 releases tomorrow so that the plugin BOM can include those new releases.

@jglick
Copy link
Member Author

jglick commented Oct 28, 2024

Well jenkinsci/configuration-as-code-plugin#2580 is outside my control. jenkinsci/support-core-plugin#592 I could merge myself but would prefer to wait for a review from somebody.

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.

4 participants