[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/trafficserver/pull/971


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r78096463
  
--- Diff: lib/records/I_RecCore.h ---
@@ -168,11 +169,15 @@ int RecGetRecordBool(const char *name, RecBool 
*rec_byte, bool lock = true);
 //
 // Record Attributes Reading
 //
+// Values with names that have this prefix are exempted from being 
considered "unrecognized".
+const char REC_PLUGIN_CONFIG_NAME_PREFIX[] = "proxy.config.plugin.";
+static const size_t REC_PLUGIN_CONFIG_NAME_PREFIX_LEN = 
sizeof(REC_PLUGIN_CONFIG_NAME_PREFIX)-1;
--- End diff --

You can't get the value of the record unless it is registered, so I don't 
think this saves you any work.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r78096159
  
--- Diff: lib/records/I_RecCore.h ---
@@ -168,11 +169,15 @@ int RecGetRecordBool(const char *name, RecBool 
*rec_byte, bool lock = true);
 //
 // Record Attributes Reading
 //
+// Values with names that have this prefix are exempted from being 
considered "unrecognized".
+const char REC_PLUGIN_CONFIG_NAME_PREFIX[] = "proxy.config.plugin.";
+static const size_t REC_PLUGIN_CONFIG_NAME_PREFIX_LEN = 
sizeof(REC_PLUGIN_CONFIG_NAME_PREFIX)-1;
--- End diff --

I think it's a nice convenience to avoid additional API calls or work.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r78096037
  
--- Diff: lib/records/RecCore.cc ---
@@ -514,6 +514,18 @@ RecLookupMatchingRecords(unsigned rec_type, const char 
*match, void (*callback)(
   return REC_ERR_OKAY;
 }
 
+void
+RecLookupIterateRecords(std::function f)
--- End diff --

Yes, I saw the `RecLookup...` functions and modeled the new call on those. 
I didn't see `RecDumpRecords`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r78089641
  
--- Diff: lib/records/I_RecCore.h ---
@@ -168,11 +169,15 @@ int RecGetRecordBool(const char *name, RecBool 
*rec_byte, bool lock = true);
 //
 // Record Attributes Reading
 //
+// Values with names that have this prefix are exempted from being 
considered "unrecognized".
+const char REC_PLUGIN_CONFIG_NAME_PREFIX[] = "proxy.config.plugin.";
+static const size_t REC_PLUGIN_CONFIG_NAME_PREFIX_LEN = 
sizeof(REC_PLUGIN_CONFIG_NAME_PREFIX)-1;
--- End diff --

I don't think this is necessary now that you are checking whether the 
record is registered?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r78089078
  
--- Diff: lib/records/RecCore.cc ---
@@ -514,6 +514,18 @@ RecLookupMatchingRecords(unsigned rec_type, const char 
*match, void (*callback)(
   return REC_ERR_OKAY;
 }
 
+void
+RecLookupIterateRecords(std::function f)
--- End diff --

Isn't this the same as ``RecDumpRecords()``? Do we need to have both?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r77874593
  
--- Diff: proxy/Main.cc ---
@@ -476,6 +476,18 @@ check_config_directories(void)
   }
 }
 
+namespace {
+  // Values with names that have this prefix are exempted from being 
considered "unrecognized".
+  const char PLUGIN_CONFIG_RECORD_PREFIX[] = "proxy.config.plugin.";
--- End diff --

This is the root of the problem in that if arbitrary config names can be 
used by plugins, you can't do validity checks. The consensus at the bug scrub 
was it was reasonable to restrict plugin configuration values to that subtree.

I could move the check after plugin initialization so that if a plugin does 
call {{TSMgmtStringCreate}} it doesn't generate a warning.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r77854033
  
--- Diff: proxy/Main.cc ---
@@ -476,6 +476,18 @@ check_config_directories(void)
   }
 }
 
+namespace {
+  // Values with names that have this prefix are exempted from being 
considered "unrecognized".
+  const char PLUGIN_CONFIG_RECORD_PREFIX[] = "proxy.config.plugin.";
--- End diff --

I think the right check is to test whether there are any unregistered local 
or config records. This can be done periodically since new records can be added 
at config reload time. A records that is present in ``records.config`` will be 
not be marked as registered intul a plugin calls ``TSMgmtStringCreate()`` or 
the equivalent.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/971#discussion_r77714260
  
--- Diff: proxy/Main.cc ---
@@ -476,6 +476,18 @@ check_config_directories(void)
   }
 }
 
+namespace {
+  // Values with names that have this prefix are exempted from being 
considered "unrecognized".
+  const char PLUGIN_CONFIG_RECORD_PREFIX[] = "proxy.config.plugin.";
--- End diff --

This is a big assumption. I have internal plugins that use configuration 
values that are not in this namespace.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #971: TS-1882: Check for unrecognized configurati...

[SolidWallOfCode]

GitHub user SolidWallOfCode opened a pull request:

https://github.com/apache/trafficserver/pull/971

TS-1882: Check for unrecognized configuration values.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/SolidWallOfCode/trafficserver ts-1882

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/trafficserver/pull/971.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #971


commit 3e15966e7fd99242d2bdec0a20fb3966b81d959f
Author: Alan M. Carroll 
Date:   2016-09-06T20:34:21Z

TS-1882: Check for unrecognized configuration values.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---