Skip to content

Commit 4fae560

Browse files
austbaustb
committed
(PDB-5338) Strip unrecognized keys from catalog resources
Previously, if the agent wanted to add a new catalog resource attribute, current and past versions of PuppetDB would fail to store the new catalog due to the unexpected field. This could cause tight coupling of Puppet Agent and PuppetDB versions in a non-major release, which is undesirable. To enable the agent to add the resource kind field, as well as future resource fields without breaking PuppetDB, we need to strip the unknown attributes from a catalog's resources because we do not currently have a column to store them in. For new resource attributes added in the future, PuppetDB will log one warning message per-startup for any keys it does not recognize.
1 parent 95d3524 commit 4fae560

File tree

5 files changed

+69
-23
lines changed

5 files changed

+69
-23
lines changed

‎src/puppetlabs/puppetdb/catalogs.clj‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
(:require [clojure.core.match :refer [match]]
8989
[clojure.set :as set]
9090
[clojure.string :as string]
91+
[clojure.tools.logging :as log]
9192
[puppetlabs.puppetdb.cheshire :as json]
9293
[puppetlabs.puppetdb.schema :as pls]
9394
[puppetlabs.puppetdb.time :refer [to-timestamp]]
@@ -276,13 +277,32 @@
276277

277278
;; ## Resource normalization
278279

280+
;; the kind field was added to improve agent/server communication, so resources
281+
;; are expected to have it, but we do not store it. Other unrecognized keys will
282+
;; be added to it so that each unrecognized key is only logged once per startup.
283+
(def already-seen-unrecognized-keys (atom #{:kind}))
284+
285+
(defn- strip-unknown-attributes
286+
"Removes unknown attributes from a resource. This is essential for the forward compatibility
287+
of PuppetDB when the puppet agent makes additions to its resource definition."
288+
[{:keys [type title] :as resource} expected-keys already-seen-unrecognized-keys]
289+
(let [unrecognized-keys (set/difference (set (keys resource))
290+
expected-keys
291+
@already-seen-unrecognized-keys)]
292+
(when-let [ks (seq unrecognized-keys)]
293+
(log/warn (trs "Ignoring unrecognized catalog resource key(s) {0}. Future warnings will be surpressed."
294+
ks type title))
295+
(swap! already-seen-unrecognized-keys set/union unrecognized-keys))
296+
(select-keys resource expected-keys)))
297+
279298
(defn transform-resource*
280-
"Normalizes the structure of a single `resource`. Right now this just means
281-
setifying the tags."
282-
[resource]
299+
"Normalizes the structure of a single `resource`."
300+
[resource expected-keys already-seen-unrecognized-keys]
283301
{:pre [(map? resource)]
284302
:post [(set? (:tags %))]}
285-
(transform-tags resource))
303+
(-> resource
304+
transform-tags
305+
(strip-unknown-attributes expected-keys already-seen-unrecognized-keys)))
286306

287307
(defn transform-resources
288308
"Turns the list of resources into a mapping of
@@ -292,9 +312,13 @@
292312
(not (map? resources))]
293313
:post [(map? (:resources %))
294314
(= (count resources) (count (:resources %)))]}
295-
(let [new-resources (into {} (for [{:keys [type title] :as resource} resources
315+
(let [already-seen-unrecognized-keys already-seen-unrecognized-keys
316+
expected-keys #{:type :title :exported :file :line :tags :aliases :parameters}
317+
new-resources (into {} (for [{:keys [type title] :as resource} resources
296318
:let [resource-spec {:type type :title title}
297-
new-resource (transform-resource* resource)]]
319+
new-resource (transform-resource* resource
320+
expected-keys
321+
already-seen-unrecognized-keys)]]
298322
[resource-spec new-resource]))]
299323
(assoc catalog :resources new-resources)))
300324

‎test/puppetlabs/puppetdb/catalogs_test.clj‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -132,19 +132,21 @@
132132
:parameters {:ensure "present"
133133
:user "root"
134134
:group "root"
135-
:source "puppet:///foobar/foo/bar"}}]}]
135+
:source "puppet:///foobar/foo/bar"}}]}
136+
normalized-catalog {:resources {{:type "File" :title "/etc/foobar"}
137+
{:type "File"
138+
:title "/etc/foobar"
139+
:exported false
140+
:line 1234
141+
:file "/tmp/foobar.pp"
142+
:tags #{"class" "foobar"}
143+
:parameters {:ensure "present"
144+
:user "root"
145+
:group "root"
146+
:source "puppet:///foobar/foo/bar"}}}}]
136147
(is (= (-> catalog
137148
(transform-resources))
138-
{:resources {{:type "File" :title "/etc/foobar"} {:type "File"
139-
:title "/etc/foobar"
140-
:exported false
141-
:line 1234
142-
:file "/tmp/foobar.pp"
143-
:tags #{"class" "foobar"}
144-
:parameters {:ensure "present"
145-
:user "root"
146-
:group "root"
147-
:source "puppet:///foobar/foo/bar"}}}}))
149+
normalized-catalog))
148150

