diff --git a/.github/workflows/openapi_conformance.yml b/.github/workflows/openapi_conformance.yml new file mode 100644 index 00000000000..0a3572be026 --- /dev/null +++ b/.github/workflows/openapi_conformance.yml @@ -0,0 +1,102 @@ +# Check the traffic our request specs already generate against the OpenAPI +# description in docs/openapi, and report what it finds. +# +# Informational: the description already disagrees with the API in a handful +# of places, so the run publishes a digest rather than failing. Dispatch it +# with `strict` to see what gating would do. +name: OpenAPI Conformance + +concurrency: + group: '${{ github.workflow }}-${{ github.head_ref || github.run_id }}' + cancel-in-progress: true + +on: + workflow_dispatch: + inputs: + strict: + description: 'Fail the job on a non-conforming response' + type: boolean + default: false + pull_request: + branches: [ main ] + paths: + - 'app/**' + - 'lib/**' + - 'docs/openapi/**' + - 'spec/request/**' + - 'spec/support/openapi_conformance.rb' + - 'spec/support/request_spec_helper.rb' + - '.github/workflows/openapi_conformance.yml' + +permissions: + contents: read + +jobs: + Conformance: + name: Request specs vs. OpenAPI description + runs-on: ubuntu-latest + timeout-minutes: 45 + services: + postgres: + image: postgres:17 + env: + POSTGRES_PASSWORD: rootpassword + # Test-only durability tuning: skip the work that backs durability. + command: >- + postgres + -c fsync=off + -c synchronous_commit=off + -c full_page_writes=off + options: >- + --tmpfs /var/lib/postgresql/data:rw,size=2g + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + ports: + - 5432:5432 + env: + DB: postgres + POSTGRES_CONNECTION_PREFIX: postgres://postgres:rootpassword@localhost:5432 + OPENAPI_CONFORMANCE: ${{ (github.event_name == 'workflow_dispatch' && inputs.strict) && 'strict' || '1' }} + steps: + - uses: actions/checkout@v7 + - uses: ./.github/workflows/composite/setup + - uses: actions/setup-node@v7 + with: + node-version: 24 + + # openapi_first takes a single file, so the multi-file redocly source + # has to be bundled before the specs can read it. + - name: Build the API description + working-directory: docs/openapi + run: | + yarn install --frozen-lockfile + yarn build + + - name: Prepare the database + run: bundle exec rake db:recreate + + - name: Run request specs against the description + run: bundle exec rspec spec/request --format progress + + - name: Publish the report + if: always() + run: | + if [ -f out/openapi_conformance.md ]; then + cat out/openapi_conformance.md >> "$GITHUB_STEP_SUMMARY" + else + echo '## OpenAPI conformance' >> "$GITHUB_STEP_SUMMARY" + echo 'No report produced -- the run failed before the specs finished.' >> "$GITHUB_STEP_SUMMARY" + fi + + - name: Upload the report + if: always() + uses: actions/upload-artifact@v7 + with: + name: openapi-conformance + path: | + out/openapi_conformance.md + out/openapi_coverage.html + if-no-files-found: warn + retention-days: 7 diff --git a/Gemfile b/Gemfile index 5a1befeebc5..43169123c47 100644 --- a/Gemfile +++ b/Gemfile @@ -51,6 +51,7 @@ end group :test do gem 'factory_bot', '~> 6.5' gem 'mock_redis' + gem 'openapi_first' gem 'parallel_tests' gem 'rack-test' gem 'rspec', '~> 3.13.2' diff --git a/Gemfile.lock b/Gemfile.lock index 8a9c6a7313c..8211a15a9db 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -109,6 +109,7 @@ GEM grpc (1.84.0-x86_64-linux-gnu) google-protobuf (>= 3.25, < 5.0) googleapis-common-protos-types (~> 1.0) + hana (1.3.7) hashdiff (1.2.1) highline (3.0.1) httpclient (2.9.0) @@ -127,6 +128,11 @@ GEM json-schema (6.2.0) addressable (~> 2.8) bigdecimal (>= 3.1, < 5) + json_schemer (2.5.0) + bigdecimal + hana (~> 1.3) + regexp_parser (~> 2.0) + simpleidn (~> 0.2) kramdown (2.5.2) rexml (>= 3.4.4) kramdown-parser-gfm (1.1.0) @@ -174,6 +180,11 @@ GEM bigdecimal (>= 3.0) ostruct (>= 0.2) open3 (0.2.1) + openapi_first (4.0.1) + drb (~> 2.0) + hana (~> 1.3) + json_schemer (>= 2.1, < 3.0) + rack (>= 2.2, < 4.0) openssl (4.0.2) ostruct (0.6.3) parallel (2.3.0) @@ -353,6 +364,7 @@ GEM sequel (>= 4.38.0) sexp_processor (4.17.6) simplecov (1.3.2) + simpleidn (0.3.0) sinatra (4.2.1) logger (>= 1.6.0) mustermann (~> 3.0) @@ -462,6 +474,7 @@ DEPENDENCIES newrelic_rpm nokogiri (>= 1.10.5) oj + openapi_first openssl (>= 3.2) parallel_tests pg diff --git a/docs/openapi/README.md b/docs/openapi/README.md index 95ced01ebaa..2a3dcbf6eed 100644 --- a/docs/openapi/README.md +++ b/docs/openapi/README.md @@ -112,6 +112,45 @@ This is useful for: - Validating a mock server's implementation against the OpenAPI spec. - Quickly checking a live API for compliance without running the full `capi-bara-tests` suite. +## Checking the description against the request specs + +The Cloud Controller's rspec request specs drive the real rack app, so every +example is already a request/response pair that this description either covers +or doesn't. `spec/support/openapi_conformance.rb` wraps that app with +[openapi_first](https://rubygems.org/gems/openapi_first) and records the +verdict -- no deployed CF, no proxy, no traffic capture. + +It is off unless `OPENAPI_CONFORMANCE` is set, and it reads the bundled +description, so build that first: + +```bash +cd docs/openapi && yarn install && yarn build +cd ../.. && DB=postgres OPENAPI_CONFORMANCE=1 bundle exec rspec spec/request +``` + +That writes `out/openapi_conformance.md` (a digest of coverage and violations) +and `out/openapi_coverage.html` (every route, with the ones nothing exercised). +`OPENAPI_CONFORMANCE=strict` turns a non-conforming response into a spec +failure instead of a line in the report. + +Violations reading `does not match format: uuid` are counted but not listed. +Around 250 places in `spec/request` hand a model a literal guid such as +`'app1_guid'` and then assert on it, so that check only has signal against +recorded traffic, not against fixtures. + +5xx responses are left out of coverage. They are described on nearly every +operation, but request specs almost never produce them, so they only drag the +number down; the HTML report lists them as skipped. A skipped response isn't +tracked, so a malformed 5xx body won't show up as a violation either. + +The `OpenAPI Conformance` GitHub Action runs this on pull requests and +publishes the digest to the job summary. The step log carries the same digest +plus, in a collapsed "OpenAPI coverage details" group, every route that isn't +fully covered -- the HTML report is also uploaded as an artifact, but there is +no need to download it. It is informational -- the +description already disagrees with the API in a handful of places, so there is +nothing honest to gate on yet. + ## Contributing Contributions are welcome! Please feel free to submit a pull request or open an issue to discuss any changes. diff --git a/docs/openapi/apis/cf/latest/components/schemas/AppUsageEvent.yaml b/docs/openapi/apis/cf/latest/components/schemas/AppUsageEvent.yaml index 5cf0cb50fe2..0a71f28d149 100644 --- a/docs/openapi/apis/cf/latest/components/schemas/AppUsageEvent.yaml +++ b/docs/openapi/apis/cf/latest/components/schemas/AppUsageEvent.yaml @@ -15,7 +15,7 @@ properties: type: object properties: guid: - type: string + type: [string, "null"] description: Unique identifier of the app that this event pertains to, if applicable name: type: [string, "null"] @@ -57,7 +57,7 @@ properties: type: object properties: guid: - type: string + type: [string, "null"] description: Unique identifier of the task that this event pertains to, if applicable name: type: [string, "null"] diff --git a/docs/openapi/apis/cf/latest/components/schemas/Build.yaml b/docs/openapi/apis/cf/latest/components/schemas/Build.yaml index 1f0286c2f0f..ae9ab4da14b 100644 --- a/docs/openapi/apis/cf/latest/components/schemas/Build.yaml +++ b/docs/openapi/apis/cf/latest/components/schemas/Build.yaml @@ -55,7 +55,7 @@ allOf: type: object properties: guid: - type: string + type: [string, "null"] description: The guid of the user that created the build name: type: [string, "null"] diff --git a/docs/openapi/apis/cf/latest/components/schemas/Deployment.yaml b/docs/openapi/apis/cf/latest/components/schemas/Deployment.yaml index ce00d593125..08cde5625c4 100644 --- a/docs/openapi/apis/cf/latest/components/schemas/Deployment.yaml +++ b/docs/openapi/apis/cf/latest/components/schemas/Deployment.yaml @@ -80,7 +80,7 @@ allOf: type: object properties: guid: - type: string + type: [string, "null"] format: uuid description: "The app\u2019s [current droplet guid](#get-current-droplet-association-for-an-app) before the deployment was created" new_processes: diff --git a/docs/openapi/apis/cf/latest/components/schemas/RelationshipToOne.yaml b/docs/openapi/apis/cf/latest/components/schemas/RelationshipToOne.yaml index 89f05cdb347..f9f22a153a3 100644 --- a/docs/openapi/apis/cf/latest/components/schemas/RelationshipToOne.yaml +++ b/docs/openapi/apis/cf/latest/components/schemas/RelationshipToOne.yaml @@ -2,7 +2,11 @@ type: object properties: data: type: [object, "null"] - $ref: './Relationship.yaml' + properties: + guid: + type: string + format: uuid + description: The GUID of the resource links: type: object properties: diff --git a/docs/openapi/apis/cf/latest/paths/Processes.yaml b/docs/openapi/apis/cf/latest/paths/Processes.yaml index 278fa7f5eed..00dfb1dd1cc 100644 --- a/docs/openapi/apis/cf/latest/paths/Processes.yaml +++ b/docs/openapi/apis/cf/latest/paths/Processes.yaml @@ -236,8 +236,7 @@ - STARTING - DOWN since: - type: number - format: double + type: integer links: type: object properties: diff --git a/spec/request/processes_spec.rb b/spec/request/processes_spec.rb index fcceb53eb90..9f6aaf29bf0 100644 --- a/spec/request/processes_spec.rb +++ b/spec/request/processes_spec.rb @@ -679,7 +679,7 @@ end let(:instances_reporters) { double(:instances_reporters) } - let(:usage_time) { Time.now.utc.to_s } + let(:usage_time) { Time.now.utc.to_datetime.rfc3339 } let(:expected_response) do { diff --git a/spec/support/openapi_conformance.rb b/spec/support/openapi_conformance.rb new file mode 100644 index 00000000000..a16184dcf88 --- /dev/null +++ b/spec/support/openapi_conformance.rb @@ -0,0 +1,189 @@ +# frozen_string_literal: true + +# Spike: check the traffic our request specs already generate against the +# OpenAPI description in docs/openapi. +# +# The request specs drive the real rack app through rack-test, so every +# example is a request/response pair the description either covers or +# doesn't. openapi_first wraps that app and records the verdict, which gives +# us contract checking without a deployed CF, a proxy, or a traffic capture. +# +# Off unless OPENAPI_CONFORMANCE is set, so `rake spec` is untouched -- the +# gem isn't even required. +# +# OPENAPI_CONFORMANCE=1 record violations, fail nothing +# OPENAPI_CONFORMANCE=strict raise on the first request/response that +# doesn't conform +# +# It reads the bundled description: openapi_first takes a single file and +# docs/openapi is a multi-file redocly source, so `yarn build` has to run first. +module OpenapiConformance + DESCRIPTION = File.expand_path('../../docs/openapi/dist/latest/openapi.yaml', __dir__) + OUT_DIR = File.expand_path('../../out', __dir__) + HTML_REPORT = File.join(OUT_DIR, 'openapi_coverage.html') + MARKDOWN_REPORT = File.join(OUT_DIR, 'openapi_conformance.md') + + # Rows past this get cut from the markdown -- the HTML report has them all, + # and a step summary that long is unreadable anyway. + MAX_ROWS = 50 + + # `format: uuid` failures are ours, not the API's: 248 places in + # spec/request hand a model a literal guid like 'app1_guid', and specs + # assert on those literals, so they can't just be swapped for real UUIDs. + # Real Cloud Controller guids are SecureRandom.uuid, so this check only has + # signal against recorded traffic, not against fixtures. Counted, not listed. + UUID_FORMAT_ERROR = /does not match format: uuid/ + + # Only suppress when every clause is a uuid complaint -- a compound error + # like "... format: uuid. value at `/x` is not an object" still matters. + def self.uuid_noise?(message) + return false if message.nil? + + clauses = message.sub(/\A(Request body invalid|Response body is invalid|Path segment is invalid): /, '').split('. ') + clauses.all? { |clause| clause.match?(UUID_FORMAT_ERROR) } + end + + # openapi_first takes one reporter. This one writes the HTML report to read + # after the fact, plus a short markdown digest for the CI log and the job + # summary. The gem's own terminal reporter prints every route not fully + # covered -- around a thousand lines -- so it only runs in CI, folded away. + class Reporter + def initialize(**_options); end + + def report(coverage_result) + FileUtils.mkdir_p(OUT_DIR) + OpenapiFirst::Test::Coverage::HtmlReporter.new(output: HTML_REPORT).report(coverage_result) + + markdown = render(coverage_result) + File.write(MARKDOWN_REPORT, markdown) + puts markdown + print_details(coverage_result) if ENV['GITHUB_ACTIONS'] == 'true' + end + + private + + # The per-route detail otherwise only exists in the HTML artifact. In + # GitHub Actions it goes into a collapsed log group, so it can be read in + # the job log without downloading anything and doesn't bury the digest. + # Local runs skip it: the HTML report is right there in out/. + def print_details(coverage_result) + puts '::group::OpenAPI coverage details (every route not fully covered)' + OpenapiFirst::Test::Coverage::TerminalReporter.new.report(coverage_result) + puts '::endgroup::' + end + + def render(coverage_result) + lines = ["## OpenAPI conformance\n"] + coverage_result.plans.each { |plan| lines.concat(render_plan(plan)) } + lines.join("\n") + end + + def render_plan(plan) + exercised = plan.routes.count { |route| route.requests.any?(&:requested?) } + violations, suppressed = partition_violations(plan) + + lines = [ + "- Coverage: **#{plan.coverage.round(2)}%**", + "- Described routes exercised: **#{exercised}/#{plan.routes.size}**", + "- Responses skipped (5xx): #{plan.skipped_responses_count}", + "- Conformance violations: **#{violations.size + suppressed.size}**" + ] + lines << " (#{suppressed.size} of them `format: uuid` vs. synthetic test guids, not listed)" if suppressed.any? + lines << '' + return lines if violations.empty? + + lines << '| Route | What | Error |' + lines << '| --- | --- | --- |' + violations.first(MAX_ROWS).each do |route, what, error| + lines << "| #{route} | #{what} | #{error} |" + end + lines << "\n_#{violations.size - MAX_ROWS} more omitted; see the HTML report._" if violations.size > MAX_ROWS + lines + end + + # A violation is something the specs actually exercised where every + # attempt was rejected. Routes nothing touched are a coverage gap, not a + # contract breach, and they are the overwhelming majority right now. + def violations(plan) + plan.routes.flat_map do |route| + label = "`#{route.request_method.upcase} #{route.path}`" + bad_requests = route.requests.select { |req| req.requested? && !req.any_valid_request? } + bad_responses = route.responses.select { |res| res.responded? && !res.any_valid_response? } + + bad_requests.map { |req| [label, 'request', req.last_error_message] } + + bad_responses.map { |res| [label, "response #{res.status}", res.last_error_message] } + end + end + + def partition_violations(plan) + violations(plan).partition { |(_route, _what, error)| !OpenapiConformance.uuid_noise?(error) } + end + end + + class << self + def enabled? + !ENV['OPENAPI_CONFORMANCE'].to_s.empty? + end + + def strict? + ENV['OPENAPI_CONFORMANCE'] == 'strict' + end + + # Wraps the app the request specs drive. Returns it untouched when the + # check is off, so a normal run pays nothing. + def wrap(rack_app) + return rack_app unless enabled? + + OpenapiFirst::Test.app(rack_app, api: :cloud_controller) + end + + def setup! + return unless enabled? + + unless File.exist?(DESCRIPTION) + raise "OPENAPI_CONFORMANCE is set but #{DESCRIPTION} is missing. " \ + 'Build it first: cd docs/openapi && yarn install && yarn build' + end + + require 'openapi_first' + + OpenapiFirst::Test.setup do |test| + test.register(DESCRIPTION, as: :cloud_controller) + + # The description covers part of the API, so the request specs hit + # plenty of endpoints it says nothing about. That is the description + # being incomplete, not the app being wrong. + test.ignore_unknown_requests = true + + # Default is to turn a non-conforming request or response into a spec + # failure. Off unless asked, so one violation doesn't hide the rest of + # the picture -- and so this can be added without breaking the suite. + if strict? + # Even here the uuid complaints stay out of the way -- they are test + # fixtures, so failing on them would make strict mode unusable. + test.ignore_request_error { |request| uuid_noise?(request.error&.message) } + test.ignore_response_error { |response, _rack_request| uuid_noise?(response.error&.message) } + else + test.ignore_request_error { true } + test.response_raise_error = false + end + + # 5xx responses are described on nearly every operation, but request + # specs almost never produce them -- 245 of the 950 untested responses + # in the first run. Skipping takes them out of the coverage number and + # the report shows them as skipped rather than untested. The cost: a + # skipped response isn't tracked at all, so a malformed 5xx body would + # not show up as a violation. + test.skip_response_coverage { |response| response.status.to_s.start_with?('5') } + + # :warn reports without the exit 2 that an incomplete description + # would otherwise trigger. Finding out what the number actually is is + # the point of the spike. + test.report_coverage = :warn + test.coverage_reporter = Reporter + end + end + end +end + +OpenapiConformance.setup! diff --git a/spec/support/request_spec_helper.rb b/spec/support/request_spec_helper.rb index e19b0be76c3..5cc66c4f511 100644 --- a/spec/support/request_spec_helper.rb +++ b/spec/support/request_spec_helper.rb @@ -5,6 +5,9 @@ def app test_config = TestConfig.config_instance request_metrics = VCAP::CloudController::Metrics::RequestMetrics.new request_logs = VCAP::CloudController::Logs::RequestLogs.new(Steno.logger('request.logs')) - VCAP::CloudController::RackAppBuilder.new.build(test_config, request_metrics, request_logs) + rack_app = VCAP::CloudController::RackAppBuilder.new.build(test_config, request_metrics, request_logs) + + # A no-op unless OPENAPI_CONFORMANCE is set. See openapi_conformance.rb. + OpenapiConformance.wrap(rack_app) end end