-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
JWT Authentication with service intentions: xds package update #17414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Working in xds is not easy but this looks good.
I had a few blocking comments and general questions
6e2b7b6
to
50ba1fd
Compare
agent/xds/jwt_authn.go
Outdated
rule := buildRouteRule(provider, nil, "/") | ||
rules = append(rules, rule) | ||
} | ||
} | ||
} | ||
|
||
if len(intentions) == 0 && len(providers) == 0 { | ||
if len(intentions) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved before the for-loop over intentions
for the early return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check for len(providers) == 0
following the loop as well, right? For the common case where there are intentions but no intentions contain any JWT requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, thanks for the correction. Maybe this can stay here and simplify to if len(providers) == 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah true, i will update it! thank you both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more minor comment but LGTM! 👍
6421487
to
1146f2a
Compare
* JWT Authentication with service intentions: update xds package to translate config to envoy
Description
This PR enables envoy config updates when we write jwt providers/intentions. Porting these changes from the enterprise PR
Testing & Reproduction steps
./bin/consul agent -dev
consul services register your-service.hcl your-proxy.hcl
consul connect envoy -sidecar-for your-service -grpc-addr 127.0.0.1:8502
consul config write provider.hcl intention.hcl
curl "localhost:19000/config_dump?format=json" > test.json
*.golden
in this PRsample intention
proxy-default to enable http mode
Links
todo