Skip to content
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

client: split identity_hook across allocrunner and taskrunner #18431

Merged
merged 44 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
0082084
moved identity hook to allocrunner
pkazmierczak Sep 7, 2023
a476edd
splitting the identity hook
pkazmierczak Sep 8, 2023
75c41b3
fix errors with getIdentities
pkazmierczak Sep 11, 2023
734f8ce
fixed identity_hook errors
pkazmierczak Sep 12, 2023
7d3e21e
clean up
pkazmierczak Sep 12, 2023
b959429
renew
pkazmierczak Sep 13, 2023
8a56035
fix allocHookResrouces signatures
pkazmierczak Sep 13, 2023
0809420
communication between allocrunner and taskrunner hooks via channels
pkazmierczak Sep 14, 2023
1209445
error handling fixes
pkazmierczak Sep 14, 2023
c5dc60e
removed obsolete code and comments
pkazmierczak Sep 14, 2023
0bba786
TaskIdentity helper struct
pkazmierczak Sep 14, 2023
b49a6ea
gofmt snafu
pkazmierczak Sep 14, 2023
c485631
widmgr -> signer; identity_hook -> widmgr
schmichael Sep 15, 2023
bce993c
new allocrunner identity_hook
pkazmierczak Sep 15, 2023
cb1aa30
handle stopping
pkazmierczak Sep 15, 2023
76796ac
wip
pkazmierczak Sep 15, 2023
f7a9ec5
pass widmgr around
pkazmierczak Sep 15, 2023
a2ada2d
handle incoming signed identities in the taskrunner hook
pkazmierczak Sep 15, 2023
f34374b
switch to Watch-only call scheme
schmichael Sep 15, 2023
7e6c2df
initialize the new allocrunner hook
pkazmierczak Sep 18, 2023
b17de6d
allocrunner identity_hook unit test
pkazmierczak Sep 18, 2023
d2fa276
remove widmgr from client
pkazmierczak Sep 18, 2023
19903b1
moved things around
pkazmierczak Sep 18, 2023
adc07da
taskrunner hook unit tests
pkazmierczak Sep 19, 2023
3679206
superfluous newIdentityHook call
pkazmierczak Sep 19, 2023
265f6cd
signer should error if there are no identities to sign, and NewIdenti…
pkazmierczak Sep 19, 2023
48079f4
client: handle tasks without alt identities and fix tests
lgfa29 Sep 19, 2023
e8ef776
addressed review comments from @tgross
pkazmierczak Sep 20, 2023
e3dd3d3
applied Luiz's review comments
pkazmierczak Sep 20, 2023
60d0a95
Update client/allocrunner/alloc_runner.go
pkazmierczak Sep 20, 2023
fa36330
Merge branch 'main' into f-consul-wi-consul_hook
pkazmierczak Sep 20, 2023
9592485
Apply suggestions from code review
pkazmierczak Sep 20, 2023
f5aa137
multierror in the RPC
pkazmierczak Sep 20, 2023
e2852ff
Merge branch 'f-consul-wi-consul_hook' of github.com:hashicorp/nomad …
pkazmierczak Sep 20, 2023
9e47778
gofmt
pkazmierczak Sep 20, 2023
ea9635d
addressed more of Luiz's comments
pkazmierczak Sep 20, 2023
7c5bdc5
close watchers on stopCtx.Done()
pkazmierczak Sep 20, 2023
ddce193
fix widspec loop
pkazmierczak Sep 20, 2023
5200e7a
revert m.send select
pkazmierczak Sep 20, 2023
0bdce1d
multiple watchers (map of maps)
pkazmierczak Sep 21, 2023
3de57bd
multiple watchers (map of slices)
pkazmierczak Sep 21, 2023
75aa938
logger name for widmgr
pkazmierczak Sep 21, 2023
eb1aba3
do not set minExp to arbitrary value
pkazmierczak Sep 21, 2023
dad0e65
applied Tim's comments
pkazmierczak Sep 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions client/allocrunner/alloc_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,11 @@ type allocRunner struct {
// partitions is an interface for managing cpuset partitions
partitions cinterfaces.CPUPartitions

// widmgr fetches workload identities
widmgr *widmgr.WIDMgr
// widsigner signes workload identities
widsigner widmgr.IdentitySigner

// widmgr manages workload identity signatures
widmgr widmgr.IdentityManager
}

// NewAllocRunner returns a new allocation runner.
Expand Down Expand Up @@ -251,7 +254,7 @@ func NewAllocRunner(config *config.AllocRunnerConfig) (interfaces.AllocRunner, e
wranglers: config.Wranglers,
partitions: config.Partitions,
hookResources: cstructs.NewAllocHookResources(),
widmgr: config.WIDMgr,
widsigner: config.WIDSigner,
}

