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

Update ssl.session_cache.enabled to be session_cache.value #11902

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ezelkow1
Copy link
Member

@ezelkow1 ezelkow1 commented Dec 6, 2024

Fixes #11901

@ezelkow1 ezelkow1 self-assigned this Dec 6, 2024
@maskit
Copy link
Member

maskit commented Dec 6, 2024

I thought the implementation should be changed instead because the documentation is what users see and new users who didn't use ATS 9 would be required to update their settings. But, unfortunately, convert2yaml.py disagrees.

'proxy.config.ssl.origin_session_cache': 'proxy.config.ssl.origin_session_cache.enabled',
'proxy.config.ssl.session_cache': 'proxy.config.ssl.session_cache.value',

I think the best way is to support the both names (.value and .enabled) for now, and introduce new name (.mode) to address the inconsistency between session_cache and origin_session_cache.

@ezelkow1
Copy link
Member Author

ezelkow1 commented Dec 6, 2024

yea I did notice that the upgrading to 10 doc does mention some of these. So there is a reference there for previous users:
https://docs.trafficserver.apache.org/en/10.0.x/release-notes/upgrading.en.html

Also having the old one there is apparently required for the upgrading document to be buildable? This seems like an odd dependency on an old name:

Warning, treated as error:
/home/jenkins/workspace/Github_Builds/docs/src/doc/release-notes/upgrading.en.rst:175:No definition found for configuration variable 'proxy.config.ssl.session_cache.enabled'

@bryancall bryancall requested a review from brbzull0 December 9, 2024 23:14
@bryancall
Copy link
Contributor

It looks like there are other configuration options named proxy..enabled and this is the only one with proxy..value. It would be great if this was consistent with other settings. @maskit suggested supporting both configuration option names for now. I would lean towards this as a solution too.

@brbzull0
Copy link
Contributor

brbzull0 commented Dec 11, 2024

There was a reason why we have value and enabled.

In this case Implementation seems fine, at least it follows what was the logic when this fields were renamed:

Naming Changes:

All these renamed records are subject to the following reasoning:

If a value is meant to be used as a toggle on/off, I’ve used the name enabled.
If a value is meant to be used as an enumeration, I’ve used the name value

So proxy.config.ssl.origin_session_cache.enabled and proxy.config.ssl.session_cache.value were renamed based on the above, but documentation wasn't properly updated.

I do agree that mode is more appropriate than value, but seems late to make this ammend.

upgrading.en.rst needs to be updated too.

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

Successfully merging this pull request may close these issues.

ssl.session_cache.enabled unknown
4 participants