Segment-free cookbook data (RFC 67) #1034

thommay · 2016-12-14T17:15:11Z

https://chef.github.io/chef-rfc/rfc067-cookbook-segment-deprecation.html
This bumps API version for cookbook requests to 2.0, and transforms
cookbook metadata to provide all_files.

stevendanna · 2017-01-23T11:30:10Z

@thommay Any way the maintainers can help out here? Maybe we could set up some time to pair on this.

thommay · 2017-03-31T09:37:59Z

OK, here's the state of the game. The work in chef and chef-zero is complete and works. This now needs pedant updates to:

stop testing for deprecated cookbook metadata
test for v2 cookbook manifests

once that's done, this PR needs to get the work completed to down convert from v2 to v0 manifests.

stevendanna · 2017-11-27T11:54:48Z

@thommay I'm going through old PRs and this feels like something that we still definitely want. (I'm assuming here, given that we haven't needed it so far, I'm guessing we just have client-side code that deals with the old format still). Any chance this is something that could end up on communities board this year?

thommay · 2017-11-27T13:03:42Z

I would very much like to get this on our board this year but the habitat work is (rightly!) taking cycles right now

thommay · 2018-04-11T17:51:51Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+                Data).
+
+populate_segments(Data, Metadata) ->
+    Md1 = [ ej:set({Segment}, Metadata, []) || Segment <- ?COOKBOOK_SEGMENTS ],


This is clearly wrong but I can't figure out how to do what I want, which is to create each segment as an empty list before populating them. Is there a better way?

thommay · 2018-04-11T18:01:26Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

@@ -290,10 +331,11 @@ file_list_spec() ->

 -spec extract_checksums(ejson_term()) -> [binary()].
 extract_checksums(CBJson) ->
+    Segments = [ <<"all_files">> | ?COOKBOOK_SEGMENTS ],


We only add all_files to the list here because we don't want to have it treated as a segment generally (because it's not one!)

srenatus

Looked at the Erlang code, left some comments/questions 😃

srenatus · 2018-04-23T06:35:25Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

@@ -206,10 +217,18 @@ set_default_values(Cookbook) ->

 -spec validate_cookbook(Cookbook :: ej:json_object(),
                        {UrlName :: binary(),
-                         UrlVersion :: binary()}) -> {ok, ej:json_object()}.
-validate_cookbook(Cookbook, {UrlName, UrlVersion}) ->
+                         UrlVersion :: binary()}, true | false) -> {ok, ej:json_object()}.


[nit] I think I'd prefer

-spec validate_cookbook(Cookbook :: ej:json_object(), {UrlName :: binary(), UrlVersion :: binary()}, AllFiles :: boolean()) -> {ok, ej:json_object()}.

...where I find including some "name" more important than replacing true | false with boolean() 😃 -- I was wondering first what the lest argument might be. I mean, the next line tells me, too, so, this is just nitpicking.

srenatus · 2018-04-23T06:35:46Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

@@ -186,15 +195,17 @@ compress_maybe(Data, Type) ->
 %% @doc Convert a binary JSON string representing a Chef Cookbook Version into an
 %% EJson-encoded Erlang data structure.
 %% @end
-spec parse_binary_json(binary(), {binary(), binary()}) -> { ok, ejson_term() }. % or throw
-parse_binary_json(Bin, {UrlName, UrlVersion}) ->
+-spec parse_binary_json(binary(), {binary(), binary()}, true | false) -> { ok, ejson_term() }. % or throw


[nit] boolean() ?

srenatus · 2018-04-23T07:10:02Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+%% so for data that is v0 on disk we have to prepend the segment to the name
+add_segment_to_filename(Segment, File) ->
+    Fn = ej:get({<<"name">>}, File),
+    Fn1 = iolist_to_binary([Segment, "/", Fn]),


Do we know/care that what we get in the "name" field is good enough to be fed into iolist_to_binary?

Weird things could happen:

2> iolist_to_binary(["foo", "/", 10]). <<"foo/\n">> 3> iolist_to_binary(["foo", "/", -1]). ** exception error: bad argument in function iolist_to_binary/1 called as iolist_to_binary(["foo","/",-1]) 4> iolist_to_binary(["foo", "/", false]). ** exception error: bad argument in function iolist_to_binary/1 called as iolist_to_binary(["foo","/",false])

Update: found it, we've validated that it's a string ✅

srenatus · 2018-04-23T07:21:12Z

src/oc_erchef/apps/chef_objects/test/chef_cookbook_version_tests.erl

