yahoojapan / authorization-proxy

Moved to https://github.com/AthenZ/authorization-proxy
https://github.com/AthenZ/authorization-proxy
Apache License 2.0
35 stars 9 forks source link

[major] fix config.yaml naming consistency #64

Closed WindzCUHK closed 4 years ago

WindzCUHK commented 4 years ago

Description

TODO

  1. [x] update config yaml to v2.0.0
  2. [x] update go.mod (depends on: https://github.com/yahoojapan/athenz-authorizer/pull/69)

changes

  1. fix multiple naming consistency problem in config.go.
  2. remove role token force enable
  3. config version to v2.0.0
  4. move unit test data to /test/data
  5. add log level support
  6. fix lint: SA1019: t.BuildNameToCertificate is deprecated: NameToCertificate only allows associating a single certificate with a given name. Leave that field nil to let the library select the first compatible chain from Certificates. (staticcheck)
  7. handle timeout and ca in config.Athenz

https://github.com/yahoojapan/authorization-proxy/blob/20ac59579566b5b19719f6b043ca962762c99819/test/data/example_config.yaml#L1-L69

Details

Suggested naming convention rules:

  1. using camelCase in yaml
  2. type: duration
    1. must contain duration in the comment
    2. must end with noun that has the meaning "time range"
      1. expiry, period, delay, duration
      2. time
  3. type: boolean
    1. NO enable prefix
  4. type: array
    1. ADD s suffix

Remarks

  1. log level
    1. https://github.com/kpango/glg/blob/f87edaada63804d40fdf6f951b87690d3971e266/glg.go#L205-L262
    2. https://github.com/kpango/glg/blob/f87edaada63804d40fdf6f951b87690d3971e266/glg.go#L78-L95
  2. About ETag, People use ETag, Etag and etag in docs.
    1. golang use ETag as constant
    2. S3 using ETag as constant
  3. https://kubernetes.io/docs/contribute/style/style-guide/#avoid-latin-phrases
  4. https://cloud.google.com/apis/design/naming_convention#time_and_duration
    1. using long form duration

Type of change

Flags

Related issue/PR


Checklist

Checklist for maintainer

ctyano commented 4 years ago

@WindzCUHK (Perhaps out of the scope of this ticket, but) It's better to add a filed to allow configuration to change from Authorization: Bearer like other headers.(Although using another header name than Authorization: Bearer is against the standard.) https://github.com/yahoojapan/authorization-proxy/blob/745655002bae0ca52b8ab8a0d937691332019aff/config/config.go#L214-L233

WindzCUHK commented 4 years ago

@tatyano

It's better to add a filed to allow configuration to change from Authorization: Bearer like other headers.

created an issue: https://github.com/yahoojapan/athenz-authorizer/issues/68

codecov-commenter commented 4 years ago

Codecov Report

Merging #64 into master will increase coverage by 0.64%. The diff coverage is 98.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   87.02%   87.66%   +0.64%     
==========================================
  Files          14       14              
  Lines         655      689      +34     
==========================================
+ Hits          570      604      +34     
  Misses         75       75              
  Partials       10       10              
Impacted Files Coverage Δ
config/config.go 100.00% <ø> (ø)
handler/handler.go 92.85% <ø> (ø)
service/option.go 100.00% <ø> (ø)
main.go 56.84% <96.29%> (+10.78%) :arrow_up:
handler/transport.go 100.00% <100.00%> (ø)
router/debug_router.go 95.65% <100.00%> (ø)
router/debug_routes.go 88.88% <100.00%> (ø)
service/server.go 97.63% <100.00%> (ø)
service/tls.go 78.37% <100.00%> (-0.57%) :arrow_down:
usecase/authz_proxyd.go 100.00% <100.00%> (ø)

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 b6988bf...7ab155b. Read the comment docs.

ssunorz commented 4 years ago

It looks like there's something in this file that needs to be fixed.

https://github.com/yahoojapan/authorization-proxy/blob/44035804e0ec9e1b8c8f5777736bdc7db6baaaae/k8s/gen.sh

WindzCUHK commented 4 years ago

It looks like there's something in this file that needs to be fixed.

https://github.com/yahoojapan/authorization-proxy/blob/44035804e0ec9e1b8c8f5777736bdc7db6baaaae/k8s/gen.sh

fixed in: 19004f9