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.

Sign up for GitHub

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

Jump to bottom

Fixed workload attestation when using AWS Node Resolver #327

Merged
merged 2 commits into from
Jan 25, 2018
Merged

Fixed workload attestation when using AWS Node Resolver #327

merged 2 commits into from
Jan 25, 2018

Conversation

paul-argeniss
Copy link
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Workloads fail to receive identity when using AWS Node Resolver plugin.

Description of change

  • Changed fetchRegistrationEntries so that it looks for all possible combinations of selectors.
  • Changed Cache Manager to identity the registration entry with the magic parentSpiffeID (feel free to suggest a better name) that identities the node entry and keep track of its spiffeID.

Which issue this PR fixes
Fixes #326

@ghost ghost added the CLA label Jan 19, 2018
@paul-argeniss paul-argeniss force-pushed the fix-drew branch 2 times, most recently from d9f240d to e8d611f Compare January 19, 2018 21:48
evan2645
evan2645 reviewed Jan 20, 2018
View reviewed changes
pkg/agent/cache/manager.go
@@ -76,6 +78,8 @@ type manager struct {
once sync.Once
doneCh chan struct{}
pipelineCnt int32
aliasSPIFFEID string
magicSPIFFEID string
Copy link
Member

Choose a reason for hiding this comment

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

nit: serverSPIFFEID

Copy link
Member

Choose a reason for hiding this comment

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

These should probably be URL type instead of string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

serverSPIFFEID already exists here and it's a string, that's why I'm also using strings here. I can change it if you feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

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

it's ok, we can fix it later. we'll probably need to refactor all this anyways once the workload api updates land.

pkg/agent/cache/manager.go Outdated
entry := m.getBaseSVIDEntry()
svid = entry.svid
key = entry.key
} else {
m.log.Debug("Unknown parent ", parentId, "... ignoring")
Copy link
Member

Choose a reason for hiding this comment

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

An info or warn level may be more appropriate, since reaching this point means something is probably wrong. Also, you can use Infof or Warnf for proper string formatting

evan2645
evan2645 reviewed Jan 21, 2018
View reviewed changes
pkg/server/node.go Outdated
if err != nil {
return nil, err
}
for combination := range selector.PowerSet(selector.NewSet(selectors)) {
Copy link
Member

Choose a reason for hiding this comment

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

Tests to capture this updated logic would be awesome... I feel we'll probably refactor this handler in the near future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test.

kunzimariano
kunzimariano reviewed Jan 22, 2018
View reviewed changes
functional/Makefile Outdated
@@ -13,7 +13,7 @@ all: clean build server agent test
build:
docker-compose build runner
docker-compose up -d runner
docker-compose exec runner bash -c 'mkdir -p $(DOCKER_SPIRE) && cp -R /spire-orig/* $(DOCKER_SPIRE)'
docker-compose exec runner bash -c 'mkdir -p $(DOCKER_SPIRE) $(DOCKER_SPIRE)/.data && cp -R /spire-orig/* $(DOCKER_SPIRE)'
Copy link
Member

Choose a reason for hiding this comment

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

Given that we are using mkdir -p, isn't doing
mkdir -p $(DOCKER_SPIRE)/.data
enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

evan2645
evan2645 previously approved these changes Jan 23, 2018
View reviewed changes
Copy link
Member

@evan2645 evan2645 left a comment

Choose a reason for hiding this comment

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

LGTM

@drrt drrt merged commit c37711f into spiffe:master Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kunzimariano kunzimariano kunzimariano left review comments

@walmav walmav walmav approved these changes

@evan2645 evan2645 evan2645 approved these changes

@drrt drrt drrt approved these changes

@amartinezfayo amartinezfayo Awaiting requested review from amartinezfayo

@y2bishop2y y2bishop2y Awaiting requested review from y2bishop2y

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Workloads fail to receive identity when using AWS Node Resolver plugin
5 participants
@paul-argeniss @kunzimariano @drrt @evan2645 @walmav