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

test, oauth2: Make sure config test runs field validation #13496

Merged
merged 12 commits into from
Oct 16, 2020
Merged

test, oauth2: Make sure config test runs field validation #13496

merged 12 commits into from
Oct 16, 2020

Conversation

dio
Copy link
Member

@dio dio commented Oct 11, 2020

Commit Message: This makes sure config tests for oauth2 filter checks field validations. This also handles possible nullptr values for token_secret and hmac_secret fields due to misconfigurations (for example: token_secret: {}). The example config in the docs is also updated.

Risk Level: Low
Testing: Updated
Docs Changes: Updated example config
Release Notes: N/A
Fixes #13023

Signed-off-by: Dhi Aurrahman dio@tetrate.io

This makes sure config tests for oauth2 filter checks fields
validations. This also handles nullptr values for token_secret and
hmac_secret fields due to misconfigurations (for example: token_secret:
{}). Example config in the docs is also updated.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few comments

@@ -81,7 +81,7 @@ class HttpFilterNameValues {
// AWS Lambda filter
const std::string AwsLambda = "envoy.filters.http.aws_lambda";
// OAuth filter
const std::string OAuth = "envoy.filters.http.oauth";
const std::string OAuth = "envoy.filters.http.oauth2";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this safe to change? I think we prefer to use the proto type to identify the filter to create, so I think yes?

Copy link
Member Author

@dio dio Oct 12, 2020

Choose a reason for hiding this comment

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

Yes. This is for consistency since we print the name of extensions at startup, i.e. [info][main] [external/envoy/source/server/server.cc:309]: envoy.filters.http: ...

test/extensions/filters/http/oauth2/config_test.cc Outdated Show resolved Hide resolved
test/extensions/filters/http/oauth2/config_test.cc Outdated Show resolved Hide resolved
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Oct 14, 2020

@snowp I think have updated the patch following your comments. Please help to take a look. Thank you!

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Oct 14, 2020

My bad. One of the tests was failed since missing secret names.

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

@snowp
Copy link
Contributor

snowp commented Oct 16, 2020

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13496 (comment) was created by @snowp.

see: more, trace.

@mattklein123 mattklein123 merged commit 9dce187 into envoyproxy:master Oct 16, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 17, 2020
* master: (22 commits)
  delay health checks until transport socket secrets are ready. (envoyproxy#13516)
  test, oauth2: Make sure config test runs field validation (envoyproxy#13496)
  [http] swap codec implementations to default new (envoyproxy#13579)
  wasm: update proxy-wasm-cpp-host (envoyproxy#13606)
  postgres: do not copy and linearize received data when it is not going to be used (envoyproxy#13393)
  configs: Update configs v2 -> v3 (envoyproxy#13562)
  http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling (envoyproxy#13546)
  dependencies: track untracked implied dependencies, wrapup dashboard. (envoyproxy#13571)
  listener: add match all filter chain (envoyproxy#13449)
  fix mistakes in docstrings (envoyproxy#13603)
  ratelimit: add route entry metadata to ratelimit actions (envoyproxy#13269)
  cluster manager: avoid immediate activation for dynamic inserted cluster when initialize (envoyproxy#12783)
  ext_authz: Avoid calling check multiple times (envoyproxy#13288)
  docs: Unexclude remaining configs from validation (envoyproxy#13534)
  build: update rules_rust to allow Rustc in RBE (envoyproxy#13595)
  docs: Update sphinxext.rediraffe (envoyproxy#13589)
  Deprecate moonjit support on Windows before beta (envoyproxy#13541)
  dependencies: bump LuaJIT to 2.1 branch HEAD @ e9af1ab. (envoyproxy#13474)
  docs: add TLS stats to cluster stats doc (envoyproxy#13561)
  ci: stop building alpine-debug images in favor of ubuntu-based debug image (envoyproxy#13598)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
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.

Oauth2 extention not work
3 participants