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

transparent proxy: add jobspec support #20144

Merged
merged 3 commits into from
Mar 21, 2024
Merged

transparent proxy: add jobspec support #20144

merged 3 commits into from
Mar 21, 2024

Conversation

tgross
Copy link
Member

@tgross tgross commented Mar 15, 2024

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)

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
Copy link
Contributor

@angrycub angrycub left a 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.

Comment on lines +119 to +123
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)
Copy link
Contributor

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:

Suggested change
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)

Copy link
Member Author

@tgross tgross Mar 20, 2024

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 be invalid user UID "4294967295": strconv.ParseUint parsing "4294967295": value is out of range
  • using e.Err would be the much nicer invalid user UID "4294967295": value is out of range

nomad/structs/connect.go Outdated Show resolved Hide resolved
nomad/structs/connect.go Outdated Show resolved Hide resolved
nomad/structs/diff.go Outdated Show resolved Hide resolved
nomad/structs/diff.go Outdated Show resolved Hide resolved

// UID of the Envoy proxy. Defaults to the default Envoy proxy container
// image user.
UID string
Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines +1026 to +1028
for _, port := range tp.ExcludeOutboundPorts {
hashIntIfNonZero(h, "ExcludeOutboundPorts", int(port))
}
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

nomad/structs/connect.go Show resolved Hide resolved
Copy link
Contributor

@angrycub angrycub left a comment

Choose a reason for hiding this comment

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

🚀

@tgross tgross merged commit 1845508 into f-tproxy Mar 21, 2024
18 checks passed
@tgross tgross deleted the f-tproxy-jobspec branch March 21, 2024 13:22
@tgross tgross mentioned this pull request Mar 21, 2024
10 tasks
tgross added a commit that referenced this pull request Mar 22, 2024
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
tgross added a commit that referenced this pull request Mar 26, 2024
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
tgross added a commit that referenced this pull request Mar 27, 2024
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
tgross added a commit that referenced this pull request Mar 27, 2024
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
tgross added a commit that referenced this pull request Mar 29, 2024
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
tgross added a commit that referenced this pull request Apr 3, 2024
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
tgross added a commit that referenced this pull request Apr 4, 2024
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
tgross added a commit that referenced this pull request Apr 4, 2024
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
philrenaud pushed a commit that referenced this pull request Apr 18, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants