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

Breaking Change in CKAN v2.10.3: Missing beaker.session.validate_key Parameter Causes Errors #7999

Closed
khashashin opened this issue Jan 3, 2024 · 5 comments

Comments

@khashashin
Copy link

CKAN version

2.10.3

Describe the bug

We have recently encountered a breaking change in CKAN version 2.10.3 related to the beaker.session.validate_key configuration parameter. In our setup, CKAN runs under an NGINX Proxy server, and we use Docker for deployment.

Previous Behavior (v2.10.1):
In CKAN version 2.10.1, which we were using, the command for generating the ckan.ini configuration file did not include the beaker.session.validate_key parameter. This led us to believe that the parameter was not mandatory or automatically handled by CKAN.

Current Issue (v2.10.3):
Upon updating to version 2.10.3, we faced accessibility issues with CKAN. It could not accept requests due to an error stemming from the absence of the beaker.session.validate_key parameter in the configuration. This parameter is now enforced as mandatory in config_declaration.yaml as per commit 06d72d2d8cfb61e4773097cd80f768a323b1ffef.

Impact:
This change has caused significant disruption as our application, which was expected to run smoothly post-upgrade, failed to do so. It took us a considerable amount of time to identify the root cause of the issue.

Proposed Solution:

  1. Update Documentation: Clearly document this change in the migration guide for version 2.10.3 to alert users about the need to include beaker.session.validate_key in their ckan.ini file.
  2. Backward Compatibility: Consider implementing a fallback mechanism in CKAN for this parameter, where it generates a default value if not explicitly provided in the configuration. This would ensure smoother transitions for existing deployments.
  3. Improved Communication: It would be beneficial for the CKAN community to proactively communicate such breaking changes in future releases.
@khashashin
Copy link
Author

Additional Comment: Acknowledgement of Changelog and Further Concerns

Acknowledgement:
Upon further review of the CKAN Changelog, I noticed that the requirement for beaker.session.validate_key in version 2.10.3 is indeed mentioned. I appreciate the documentation of this change and apologize if my initial issue overlooked this detail. However, I would still like to address a significant concern related to this change.

Concern Regarding Package Versioning and Breaking Changes:
Despite the documentation, there remains a practical issue with how CKAN's versioning and package registry is managed. Our Docker setup was locked to python-ckan_2.10-jammy_amd64.deb, with the understanding that this would safeguard us against unexpected breaking changes until a major version release (version 3+). However, the registry does not allow locking to patch versions, only to minor versions. This means that when the 2.10.3 update was released, it automatically became part of our deployment without explicit consent, bringing along the breaking change.

Implications:

  1. Automated Builds and Testing Environments: In environments where Docker builds are automated, or in testing environments, such introductions of breaking changes can lead to unexpected failures and significant troubleshooting overhead.
  2. Version Locking Limitations: The inability to lock to patch versions in the package registry limits our control over the deployment environment, increasing the risk of unintended disruptions.

Recommendations:

  1. Enhanced Version Control: Consider providing more granular control in the package registry, allowing users to lock versions down to specific patches. This would significantly reduce the risk of unintentional updates introducing breaking changes.
  2. Clearer Communication: While the changelog does mention this change, the practical impact on deployment strategies like ours might not be immediately apparent. It could be beneficial to highlight the potential implications of such changes more clearly, especially in terms of deployment and version control.

@smotornyuk
Copy link
Member

Note for person who will work on this issue: validate-key must be set to beaker.session.secret by default

@ThrawnCA
Copy link
Contributor

Our Docker setup was locked to python-ckan_2.10-jammy_amd64.deb, with the understanding that this would safeguard us against unexpected breaking changes until a major version release (version 3+). However, the registry does not allow locking to patch versions, only to minor versions.

You are correct. CKAN does not follow Semantic Versioning; the move from 2.8 to 2.9 was full of breaking changes, and the move from 2.9 to 2.10 likewise. (It would be nice if that changed, but I've seen no sign of appetite for it.)

tino097 added a commit that referenced this issue Feb 22, 2024
@amercader
Copy link
Member

I'm going to close this as the actual issue (error when missing beaker.session.validate_key config option is resolved but I wanted to give a bit more context and follow up on @khashashin great feedback.

For historical reasons CKAN doesn't follow Semantic Versioning strictly, as described in the docs minor releases (e.g. 2.8 -> 2.9 or 2.9->2.10) do contain backwards incompatible changes, although we try to minimize them as much as possible.

Patch releases (eg. 2.10.3 -> 2.10.4) should not contain backwards incompatible changes, but sometimes we are forced to introduce them for security reasons, either to fix an active vulnerability or because the tech team decides that the default behavior of CKAN exposes users unnecessarily. The beaker.session.validate_key was part of a wider change in how sessions are handled to make it more robust. While as you say it was mentioned in the CHANGELOG we can make things more prominent in the future.

Regarding the pinning of patch versions in the package registry, we will likely revisit how packages are built really soon and we can consider adding patch versions to allow pinning (as we do with our Docker images). But in any case, it's important to remember that the latest patch release is the only one supported and that running an old one might mean being exposed to vulnerabilities.

@ThrawnCA
Copy link
Contributor

ThrawnCA commented Apr 3, 2024

Perhaps it's just because we run a few dozen plugins, but my team's experience has been that the upgrades from 2.8 to 2.9, and from 2.9 to 2.10, were a whole program of work each. I really think it's worth reconsidering the current distinction between major and minor version increments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants