Proof of concept of a common plugin base #47972
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
hugoShaka
changed the title
| func (a *Launcher) checkTeleportVersion(ctx context.Context) (proto.PingResponse, error) { | ||
| pong, err := a.teleportClient.Ping(ctx) | ||
| if err != nil { | ||
| if trace.IsNotImplemented(err) { | ||
| return pong, trace.Wrap(err, "server version must be at least %s", minServerVersion) | ||
| } | ||
| return pong, trace.Wrap(err, "Unable to get Teleport server version") | ||
| } | ||
| err = utils.CheckMinVersion(pong.ServerVersion, minServerVersion) | ||
| return pong, trace.Wrap(err) | ||
| } |
There was a problem hiding this comment.
Is this still relevant? The minimum version this would be released in is v15 which seems highly unlikely to be running against a v6 control plane.
| return trace.Wrap(err) | ||
| } | ||
|
|
||
| a.PluginName = a.Conf.PluginName() |
There was a problem hiding this comment.
Why does this need to be updated?
| } | ||
|
|
||
| type Service interface { | ||
| Run(ctx context.Context, clt teleport.Client, name string /* TODO add status sink */) error |
There was a problem hiding this comment.
What does the name here convey? Why does it need to be given to the service?
| return wrapAPIClient(clt), nil | ||
| } | ||
|
|
||
| // TODO: check if we can get rid of this? |
There was a problem hiding this comment.
+1 - Can teleport.Client be updated to use the matching signature so this can be eliminated?
| group.Go(func() error { | ||
| return service.Run(ctx, a.teleportClient, a.PluginName) | ||
| }) | ||
| } | ||
|
|
||
| return group.Wait() |
There was a problem hiding this comment.
What is the expected behavior if any one of the services fails, causing this to return? Do we expect systemd/kubernetes to restart the plugin?
| } | ||
|
|
||
| group, ctx := errgroup.WithContext(ctx) | ||
| for _, service := range a.Services { |
There was a problem hiding this comment.
Should we warn if there are no services configured?
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
|
|
||
| package common |
There was a problem hiding this comment.
Suggestion: avoid packages like common, utils, helpers, etc.
https://go.dev/wiki/CodeReviewComments#package-names
https://go.dev/doc/effective_go#package-names
| type accessListReminderService struct { | ||
| // provided on creation | ||
| notifier Notifier | ||
| clock clockwork.Clock | ||
|
|
||
| // provided at runtime | ||
| pluginName string | ||
| teleportClient teleport.Client | ||
|
|
||
| // initialized but the service itself | ||
| pluginData *pd.CompareAndSwap[pd.AccessListNotificationData] | ||
| } | ||
|
|
||
| func NewAccessListReminderService(notifier Notifier) launcher.Service { | ||
| return &accessListReminderService{ | ||
| notifier: notifier, | ||
| clock: clockwork.NewRealClock(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: remove the stutter and export the service
https://go.dev/wiki/CodeReviewComments#interfaces
https://go.dev/doc/effective_go#package-names
| type accessListReminderService struct { | |
| // provided on creation | |
| notifier Notifier | |
| clock clockwork.Clock | |
| // provided at runtime | |
| pluginName string | |
| teleportClient teleport.Client | |
| // initialized but the service itself | |
| pluginData *pd.CompareAndSwap[pd.AccessListNotificationData] | |
| } | |
| func NewAccessListReminderService(notifier Notifier) launcher.Service { | |
| return &accessListReminderService{ | |
| notifier: notifier, | |
| clock: clockwork.NewRealClock(), | |
| } | |
| } | |
| type ReminderService struct { | |
| // provided on creation | |
| notifier Notifier | |
| clock clockwork.Clock | |
| // provided at runtime | |
| pluginName string | |
| teleportClient teleport.Client | |
| // initialized but the service itself | |
| pluginData *pd.CompareAndSwap[pd.AccessListNotificationData] | |
| } | |
| func NewReminderService(notifier Notifier) *ReminderService { | |
| return &ReminderService{ | |
| notifier: notifier, | |
| clock: clockwork.NewRealClock(), | |
| } | |
| } |
| a.teleportClient = clt | ||
| a.pluginName = name | ||
|
|
||
| a.pluginData = pd.NewCAS( |
There was a problem hiding this comment.
Isn't this a bit racy?
| type accessRequestService struct { | ||
| // provided on creation | ||
| notifier Notifier | ||
| approvalUser string | ||
| staticRecipients common.RawRecipientsMap | ||
|
|
||
| // provided at runtime | ||
| pluginName string | ||
| teleportClient teleport.Client | ||
|
|
||
| // initialized but the service itself | ||
| pluginData *pd.CompareAndSwap[PluginData] | ||
| accessMonitoringRules *accessmonitoring.RuleHandler | ||
| } | ||
|
|
||
| func NewAccessRequestService(notifier Notifier, staticRecipients common.RawRecipientsMap, approvalUser string) launcher.Service { | ||
| return &accessRequestService{ | ||
| notifier: notifier, | ||
| approvalUser: approvalUser, | ||
| staticRecipients: staticRecipients, | ||
| } | ||
| } |
There was a problem hiding this comment.
Same suggestions here as in the other package
| type accessRequestService struct { | |
| // provided on creation | |
| notifier Notifier | |
| approvalUser string | |
| staticRecipients common.RawRecipientsMap | |
| // provided at runtime | |
| pluginName string | |
| teleportClient teleport.Client | |
| // initialized but the service itself | |
| pluginData *pd.CompareAndSwap[PluginData] | |
| accessMonitoringRules *accessmonitoring.RuleHandler | |
| } | |
| func NewAccessRequestService(notifier Notifier, staticRecipients common.RawRecipientsMap, approvalUser string) launcher.Service { | |
| return &accessRequestService{ | |
| notifier: notifier, | |
| approvalUser: approvalUser, | |
| staticRecipients: staticRecipients, | |
| } | |
| } | |
| type Service struct { | |
| // provided on creation | |
| notifier Notifier | |
| approvalUser string | |
| staticRecipients common.RawRecipientsMap | |
| // provided at runtime | |
| pluginName string | |
| teleportClient teleport.Client | |
| // initialized but the service itself | |
| pluginData *pd.CompareAndSwap[PluginData] | |
| accessMonitoringRules *accessmonitoring.RuleHandler | |
| } | |
| func NewService(notifier Notifier, staticRecipients common.RawRecipientsMap, approvalUser string) *Service { | |
| return &Service{ | |
| notifier: notifier, | |
| approvalUser: approvalUser, | |
| staticRecipients: staticRecipients, | |
| } | |
| } |
No description provided.