// Create the logger based on the allocation ID
Expand All @@ -269,6 +272,10 @@ func NewAllocRunner(config *config.AllocRunnerConfig) (interfaces.AllocRunner, e
ar.shutdownDelayCtx = shutdownDelayCtx
ar.shutdownDelayCancelFn = shutdownDelayCancel

// initialize the workload identity manager
widmgr := widmgr.NewWIDMgr(ar.widsigner, alloc, ar.logger)
ar.widmgr = widmgr

// Initialize the runners hooks.
if err := ar.initRunnerHooks(config.ClientConfig); err != nil {
return nil, err
Expand Down
1 change: 1 addition & 0 deletions client/allocrunner/alloc_runner_hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func (ar *allocRunner) initRunnerHooks(config *clientconfig.Config) error {
// directory path exists for other hooks.
alloc := ar.Alloc()
ar.runnerHooks = []interfaces.RunnerHook{
newIdentityHook(hookLogger, ar),
newAllocDirHook(hookLogger, ar.allocDir),
newUpstreamAllocsHook(hookLogger, ar.prevAllocWatcher),
newDiskMigrationHook(hookLogger, ar.prevAllocMigrator, ar.allocDir),
Expand Down
51 changes: 51 additions & 0 deletions client/allocrunner/identity_hook.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package allocrunner

import (
"context"

log "github.com/hashicorp/go-hclog"
"github.com/hashicorp/nomad/client/allocrunner/interfaces"
"github.com/hashicorp/nomad/client/widmgr"
)

type identityHook struct {
ar *allocRunner
widmgr widmgr.IdentityManager
logger log.Logger
}

func newIdentityHook(logger log.Logger, ar *allocRunner) *identityHook {
h := &identityHook{
ar: ar,
widmgr: ar.widmgr,
}
h.logger = logger.Named(h.Name())
return h
}

func (*identityHook) Name() string {
return "identity"
}

func (h *identityHook) Prerun() error {
// run the renewal
if err := h.widmgr.Run(); err != nil {
return err
}

return nil
}

// Stop implements interfaces.TaskStopHook
func (h *identityHook) Stop(context.Context, *interfaces.TaskStopRequest, *interfaces.TaskStopResponse) error {
h.widmgr.Shutdown()
return nil
}

// Shutdown implements interfaces.ShutdownHook
func (h *identityHook) Shutdown() {
h.widmgr.Shutdown()
}
84 changes: 84 additions & 0 deletions client/allocrunner/identity_hook_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package allocrunner

import (
"context"
"testing"
"time"

"github.com/shoenig/test/must"

"github.com/hashicorp/nomad/ci"
"github.com/hashicorp/nomad/client/allocrunner/interfaces"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/client/widmgr"
"github.com/hashicorp/nomad/helper/testlog"
"github.com/hashicorp/nomad/nomad/mock"
"github.com/hashicorp/nomad/nomad/structs"
)

// statically assert network hook implements the expected interfaces
var _ interfaces.RunnerPrerunHook = (*identityHook)(nil)
var _ interfaces.ShutdownHook = (*identityHook)(nil)
var _ interfaces.TaskStopHook = (*identityHook)(nil)

func TestIdentityHook_Prerun(t *testing.T) {
ci.Parallel(t)

ttl := 30 * time.Second

wid := &structs.WorkloadIdentity{
Name: "testing",
Audience: []string{"consul.io"},
Env: true,
File: true,
TTL: ttl,
}

alloc := mock.Alloc()
task := alloc.LookupTask("web")
task.Identity = wid
task.Identities = []*structs.WorkloadIdentity{wid}

allocrunner, stopAR := TestAllocRunnerFromAlloc(t, alloc)
defer stopAR()

logger := testlog.HCLogger(t)

// setup mock signer and WIDMgr
mockSigner := widmgr.NewMockWIDMgr(task.Identities)
mockWIDMgr := widmgr.NewWIDMgr(mockSigner, alloc, logger)
allocrunner.widmgr = mockWIDMgr
allocrunner.widsigner = mockSigner

// do the initial signing
_, err := mockSigner.SignIdentities(1, []*structs.WorkloadIdentityRequest{
{
AllocID: alloc.ID,
TaskName: task.Name,
IdentityName: task.Identities[0].Name,
},
})
must.NoError(t, err)

start := time.Now()
hook := newIdentityHook(logger, allocrunner)
must.Eq(t, hook.Name(), "identity")
must.NoError(t, hook.Prerun())

sid, err := hook.widmgr.Get(cstructs.TaskIdentity{TaskName: task.Name, IdentityName: task.Identities[0].Name})
Copy link
Member

Choose a reason for hiding this comment

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

The hook.Prerun runs the widmgr in a goroutine. Do we need to ensure it's run at least once before we call this Get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should. Is there a smart way of doing it? Elsewhere in tests I just time.Sleep(time.Second) which is... inelegant.

Copy link
Member

Choose a reason for hiding this comment

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

Usually I tried to check for a side effect. Ex. because this is a unit test you could try to grab the internal lock and check whatever state that Get expects before calling Get. Or you could use must.Wait to retry the Get until the state converges the way you expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, here it's hard because the state is only available to WIDMgr, the lock isn't public and I can't access it from the unit test. I'll just time.Sleep(time.Second) like we do in the taskrunner tests.

must.Nil(t, err)
must.Eq(t, sid.IdentityName, task.Identity.Name)
must.NotEq(t, sid.JWT, "")

// pad expiry time with a second to be safe
must.True(t, inTimeSpan(start.Add(ttl).Add(-1*time.Second), start.Add(ttl).Add(1*time.Second), sid.Expiration))

must.NoError(t, hook.Stop(context.Background(), nil, nil))
}

func inTimeSpan(start, end, check time.Time) bool {
return check.After(start) && check.Before(end)
}
Loading