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

add TLS support for REST #32

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Conversation

isogram
Copy link
Collaborator

@isogram isogram commented Nov 21, 2023

  • added new three env. USE_REST_TLS, REST_TLS_CERT_FILE, REST_TLS_KEY_FILE
  • added logic to serve HTTPS for REST

@isogram isogram requested a review from a team as a code owner November 21, 2023 05:50
@codecov-commenter
Copy link

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (ee200ce) 18.66% compared to head (eda95f9) 18.57%.

❗ Current head eda95f9 differs from pull request most recent head 7df9aff. Consider uploading reports for the commit 7df9aff to get more accurate results

Files Patch % Lines
config/env/environment.go 0.00% 12 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   18.66%   18.57%   -0.10%     
==========================================
  Files          41       41              
  Lines        2454     2466      +12     
==========================================
  Hits          458      458              
- Misses       1981     1993      +12     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@isogram isogram requested a review from dimassantoso November 21, 2023 14:32
@@ -23,6 +23,9 @@ type (
jaegerMaxPacketSize int
sharedListener cmux.CMux
graphqlOption graphqlserver.Option
tls bool
certFile string
Copy link
Member

Choose a reason for hiding this comment

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

langsung pake tlsConfig aja gimana wa @isogram ?

Suggested change
certFile string
tlsConfig *tls.Config

@@ -138,3 +144,24 @@ func AddGraphQLOption(opts ...graphqlserver.OptionFunc) OptionFunc {
MiddlewareExcludeURLPath[o.graphqlOption.RootPath] = struct{}{}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

yg tambahan disini jadi cukup 1 aja

Suggested change
// SetTLSConfig option func
func SetTLSConfig(tlsConfig *tls.Config) OptionFunc {
return func(o *option) {
o.tlsConfig = tlsConfig
}
}

@@ -86,10 +92,44 @@ func NewServer(service factory.ServiceFactory, opts ...OptionFunc) factory.AppSe

func (s *restServer) Serve() {
var err error
if s.listener != nil {
err = s.httpEngine.Serve(s.listener)

Copy link
Member

Choose a reason for hiding this comment

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

jadi ketika Serve cukup begini aja

Suggested change
func (s *restServer) Serve() {
var err error
if s.listener == nil {
s.listener, err = net.Listen("tcp", s.httpEngine.Addr)
if err != nil {
log.Panicf("REST TCP Listener: Unexpected Error: %v", err)
}
}
if s.opt.tlsConfig != nil {
s.httpEngine.TLSConfig = s.opt.tlsConfig
s.listener = tls.NewListener(s.listener, s.opt.tlsConfig)
}
err = s.httpEngine.Serve(s.listener)
switch err.(type) {
case *net.OpError:
log.Panicf("REST Server: Unexpected Error: %v", err)
}
}

@@ -44,6 +44,13 @@ type Env struct {
UsePostgresListenerWorker bool
// UseRabbitMQWorker env
UseRabbitMQWorker bool
// UseRESTTls config
UseRESTTls bool
Copy link
Member

Choose a reason for hiding this comment

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

dan jadi gaperlu ada tambahan default env

@@ -16,6 +16,9 @@ func SetupRESTServer(service factory.ServiceFactory, opts ...restserver.OptionFu
restserver.SetSharedListener(service.GetConfig().SharedListener),
restserver.SetDebugMode(env.BaseEnv().DebugMode),
restserver.SetJaegerMaxPacketSize(env.BaseEnv().JaegerMaxPacketSize),
restserver.SetTlsOption(env.BaseEnv().UseRESTTls),
Copy link
Member

Choose a reason for hiding this comment

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

jadi yg disini udah gaperlu, cukup lewat configs/app_factory.go dari sisi app-nya saja

(dari sisi app-nya)

Suggested change
restserver.SetTlsOption(env.BaseEnv().UseRESTTls),
if env.BaseEnv().UseREST {
apps = append(apps, appfactory.SetupRESTServer(service, restserver.SetTLSConfig(getTLSConfig())))
...

@agungdwiprasetyo agungdwiprasetyo changed the base branch from master to development November 22, 2023 23:35
@agungdwiprasetyo agungdwiprasetyo merged commit 2443ea3 into golangid:development Nov 22, 2023
1 check passed
@isogram
Copy link
Collaborator Author

isogram commented Nov 23, 2023

ok gue PR ke changesnya ke development yak @agungdwiprasetyo

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