149151
(let [resources (:resources catalog)
150152
new-resources (conj resources (first resources))
@@ -159,7 +161,15 @@
159161
;; missing resources aren't allowed
160162
(is (thrown? AssertionError (transform-resources {})))
161163
;; pre-created resource maps aren't allow
162-
(is (thrown? AssertionError (transform-resources {:resources {}}))))))
164+
(is (thrown? AssertionError (transform-resources {:resources {}}))))
165+
166+
(testing "kind attribute and other unrecognized keys are removed from resources"
167+
(let [catalog (update catalog :resources (partial map
168+
#(assoc %
169+
:kind "unknown"
170+
:unknown-key "new resource attribute")))]
171+
(is (= (transform-resources catalog)
172+
normalized-catalog))))))
163173

164174
(deftest test-v9-conversion
165175
(testing "v8->v9"

‎test/puppetlabs/puppetdb/examples.clj‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,22 +118,26 @@
118118
:title "Settings"
119119
:parameters {}
120120
:tags ["class" "settings"]
121-
:type "Class"}
121+
:type "Class"
122+
:kind "unknown"}
122123
{:exported false
123124
:title "main"
124125
:parameters {:name "main"}
125126
:tags ["class"]
126-
:type "Class"}
127+
:type "Class"
128+
:kind "unknown"}
127129
{:exported false
128130
:title "main"
129131
:parameters {:name "main"}
130132
:tags ["stage"]
131-
:type "Stage"}
133+
:type "Stage"
134+
:kind "unknown"}
132135
{:exported false
133136
:title "default"
134137
:parameters {}
135138
:tags ["node" "default" "class"]
136-
:type "Node"}]
139+
:type "Node"
140+
:kind "unknown"}]
137141
:version "1332533763"
138142
:transaction_uuid "68b08e2a-eeb1-4322-b241-bfdf151d294b"
139143
:catalog_uuid "68b08e2a-eeb1-4322-b241-bfdf151d294b"
@@ -167,6 +171,7 @@
167171
:exported false
168172
:file "/tmp/foo"
169173
:line 10
174+
:kind "unknown"
170175
:tags ["file" "class" "foobar"]
171176
:parameters {:ensure "directory"
172177
:group "root"
@@ -182,6 +187,7 @@
182187
:exported false
183188
:file "/tmp/foo"
184189
:line 10
190+
:kind "unknown"
185191
:tags ["file" "class" "foobar"]
186192
:parameters {:ensure "directory"
187193
:group "root"
@@ -195,6 +201,7 @@
195201
:exported false
196202
:file "/tmp/foo"
197203
:line 10
204+
:kind "unknown"
198205
:tags ["file" "class" "foobar"]
199206
:parameters {:ensure "directory"
200207
:group "root"

‎test/puppetlabs/puppetdb/generative/generators.clj‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
(defgen :resource/exported gen/boolean)
6363
(defgen :resource/file (string+ 1024))
6464
(defgen :resource/line pg-pos-integer)
65+
(defgen :resource/kind (string+ 15))
6566

6667
(defgen :resource/tag (gen/string-from-regex #"[a-z0-9_][a-z0-9_:\-.]*"))
6768
(defgen :resource/tags (gen/vector :resource/tag 0 3))
@@ -82,6 +83,7 @@
8283
:resource/exported
8384
:resource/file
8485
:resource/line
86+
:resource/kind
8587
:resource/parameters
8688
:resource/tags))
8789

‎test/puppetlabs/puppetdb/testutils/catalogs.clj‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@
4444
clojure.walk/stringify-keys
4545
(dissoc "hash")
4646
(update "producer_timestamp" to-string)
47-
(update "resources" (fn [resources] (set (map #(update % "tags" set) resources))))
47+
(update "resources" (fn [resources] (->> resources
48+
(map #(update % "tags" set))
49+
(map #(dissoc % "kind"))
50+
set)))
4851
(update "edges" (fn [edges] (set (map #(update % "relationship" name) edges))))
4952
;; In our terminus code, the version is sometimes being serialized as a JSON
5053
;; integer, rather than a string. The correct data type is String.

0 commit comments

Comments
 (0)