manager: new namespace manager based on etcd #13
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: xhe <xw897002528@gmail.com>
A completely refactor, or rewritten of the old double buffer management of namespace. Also a preparetion for later proxy server refactor. 1. no need to prepare the namespace before commit. Now reloading namespaces be like: 1. use HTTP api modify persisted namespace configs on etcd. 2. commit will pull from etcd, and perform a swap of namespace state under the guard of mutex. Etcd serves as "dirty buffer" for the in-memory namespace manager. It is acceptable because client will connect etcd by local loopback network. 2. remove '/util/datastructure' 3. remove username to namespace mapping because username are removed in previous PR. Signed-off-by: xhe <xw897002528@gmail.com>
xhebox
commented
Jul 25, 2022
Comment on lines
-53
to
-75
| var minTLSVersion uint16 = tls.VersionTLS11 | ||
| switch minTLSVer { | ||
| case "TLSv1.0": | ||
| minTLSVersion = tls.VersionTLS10 | ||
| case "TLSv1.1": | ||
| minTLSVersion = tls.VersionTLS11 | ||
| case "TLSv1.2": | ||
| minTLSVersion = tls.VersionTLS12 | ||
| case "TLSv1.3": | ||
| minTLSVersion = tls.VersionTLS13 | ||
| case "": | ||
| default: | ||
| logutil.BgLogger().Warn( | ||
| "Invalid TLS version, using default instead", | ||
| zap.String("tls-version", minTLSVer), | ||
| ) | ||
| } | ||
| if minTLSVersion < tls.VersionTLS12 { | ||
| logutil.BgLogger().Warn( | ||
| "Minimum TLS version allows pre-TLSv1.2 protocols, this is not recommended", | ||
| ) | ||
| } | ||
|
|
| @@ -90,28 +62,10 @@ func CreateServerTLSConfig(ca, key, cert, minTLSVer, path string, rsaKeySize int | |||
| } | |||
| } | |||
|
|
|||
| // This excludes ciphers listed in tls.InsecureCipherSuites() and can be used to filter out more | |||
There was a problem hiding this comment.
Checked source code, it is useless. CipherSuites() is secure and does not include suites from InsecureCipherSuites().
| @@ -155,7 +155,7 @@ func NewServer(ctx context.Context, cfg *config.Config, logger *zap.Logger, name | |||
| return | |||
| } | |||
|
|
|||
| err = srv.NamespaceManager.Init(nss, srv.ObserverClient) | |||
| err = srv.NamespaceManager.Init(logger.Named("nsmgr"), nss, srv.ObserverClient) | |||
xhebox
commented
Jul 25, 2022
Comment on lines
-113
to
-118
| func (n *NamespaceWrapper) mustGetCurrentNamespace() Namespace { | ||
| ns, ok := n.nsmgr.getCurrentNamespaces().Get(n.name) | ||
| if !ok { | ||
| panic(errors.New("namespace not found")) | ||
| } | ||
| return ns |
There was a problem hiding this comment.
It is removed. We don't need to get namespace under nsmgr mutex everytime for latest namespace config.
The old connection is hard to follow new config anyway. For connection states that possibly will be attached to namespace dynamically, we could execute in-place reload:
ns := NewNamespaceFromConfig()
oldNS.Reload(ns)
oldNS could have an rwmutex, it will refresh config and take care of all old connection states. Thus reimplementing interface by a wrapper is not necessary.
djshow832
reviewed
Jul 25, 2022
Comment on lines
+3
to
+5
| security: | ||
| cert: "./ttt" | ||
| key: "./ttt" |
There was a problem hiding this comment.
What's its purpose? The cert to connect to the client?
There was a problem hiding this comment.
Yes, one for frontend, one for backend. And we can set the default cert or whatever in the global config.
Signed-off-by: xhe <xw897002528@gmail.com>
djshow832
approved these changes
Jul 26, 2022
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #11
Problem Summary: A new, simplified namespace manager.
What is changed and how it works: Included in the commit message. Note that:
util/security/tls.go.NewProxyConfig=>NewConfigmanager/namespace/domain.go. I don't think there is a need of abstraction, if interface will only be implemented by one struct.Check List
Tests
Notable changes
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.