-
Notifications
You must be signed in to change notification settings - Fork 496
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
Use Validate Hook to control the total count of AutoCertManager #1155
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1155 +/- ##
==========================================
- Coverage 81.25% 81.02% -0.23%
==========================================
Files 149 149
Lines 16946 16962 +16
==========================================
- Hits 13769 13743 -26
- Misses 2524 2562 +38
- Partials 653 657 +4 ☔ View full report in Codecov by Sentry. |
pkg/object/httpserver/mux.go
Outdated
acm := autocertmanager.GetGlobalAutoCertManager() | ||
if acm == nil { | ||
logger.Errorf("BUG: autocert manager not found") | ||
stdw.WriteHeader(http.StatusServiceUnavailable) | ||
return | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines are only need when the request is an HTTP01 challenge, so they could be moved into the next if
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
pkg/object/httpserver/spec.go
Outdated
tlsConf.GetCertificate = func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
return autocertmanager.GetCertificate(chi, !spec.AutoCert /* tokenOnly */) | ||
return acm.GetCertificate(chi, !spec.AutoCert /* tokenOnly */) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could acm
be nil here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We chose to make AutoCertManager to BusinessController because its properties do not seem like a system controller.
pkg/supervisor/supervisor.go
Outdated
err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()), | ||
spec.JSONConfig()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per Go style guide.
err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()), | |
spec.JSONConfig()) | |
err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()), spec.JSONConfig()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
pkg/object/httpserver/spec.go
Outdated
acm, exists := autocertmanager.GetGlobalAutoCertManager() | ||
if !exists { | ||
return nil, fmt.Errorf("BUG: autocert manager is not initialized") | ||
} | ||
|
||
tlsConf.GetCertificate = func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
return acm.GetCertificate(chi, !spec.AutoCert /* tokenOnly */) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to keep the atomic.Value
in AutoCertManager. Here, acm
here is the instance that existed when this HTTPServer is created.
Then suppose the user update the AutoCertManager or simply delete it. Then tlsConf.GetCertificate
still use the old acm
value.
I think every time GetCertificate
is called, then we should dynamic load AutoCertManager
and use the newest one. And since autocertmanager.GetGlobalAutoCertManager()
is not very efficiency compare to actomic. We should use atomic.Value
back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use atomic.Value because it is called infrequently. But you're right we need to get acm in real time in tlsConf.GetCertificate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tlsConf. GetCertificate
is used to get certificate based on client hello message. When a client connect to https
server, they send a client hello message. And golang use this client hello info to get certificate from autocertmanager. I am not sure if golang use cache for this. If not, it will be called every time a client connect to server. That can be a lot...
Anyway, atomic.Value
brings no harm at all. Only benefits of efficiency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
…o control the global instance of AutoCertManager
pkg/object/httpserver/spec.go
Outdated
acm, exists := autocertmanager.GetGlobalAutoCertManager() | ||
if !exists { | ||
return nil, fmt.Errorf("there is no AutoCertManager") | ||
} | ||
|
||
tlsConf.GetCertificate = func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) { | ||
return acm.GetCertificate(chi, !spec.AutoCert /* tokenOnly */) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tlsConf.GetCertificate = func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) {
acm, exists := autocertmanager.GetGlobalAutoCertManager()
if !exists {
return nil, fmt.Errorf("there is no AutoCertManager")
}
return acm.GetCertificate(chi, !spec.AutoCert /* tokenOnly */)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside of GetCertificate
, we need to load atomic.Value
every time to get newest instance...
Based on the last pr #1144