Skip to content

feat(sdk): static stub configuration - #18

Merged
joe0BAB merged 7 commits into
mainfrom
feat/stub-iteration-2
Jun 20, 2025
Merged

feat(sdk): static stub configuration#18
joe0BAB merged 7 commits into
mainfrom
feat/stub-iteration-2

Conversation

@joe0BAB

@joe0BABjoe0BAB commented Jun 17, 2025

Copy link
Copy Markdown
Collaborator

There's a set of static configuration on the stub that either

  • gets set from the secret engine for automatically launched plugins
  • gets set via options for manually launched plugins
  • gets implicitly pulled from the binary name for manually launched plugins

As this is somewhat implicit but very externally facing, I want to make sure everything is well tested and documented.

@joe0BABjoe0BAB self-assigned this Jun 17, 2025
@joe0BAB
joe0BAB marked this pull request as ready for review June 17, 2025 15:05
@joe0BAB
joe0BAB requested a review from BenehikoJune 17, 2025 15:07
Benehiko
Benehiko reviewed Jun 17, 2025
View reviewed changes
Comment threadpkg/adaptation/adaptation.go Outdated
Comment threadpkg/stub/stub.go
"context"
"time"

"github.com/sirupsen/logrus"

@BenehikoBenehikoJun 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can replace logrus in favor of a better logger - this is a discussion for another time, but just thought I would call it out.

@joe0BABjoe0BABJun 17, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix logging is on my todo list for later as well. Just wanted to have something so I can place log statements. But don't like it much either.

@BenehikoBenehikoJun 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zap looks pretty good
https://github.com/uber-go/zap

Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
@joe0BAB
joe0BABforce-pushed the feat/stub-iteration-2 branch from 3baf6e6 to e403ca7CompareJune 17, 2025 16:03
@joe0BAB
joe0BAB requested a review from BenehikoJune 18, 2025 08:54
joe0BAB
joe0BAB commented Jun 18, 2025
View reviewed changes
Comment threadpkg/stub/config.go Outdated
Benehiko
Benehiko reviewed Jun 18, 2025
View reviewed changes
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/stub/config.go Outdated
@joe0BAB
joe0BABforce-pushed the feat/stub-iteration-2 branch 2 times, most recently from 3040893 to da765d0CompareJune 18, 2025 14:39
@joe0BAB
joe0BAB requested a review from BenehikoJune 18, 2025 14:41
@joe0BAB
joe0BABforce-pushed the feat/stub-iteration-2 branch 2 times, most recently from bef220d to c2a33c4CompareJune 18, 2025 14:54
joe0BAB
joe0BAB commented Jun 18, 2025
View reviewed changes
Comment threadpkg/stub/config.go
if cfg.name == "" {
if len(os.Args) == 0 {
// This should never happen in practice but can happen in tests or when something else empties os.Args for whatever reason.
return nil, errors.New("plugin name must be specified (could not derive from os.Args)")

@joe0BABjoe0BABJun 18, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit unfortunate. But I believe eg go test does it (that made the tests crash on CI so I noticed). Otherwise we could have moved that up to the line where the struct gets created initially.

@joe0BAB
joe0BABforce-pushed the feat/stub-iteration-2 branch from c2a33c4 to f09f590CompareJune 18, 2025 15:13
@joe0BAB
joe0BABforce-pushed the feat/stub-iteration-2 branch from f09f590 to 1736c4dCompareJune 18, 2025 15:14
Benehiko
Benehiko reviewed Jun 18, 2025
View reviewed changes
Comment threadpkg/stub/config.go Outdated
Comment threadpkg/adaptation/adaptation.go
Comment on lines +51 to +53
if result.RegistrationTimeout == 0 {
return nil, errors.New("plugin registration timeout is required")
}

@BenehikoBenehikoJun 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if it's necessary to error on a timeout value, perhaps just fallback to a sane default instead?

@joe0BABjoe0BABJun 20, 2025

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep for now. In NRI they didn't error on this which made them miss that in NRI configuring the registration timeout is actually bugged / doesn't work.

Comment threadpkg/adaptation/adaptation.go Outdated
Comment threadpkg/adaptation/adaptation.go Outdated
Comment threadpkg/adaptation/adaptation_test.go Outdated
Comment threadpkg/stub/config_test.go Outdated
Comment threadpkg/stub/config_test.go Outdated
@joe0BAB
joe0BAB requested a review from BenehikoJune 20, 2025 07:34
Benehiko
Benehiko reviewed Jun 20, 2025
View reviewed changes

@BenehikoBenehiko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change, then we're good to go!

Comment threadpkg/stub/config.go Outdated
@joe0BAB
joe0BABforce-pushed the feat/stub-iteration-2 branch from deff3f1 to 292648eCompareJune 20, 2025 08:15
@joe0BAB
joe0BAB requested a review from BenehikoJune 20, 2025 08:16
@joe0BAB
joe0BAB merged commit 95e012a into mainJun 20, 2025
@joe0BAB
joe0BAB deleted the feat/stub-iteration-2 branch June 20, 2025 08:18
@joe0BABjoe0BAB linked an issue Jun 23, 2025 that may be closed by this pull request
Stub (plugin client SDK part) V0 #27
Closed
@joe0BABjoe0BAB mentioned this pull request Jun 23, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenehikoBenehikoBenehiko approved these changes

Assignees

@joe0BABjoe0BAB

Labels

None yet

Projects

None yet

Milestone

Secrets Engine V0: Daemon & Plugin Sy...

Development

Successfully merging this pull request may close these issues.

Stub (plugin client SDK part) V0

2 participants

@joe0BAB@Benehiko