@@ -219,7 +225,7 @@ assemble_cookbook_ejson_test_() ->
                                              AuthzId,
                                              CBEJson),

-              MinCb = chef_cookbook_version:minimal_cookbook_ejson(Record, VHostUrl),
+              MinCb = chef_cookbook_version:minimal_cookbook_ejson(Record, VHostUrl, 1),
              ?assertEqual(undefined, ej:get({"metadata", "attributes"}, MinCb)),
              ?assertEqual(undefined, ej:get({"metadata", "long_description"}, MinCb)),
              ?assertEqual({[{<<"ruby">>, []}]}, ej:get({"metadata", "dependencies"}, MinCb))


❓ Looks like we've updated all existing tests, and added a minimal test for the new version. Are there perhaps more interesting test cases to include?

(Unrelated thought: what happens in this case?

"all_files": [ { "name": "metadata.rb", "checksum": "d18c630c8a68ffa4852d13214d0525a6", "path": ".////metadata.rb", # same path, mostly "specificity": "default" }, { "name": "metadata.rb", "checksum": "967087a09f48f234028d3aa27a094882", "path": "metadata.rb", "specificity": "default" } { "name": "metadata.rb", "checksum": "967087a09f48f234028d3aa27a094882", "path": "foobar.rb", # inconsistent path/name? "specificity": "default" },,

)

Hmm unless it's a copy-paste mishap, the "inconsistent" part seems to be expected in the RFC (templates/default/unicorn.rb.erb vs templates/unicorn.rb.erb)... 🤔

markan

This generally looks good. I've not gone through the pedant tests in great detail yet, mostly focussing on the erlang side; I'll make another pass later today with that in mind.

markan · 2018-05-15T06:10:00Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

-                         UrlVersion :: binary()}) -> {ok, ej:json_object()}.
-validate_cookbook(Cookbook, {UrlName, UrlVersion}) ->
+                         UrlVersion :: binary()}, true | false) -> {ok, ej:json_object()}.
+validate_cookbook(Cookbook, {UrlName, UrlVersion}, AllFiles) when AllFiles =:= false ->


validate_cookbook(Cookbook, {UrlName, UrlVersion}, false) might be more idiomatic.

markan · 2018-05-15T06:31:56Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+                        {{opt, <<"platforms">>}, constraint_map_spec(cookbook_name)},
+
+                        {{opt, <<"dependencies">>}, constraint_map_spec(cookbook_name)}
+                       ]}},


I note the v0 spec has recommendations, suggestions, conflicting, etc. but they seem to be missing here. The RFC seems to thing the fields are still valid, but they aren't being checked here. Are they supposed to be different? If not, we could DRY the thing up by having a common core and appending the appropriate fields.

RFC 85 removes those fields, and no v2 client will send them.

