buzzfeed / sso

sso, aka S.S.Octopus, aka octoboi, is a single sign-on solution for securing internal services
MIT License
3.09k stars 186 forks source link

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

Closed Jusshersmith closed 3 years ago

Jusshersmith commented 4 years ago

Problem

sso_auth was migrated to use go-micro in https://github.com/buzzfeed/sso/pull/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:

In general, the patterns used in this pull request are the same ones used in https://github.com/buzzfeed/sso/pull/212

Review notes

Some notes to support reviewing:

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
codecov[bot] commented 4 years ago

Codecov Report

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

:exclamation: 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%) :arrow_down:
internal/proxy/proxy_config.go 76.21% <ø> (ø)
internal/proxy/options.go 82.07% <85.00%> (-1.52%) :arrow_down:
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%) :arrow_up:

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 commented 4 years ago

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.