Fix invalid l5d-require-id for some tap requests#3210
Merged
Conversation
PR #3154 introduced an `l5d-require-id` header to Tap requests. That header string was constructed based on the TapByResourceRequest, which includes 3 notable fields (type, name, namespace). For namespace-level requests (via commands like `linkerd tap ns linkerd`), type == `namespace`, name == `linkerd`, and namespace == "". This special casing for namespace-level requests yielded invalid `l5d-require-id` headers, for example: `pd-sa..serviceaccount.identity.linkerd.cluster.local`. Fix `l5d-require-id` string generation to account for namespace-level requests. The bulk of this change is tap unit test updates to validate the fix. Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy
commented
Aug 8, 2019
| ns := req.GetTarget().GetResource().GetNamespace() | ||
| ns := res.GetNamespace() | ||
| if res.GetType() == pkgK8s.Namespace { | ||
| ns = res.GetName() |
Collaborator
|
Integration test results for 5d4a1ab: success 🎉 |
kleimkuhler
approved these changes
Aug 8, 2019
Contributor
kleimkuhler
left a comment
There was a problem hiding this comment.
This looks great! I'd like to be a little more familiar with the reflection going on in the tests, but have not had the chance to step through running this locally. However, the code changes look good and if this passes then 👍 from me for getting this in the edge tomorrow.
| } | ||
|
|
||
| objects, err := s.k8sAPI.GetObjects(req.Target.Resource.Namespace, req.Target.Resource.Type, req.Target.Resource.Name) | ||
| objects, err := s.k8sAPI.GetObjects(res.GetNamespace(), res.GetType(), res.GetName()) |
Contributor
There was a problem hiding this comment.
Thanks for switching these over!
| k8sAPI.Sync() | ||
|
|
||
| err = fakeGrpcServer.TapByResource(&exp.req, &stream) | ||
| if !reflect.DeepEqual(err, exp.err) { |
Contributor
There was a problem hiding this comment.
Ah this is good to see. Thanks for adding this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #3154 introduced an
l5d-require-idheader to Tap requests. Thatheader string was constructed based on the TapByResourceRequest, which
includes 3 notable fields (type, name, namespace). For namespace-level
requests (via commands like
linkerd tap ns linkerd), type ==namespace, name ==linkerd, and namespace == "". This special casingfor namespace-level requests yielded invalid
l5d-require-idheaders,for example:
pd-sa..serviceaccount.identity.linkerd.cluster.local.Fix
l5d-require-idstring generation to account for namespace-levelrequests. The bulk of this change is tap unit test updates to validate
the fix.
Signed-off-by: Andrew Seigner siggy@buoyant.io