You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/06/03 20:24:20 UTC
[GitHub] [trafficcontrol] rob05c opened a new pull request, #6883: Fix t3c cache to invalidate on version change
rob05c opened a new pull request, #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883
Previously, both upgrades and downgrades of `trafficcontrol-cache-config` could cause strange issues in config generation, from trying to re-use a cache with incompatibilities resulting in missing or different fields.
This fixes t3c to invalidate the cache if the running version is different than the version which created the cache.
## Which Traffic Control components are affected by this PR?
- Traffic Control Cache Config (`t3c`, formerly ORT)
## What is the best way to verify this PR?
Run a previous version of t3c, upgrade, verify from log messages and/or HTTP IMS requests that the cache is not used.
## If this is a bugfix, which Traffic Control versions contained the bug?
- 6.0.0
- 6.0.1
- 6.0.2
- 6.1.0
## PR submission checklist
- ~[x] This PR has tests~ no tests, testing requires HTTP requests and thus can't be unit tested, and integration tests don't support multiple versions yet.
- ~[x] This PR has documentation~ no docs, cache behavior is not documented.
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6883: Fix t3c cache to invalidate on version change
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883#discussion_r916198979
##########
cache-config/t3cutil/getdata.go:
##########
@@ -54,6 +54,10 @@ type TCCfg struct {
// OldCfg is the previously fetched ConfigData, for 'config' requests. May be nil.
OldCfg *ConfigData
+
+ // T3cVersion is the version of the t3c app ecosystem
+ // This value will be the same for any t3c app.
+ T3CVersion string
Review Comment:
> It just seems a little strange to tell a utility which could be wrong when the utility ostensibly always knows the correct value.
To clarify: it's in this struct, because this struct is what get passed from the app (`t3c-apply`) to the library function that needs it in `t3cutil`. It should never be changed.
I could have made it impossible to change, by making it a function of the struct instead. But that isn't idiomatic Go, and it adds more complexity and increases the dev cost to read and write it. I decided the complexity and readability cost wasn't worth the very minor safety gain (especially considering Go standard libraries use non-const variables all over the place, that are equally "unsafe").
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6883: Fix t3c cache to invalidate on version change
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883#discussion_r916189290
##########
cache-config/t3cutil/getdata.go:
##########
@@ -54,6 +54,10 @@ type TCCfg struct {
// OldCfg is the previously fetched ConfigData, for 'config' requests. May be nil.
OldCfg *ConfigData
+
+ // T3cVersion is the version of the t3c app ecosystem
Review Comment:
Changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6883: Fix t3c cache to invalidate on version change
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883#discussion_r892696349
##########
cache-config/t3cutil/getdata.go:
##########
@@ -54,6 +54,10 @@ type TCCfg struct {
// OldCfg is the previously fetched ConfigData, for 'config' requests. May be nil.
OldCfg *ConfigData
+
+ // T3cVersion is the version of the t3c app ecosystem
Review Comment:
should be a capital "C" to match the field name
##########
cache-config/t3cutil/getdatacfg.go:
##########
@@ -173,15 +177,27 @@ func MakeReqMetaData(respHdr http.Header) ReqMetaData {
//
// The cacheHostName is the hostname of the cache to get config generation data for.
//
+// The oldCfg is previous config data which was cached. May be nil, if the caller has no previous data.
+// If it exists and is usable, If-Modified-Since requests will be made and the cache re-used where possible.
+//
+// The version is a unique version of the application, which should change with any compatibility changes.
+// Old config with a different version than the current won't be used (though in the future, smarter compatibility could be added).
+//
// The revalOnly arg is whether to only get data necessary to revalidate, versus all data necessary to generate cache config.
-func GetConfigData(toClient *toreq.TOClient, disableProxy bool, cacheHostName string, revalOnly bool, oldCfg *ConfigData) (*ConfigData, error) {
+func GetConfigData(toClient *toreq.TOClient, disableProxy bool, cacheHostName string, revalOnly bool, oldCfg *ConfigData, version string) (*ConfigData, error) {
start := time.Now()
defer func() { log.Infof("GetTOData took %v\n", time.Since(start)) }()
toIPs := &sync.Map{} // each Traffic Ops request could get a different IP, so track them all
toData := &ConfigData{}
+ toData.Version = version
toData.MetaData.CacheHostName = cacheHostName
+ if oldCfg != nil && oldCfg.Version != toData.Version {
+ log.Infof("old config version '%v' doesn't match current version '%v', old config will not be used!\n", oldCfg.Version, toData.Version)
Review Comment:
For the best safety at compile-time, format-strings should use the most specific formatting parameter possible - `%s` in this case.
##########
cache-config/t3cutil/getdata.go:
##########
@@ -54,6 +54,10 @@ type TCCfg struct {
// OldCfg is the previously fetched ConfigData, for 'config' requests. May be nil.
OldCfg *ConfigData
+
+ // T3cVersion is the version of the t3c app ecosystem
+ // This value will be the same for any t3c app.
+ T3CVersion string
Review Comment:
Should this be override-able through the configuration file? It just seems a little strange to tell a utility which could be wrong when the utility ostensibly always knows the correct value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6883: Fix t3c cache to invalidate on version change
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883#discussion_r916196067
##########
cache-config/t3cutil/getdatacfg.go:
##########
@@ -173,15 +177,27 @@ func MakeReqMetaData(respHdr http.Header) ReqMetaData {
//
// The cacheHostName is the hostname of the cache to get config generation data for.
//
+// The oldCfg is previous config data which was cached. May be nil, if the caller has no previous data.
+// If it exists and is usable, If-Modified-Since requests will be made and the cache re-used where possible.
+//
+// The version is a unique version of the application, which should change with any compatibility changes.
+// Old config with a different version than the current won't be used (though in the future, smarter compatibility could be added).
+//
// The revalOnly arg is whether to only get data necessary to revalidate, versus all data necessary to generate cache config.
-func GetConfigData(toClient *toreq.TOClient, disableProxy bool, cacheHostName string, revalOnly bool, oldCfg *ConfigData) (*ConfigData, error) {
+func GetConfigData(toClient *toreq.TOClient, disableProxy bool, cacheHostName string, revalOnly bool, oldCfg *ConfigData, version string) (*ConfigData, error) {
start := time.Now()
defer func() { log.Infof("GetTOData took %v\n", time.Since(start)) }()
toIPs := &sync.Map{} // each Traffic Ops request could get a different IP, so track them all
toData := &ConfigData{}
+ toData.Version = version
toData.MetaData.CacheHostName = cacheHostName
+ if oldCfg != nil && oldCfg.Version != toData.Version {
+ log.Infof("old config version '%v' doesn't match current version '%v', old config will not be used!\n", oldCfg.Version, toData.Version)
Review Comment:
For the record, I don't agree, in my opinion it reduces readability without increasing safety. It also makes this code not idiomatic with the rest of t3c and lib/go-atscfg.
But it's more important to get this fix in. Changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6883: Fix t3c cache to invalidate on version change
Posted by GitBox <gi...@apache.org>.
rob05c commented on code in PR #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883#discussion_r916188417
##########
cache-config/t3cutil/getdata.go:
##########
@@ -54,6 +54,10 @@ type TCCfg struct {
// OldCfg is the previously fetched ConfigData, for 'config' requests. May be nil.
OldCfg *ConfigData
+
+ // T3cVersion is the version of the t3c app ecosystem
+ // This value will be the same for any t3c app.
+ T3CVersion string
Review Comment:
It doesn't make sense to ever override this. This version is used by the `t3c` app (via library functions later) writing a cache file.
Its purpose is so a future `t3c` app can read it, compare to its own version, and determine compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #6883: Fix t3c cache to invalidate on version change
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #6883:
URL: https://github.com/apache/trafficcontrol/pull/6883
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org