Skip to content

sso_proxy: update to use go-micro for configuration management #279

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.

Sign up for GitHub

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

Jump to bottom
Merged
Jusshersmith merged 1 commit into from
Apr 9, 2021
Merged

sso_proxy: update to use go-micro for configuration management #279

Jusshersmith merged 1 commit into from
Apr 9, 2021

Conversation

@Jusshersmith
Copy link
Contributor

@Jusshersmith Jusshersmith commented Jan 13, 2020 •
edited
Loading

Problem

sso_auth was migrated to use go-micro in #212 to manage variable configuration. sso_proxy is still using the original outdated methods and should be updated to also use go-micro

Solution

Update sso_proxy to use go-micro. No feature changes are intended to be part of this change, apart from the behaviour of the following two variables:

  • SKIP_AUTH_PREFLIGHT and PASS_ACCESS_TOKEN are now upstream specific and should be specified as part of upstream configs, not global/environment variables.

In general, the patterns used in this pull request are the same ones used in #212

Review notes

Some notes to support reviewing:

  • The bulk of changes here consist of creating the new configuration structure, and altering functions and methods to reference that new structure.

  • Numerous functions have been moved out of oauthproxy.go and into options.go:

    • SetCookieStore
    • SetRequestSigner
    • SetUpstreamConfig
    • SetProxyHandler
    • SetValidators
    • SetProvider

  • A new function, SetUpstreamConfigs, largely consists of some logic that was in the options.Validate method.

The below table shows all old variables, and their new equivalent version.

old new
session configs
COOKIE_NAME SESSION_COOKIE_NAME
COOKIE_SECRET SESSION_COOKIE_SECRET
COOKIE_EXPIRE SESSION_COOKIE_EXPIRE
COOKIE_DOMAIN SESSION_COOKIE_DOMAIN
COOKIE_HTTP_ONLY SESSION_COOKIE_HTTPONLY
COOKIE_SECURE SESSION_COOKIE_SECURE
SESSION_LIFETIME_TTL SESSION_TTL_LIFETIME
SESSION_VALID_TTL SESSION_TTL_VALID
GRACE_PERIOD_TTL SESSION_TTL_GRACEPERIOD
---
upstream configs
DEFAULT_ALLOWED_EMAIL_DOMAINS UPSTREAM_DEFAULT_EMAIL_DOMAINS
DEFAULT_ALLOWED_EMAIL_ADDRESSES UPSTREAM_DEFAULT_EMAIL_ADDRESSES
DEFAULT_ALLOWED_GROUPS UPSTREAM_DEFAULT_GROUPS
DEFAULT_UPSTREAM_TIMEOUT UPSTREAM_DEFAULT_TIMEOUT
DEFAULT_UPSTREAM_TCP_RESET_DEADLINE UPSTREAM_DEFAULT_RESETDEADLINE
UPSTREAM_CONFIGS UPSTREAM_CONFIGFILE
CLUSTER UPSTREAM_CLUSTER
SCHEME UPSTREAM_SCHEME
PROVIDER UPSTREAM_DEFAULT_PROVIDER
SKIP_AUTH_PREFLIGHT now configured in upstream configs
PASS_ACCESS_TOKEN now configured in upstream configs
---
server configs
PORT SERVER_PORT
SHUTDOWN_TIMEOUT SERVER_TIMEOUT_SHUTDOWN
TCP_READ_TIMEOUT SERVER_TIMEOUT_READ
TCP_WRITE_TIMEOUT SERVER_TIMEOUT_WRITE
---
metrics configs
STATSD_PORT METRICS_STATSD_PORT
STATSD_HOST METRICS_STATSD_HOST
---
logging configs
REQUEST_LOGGING LOGGING_ENABLE
LOGGING_LEVEL
---
client configs
CLIENT_ID CLIENT_ID
CLIENT_SECRET CLIENT_SECRET
---
request signature configs
REQUEST_SIGNATURE_KEY REQUESTSIGNER_KEY
---
provider configs
PROVIDER PROVIDER_TYPE
PROVIDER_URL PROVIDER_URL_EXTERNAL
PROVIDER_URL_INTERNAL PROVIDER_URL_INTERNAL
SCOPE PROVIDER_SCOPE

