server: break up Intention.Apply monolithic method #9007
Conversation
| authz acl.Authorizer, | ||
| entMeta *structs.EnterpriseMeta, | ||
| args *structs.IntentionRequest, | ||
| ) (*structs.ServiceIntentionsConfigEntry, error) { | ||
| if !args.Intention.CanWrite(authz) { |
There was a problem hiding this comment.
Previously this method did 2 acl checks because destination renames were allowed. I removed one of them, and for consistency with delete-by-id I moved it to after the intention lookup.
rboyer
force-pushed
the
split-intention-apply-rpc
branch
from
e0079e0
to
c2e82e2
Compare
rboyer
force-pushed
the
split-intention-apply-rpc
branch
from
c2e82e2
to
efe47e4
Compare
| if done, err := s.srv.ForwardRPC("Intention.Apply", args, args, reply); done { | ||
| return err | ||
| } | ||
| defer metrics.MeasureSince([]string{"consul", "intention", "apply"}, time.Now()) |
There was a problem hiding this comment.
This predates this PR, but any idea why there are two of these?
There was a problem hiding this comment.
My guess is that we had failed to "namespace" the metrics initially and came back to fix that, but left the old ones around "for a short time". @mkcp
| } | ||
| deleted := upsertEntry.DeleteSourceByLegacyID(args.Intention.ID) | ||
| if !deleted { | ||
| return "", nil, fmt.Errorf("Cannot delete non-existent intention: '%s'", args.Intention.ID) |
There was a problem hiding this comment.
Hmm explaining why this function failed from the outside seems a bit presumptive. It's fine now, but there could be other sources of failure down the line. Wondering if this would benefit from returning delete, found or maybe have a more generic error message.
Thoughts?
There was a problem hiding this comment.
So in this particular method computeApplyChangesLegacyDelete I was trying extra hard to keep the same weird semantics that the legacy intention delete-by-ID had.
In the newer delete-by-source-and-dest version, deletes are idempotent and if you delete something that doesn't exist it does nothing and returns 👍
| } else { | ||
| *reply = "" | ||
| } | ||
| // NOTE: validation errors may be misleading! |
There was a problem hiding this comment.
Where is the validation invoked? Maybe this comment should be moved closer to where it would apply
There was a problem hiding this comment.
So I left this in for myself and didn't come back and figure out how to improve things. In the #9151 followup I think I actually removed these comments for now.
The gist is that all of the real validation happens in the config entry Validate() method, and the error messages refer to fields on the config entry, not the structs.Intention object when those errors eventually percolate up and fail on the legacy api endpoints instead.
|
🍒 Starting backport cherry picking. To cherry-pick post-merge, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/280557. |
|
🍒✅ Cherry pick of commit c003871 onto |
The Intention.Apply RPC is quite large, so this PR attempts to break it down into smaller functions and dissolves the pre-config-entry approach to the breakdown as it only confused things.
The
Intention.ApplyRPC is quite large, so this PR attempts to break it down into smaller functions and dissolves the pre-config-entry approach to the breakdown as it only confused things.