-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
transparent proxy: add jobspec support #20144
Conversation
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
fe05196
to
7f8c8f9
Compare
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.
Had some questions and it looks like the Equals func could have a bug.
e, ok := err.(*strconv.NumError) | ||
if !ok { | ||
return fmt.Errorf("invalid user ID %q: %w", uidRaw, err) | ||
} | ||
return fmt.Errorf("invalid user ID %q: %w", uidRaw, e.Err) |
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.
Since the error output is the same, could we simplify this to:
e, ok := err.(*strconv.NumError) | |
if !ok { | |
return fmt.Errorf("invalid user ID %q: %w", uidRaw, err) | |
} | |
return fmt.Errorf("invalid user ID %q: %w", uidRaw, e.Err) | |
return fmt.Errorf("invalid user ID %q: %w", uidRaw, e.Err) |
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.
strconv
doesn't guarantee that we always get a strconv.NumError
(although the current implementation always does), so we can't guarantee there's a Err
field.
But also, it turns out the error output isn't the same, and the Err
field removes repetition and Golang jargon the user doesn't care about. For example, on a range error for an input of max uint32:
- using
e.Error()
would beinvalid user UID "4294967295": strconv.ParseUint parsing "4294967295": value is out of range
- using
e.Err
would be the much nicerinvalid user UID "4294967295": value is out of range
|
||
// UID of the Envoy proxy. Defaults to the default Envoy proxy container | ||
// image user. | ||
UID string |
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.
In the implementation, we check in validate if the provided UIDs are uints. Is the choice of string here to futureproof the value? Same question for ExcludeUIDs.
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.
The Consul SDK we're writing to in the network configuration takes strings (likely for futureproofing, as you've noted), so we're using the same types here.
for _, port := range tp.ExcludeOutboundPorts { | ||
hashIntIfNonZero(h, "ExcludeOutboundPorts", int(port)) | ||
} |
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.
Could/should this be expressed as
hashTags(h, helper.ConvertSlice(tp.ExcludeOutboundPorts, func(a uint16) string { return fmt.Sprint(a) } )
to match the consulTProxyDiff
func?
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.
The hash and the diff have two different consumers. The diff function is for showing the user plan diffs. Whereas the hash is for creating a stable service hash for purposes of creating a deterministic service ID we write to Consul. I'm not sure it makes sense to do extra allocations for this.
That being said given your question about the Equal
method, maybe these all need to be sorted first.
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.
Hm, having thought about it a bit more... all of the hash functions over slices take the user's order, even if the order isn't meaningful to Consul. The tags
is a good example of that. Maybe better to keep similar to the other hash behavior.
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.
🚀
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client. Ref: #10628
Add a transparent proxy block to the existing Connect sidecar service proxy block. This changeset is plumbing required to support transparent proxy configuration on the client, and is first of a series of PRs that will target the
f-tproxy
branch which we intend to ship in Nomad 1.8.Ref: #10628
Ref: NMD-191 (internal link)