[nginx] regression: proxied compliance requests get HTTP 404 #1253

stevendanna · 2017-05-04T10:46:23Z

The compliance proxy no longer included the owner or profile name in
the upstream requests because of a bad rewrite rule. This corrects
the rewrite rule.

Testing

Since there are no tests for this endpoint, I've tested it manually
using the following procedure:

Configure /etc/opscode/chef-server.rb as follows:

data_collector['token'] = 'foobar'
profiles['root_url'] = 'http://localhost:9998'

Use socat to start an SSL listener on 9998:

socat OPENSSL-LISTEN:9998,reuseaddr,fork,verify=0,\
cert=/var/opt/opscode/nginx/ca/api.chef-server.dev.crt,\
key=/var/opt/opscode/nginx/ca/api.chef-server.dev.key,\
dhparam=/var/opt/opscode/nginx/ca/dhparams.pem - | grep 'GET'

Use knife raw to make reqeusts and check the URL recieved by (2)

knife raw /organizations/testorg/owners/foobar/compliance/bob -c /etc/opscode/pivotal.rb
knife raw /compliance/organizations/testorg/owners/foobar/compliance/bob -c /etc/opscode/pivotal.rb

Before this change, the following two requests were recieved:

GET /compliance/profiles/ HTTP/1.0
GET /compliance/profiles/ HTTP/1.0

After this change:

GET /compliance/profiles/foobar/bob HTTP/1.0
GET /compliance/profiles/foobar/bob HTTP/1.0

Signed-off-by: Steven Danna steve@chef.io

The compliance proxy no longer included the owner or profile name in the upstream requests because of a bad rewrite rule. This corrects the rewrite rule. *Testing* Since there are no tests for this endpoint, I've tested it manually using the following procedure: 1. Configure /etc/opscode/chef-server.rb as follows: data_collector['token'] = 'foobar' profiles['root_url'] = 'http://localhost:9998' 2. Use `socat` to start an SSL listener on 9998: socat OPENSSL-LISTEN:9998,reuseaddr,fork,verify=0,\ cert=/var/opt/opscode/nginx/ca/api.chef-server.dev.crt,\ key=/var/opt/opscode/nginx/ca/api.chef-server.dev.key,\ dhparam=/var/opt/opscode/nginx/ca/dhparams.pem - | grep 'GET' 3. Use `knife raw` to make reqeusts and check the URL recieved by (2) knife raw /organizations/testorg/owners/foobar/compliance/bob -c /etc/opscode/pivotal.rb knife raw /compliance/organizations/testorg/owners/foobar/compliance/bob -c /etc/opscode/pivotal.rb Before this change, the following two requests were recieved: GET /compliance/profiles/ HTTP/1.0 GET /compliance/profiles/ HTTP/1.0 After this change: GET /compliance/profiles/foobar/bob HTTP/1.0 GET /compliance/profiles/foobar/bob HTTP/1.0 Signed-off-by: Steven Danna <steve@chef.io>

stevendanna · 2017-05-04T10:47:14Z

cc @chef/chef-server-maintainers @chef/compliance-core

This is still WIP as I work out how we want to test this in a more auotmated way, but I'd love for someone to verify those are the redirects we want now.

This adds tests for our nginx proxy config for compliance requests. It starts a stub compliance server on the configured port. If the requests are rewritten as expected, the stub service will return 200. Since this requires the chef-server.rb to contain proxy config, we disable these tests by default, but can enable them in our CI configuration. Signed-off-by: Steven Danna <steve@chef.io>

stevendanna · 2017-05-04T11:53:49Z

Now with tests

stevendanna · 2017-05-04T11:54:39Z

To enable these tests in wilson, we will need to modify the test config in opscode-ci and add --compliance-proxy-tests to https://github.com/chef/chef-server/blob/master/ci/run_tests.sh

marcparadise · 2017-05-04T14:19:53Z

oc-chef-pedant/spec/api/compliance_proxy_spec.rb

