-
Notifications
You must be signed in to change notification settings - Fork 21
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
Make provider support more modular #795
base: main
Are you sure you want to change the base?
Conversation
Out-of-tree Provider Guide:
For implementation details, refer to To streamline the out-of-tree provider experience, we can follow Grafana k6's approach with their xk6 toolkit. Similar to how xk6 simplifies building k6 with custom extensions, we can provide a toolkit that would:
This would further reduce the complexity of creating and maintaining out-of-tree providers. |
9bcfa5c
to
205ea4d
Compare
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.
minor comments (+ linter had failed). And I'd suggest to comment on all of the exported functions/methods/variables/constants
ctx context.Context, | ||
propnCfg *credspropagation.PropagationCfg, | ||
l logr.Logger, |
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.
is it expected to use contexts without the controller-runtime
's instrumentation meaning without the logger key? if not, then i'd suggest simplifying the signature and removing the logr.Logger
parameter since it is accessible via the context parameter
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.
Not sure what you mean about context. Function itself is called here https://github.com/s3rj1k/hmc/blob/provider-modules/internal/controller/managedcluster_controller.go#L722
Removing logger from signature would force fetching logger as many time as there are providers.
Technically possible to remove.
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.
Yep, I made a mistake. The instrumentation of the logger's key is indeed in the logr package itself, so it'd be possible. I think having hundreds of providers won't affect efficiency too much, and we only expect a handful of providers. To sum up, calling the "sigs.k8s.io/controller-runtime".LoggerFrom(ctx)
within implementations looks okay-ish to me
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.
func (p *Provider) CredentialPropagationFunc() func(
ctx context.Context,
propnCfg *credspropagation.PropagationCfg,
l logr.Logger,
) (enabled bool, err error) {
return func(
ctx context.Context,
propnCfg *credspropagation.PropagationCfg,
) (enabled bool, err error) {
l := ctrl.LoggerFrom(ctx) // ctrl -> "sigs.k8s.io/controller-runtime"
l.Info(p.GetTitleName() + " creds propagation start")
enabled, err = true, PropagateSecrets(ctx, propnCfg)
return enabled, err
}
}
Adds "sigs.k8s.io/controller-runtime"
dependency for plugin, not sure that this is a good idea.
To avoid that we would need to copy this function out of controller-runtime
// FromContext returns a logger with predefined values from a context.Context.
func FromContext(ctx context.Context, keysAndValues ...interface{}) logr.Logger {
log := Log
if ctx != nil {
if logger, err := logr.FromContext(ctx); err == nil {
log = logger
}
}
return log.WithValues(keysAndValues...)
}
|
||
import ( | ||
"github.com/Mirantis/hmc/pkg/manager" | ||
_ "github.com/Mirantis/hmc/pkg/providers/azure" |
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.
the aws
provider is missing?
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.
No, due to how unit-tests are structured aws
is enabled unconditionally.
Basically, unit-tests (mostly webhooks) use aws
fixtures for testing, so there is no reason to have that provider optionally disable and will also break tests.
To make this possible some dummy
provider is needed, or we can have aws
and avoid questions why dummy
provider exists
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.
uh, I see, so basically in terms of runtime, the manager (binary) will have all of the providers (k0s, sveltos (included by default), azure's and vsphere's init functions are called here, and the aws
comes from the tests themselves), correct? if so, i'd comment on it explicitly otherwise it's not very clear (to me personally) from first glance
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.
Not from tests, due to tests aws
is registered in, which makes aws
a Tier-1 provider.
I've gently removed azure
unit-tests dependency, so it's only aws
is like this, forever loved and supported provider.
This basically tells this story, not sure that whole unit-test story needs to be put in comment, if you think this will benefit us, please suggest what to put there.
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.
yep, due to the import from the tests. that's clear. ok then, I think we can skip this one, I will leave this comment for a while if any of the rest reviewers would like to participate, otherwise, I'll resolve it
013a707
to
071312b
Compare
Linter issues are fixed. |
5db2e65
to
c1dc6b4
Compare
c1dc6b4
to
35558c8
Compare
Refactors provider support to make it more modular through:
ProviderModule
interface for consistent provider implementationsSomewhat based on experience of adding new provider in #697