markan · 2018-05-15T19:29:08Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+
+    lists:foldl(fun(File, CB) ->
+                        { Segment, Record } = remove_segment_from_filename(File),
+                        case lists:member(Segment, ?COOKBOOK_SEGMENTS) of


What happens to files that are uploaded via a V2 call that don't fit in the V0 segment pattern? The RFC doesn't mention that, but I see @stevendanna commented about it (chef-boneyard/chef-rfc#161 (comment)). What was the decision? It looks like we silently drop them; I wonder if we might want to error instead, or otherwise we could silently be breaking cookbooks.

I think the only reasonable thing we can do is drop them; our choice is to drop them - and have cookbooks that only encompass the functionality that chef 12 can provide, whilst upgrading to 13 will handle everything; or we error, at which point cookbooks uploaded on 13 will almost always entirely stop working on 12, because inevitably we'll upload something (like a test directory) that 12 doesn't understand but that doesn't affect running the cookbook.

Understand the reasoning, but it strikes me as creating a really painful debug experience where those files are needed in the cookbook, but silently dropped on an older version of chef.
This is where something like a 'required_chef_version' cookbook metadata field would be nice; you could allow uploading non-standard paths only if the required_chef_version was set for a version that supported , and error on downloading cookbooks to chef versions older than supported.

markan · 2018-05-15T19:32:39Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+    end.
+
+populate_all_files(Segment, Data, Metadata) ->
+    lists:foldl(fun(File, CB) ->


Since Segment is a constant, It might be easier to invert the order; e.g.

ExpandedFiles = case Segment of <<"root_files">> -> Data; _ -> [ add_segment_to_filename(Segment, File) || File <- Data ] end, ej:set({<<"all_files">>}, Metadata, ExpandedFiles)

markan · 2018-05-15T19:45:41Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+    lists:foldl(fun(File, CB) ->
+                        { Segment, Record } = remove_segment_from_filename(File),
+                        case lists:member(Segment, ?COOKBOOK_SEGMENTS) of
+                            true -> ej:set_p({Segment, new}, CB, Record);


I tend to do much of my building of JSON structures outside of EJSON, then insert in the end. IMHO it can lead to clearer code. In particular, the ej:set_p pattern with new caught me by surprise, (it's not used anywhere else in chef-server or a1 that I can see.
I might have done this by folding over a map of segments to arrays, inserting into that, and then for each recognized COOKBOOK_SEGMENT extracting the built array from the map and ej:set-ting the field.
That's a matter of taste though.
As a secondary thing, I do like splitting things into smaller functions when possible; mostly because redbug doesn't really let you trace an anonymous function easily.

So you'd break out the function the second foldl takes (assuming the structure remains the same) into a real function?

thommay · 2018-05-16T14:52:27Z

~~The ChefFS failure is waiting on chef/chef#7270~~

markan · 2018-05-18T04:31:55Z

oc-chef-pedant/lib/pedant/rspec/cookbook_util.rb

+          opts[:payload].each do |part, files|
+            all_files += files.each_with_object([]) do |f, acc|
+              file = f.dup
+              file["name"] = "#{part}/#{file["name"]}"


Looking at the usage: https://github.com/chef/chef-server/pull/1034/files#diff-d5911d79b4d2796916287911d6a428fcR69 it seems like we'd need to do something different for root_files?

markan · 2018-05-18T04:36:50Z

src/oc_erchef/apps/chef_objects/src/chef_cookbook_version.erl

+                                case ej:get({Segment}, CB) of
+                                    undefined -> CB;
+                                    Data ->
+                                        CB1 = populate_all_files(Segment, Data, CB),


It looks like we replace all_files in populate_all_files above, so this fold would only keep the last segment.

markan · 2018-05-18T04:40:25Z

I pushed a branch ma/kill_segment_tweaks with some ideas on refactoring the code.

markan · 2018-05-31T00:03:56Z

.travis.yml

@@ -83,7 +83,7 @@ matrix:
      script: bundle exec rake chef_zero_spec
      env:
        - "PEDANT_OPTS=\"--skip-oc_id\""
-        - "GEMFILE_MOD=\"gem 'rake'; gem 'chef-zero', github: 'chef/chef-zero', tag: 'v14.0.5' \""
+        - "GEMFILE_MOD=\"gem 'rake'; gem 'chef-zero', github: 'chef/chef-zero'; gem 'chef', github: 'chef/chef' \""


The pin to 14.0.5 was due to the latest chef-zero having issues with chef 13; it might be better to wait on this unpinning until we no longer pin the rest of chef server to chef 13.

it actually does no good anyway, so yeah, i'll revert it.

markan · 2018-06-01T23:42:31Z

I think this is really close to merging.
TODO
[] One last review by prior reviewers (@stevendanna @srenatus)
[] Squash/rebase once master is green
[] Make sure commit message makes sense in terms of auto changelog generation.

markan

I think this is ready to go except for a few minor tweaks.

thommay · 2018-06-04T12:06:15Z

http://wilson.cd.chef.co/job/chef-server-trigger-ad_hoc/85/downstreambuildview/

Implement RFC 67, which causes us to bump our max API version to 2. * Accept segmentless and segmented downloads and uploads of cookbooks * Add pedant tests Signed-off-by: Thom May <[email protected]>

thommay mentioned this pull request Jan 6, 2017

Remove cookbook segments chef/chef-zero#250

Merged

stevendanna assigned stevendanna and thommay Jan 20, 2017

thommay mentioned this pull request Mar 31, 2017

V2 Cookbook Manifests chef/chef#5929

Merged

thommay force-pushed the tm/kill_segments branch 2 times, most recently from 002d3a7 to 6743773 Compare March 31, 2017 13:05

thommay force-pushed the tm/kill_segments branch from 85b3e60 to 0986590 Compare April 9, 2018 17:31

thommay requested a review from a team April 9, 2018 17:31

thommay force-pushed the tm/kill_segments branch 4 times, most recently from c609793 to 670849c Compare April 11, 2018 17:47

thommay commented Apr 11, 2018

View reviewed changes

thommay force-pushed the tm/kill_segments branch 2 times, most recently from 0b065c4 to 5239a91 Compare April 18, 2018 14:29

srenatus reviewed Apr 23, 2018

View reviewed changes

thommay force-pushed the tm/kill_segments branch from 5239a91 to 8bfb8df Compare May 15, 2018 13:40

markan suggested changes May 15, 2018

View reviewed changes

thommay force-pushed the tm/kill_segments branch from 8bfb8df to a90f4a8 Compare May 16, 2018 14:34

markan reviewed May 18, 2018

View reviewed changes

thommay requested a review from a team as a code owner May 30, 2018 11:15

thommay force-pushed the tm/kill_segments branch from 8d89bea to 88212bb Compare May 30, 2018 11:17

markan reviewed May 31, 2018

View reviewed changes

thommay force-pushed the tm/kill_segments branch from 88212bb to f4cc82c Compare June 1, 2018 16:49

markan approved these changes Jun 1, 2018

View reviewed changes

thommay force-pushed the tm/kill_segments branch from f4cc82c to a331618 Compare June 4, 2018 09:09

thommay force-pushed the tm/kill_segments branch from a331618 to 545e1c8 Compare June 8, 2018 10:43

markan force-pushed the tm/kill_segments branch from 545e1c8 to d3de85c Compare June 12, 2018 22:33

thommay force-pushed the tm/kill_segments branch 2 times, most recently from b87c21f to b96bd84 Compare June 13, 2018 12:13

Segmentless cookbook data (RFC 67)

95b5318

Implement RFC 67, which causes us to bump our max API version to 2. * Accept segmentless and segmented downloads and uploads of cookbooks * Add pedant tests Signed-off-by: Thom May <[email protected]>

thommay force-pushed the tm/kill_segments branch from b96bd84 to 95b5318 Compare June 13, 2018 12:13

markan merged commit 36ec5f6 into master Jun 13, 2018

markan deleted the tm/kill_segments branch June 13, 2018 16:32

btm mentioned this pull request Aug 29, 2019

Implementing RFC 67 - Cookbook Segment Deprecation #1023

Closed

5 tasks

This was referenced Oct 9, 2019

Since Chef Server 12.18.14 Chef Guard fails to process cookbook checks schubergphilis/chef-guard#133

Open

RFC 67 is not documented chef/chef-web-docs#2165

Open

btm mentioned this pull request Jun 11, 2020

[WIP] [design-proposal] create compliance phase in infra-client chef/chef#9986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Segment-free cookbook data (RFC 67) #1034

Segment-free cookbook data (RFC 67) #1034

thommay commented Dec 14, 2016 •

edited

Loading

stevendanna commented Jan 23, 2017

thommay commented Mar 31, 2017 •

edited

Loading

stevendanna commented Nov 27, 2017

thommay commented Nov 27, 2017

thommay Apr 11, 2018

thommay Apr 11, 2018

srenatus left a comment

srenatus Apr 23, 2018

srenatus Apr 23, 2018

srenatus Apr 23, 2018

srenatus Apr 23, 2018

markan left a comment

markan May 15, 2018

markan May 15, 2018

thommay May 16, 2018

markan May 15, 2018

thommay May 16, 2018

markan May 18, 2018

markan May 15, 2018

markan May 15, 2018

thommay May 16, 2018 •

edited

Loading

thommay commented May 16, 2018 •

edited

Loading

markan May 18, 2018

markan May 18, 2018

markan commented May 18, 2018

markan May 31, 2018

thommay May 31, 2018

markan commented Jun 1, 2018

markan left a comment •

edited

Loading

thommay commented Jun 4, 2018

Segment-free cookbook data (RFC 67) #1034

Segment-free cookbook data (RFC 67) #1034

Conversation

thommay commented Dec 14, 2016 • edited Loading

stevendanna commented Jan 23, 2017

thommay commented Mar 31, 2017 • edited Loading

stevendanna commented Nov 27, 2017

thommay commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thommay May 16, 2018 • edited Loading

Choose a reason for hiding this comment

thommay commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markan commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markan commented Jun 1, 2018

markan left a comment • edited Loading

Choose a reason for hiding this comment

thommay commented Jun 4, 2018

thommay commented Dec 14, 2016 •

edited

Loading

thommay commented Mar 31, 2017 •

edited

Loading

thommay May 16, 2018 •

edited

Loading

thommay commented May 16, 2018 •

edited

Loading

markan left a comment •

edited

Loading