@Jusshersmith Jusshersmith changed the title sso_proxy: update to use go-micro for configuration management. sso_proxy: update to use go-micro for configuration management Jan 13, 2020
@Jusshersmith Jusshersmith force-pushed the jusshersmith-sso-proxy-go-config branch from 2e1df69 to 03bcb1f Compare February 24, 2020 23:53
@codecov
Copy link

codecov bot commented Feb 24, 2020 •
edited
Loading

Codecov Report

Merging #279 (af24c7b) into master (d2e1ee5) will increase coverage by 0.89%.
The diff coverage is 83.85%.

❗ Current head af24c7b differs from pull request most recent head d5a7f80. Consider uploading reports for the commit d5a7f80 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   61.84%   62.73%   +0.89%     
==========================================
  Files          57       58       +1     
  Lines        4188     4286      +98     
==========================================
+ Hits         2590     2689      +99     
+ Misses       1388     1382       -6     
- Partials      210      215       +5
Impacted Files Coverage Δ
internal/proxy/logging_handler.go 16.32% <0.00%> (ø)
internal/proxy/proxy.go 16.00% <0.00%> (-2.61%) ⬇️
internal/proxy/proxy_config.go 76.21% <ø> (ø)
internal/proxy/options.go 82.07% <85.00%> (-1.52%) ⬇️
internal/proxy/configuration.go 89.80% <89.80%> (ø)
internal/proxy/metrics.go 86.20% <100.00%> (ø)
internal/proxy/oauthproxy.go 54.26% <100.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3c8a2a...d5a7f80. Read the comment docs.

@Jusshersmith Jusshersmith force-pushed the jusshersmith-sso-proxy-go-config branch from 46341b9 to 63a5a1a Compare March 3, 2020 00:08
@Jusshersmith Jusshersmith mentioned this pull request Mar 3, 2020
Merged
@Jusshersmith Jusshersmith force-pushed the jusshersmith-sso-proxy-go-config branch 5 times, most recently from fc55d98 to 81b0d2d Compare July 27, 2020 13:30
@Jusshersmith Jusshersmith force-pushed the jusshersmith-sso-proxy-go-config branch from fbc8357 to 337884d Compare August 6, 2020 10:41
@Jusshersmith Jusshersmith marked this pull request as ready for review August 6, 2020 17:58
@Jusshersmith
Copy link
Contributor Author

Jusshersmith commented Aug 11, 2020 •
edited
Loading

Any thoughts on the naming and/or structure of configuration variables themselves would be appreciated. I've tried to migrate all config variables while maintaining a tidy, logical structure..but that's not always straight forward.

Jusshersmith
Jusshersmith commented Aug 14, 2020
View reviewed changes
Integralist
Integralist previously approved these changes Aug 14, 2020
View reviewed changes
Copy link

@Integralist Integralist left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to approve although this was difficult for me to review.

I would personally have preferred mulitple PRs for the sake of the various refactoring work (e.g. one PR to move loads of code from one file to another, then a separate PR for the cookie renaming etc). That way focusing in on the actual meat of the changes would be easier.

That said I appreciated the effort put into the README.

@Jusshersmith Jusshersmith force-pushed the jusshersmith-sso-proxy-go-config branch from 3807a21 to 57bde86 Compare February 26, 2021 16:00
@Jusshersmith Jusshersmith force-pushed the jusshersmith-sso-proxy-go-config branch from af24c7b to d5a7f80 Compare April 9, 2021 11:51
@Jusshersmith Jusshersmith merged commit 348c2fc into master Apr 9, 2021
@Jusshersmith Jusshersmith deleted the jusshersmith-sso-proxy-go-config branch April 9, 2021 17:04
This was referenced Jun 24, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@benjsto benjsto benjsto approved these changes

@Integralist Integralist Integralist left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Jusshersmith @benjsto @Integralist