Use Validate Hook to control the total count of AutoCertManager #1155

xxx7xxxx · 2023-12-02T08:40:02Z

Based on the last pr #1144

codecov-commenter · 2023-12-02T08:43:37Z

Codecov Report

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

Comparison is base (e8dc3e8) 81.25% compared to head (cb57009) 81.02%.

Files	Patch %	Lines
pkg/object/autocertmanager/autocertmanager.go	33.33%	22 Missing ⚠️
pkg/object/httpserver/spec.go	11.11%	7 Missing and 1 partial ⚠️
pkg/object/httpserver/mux.go	87.50%	1 Missing ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

localvar · 2023-12-03T11:19:32Z

pkg/object/httpserver/mux.go

+	acm := autocertmanager.GetGlobalAutoCertManager()
+	if acm == nil {
+		logger.Errorf("BUG: autocert manager not found")
+		stdw.WriteHeader(http.StatusServiceUnavailable)
+		return
+	}
+


these lines are only need when the request is an HTTP01 challenge, so they could be moved into the next if block.

localvar · 2023-12-03T11:28:59Z

pkg/object/httpserver/spec.go

 	tlsConf.GetCertificate = func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) {
-		return autocertmanager.GetCertificate(chi, !spec.AutoCert /* tokenOnly */)
+		return acm.GetCertificate(chi, !spec.AutoCert /* tokenOnly */)


could acm be nil here?

We chose to make AutoCertManager to BusinessController because its properties do not seem like a system controller.

localvar · 2023-12-03T11:32:23Z

pkg/supervisor/supervisor.go

+	err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()),
+		spec.JSONConfig())


per Go style guide.

Suggested change

err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()),

spec.JSONConfig())

err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()), spec.JSONConfig())

suchen-sci · 2023-12-07T03:29:42Z

pkg/object/httpserver/spec.go

+		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 */)
+		}


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.

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.

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.

pkg/object/autocertmanager/autocertmanager.go

…o control the global instance of AutoCertManager

suchen-sci · 2023-12-07T08:28:20Z

pkg/object/httpserver/spec.go

+		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 */)
+		}


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 */) }

Inside of GetCertificate, we need to load atomic.Value every time to get newest instance...

xxx7xxxx requested review from localvar and suchen-sci December 2, 2023 08:40

localvar reviewed Dec 3, 2023

View reviewed changes

xxx7xxxx changed the title ~~Transform AutoCertManager to a system controller~~ Use Validate Hook to control the total count of AutoCertManager Dec 5, 2023

xxx7xxxx force-pushed the autocert branch from 0cdc7fc to 858ff35 Compare December 5, 2023 10:14

suchen-sci requested a review from localvar December 7, 2023 02:37

suchen-sci reviewed Dec 7, 2023

View reviewed changes

pkg/object/autocertmanager/autocertmanager.go Show resolved Hide resolved

xxx7xxxx added 4 commits December 7, 2023 16:13

Transform AutoCertManager to a system controller

431bcd3

Convert AutoCertManager to busisness controller & use validate hook t…

03da2ca

…o control the global instance of AutoCertManager

Allow to not find AutoCertManager

b7bbeaf

Use global acm to improve efficiency

800a208

xxx7xxxx force-pushed the autocert branch from 858ff35 to 800a208 Compare December 7, 2023 08:22

suchen-sci reviewed Dec 7, 2023

View reviewed changes

Use global acm in real time

cb57009

suchen-sci approved these changes Dec 7, 2023

View reviewed changes

localvar approved these changes Dec 8, 2023

View reviewed changes

xxx7xxxx added this pull request to the merge queue Dec 8, 2023

Merged via the queue into easegress-io:main with commit c3ab195 Dec 8, 2023
8 checks passed

xxx7xxxx deleted the autocert branch December 13, 2023 02:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Validate Hook to control the total count of AutoCertManager #1155

Use Validate Hook to control the total count of AutoCertManager #1155

xxx7xxxx commented Dec 2, 2023

codecov-commenter commented Dec 2, 2023 •

edited

Loading

localvar Dec 3, 2023

xxx7xxxx Dec 5, 2023

localvar Dec 3, 2023

xxx7xxxx Dec 5, 2023

localvar Dec 3, 2023

xxx7xxxx Dec 5, 2023

suchen-sci Dec 7, 2023

xxx7xxxx Dec 7, 2023

suchen-sci Dec 7, 2023

xxx7xxxx Dec 7, 2023

suchen-sci Dec 7, 2023

suchen-sci Dec 7, 2023

		err = s.cls.Put(s.cls.Layout().ConfigObjectKey(spec.Name()),
		spec.JSONConfig())

Use Validate Hook to control the total count of AutoCertManager #1155

Use Validate Hook to control the total count of AutoCertManager #1155

Conversation

xxx7xxxx commented Dec 2, 2023

codecov-commenter commented Dec 2, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 2, 2023 •

edited

Loading