Adding TLS cert refreshing from KeyVault #1426

ramiro-gamarra · 2022-06-09T02:23:14Z

Reason for Change:

This PR adds a CertRefresher construct, which allows background pull of new certificates from KeyVault. We use this construct to provide a tls.Certificate in a dynamic manner to an HTTP server that uses a tls.Config.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

rbtr

some suggestions but looks good already 🚀

cns/service.go

keyvault/certrefresher.go

timraymond · 2022-06-10T19:31:37Z

common/listener.go

+	}
+
+	go func() {
+		errChan <- cr.Refresh(ctx, tlsSettings.KeyVaultCertificateRefreshInterval)


I don't think this will ever return cleanly unless the certificate's expiration is before the next refresh interval. The context has no cancellation. That might be fine if it's intended to share the lifetime of the application, but I figured I'd mention it.

Yeah, ideally, context would be propagated all the way from main to handle signals cleanly but there are a few layers of indirection unfortunately, so I opted for a context.TODO() in the meantime. This goroutine is indeed tied to the lifetime of the application so a temporary leak might be ok for now. Since I have some changes to push, I may try to propagate context, but will depend on size of changes.

keyvault/certrefresher.go

keyvault/certrefresher_test.go

aegal

this looks good

ramiro-gamarra

Added some context to a few of the changes

ramiro-gamarra · 2022-06-14T16:59:11Z

cns/configuration/cns_config.json

+    },
+    "MSISettings": {
+        "ResourceID": ""
+    }


adding these to keep this documentation up to date

ramiro-gamarra · 2022-06-14T17:38:09Z

cns/logger/log.go

-	}
+	m            sync.RWMutex
+	Orchestrator string
+	NodeID       string


motivation for changes in this file are making this cns logger easier to stub out in tests, as well as addressing what I'm pretty sure is a race condition when setting these fields, since the logging methods here read them all the time but they are set, possibly multiple times, from an http handler.

ramiro-gamarra · 2022-06-14T17:42:19Z

keyvault/certrefresher.go

+		return nil
+	}
+
+	if err := retry.Do(retryFn, retry.Context(ctx), retry.Delay(time.Second), retry.DelayType(retry.FixedDelay)); err != nil {


changed behavior a bit from latest revision. the intent is now to retry to fetch from keyvault until a deadline set to the current cert expiration date is met. basically, we try to keep cns alive for as long as the cert is valid.

ramiro-gamarra · 2022-06-14T18:15:57Z

common/listener.go

 	return &listener, nil
 }

-func GetTlsConfig(tlsSettings localtls.TlsSettings) (*tls.Config, error) {


moved the below functions out of this package because injecting a cns logger here creates an import cycle. we should really do away with common

ramiro-gamarra · 2022-06-14T18:23:36Z

keyvault/certrefresher.go

+	GetLatestTLSCertificate(ctx context.Context, certName string) (tls.Certificate, error)
+}
+
+type logger interface {


I switched from zap to existing logging mechanism to keep any possible side effects from this PR to a minimum. I enjoyed adding logs with zap so when given a chance I plan to revisit this package (or CNS in general) to contribute to its adoption.

timraymond · 2022-06-15T15:42:43Z

cns/logger/log.go


-// Initialize CNS Logger
-func InitLogger(fileName string, logLevel, logTarget int, logDir string) {
-	Log = &CNSLogger{


ditto, but maybe fixing the logger should be a separate set of changes (that go first, if necessary)? scope is creeping 🙁

I don't mind extracting this out if it makes it easier to review

timraymond

I think this looks good, but I'll defer to someone with more familiarity in CNS for approval. I've got just a couple of questions / suggestions.

timraymond · 2022-06-15T15:47:04Z

cns/logger/log.go

+func (c *CNSLogger) getOrchestratorAndNodeID() (orch, nodeID string) {
+	c.m.RLock()
+	orch, nodeID = c.Orchestrator, c.NodeID
+	c.m.RUnlock()
+	return
+}


It doesn't seem like the named returns are super necessary here unless I'm missing something...

Suggested change

func (c *CNSLogger) getOrchestratorAndNodeID() (orch, nodeID string) {

c.m.RLock()

orch, nodeID = c.Orchestrator, c.NodeID

c.m.RUnlock()

return

}

func (c *CNSLogger) getOrchestratorAndNodeID() (orch, nodeID string) {

c.m.RLock()

defer c.m.RUnlock()

return c.Orchestrator, c.NodeID

}

the linter actually suggested this. I kind of agree, only when there are multiple returns of the same type.

Yeah, I can see the names would be useful in the signature. I think in this case I would name them, but still explicitly return. Edited the suggestion to show what I mean.

timraymond · 2022-06-15T15:55:00Z

cns/logger/log.go

+func Close() {
+	Log.Close()
+}
+
+func InitLogger(fileName string, logLevel, logTarget int, logDir string) {
+	Log, _ = NewCNSLogger(fileName, logLevel, logTarget, logDir)
+}
+
+func InitAI(aiConfig aitelemetry.AIConfig, disableTraceLogging, disableMetricLogging, disableEventLogging bool) {
+	Log.InitAI(aiConfig, disableTraceLogging, disableMetricLogging, disableEventLogging)
+}
+
+func SetContextDetails(orchestrator, nodeID string) {
+	Log.SetContextDetails(orchestrator, nodeID)
+}
+
+func Printf(format string, args ...interface{}) {
+	Log.Printf(format, args...)
+}
+
+func Debugf(format string, args ...interface{}) {
+	Log.Debugf(format, args...)
+}
+
+func LogEvent(event aitelemetry.Event) {
+	Log.LogEvent(event)
+}
+
+func Errorf(format string, args ...interface{}) {
+	Log.Errorf(format, args...)
+}
+
+func Request(tag string, request interface{}, err error) {
+	Log.Request(tag, request, err)
+}
+
+func Response(tag string, response interface{}, returnCode types.ResponseCode, err error) {
+	Log.Response(tag, response, returnCode, err)
+}
+
+func ResponseEx(tag string, request, response interface{}, returnCode types.ResponseCode, err error) {
+	Log.ResponseEx(tag, request, response, returnCode, err)
+}
+
+func SendMetric(metric aitelemetry.Metric) {
+	Log.SendMetric(metric)
 }


Might be nice to move these functions off to another file just to really call out that this is a compatibility layer.

Good idea. Will address in the PR I'm extracting

timraymond · 2022-06-15T16:01:02Z

cns/logger/log.go

+		CustomDimensions: map[string]string{
+			OrchestratorTypeStr: orch,
+			NodeIDStr:           nodeID,
+		},


Is it worth storing a map as just a general bag of keys and values like many structured logging tools do? We can set "orchestrator" and "nodeID" by default in there, but have the flexibility to add more as necessary. Unsure what the limitations of AI are though, so it may not be feasible :)

If I understand correctly, is the suggestion to add a map of default custom dimensions to CNSLogger? I would agree, and we do something similar in DNC. In this case, these properties are lazily added; there will be a time during CNS runtime when they haven't been set by DNC yet, so they are not quite "default". Ideally, once set they should never change, but I don't want to change behavior too much this time around.

rbtr

lgtm

ramiro-gamarra requested a review from a team as a code owner June 9, 2022 02:23

ramiro-gamarra requested review from rbtr and removed request for a team June 9, 2022 02:23

ramiro-gamarra requested a review from matmerr as a code owner June 9, 2022 17:19

rbtr previously approved these changes Jun 9, 2022

View reviewed changes

cns/service.go Outdated Show resolved Hide resolved

keyvault/certrefresher.go Outdated Show resolved Hide resolved

keyvault/certrefresher.go Outdated Show resolved Hide resolved

timraymond reviewed Jun 10, 2022

View reviewed changes

aegal previously approved these changes Jun 10, 2022

View reviewed changes

ramiro-gamarra dismissed stale reviews from aegal and rbtr via 16d26cb June 14, 2022 05:26

ramiro-gamarra commented Jun 14, 2022

View reviewed changes

timraymond reviewed Jun 15, 2022

View reviewed changes

adding a cert refresher construct

6e9264d

rbtr approved these changes Jun 16, 2022

View reviewed changes

ramiro-gamarra enabled auto-merge (squash) June 16, 2022 21:10

ramiro-gamarra merged commit 69f851a into Azure:master Jun 16, 2022

Adding TLS cert refreshing from KeyVault #1426

Adding TLS cert refreshing from KeyVault #1426

Uh oh!

Conversation

ramiro-gamarra commented Jun 9, 2022

Uh oh!

rbtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aegal left a comment

Choose a reason for hiding this comment

Uh oh!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbtr Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timraymond left a comment

Choose a reason for hiding this comment

Uh oh!

timraymond Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbtr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rbtr Jun 15, 2022 •

edited

Loading

timraymond Jun 15, 2022 •

edited

Loading