Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DO NOT MERGE: Habitize the Chef Server #1416

Closed
wants to merge 99 commits into from
Closed

Conversation

markan
Copy link
Contributor

@markan markan commented Oct 27, 2017

#1416 cThis is a total WIP; and by no means is this ready to merge. It probably will change a lot as we work on things.

This represents a experimental take on what a chef-server might look like under habitat.

We're doing separate containers for each service, and starting them under control of a docker compose file. This is convenient for development, but totally sidesteps a whole bunch of operational questions.

Trying it out

Build it

hab studio enter
cd src
for dir in oc-id openresty-noroot nginx dbdpg bookshelf chef-server-ctl oc_bifrost oc_erchef; do cd $dir; build && hab pkg export docker $(ls -1t results/*.hart | head -1) ; cd ..; done

Running it (first time)

export HAB_ORIGIN=my_hab_origin
docker-compose up

Testing it

docker-compose exec chef-server-ctl chef-server-test
curl -k https://localhost/_status

Running it (subsequent times with empty data)

docker-compose down && docker system prune --volumes -f && docker-compose up

Note: the removal of volumes should not be needed once we make data bootstrapping convergent

TODO list:

oc_bifrost

  • set up binds & dependencies
  • convert configuration
  • db schema management with sqitch
  • health check hook
  • remove pkg_svc_user=root requirement

oc_erchef

  • set up binds & dependencies
  • convert configuration
  • db schema management with sqitch
  • bootstrap DB with initial data
  • health check hook
  • remove pkg_svc_user=root requirement

bookshelf

  • set up binds & dependencies
  • convert configuration
  • db schema management with sqitch
  • health check hook
  • remove pkg_svc_user=root requirement

nginx

  • set up binds & dependencies
  • convert configuration
  • health check hook
  • remove pkg_svc_user=root requirement

chef-server-ctl

  • import omnibus-ctl, private-chef-ctl-commands & oc-chef-pedant to reproduce a chef-server-ctl admin experience
  • convert configuration
  • secrets generation & sharing via ring

stretch goals:

  • test docker run requirements: --network host --user notroot:notroot --cap-drop NET_BIND_SERVICE --cap-drop SETUID --cap-drop SETGID
  • mover??
  • day 2 operations (monitoring, secrets rekey, deployment/topology guidance)
  • public builder service to continuously integrate?

before merge

  • Rebase and Squash all commits (DCO violations)
  • Create/use an "official" habitat origin and dockerhub user

@markan markan requested a review from a team October 27, 2017 23:51
@@ -6,14 +6,14 @@

access_log {{pkg.svc_var_path}}/log/access.log opscode;
ssl on;
ssl_certificate {{pkg.svc_data_path}}/ca/api.chef-server.dev.crt;
ssl_certificate_key {{pkg.svc_data_path}}/ca/api.chef-server.dev.key;
ssl_certificate {{pkg.svc_data_path}}/ca/{{cfg.server_name}.cert;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @markan startup failures are likely do to a missing second right brace here

@markan markan force-pushed the jm+ma/habitat_wip_wip_wip branch from d8366c6 to 285bda2 Compare November 3, 2017 21:00
@irvingpop irvingpop changed the title DO NOT MERGE: Jm+ma/habitat wip wip wip DO NOT MERGE: Habitize the Chef Server Nov 9, 2017
Copy link
Contributor

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 ❤️ 💪

Great work so far! Thanks for digging into this. As I mentioned in the comments, I'd encourage those of you who worked on this to consider adding your names to the MAINTAINERS file.

I did an initial read through of this and left inline comments. Overall this looks like the right direction and is much less duplication than I was originally worried about.

I know this is still WIP so hopefully I wasn't /too/ annoying with mentioning things you probably already know about. I assume you'll call for reviewers when this is ready to go, but I was procrastinating so I decided to read it now :D.

Here are some larger things I would like us to consider doing. Some (most?) could happen post-merge as follow up work. These are roughly ordered by how strongly I feel about them.

  • Document what we intend to support in this setup. I'm thinking about: Add-ons, our large number of configurables in the reconfigure cookboks, etc.

  • Document basic configuration and operation. The documentation can mark this as experimental, but we should get the docs started soon to avoid getting too far behind.

  • Move chef-server-ctl to an app-bundled application and out of omnibus/files. This may be what you mean by "import" in your current todo list.

  • A gather-logs style command for producing a tarball for support with relevant info.

  • Review the lua routing code. My sense is that a huge portion of it could be ripped out. This would reduce a lot of the duplication this change introduces and simplify our lives to boot.

  • Take the secrets bootstrapping and erchef data bootstrapping commands you've written here, move them some place where we can share them with the old syle config and use them exclusively.

  • Some sort of travis check to remind us that we now have 2 sets of configuration files. I think keeping the habitat configs separate is the best way forward for now since that allows us to be purposeful in what configuration we support; however, we don't want it to drift so something that reminds us to check for that would be great.

  • A chef_secrets provider that can read directly from the hab ring to avoid passwords in configuration files.

In order to merge this, I think we need to finish (or have dedicated resources scheduled to finish):

[ ] Integration into the build & test pipeline
[ ] Documentation on at least:

  • installation,
  • upgrade (it is OK if we don't know how to handle migrations yet, we can document that once we have a migration to run)
  • data backup

links:
- postgresql
environment:
- FOO=bar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for? I see it a few times below as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what happens when you order the "Never Ending" Copy-Pasta Bowl at the Olive Garden 😂


volumes:
postgresql-data:
erchef-data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No volume for the ES data?

#!/bin/bash

# TODO: not sure how to handle this. Sqitch bombs when it can't find the timezone
export TZ="America/Chicago"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do have to hard code this, maybe set it to UTC.

@@ -0,0 +1,40 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set -e here and in our other shell scripts?

{kernel, [{inet_dist_use_interface, {127,0,0,1}}]},
%% SASL config
{sasl, [
{sasl_error_logger, {file, "{{pkg.svc_var_path}}//logs/sasl-error.log"}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Double / here and in other places throughout the file.

@@ -0,0 +1,16 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-Merge TODO: We should really rip nearly all of these darklaunch flags out. Basically none of them wold work if changed.

@@ -0,0 +1,57 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to a location inside the erchef build so we can easily access it from the old code and the new code?

@@ -0,0 +1 @@
{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What needs this still?

%% be part of a loaded application
{node_license, 999999},
{upgrade_url, <<"http://www.chef.io/contact/on-premises-simple">>},
{actions_host, "127.0.0.1"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want actions permanently off int his setup so maybe we can remove all this stub config?

@@ -0,0 +1,98 @@
[private_chef]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other configs, we should check these for unused variables.

Elliott Davis and others added 25 commits November 17, 2017 10:22
Signed-off-by: Elliott Davis <edavis@chef.io>
The build of this works but the config is still in progress.

Signed-off-by: Elliott Davis <edavis@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Nolan Davidson <ndavidson@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Irving Popovetsky <irving@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
[data_collector_enabled]=data_collector.enabled
[data_collector_server]=data_collector.server
[data_collector_port]=data_collector.port
[data_collector_token]=data_collector.token
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to figure how to just export the top level toml table key [data_collector]=data_collector

whenever I tried it would get:

oc_erchef_1          | hab-sup(MR): Starting jeremymv2/oc_erchef
oc_erchef_1          | thread 'main' panicked at 'Struct should serialize to bytes: ValueAfterTable', /checkout/src/libcore/result.rs:906:4
oc_erchef_1          | note: Run with `RUST_BACKTRACE=1` for a backtrace.

jeremymv2 and others added 15 commits December 7, 2017 20:51
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
…rror in data_collector

Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
When rendering the chef server URI in pedant.rb config files, we want
to include ports (for example when the chef server is behind a
proxy). That creates a problem when we use the standard port, as the
URI gets normalized many places in the chain from pedant to erchef; in
particular redundant port specifications (e.g 443 for https) are
stripped out. However the specs will retain the port, causing test
failures. We normalize the URI here to make sure that the specs we
check against conform to that requirement.

Signed-off-by: Mark Anderson <mark@chef.io>
…lution

Signed-off-by: Jeremy J. Miller <jm@chef.io>
jeremymv2 added a commit to jeremymv2/chef-server that referenced this pull request Feb 14, 2018
This is a branch copy of chef#1416 rebased and with all commits squashed and authors
attributed below.

and are functional.

Co-authored-by: Elliott Davis <edavis@chef.io>
Co-authored-by: Irving Popovetsky <irving@chef.io>
Co-authored-by: Mark Anderson <mark@chef.io>
Co-authored-by: Nolan Davidson <ndavidson@chef.io>
Co-authored-by: Thomas Cate <thomascate@gmail.com>

Signed-off-by: Jeremy J. Miller <jm@chef.io>
jeremymv2 added a commit to jeremymv2/chef-server that referenced this pull request Feb 21, 2018
This is a branch copy of chef#1416 rebased and with all commits squashed and authors
attributed below.

and are functional.

Co-authored-by: Elliott Davis <edavis@chef.io>
Co-authored-by: Irving Popovetsky <irving@chef.io>
Co-authored-by: Mark Anderson <mark@chef.io>
Co-authored-by: Nolan Davidson <ndavidson@chef.io>
Co-authored-by: Thomas Cate <thomascate@gmail.com>

Signed-off-by: Jeremy J. Miller <jm@chef.io>
jeremymv2 added a commit to jeremymv2/chef-server that referenced this pull request Feb 22, 2018
This is a branch copy of chef#1416 rebased and with all commits squashed and authors
attributed below.

Co-authored-by: Elliott Davis <edavis@chef.io>
Co-authored-by: Irving Popovetsky <irving@chef.io>
Co-authored-by: Mark Anderson <mark@chef.io>
Co-authored-by: Nolan Davidson <ndavidson@chef.io>
Co-authored-by: Thomas Cate <thomascate@gmail.com>

Signed-off-by: Jeremy J. Miller <jm@chef.io>
@markan markan closed this Feb 26, 2018
@tas50 tas50 added Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. and removed Do Not Merge labels Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Waiting on Contributor A pull request that has unresolved requested actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants