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

balancerd: per-tenant metrics for http #25952

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

maddyblue
Copy link
Contributor

@maddyblue maddyblue commented Mar 12, 2024

Extract the tenant from the CNAME.

Motivation

  • This PR adds a feature that has not yet been specified.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • n/a

@pH14
Copy link
Contributor

pH14 commented Mar 12, 2024

Will this capture webhook calls as well?

@alex-hunt-materialize
Copy link
Contributor

This assumes the SNI servername and frontegg tenants are the same.

Unfortunately, the SNI servername is not the tenant. The CNAME response to looking up that SNI servername does include the tenant ID, though.

@maddyblue
Copy link
Contributor Author

Will this capture webhook calls as well?

Yes!

@maddyblue maddyblue force-pushed the balancer-http-metrics branch 2 times, most recently from 7ef8cf0 to 147fd1f Compare March 13, 2024 23:37
@maddyblue
Copy link
Contributor Author

maddyblue commented Mar 13, 2024

Lint is producing errors like

error[duplicate]: found 2 duplicate entries for crate 'enum-as-inner'
    ┌─ /home/mjibson/materialize/Cargo.lock:173:1
    │  
173 │ ╭ enum-as-inner 0.5.1 registry+https://github.com/rust-lang/crates.io-index
174 │ │ enum-as-inner 0.6.0 registry+https://github.com/rust-lang/crates.io-index
    │ ╰─────────────────────────────────────────────────────────────────────────^ lock entries
    │  
    = enum-as-inner v0.5.1
      └── sysctl v0.5.4
          └── mz-environmentd v0.92.0-dev
              ├── (dev) mz-balancerd v0.92.0-dev
              ├── (dev) mz-environmentd v0.92.0-dev (*)
              └── mz-sqllogictest v0.0.1
    = enum-as-inner v0.6.0
      └── hickory-proto v0.24.0
          └── hickory-resolver v0.24.0
              └── mz-balancerd v0.92.0-dev

and uh I have no idea what to do about that.

Copy link
Contributor

@alex-hunt-materialize alex-hunt-materialize left a comment

Choose a reason for hiding this comment

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

This looks great!

It seems that sysctl is the thing pulling in the older enum-as-inner, and there is a newer v0.5.5 that uses the newer version. Perhaps upgrading to that would resolve the lint issues?

@alex-hunt-materialize
Copy link
Contributor

Oof, looks like it has other duplicates, which aren't so simple.

@alex-hunt-materialize
Copy link
Contributor

alex-hunt-materialize commented Mar 14, 2024

Hickory DNS has idna 0.5 on their main branch. I'm not sure what to do about the quick-error thing, though, as resolv-conf 0.7.0 is the latest, and seems abandoned for the last four years, despite 24 million downloads.

@alex-hunt-materialize
Copy link
Contributor

I've opened hickory-dns/resolv-conf#35 to hopefully remove the dependency on quick-error in resolv-conf, but I doubt it will get merged any time soon.

@alex-hunt-materialize
Copy link
Contributor

It seems like a pain, but a possible solution:

  1. Update to sysctl v0.5.5.
  2. Point at a newer commit of hickory-resolver (main), which uses idna v0.5. I think our git dependency rules require us to fork for this.
  3. Add a patch directive for resolv-conf v0.7.0 to point at a fork that contains Remove dependency on quick-error hickory-dns/resolv-conf#35

@maddyblue
Copy link
Contributor Author

There are other ideas too like use the domain crate which seems to have somewhat fewer deps, but still quite a lot.

@maddyblue maddyblue force-pushed the balancer-http-metrics branch from 147fd1f to 7184df8 Compare March 19, 2024 18:35
@maddyblue maddyblue requested a review from benesch as a code owner March 19, 2024 18:35
@maddyblue maddyblue force-pushed the balancer-http-metrics branch 2 times, most recently from cfa9a85 to a8be561 Compare March 19, 2024 18:41
@maddyblue
Copy link
Contributor Author

Reworked to use the domain crate, so only 2 new crates needed to be audited. That also removed the Cargo.lock duplicates.

@maddyblue maddyblue enabled auto-merge March 19, 2024 18:42
@maddyblue
Copy link
Contributor Author

@alex-hunt-materialize the mzcompose tests do not test the CNAME -> tenant stuff because I don't know if docker compose can set up arbitrary CNAMES and didn't investigate that. After deploying this to staging we will need to do a few tests to make sure the http tenants are being correctly extracted.

@maddyblue maddyblue disabled auto-merge March 19, 2024 18:46
@maddyblue maddyblue enabled auto-merge (squash) March 19, 2024 18:47
Use the CNAME to determine the tenant.

Use the `domain` crate for CNAME lookup. This came with some new and updated
imports. `cargo vet import` was re-ran for the 4 audit vendors we trust,
yielding the cargo-vet changes to config.toml and imports.lock.
@maddyblue maddyblue force-pushed the balancer-http-metrics branch from a8be561 to 62dfe8c Compare March 20, 2024 21:51
@maddyblue maddyblue merged commit 30d9c37 into MaterializeInc:main Mar 20, 2024
73 checks passed
@maddyblue maddyblue deleted the balancer-http-metrics branch March 20, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants