From 359a4111874a5f877358c99100a1054e41a2ffe8 Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Thu, 25 Jan 2024 12:26:59 +0100 Subject: [PATCH 1/4] Support minikube env folder on juju snap Adds dot-minikube plug to support minikube env folder in the home directory by default. --- snap/snapcraft.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index 85e2165ce5c..936e1eaff4f 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -51,6 +51,7 @@ apps: - dot-google - dot-kubernetes - dot-maas + - dot-minikube - dot-openstack - dot-oracle # Needed so that arbitrary cloud/credential yaml files can be read and backups written. @@ -178,6 +179,11 @@ plugs: read: - $HOME/.maasrc + dot-minikube: + interface: personal-files + read: + - $HOME/.minikube + dot-oracle: interface: personal-files read: @@ -187,3 +193,4 @@ plugs: interface: personal-files read: - $HOME/.novarc + From 1c9ac2a365d2bb8d76bc7e6e3b08f5b63df6b0d0 Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Fri, 2 Feb 2024 09:30:20 +0100 Subject: [PATCH 2/4] Revert dot-minikube on snapcraft until auto-connect is approved --- snap/snapcraft.yaml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml index 936e1eaff4f..b3d86bf0296 100644 --- a/snap/snapcraft.yaml +++ b/snap/snapcraft.yaml @@ -51,7 +51,6 @@ apps: - dot-google - dot-kubernetes - dot-maas - - dot-minikube - dot-openstack - dot-oracle # Needed so that arbitrary cloud/credential yaml files can be read and backups written. @@ -179,11 +178,6 @@ plugs: read: - $HOME/.maasrc - dot-minikube: - interface: personal-files - read: - - $HOME/.minikube - dot-oracle: interface: personal-files read: From 66fa8dbf8d3bf929f85d5394b7ed15ad04cfdce4 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 6 Feb 2024 12:38:43 +0000 Subject: [PATCH 3/4] Suppress klog log messages The klog log messages are noisy and fill up the logs really easily. It then becomes hard to workout what is signal and what is noise. As log messages are probably important, it's worth just suppressing the message rather than blanket ignoring it. As the klog might be dumping a lot of messages for large controllers, I've ensured that we only do the request once we have a match. The only match we have atm is the `Use tokens from the TokenRequest`. We can always add more if we require. --- caas/kubernetes/provider/klog.go | 111 ++++++++++++++++++++++++------- go.mod | 2 +- 2 files changed, 87 insertions(+), 26 deletions(-) diff --git a/caas/kubernetes/provider/klog.go b/caas/kubernetes/provider/klog.go index 52782a56fbf..e7d67df95db 100644 --- a/caas/kubernetes/provider/klog.go +++ b/caas/kubernetes/provider/klog.go @@ -5,20 +5,11 @@ package provider import ( "strings" + "time" "github.com/go-logr/logr" "github.com/juju/loggo" -) - -type KlogMessagePrefixes []string - -var ( - klogIgnorePrefixes = KlogMessagePrefixes{ - "lost connection to pod", - "an error occurred forwarding", - "error copying from remote stream to local connection", - "error copying from local connection to remote stream", - } + "golang.org/x/time/rate" ) // klogAdapter is an adapter for Kubernetes logger onto juju loggo. We use this @@ -34,8 +25,7 @@ func newKlogAdapter() logr.Logger { }) } -func (k *klogAdapter) Init(info logr.RuntimeInfo) { -} +func (k *klogAdapter) Init(info logr.RuntimeInfo) {} // Enabled see https://pkg.go.dev/github.com/go-logr/logr#Logger func (k *klogAdapter) Enabled(level int) bool { @@ -43,7 +33,7 @@ func (k *klogAdapter) Enabled(level int) bool { } // Error see https://pkg.go.dev/github.com/go-logr/logr#Logger -func (k *klogAdapter) Error(err error, msg string, keysAndValues ...interface{}) { +func (k *klogAdapter) Error(err error, msg string, keysAndValues ...any) { if err != nil { k.Logger.Errorf(msg+": "+err.Error(), keysAndValues...) return @@ -56,17 +46,13 @@ func (k *klogAdapter) Error(err error, msg string, keysAndValues ...interface{}) } // Info see https://pkg.go.dev/github.com/go-logr/logr#Logger -func (k *klogAdapter) Info(level int, msg string, keysAndValues ...interface{}) { - k.Logger.Infof(msg, keysAndValues...) -} - -func (k KlogMessagePrefixes) Matches(log string) bool { - for _, v := range k { - if strings.HasPrefix(log, v) { - return true - } +func (k *klogAdapter) Info(level int, msg string, keysAndValues ...any) { + if prefix, ok := klogSuppressedPrefixes.Matches(msg); ok && prefix != nil { + prefix.Do(k.Logger.Infof, msg, keysAndValues...) + return } - return false + + k.Logger.Infof(msg, keysAndValues...) } // V see https://pkg.go.dev/github.com/go-logr/logr#Logger @@ -75,7 +61,7 @@ func (k *klogAdapter) V(level int) logr.LogSink { } // WithValues see https://pkg.go.dev/github.com/go-logr/logr#Logger -func (k *klogAdapter) WithValues(keysAndValues ...interface{}) logr.LogSink { +func (k *klogAdapter) WithValues(keysAndValues ...any) logr.LogSink { return k } @@ -83,3 +69,78 @@ func (k *klogAdapter) WithValues(keysAndValues ...interface{}) logr.LogSink { func (k *klogAdapter) WithName(name string) logr.LogSink { return k } + +// klogMessagePrefixes is a list of prefixes to ignore. +type klogMessagePrefixes []string + +var ( + klogIgnorePrefixes = klogMessagePrefixes{ + "lost connection to pod", + "an error occurred forwarding", + "error copying from remote stream to local connection", + "error copying from local connection to remote stream", + } +) + +func (k klogMessagePrefixes) Matches(log string) bool { + for _, v := range k { + if strings.HasPrefix(log, v) { + return true + } + } + return false +} + +// klogSuppressMessagePrefix is a log suppression type that suppresses log +// messages with a given prefix at a given rate. If the rate is nil, the +// suppression is disabled. +type klogSuppressMessagePrefix struct { + prefix string + rate *rate.Sometimes +} + +// Do calls the logger function if the rate allows it. If the rate is nil, the +// function is called directly, thus bypassing the rate. +func (k klogSuppressMessagePrefix) Do(loggerFn func(string, ...any), msg string, args ...any) { + // If we don't have a rate, just call the function directly. + if k.rate == nil { + loggerFn(msg, args) + return + } + + // If we have a rate, use it to suppress the message. + k.rate.Do(func() { + loggerFn(msg, args) + }) +} + +// klogSuppressMessagePrefixes is a list of prefixes to suppress and their +// suppression rates. +type klogSuppressMessagePrefixes []*klogSuppressMessagePrefix + +var ( + klogSuppressedPrefixes = klogSuppressMessagePrefixes{ + &klogSuppressMessagePrefix{ + prefix: "Use tokens from the TokenRequest API or manually created secret-based tokens instead of auto-generated secret-based tokens", + // We suppress the message at a rate of 1 per 5 minute, but we + // allow the first message to go through. + rate: &rate.Sometimes{ + First: 1, + Interval: time.Minute * 5, + }, + }, + } +) + +// Matches returns the prefix and whether it matches the log message. +func (k klogSuppressMessagePrefixes) Matches(log string) (*klogSuppressMessagePrefix, bool) { + for _, p := range k { + if p == nil { + continue + } + if strings.HasPrefix(log, p.prefix) { + return p, true + } + } + return nil, false +} diff --git a/go.mod b/go.mod index 2b04695e4b6..7b24198e7a7 100644 --- a/go.mod +++ b/go.mod @@ -110,6 +110,7 @@ require ( golang.org/x/oauth2 v0.15.0 golang.org/x/sync v0.5.0 golang.org/x/sys v0.15.0 + golang.org/x/time v0.5.0 golang.org/x/tools v0.16.0 google.golang.org/api v0.152.0 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c @@ -286,7 +287,6 @@ require ( golang.org/x/mod v0.14.0 // indirect golang.org/x/term v0.15.0 // indirect golang.org/x/text v0.14.0 // indirect - golang.org/x/time v0.5.0 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20231120223509-83a465c0220f // indirect google.golang.org/grpc v1.59.0 // indirect From 64af051681376fbb5cd8bad8e611a2206ef64302 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Wed, 7 Feb 2024 14:53:22 +0000 Subject: [PATCH 4/4] Ensure we close client The cross model remote secrets client would call to a remote model to access secret information. Unfortunately the client was never closed once it had made that request. Thus it would keep the connection alive and all of it's resources alive preventing the clean up. As the client is actively working, the garbage collector wouldn't be able to clean up the resource (the monitor in the apiclient prevented that) and it would steadly grow, based on more usage. This fix is a relatively light touch, but I can't help but think the better approach would be to have a pool of connections to other models, keyed on the sourceUUID. --- .../agent/secretsmanager/mocks/crossmodel.go | 14 ++++++++++++++ apiserver/facades/agent/secretsmanager/secrets.go | 3 +++ .../facades/agent/secretsmanager/secrets_test.go | 4 ++++ 3 files changed, 21 insertions(+) diff --git a/apiserver/facades/agent/secretsmanager/mocks/crossmodel.go b/apiserver/facades/agent/secretsmanager/mocks/crossmodel.go index c789e7ca8a7..5510bb2ebcf 100644 --- a/apiserver/facades/agent/secretsmanager/mocks/crossmodel.go +++ b/apiserver/facades/agent/secretsmanager/mocks/crossmodel.go @@ -106,6 +106,20 @@ func (m *MockCrossModelSecretsClient) EXPECT() *MockCrossModelSecretsClientMockR return m.recorder } +// Close mocks base method. +func (m *MockCrossModelSecretsClient) Close() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Close") + ret0, _ := ret[0].(error) + return ret0 +} + +// Close indicates an expected call of Close. +func (mr *MockCrossModelSecretsClientMockRecorder) Close() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockCrossModelSecretsClient)(nil).Close)) +} + // GetRemoteSecretContentInfo mocks base method. func (m *MockCrossModelSecretsClient) GetRemoteSecretContentInfo(arg0 *secrets.URI, arg1 int, arg2, arg3 bool, arg4, arg5 string, arg6 int, arg7 macaroon.Slice) (*secrets0.ContentParams, *provider.ModelBackendConfig, int, bool, error) { m.ctrl.T.Helper() diff --git a/apiserver/facades/agent/secretsmanager/secrets.go b/apiserver/facades/agent/secretsmanager/secrets.go index df20217caa0..d0f0ec7848d 100644 --- a/apiserver/facades/agent/secretsmanager/secrets.go +++ b/apiserver/facades/agent/secretsmanager/secrets.go @@ -37,6 +37,7 @@ var ( type CrossModelSecretsClient interface { GetRemoteSecretContentInfo(uri *coresecrets.URI, revision int, refresh, peek bool, sourceControllerUUID, appToken string, unitId int, macs macaroon.Slice) (*secrets.ContentParams, *secretsprovider.ModelBackendConfig, int, bool, error) GetSecretAccessScope(uri *coresecrets.URI, appToken string, unitId int) (string, error) + Close() error } // SecretsManagerAPI is the implementation for the SecretsManager facade. @@ -533,6 +534,8 @@ func (s *SecretsManagerAPI) getRemoteSecretContent(uri *coresecrets.URI, refresh if err != nil { return nil, nil, false, errors.Annotate(err, "creating remote secret client") } + defer func() { _ = extClient.Close() }() + consumerApp := commonsecrets.AuthTagApp(s.authTag) token, err := s.crossModelState.GetToken(names.NewApplicationTag(consumerApp)) if err != nil { diff --git a/apiserver/facades/agent/secretsmanager/secrets_test.go b/apiserver/facades/agent/secretsmanager/secrets_test.go index b28596e32a0..98dc8b0e9de 100644 --- a/apiserver/facades/agent/secretsmanager/secrets_test.go +++ b/apiserver/facades/agent/secretsmanager/secrets_test.go @@ -1288,6 +1288,7 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelExistingConsumerNoRe mac := apitesting.MustNewMacaroon("id") s.remoteClient = mocks.NewMockCrossModelSecretsClient(ctrl) + s.remoteClient.EXPECT().Close().Return(nil) s.crossModelState.EXPECT().GetToken(names.NewApplicationTag("mariadb")).Return("token", nil) s.secretsConsumer.EXPECT().GetSecretConsumer(uri, consumer).Return(&coresecrets.SecretConsumerMetadata{ @@ -1354,6 +1355,7 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelExistingConsumerNoRe mac := apitesting.MustNewMacaroon("id") s.remoteClient = mocks.NewMockCrossModelSecretsClient(ctrl) + s.remoteClient.EXPECT().Close().Return(nil) s.crossModelState.EXPECT().GetToken(names.NewApplicationTag("mariadb")).Return("token", nil) s.secretsConsumer.EXPECT().GetSecretConsumer(uri, consumer).Return(&coresecrets.SecretConsumerMetadata{ @@ -1425,6 +1427,7 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelExistingConsumerRefr mac := apitesting.MustNewMacaroon("id") s.remoteClient = mocks.NewMockCrossModelSecretsClient(ctrl) + s.remoteClient.EXPECT().Close().Return(nil) s.crossModelState.EXPECT().GetToken(names.NewApplicationTag("mariadb")).Return("token", nil) s.secretsConsumer.EXPECT().GetSecretConsumer(uri, consumer).Return(&coresecrets.SecretConsumerMetadata{ @@ -1496,6 +1499,7 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelNewConsumer(c *gc.C) mac := apitesting.MustNewMacaroon("id") s.remoteClient = mocks.NewMockCrossModelSecretsClient(ctrl) + s.remoteClient.EXPECT().Close().Return(nil) s.crossModelState.EXPECT().GetToken(names.NewApplicationTag("mariadb")).Return("token", nil) s.secretsConsumer.EXPECT().GetSecretConsumer(uri, consumer).Return(nil, errors.NotFoundf(""))