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

Fixes Consul token checking when policies exist within namespaces #18516

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Fixes Consul token checking when policies exist within namespaces #18516

merged 4 commits into from
Dec 11, 2023

Conversation

t-davies
Copy link
Contributor

@t-davies t-davies commented Sep 15, 2023

Re: support case #122732

Fixes a bug where Nomad fails to fetch Consul policies whilst checking a Consul token presented when submitting a job, if the policies attached to that token exist within a namespace.

[ERROR] agent.http: Request error: method=GET url=/v1/acl/policy/83073614-xxxx-xxxx-xxxx-xxxxxxxxxxxx from=127.0.0.1:54668 error="Requested policy does not exist: ACL not found"

  • When querying policies from Consul, use the namespace of the token rather than the default namespace.

e2e/connect/acls.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pkazmierczak pkazmierczak left a comment

Choose a reason for hiding this comment

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

Hey @t-davies, thanks for the bugfix, the code looks good to me!

FYI in Nomad 1.7 we will be introducing support for Workload Identity tokens, so that jobs will be able to login into Consul via JWT https://www.hashicorp.com/blog/nomad-1-7-improves-vault-and-consul-integrations-adds-numa-support

@shoenig
Copy link
Member

shoenig commented Nov 30, 2023

The actual fix LGTM but I think the e2e test needs to account for the namespace'd token on deletion

=== RUN   TestE2E/ConnectACLs/*connect.ConnectACLsE2ETest/TestConnectACLsConnectDemoNamespaced
    acls.go:272: test register Connect job w/ ACLs enabled w/ operator token
    acls.go:278: created namespace: ns-a612e32f
    acls.go:286: created operator policy: 2797be55-60b8-8797-c2db-d119405a7342
    acls.go:290: created operator token: a9828584-2ec1-b352-d31d-c4d69ef40abb
    acls.go:307: connect legacy job with ACLs enable finished
    acls.go:67: cleanup: deregister nomad job id: connectfaaa960e
    acls.go:74: cleanup: delete consul token id: 4e616feb-0a73-40e0-966f-b8412a8d3954
    acls.go:76:
                Error Trace:    /home/shoenig/Go/nomad/e2e/connect/acls.go:76
                                                        /home/shoenig/Go/nomad/e2e/framework/framework.go:265
                                                        /home/shoenig/Go/nomad/e2e/framework/framework.go:271
                Error:          Received unexpected error:
                                Unexpected response code: 404 (Cannot find token to delete)
                Test:           TestE2E/ConnectACLs/*connect.ConnectACLsE2ETest/TestConnectACLsConnectDemoNamespaced

@shoenig shoenig self-requested a review November 30, 2023 20:05
Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

.

@shoenig shoenig added this to the 1.7.x milestone Nov 30, 2023
@shoenig shoenig added backport/1.5.x backport to 1.5.x release line backport/1.6.x backport to 1.6.x release line labels Nov 30, 2023
@tgross tgross added the backport/1.7.x backport to 1.7.x release line label Dec 1, 2023
@t-davies
Copy link
Contributor Author

t-davies commented Dec 6, 2023

The actual fix LGTM but I think the e2e test needs to account for the namespace'd token on deletion

Thanks, good spot. I'm passing Namespace when deleting these (and the policies) now - also noticed we didn't clean up the namespaces either so getting rid of these too.

Once the tests run and are happy, I'll rebase the fixup!.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; thanks @t-davies!

@t-davies t-davies deleted the bugfix/consul-ent-policy-namespaces branch April 7, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.5.x backport to 1.5.x release line backport/1.6.x backport to 1.6.x release line backport/1.7.x backport to 1.7.x release line hcc/cst Admin - internal
Projects
Development

Successfully merging this pull request may close these issues.

4 participants