Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/301#issuecomment-159695223
Ok the next thing we need to do is remove the ```cert_files_vec``` from all
the functions in ```SSLUtils.cc```. The way we can do this is to add another
callback to ```SSLConfigParams``` that is triggered when any SSL-related file
is loaded. This callback can be attached in ```proxy/Main.cc``` and will just
call your new ```ProcessManager::signalConfigFileChild()```.
In ```FileManager::configFileChild()``` you need to hold the lock the whole
time you are adding the new Rollback so that the parent pointer cannot get
deleted out from under you. I think you should add an assertion that parent
must not also have a parent in order to prevent construction of arbitrary
Rollback trees.
You don't need ```ts/ink_string++.h``` in ```mgmt/FileManager.h```.
In ```FileManager::rereadConfig()```, you can't delete entries out of the
hash table while you are iterating over it. The logic of this is pretty hard to
follow and can be simplified. You don't need to track all the parents, just the
changed files. For each changed file, you know it is a parent because
```getParentRollback``` will return NULL. So you just have to walk the set of
changed files, check if each is a parent, then delete the children if it is.
I found the logic of which files trigger the change notification confusing
because it is split between ```FileManager::fileChanged``` and
```Rollback::internalUpdate```. You should clarify this either in comments or
code.
In places where you check ```isFileNameRooted()``` to determine whether the
Rollback is a child, use an explicit member function:
bool isChildRollback() const { return getParentRollback() != NULL; }
I tested this and it writes backup copies of certificates:
-rw-r--r-- 1 root nobody 2785 Nov 25 10:18
ssl_multicert.config_2
-rw-r--r-- 1 root nobody 2785 Nov 25 10:18
ssl_multicert.config
-rw------- 1 nobody nobody 12859 Nov 25 10:20 records.config
-rw-r--r-- 1 root nobody 3278 Nov 25 10:22 example.com.crt_1
-rw-r--r-- 1 root nobody 3278 Nov 25 10:22 example.com.crt
Do we really want this to happen for certificates? What about for private
keys?. This is why I suggested sending flags with the management message so
that the rollback can turn this off.
It looks like this change only tracks certificate files. What about all the
other files that contribute to the SSL context?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
