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

fix: Move Prometheus validation stanza to local schema #437

Merged

Conversation

claudio-benfatto
Copy link
Contributor

@claudio-benfatto claudio-benfatto commented May 14, 2020

Related issue

Prometheus endpoint cannot be validated by local json schema

Gist of the issue

The current v0.38.0-beta.2 release has introduced prometheus metrics and the following configuration stanza:

serve:
  prometheus:
    port: 9000
    host: localhost
    metrics_path: /metrics

however the local config.schema.json does not contain it.
This is also the schema that is loaded on startup root.go which causes the command to fail with a validation error:

INFO[0000] Config file loaded successfully.              path=/etc/auth-oathkeeper/templates/oathkeeper.yaml
FATA[0000] The configuration is invalid and could not be loaded.  [config_key=serve]="additionalProperties \"prometheus\" not allowed" config_file=/etc/auth-oathkeeper/templates/oathkeeper.yaml

Proposed changes

The proposed change is to add the prometheus validation to the local schema

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

I tested the compiled binary against an example configuration containing the prometheus stanza and everything works correctly

@CLAassistant
Copy link

CLAassistant commented May 14, 2020

CLA assistant check
All committers have signed the CLA.

@claudio-benfatto claudio-benfatto force-pushed the move_prometheus_stanza_to_local_schema branch from ac10329 to 83a8126 Compare May 14, 2020 09:05
@claudio-benfatto claudio-benfatto changed the title move prometheus validation stanza to local schema fix: Move Prometheus validation stanza to local schema May 14, 2020
@aeneasr
Copy link
Member

aeneasr commented May 14, 2020

Thank you! To prevent future regressions it would be great to have some tests for this, namely here. You can define the values over here. :)

Add prometheus config validation stanza to local
.schema/config.schema.json file
@claudio-benfatto claudio-benfatto force-pushed the move_prometheus_stanza_to_local_schema branch from 83a8126 to 99f9fb3 Compare May 14, 2020 15:32
@claudio-benfatto
Copy link
Contributor Author

Thank you! To prevent future regressions it would be great to have some tests for this, namely here. You can define the values over here. :)

Hi @aeneasr thanks a lot for pointing me to the correct locations for the tests!

@aeneasr
Copy link
Member

aeneasr commented May 16, 2020

No problem :) This helps me so much when other maintainers do it so I try to replicate it :)

@aeneasr aeneasr merged commit dcf3e14 into ory:master May 20, 2020
@aeneasr
Copy link
Member

aeneasr commented May 20, 2020

Thank you and sorry for the late merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants