-
Notifications
You must be signed in to change notification settings - Fork 983
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
Auto-discover caBundle when missing (take 2) #655
Conversation
also turns out non-test code was broken by my os.FS change
... instead of rest.InClusterConfig() - this will work in more scenarios, such as when Karpenter is not running inside of the cluster
1. wasn't finding CAData because it wasn't populated 2. use of controllerruntime in shared code (w/ webhook) was causing init()-time crash
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: e75ebd1 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61316377d78e170007700c58 |
encoded := base64.StdEncoding.EncodeToString(binary) | ||
return &encoded, nil | ||
_, err = transport.TLSConfigFor(transportConfig) // fills in CAData! | ||
logging.FromContext(ctx).Infof("Discovered caBundle, length %d", len(transportConfig.TLS.CAData)) |
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.
This should minimally be a debug statement like the other discovery bits. How often does this log line print?
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.
Only when creating a launch template
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.
done
pkg/utils/restconfig/restconfig.go
Outdated
type contextKey string | ||
|
||
const ( | ||
restKey contextKey = "restKey" |
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.
consider a struct and this style: https://github.com/knative/pkg/blob/b558677ab03404ed118ba3e179a7438fe2d8509a/apis/contexts.go#L28
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.
Ok
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.
done
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.
Looks good! Thanks for driving this through
Issue, if available:
This is a prerequisite for #626 but does not fix it.
Description of changes:
fix problems with https://github.com/awslabs/karpenter/pull/636/files
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.