+      context "GET /compliance/organizations/ORG/owners/OWNER/compliance/PROFILE" do
+        let(:request_url) { "#{platform.server}/compliance/organizations/#{platform.test_org.name}/owners/foobar/compliance/testprofile" }
+        it "retuns 200" do
+          get(request_url, admin_user).should look_like({:status => 200,


Non-blocking nit: We're moving generally expect instead of should

Moving /very slowly/ it seems :D:

sdanna@thrace ~/oc/code/opscode/chef-server/oc-chef-pedant (ssd/compliance-proxy-regression) > rg '\.should ' spec/ | wc -l 1272 sdanna@thrace ~/oc/code/opscode/chef-server/oc-chef-pedant (ssd/compliance-proxy-regression) > rg 'expect\w*[({]' spec/ | wc -l 252

marcparadise · 2017-05-04T14:29:42Z

...files/private-chef-cookbooks/private-chef/templates/default/nginx/nginx_chef_api_lb.conf.erb

@@ -101,12 +101,12 @@
    #   /organizations/ORG/owners/OWNER/compliance[/PROFILE]
    # Supports the legacy(chef-gate) URLs as well:
    #   /compliance/organizations/ORG/owners/OWNER/compliance[/PROFILE]
-    location ~ (?:/compliance)?/organizations/([^/]+)/owners/([^/]+)/compliance(.*) {
+    location ~ <%= @compliance_proxy_regex -%> {


Can ee keep this in $variable in the config template itself? Or .maybe assign the ruby variable value in here instead of passing it in? I'd prefer to keep this in code , since to my knowledge none of the other expressions come out oof the recipe - this way there's one source of truth.

Unfortunately, it doesn't seem like we can use variables in the location block, unless I've messed up in my local tests.

Signed-off-by: Steven Danna <steve@chef.io>

sdelano · 2017-05-04T15:15:27Z

...files/private-chef-cookbooks/private-chef/templates/default/nginx/nginx_chef_api_lb.conf.erb

      set $request_org $1;
      access_by_lua_block { validator.validate("GET") }
      proxy_set_header x-data-collector-token $data_collector_token;
      proxy_set_header x-data-collector-auth "version=1.0";
-      rewrite ^ "/compliance/profiles/$2$3" break;
+      rewrite ^<%= @compliance_proxy_regex -%> "/compliance/profiles/$2$3" break;


@stevendanna I'd also remove the quotes from the rewritten route. That was another inconsistency that triggered warning signs for me. It's probably fine, but you don't see that pattern anywhere in our code or in nginx docs. I think it's a holdover from translating this bit out of the Lua code.

None of the documented examples use quotes around the second argument: http://nginx.org/en/docs/http/ngx_http_rewrite_module.html Signed-off-by: Steven Danna <steve@chef.io>

stevendanna · 2017-05-04T15:36:09Z

opscode-ci config update https://github.com/chef-cookbooks/opscode-ci/pull/707

Once this is applied, we can additionally add --compliance-proxy-tests to our run_tests.sh script.

sdelano · 2017-05-04T15:42:51Z

I've verified that this is in fact the standard way to rewrite URLs for a proxy. I personally think it's ugly, but oh well.

From: http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_pass

When the URI is changed inside a proxied location using the rewrite directive, and this same configuration will be used to process a request (break):

location /name/ {
    rewrite    /name/([^/]+) /users?name=$1 break;
    proxy_pass http://127.0.0.1;
}

sdelano · 2017-05-04T15:53:46Z

Just for posterity, here are the routes that we expose in automate:

  # GET:  /compliance/profiles
  # GET: /compliance/profiles/available
  # GET:  /compliance/profiles/john/linux
  # GET:  /compliance/profiles/john/linux/tar
  # POST: /compliance/profiles/john

@stevendanna 👀 here to doublecheck that these line up?

FWIW, we might not need the to all line up, since what we're proxying is just profile storage.

stevendanna · 2017-05-04T16:01:47Z

@sdelano Hrm. The previous comment doesn't mention supporting all of those. I think these all work:

  # GET:  /compliance/profiles
  # GET:  /compliance/profiles/john/linux
  # GET:  /compliance/profiles/john/linux/tar

For this one is available a special endpoint? You can get the proxy to generate a request that looks like this, but only if you happen to know how the rewrite happens.

  # GET: /compliance/profiles/available

I don't think the post will pass validation.

sdelano · 2017-05-04T16:07:03Z

@chris-rock @arlimus @vjeffrey - Can any of you chime in here about what needs to be supported by the chef-server proxy?

stevendanna · 2017-05-04T16:08:29Z

Oh, I lied, this one doesn't work /compliance/profiles.

sdelano · 2017-05-04T16:13:23Z

@stevendanna to be clear, I don't think it has to. We saw this change working end-to-end yesterday with the audit cookbook, so I'm fairly sure it works, but since we're now in the process of translating comments to enforceable tests, we might as well make sure we're testing the correct thing.

vjeffrey · 2017-05-04T16:24:17Z

k all those endpoints you mentioned need to be supported; ie what's here: https://github.com/chef/automate/blob/master/cookbooks/delivery/templates/default/nginx-internal.conf.erb#L52

 # GET:  /compliance/profiles                => /user/compliance
  # GET: /compliance/profiles/available => /profiles
  # GET:  /compliance/profiles/john/linux     => /owners/john/compliance/linux
  # GET:  /compliance/profiles/john/linux/tar => /owners/john/compliance/linux/tar
  # POST: /compliance/profiles/john           => /owners/john/compliance

(in the next week that's going to be changing to something more along the lines of https://github.com/chef/automate/blob/master/nginx/conf/nginx.conf#L114)

sdelano · 2017-05-04T16:25:16Z

@vjeffrey do they need to be supported by the Chef Server, which IIRC is only supporting the audit cookbook?

vjeffrey · 2017-05-04T16:26:39Z

oh geez. we're getting to the edges of my knowledge base atm....
i'm pretty sure what ya'll have is fine...let me ping chris so i don't mess things up lol

chris-rock · 2017-05-04T16:39:24Z

I would prefer to keep the Chef Server dump and add additional routes to automate. But we can go with the current approach for now. I have not tested it in a real setup though.

sdelano · 2017-05-04T16:41:42Z

@chris-rock last time we chatted about this you made a story in compliance to address it on the automate end: https://github.com/chef/chef-compliance/issues/1171

We're going to move forward with this change.

stevendanna changed the title ~~[nginx] regression: proxied compliance requests get HTTP 404~~ WIP: [nginx] regression: proxied compliance requests get HTTP 404 May 4, 2017

stevendanna changed the title ~~WIP: [nginx] regression: proxied compliance requests get HTTP 404~~ [nginx] regression: proxied compliance requests get HTTP 404 May 4, 2017

marcparadise requested changes May 4, 2017

View reviewed changes

stevendanna force-pushed the ssd/compliance-proxy-regression branch from e26b81a to e532aaf Compare May 4, 2017 15:11

[pedant] Use modern rspec style

0f0e806

Signed-off-by: Steven Danna <steve@chef.io>

stevendanna force-pushed the ssd/compliance-proxy-regression branch from e532aaf to 0f0e806 Compare May 4, 2017 15:14

sdelano suggested changes May 4, 2017

View reviewed changes

[nginx] Don't quote the second argument to rewrite

67fe1aa

None of the documented examples use quotes around the second argument: http://nginx.org/en/docs/http/ngx_http_rewrite_module.html Signed-off-by: Steven Danna <steve@chef.io>

sdelano approved these changes May 4, 2017

View reviewed changes

marcparadise approved these changes May 4, 2017

View reviewed changes

sdelano merged commit cbe484a into master May 4, 2017

arlimus deleted the ssd/compliance-proxy-regression branch May 5, 2017 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nginx] regression: proxied compliance requests get HTTP 404 #1253

[nginx] regression: proxied compliance requests get HTTP 404 #1253

stevendanna commented May 4, 2017

stevendanna commented May 4, 2017

stevendanna commented May 4, 2017

stevendanna commented May 4, 2017

marcparadise May 4, 2017

stevendanna May 4, 2017

marcparadise May 4, 2017

stevendanna May 4, 2017

sdelano May 4, 2017

stevendanna commented May 4, 2017 •

edited

Loading

sdelano commented May 4, 2017 •

edited

Loading

sdelano commented May 4, 2017 •

edited

Loading

stevendanna commented May 4, 2017

sdelano commented May 4, 2017

stevendanna commented May 4, 2017

sdelano commented May 4, 2017

vjeffrey commented May 4, 2017 •

edited

Loading

sdelano commented May 4, 2017

vjeffrey commented May 4, 2017

chris-rock commented May 4, 2017

sdelano commented May 4, 2017

[nginx] regression: proxied compliance requests get HTTP 404 #1253

[nginx] regression: proxied compliance requests get HTTP 404 #1253

Conversation

stevendanna commented May 4, 2017

stevendanna commented May 4, 2017

stevendanna commented May 4, 2017

stevendanna commented May 4, 2017

marcparadise May 4, 2017

Choose a reason for hiding this comment

stevendanna May 4, 2017

Choose a reason for hiding this comment

marcparadise May 4, 2017

Choose a reason for hiding this comment

stevendanna May 4, 2017

Choose a reason for hiding this comment

sdelano May 4, 2017

Choose a reason for hiding this comment

stevendanna commented May 4, 2017 • edited Loading

sdelano commented May 4, 2017 • edited Loading

sdelano commented May 4, 2017 • edited Loading

stevendanna commented May 4, 2017

sdelano commented May 4, 2017

stevendanna commented May 4, 2017

sdelano commented May 4, 2017

vjeffrey commented May 4, 2017 • edited Loading

sdelano commented May 4, 2017

vjeffrey commented May 4, 2017

chris-rock commented May 4, 2017

sdelano commented May 4, 2017

stevendanna commented May 4, 2017 •

edited

Loading

sdelano commented May 4, 2017 •

edited

Loading

sdelano commented May 4, 2017 •

edited

Loading

vjeffrey commented May 4, 2017 •

edited

Loading