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

Replace procinfo with procfs #2433

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Conversation

pranoyk
Copy link
Contributor

@pranoyk pranoyk commented Jun 20, 2023

This PR will replace procinfo crate which is not maintained for over 5 years with procfs.

Signed-off-by: Pranoy Kumar kundu [email protected]

fixes - linkerd/linkerd2#10819

@pranoyk pranoyk force-pushed the replace-procinfo-with-procfs branch 2 times, most recently from f3f5e04 to 636d761 Compare June 21, 2023 07:03
@pranoyk pranoyk marked this pull request as ready for review June 21, 2023 07:09
@pranoyk pranoyk requested a review from a team as a code owner June 21, 2023 07:09
@pranoyk pranoyk changed the title WIP :: replace procinfo with procfs Replace procinfo with procfs Jun 21, 2023
Copy link
Member

@mateiidavid mateiidavid left a comment

Choose a reason for hiding this comment

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

Left a few comments :)

linkerd/system/Cargo.toml Outdated Show resolved Hide resolved
linkerd/system/src/linux.rs Outdated Show resolved Hide resolved
linkerd/system/src/linux.rs Outdated Show resolved Hide resolved
linkerd/system/src/linux.rs Outdated Show resolved Hide resolved
linkerd/system/src/linux.rs Outdated Show resolved Hide resolved
linkerd/system/src/linux.rs Outdated Show resolved Hide resolved
@hawkw hawkw self-requested a review June 23, 2023 16:54
@pranoyk pranoyk force-pushed the replace-procinfo-with-procfs branch from c222a57 to ec59f05 Compare June 27, 2023 06:15
@pranoyk pranoyk requested a review from mateiidavid June 27, 2023 06:34
linkerd/system/Cargo.toml Outdated Show resolved Hide resolved
linkerd/system/Cargo.toml Outdated Show resolved Hide resolved
linkerd/system/src/linux.rs Show resolved Hide resolved
Copy link
Member

@mateiidavid mateiidavid left a comment

Choose a reason for hiding this comment

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

Thanks @pranoyk for the work so far. This LGTM. I suspect we'll bump into some issues with deny since we're pulling in the same transitive dependency with two separate versions. We'll probably need to add a rule for it. There's a typo in the manifest so let's see if CI complains once that's fixed.

Also, one of the commits seems to be improperly signed. Can you please confirm you agree to making this contribution?

Thanks!

linkerd/system/Cargo.toml Outdated Show resolved Hide resolved
@pranoyk
Copy link
Contributor Author

pranoyk commented Jul 3, 2023

I agree to the DCO for all the commits in this PR.

Copy link
Member

@mateiidavid mateiidavid left a comment

Choose a reason for hiding this comment

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

Nice. Seems like it's a net change.

+54 −54 

LGTM, pretty clean. Bit of a scary change to switch deps but I think it'll be fine. Thanks!

@pranoyk
Copy link
Contributor Author

pranoyk commented Jul 11, 2023

@mateiidavid I was just wondering if we expect any more changes on this since it's being approved but not yet merged.
Do let me know if we are expecting anything else on this. :)

@mateiidavid
Copy link
Member

@mateiidavid I was just wondering if we expect any more changes on this since it's being approved but not yet merged. Do let me know if we are expecting anything else on this. :)

Hey, typically we expect more than one reviewer even if the review protection rules have been met. In this case, we're doing a change that swaps out dependencies so it might be more contentious than say a typo or dependency bump. I'm going to wait for one more review here before merging.

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me!

Comment on lines +62 to +69
# right now we have a mix of versions of this crate in the ecosystem
# procfs uses 0.36.14, tempfile uses 0.37.4
{ name = "rustix" },
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth opening a PR upstream to update procfs' dependency on rustix? then, we would be able to remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the latest version of rustix is 0.38.0
If we are planning to upgrade then should we upgrading the rustix version for both the dependencies ? Or shall we ignore it for now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I didn't see this. It would be ideal to get both dependencies on the latest rustix. That isn't a blocker for merging this PR, but it could be nice to kick off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be great. But help me understand something, I just cannot see where we are using the tempfile crate in our code base (this is another crate that uses rustix and the reason why we need to add it into deny.toml). I might be making a mistake here but in case we are not using it I can go ahead and remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like tempfile is a transitive dependency of prost-build, which is used for compiling protobufs. This is a build-time dependency, so it's not actually used by the proxy itself. Instead, it's used when building the proxy, to generate Rust code from protocol buffer files.

We can determine where a transitive dependency comes from by using cargo tree. If we pass the --invert flag to cargo tree, we get a tree whose root is the named crate and whose leaves are the crates that depend on that crate. For tempfile, it looks like this:

$ cargo tree --package tempfile --invert
tempfile v3.5.0
└── prost-build v0.11.8
    └── tonic-build v0.8.4
        └── tools v0.1.0 (/home/eliza/Code/linkerd2-proxy/tools)
        [dev-dependencies]
        └── opencensus-proto v0.1.0 (/home/eliza/Code/linkerd2-proxy/opencensus-proto)
            └── linkerd-opencensus v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/opencensus)
                ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app)
                │   ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
                │   └── linkerd2-proxy v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd2-proxy)
                └── linkerd-app-core v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/core)
                    ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
                    ├── linkerd-app-admin v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/admin)
                    │   └── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
                    │   [dev-dependencies]
                    │   └── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
                    ├── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway)
                    │   └── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
                    ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound)
                    │   ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
                    │   ├── linkerd-app-admin v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/admin) (*)
                    │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
                    │   [dev-dependencies]
                    │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
                    ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
                    ├── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound)
                    │   ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
                    │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
                    │   [dev-dependencies]
                    │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
                    └── linkerd-app-test v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/test)
                        ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
                        ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
                        └── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
                        [dev-dependencies]
                        ├── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
                        ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
                        └── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
    [dev-dependencies]
    └── linkerd-transport-header v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/transport-header)
        └── linkerd-app-core v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/core) (*)

Comment on lines +62 to +69
# right now we have a mix of versions of this crate in the ecosystem
# procfs uses 0.36.14, tempfile uses 0.37.4
{ name = "rustix" },
Copy link
Contributor

Choose a reason for hiding this comment

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

i think if we moved this from a skip section to a skip_tree section, we wouldn't need the separate entry for linux-raw-sys.

pranoyk and others added 7 commits September 20, 2023 12:01
Signed-off-by: Pranoy Kumar Kundu <[email protected]>

get limits using procfs crate

Signed-off-by: Pranoy Kumar Kundu <[email protected]>

remove unnecessary type cast and format files

Signed-off-by: Pranoy Kumar Kundu <[email protected]>

modify Stat use

Signed-off-by: Pranoy Kumar Kundu <[email protected]>
Signed-off-by: Pranoy Kumar Kundu <[email protected]>
@hawkw hawkw force-pushed the replace-procinfo-with-procfs branch from d7c4c9c to 65e88a1 Compare September 20, 2023 19:04
@hawkw hawkw merged commit 8afc722 into linkerd:main Sep 20, 2023
14 checks passed
hawkw added a commit to linkerd/linkerd2 that referenced this pull request Sep 21, 2023
Currently, the proxy [depends on an outdated version of `rustls`][1],
v0.20.8. The `rustls` dependency is via our dependency on `tokio-rustls`
v0.23.4; we don't have a direct `rustls` dependency, in order to ensure
that the version of `rustls` is always the same version as used by
`tokio-rustls`. `rustls` also has a dependency on `webpki`, and v0.20.x
of `rustls` uses the original `webpki` crate, rather than the
`rustls-webpki` crate. So, unfortunately, because we have a transitive
dep on `webpki` via `rustls`, PR linkerd/linkerd2-proxy#2465 did not
remove _all_ `webpki` deps from our dependency tree, only the direct
dependency.

This branch updates to `rustls` v0.21.x, which depends on
`rustls-webpki` rather than `webpki`, removing the `webpki` dependency.
This is accomplished by updating `tokio-rustls` to v0.24.x, implicitly
updating the transitive `rustls` dep. In order to update to the
semver-incompatible version of `rustls`, it was necessary to modify our
code in order to track some breaking API changes. I've also added a
`cargo-deny` ban for `webpki` to our `deny.toml`, to ensure that we
always use the actively-maintained `rustls-webpki` crate rather than
`webpki` classic.

Since peer certificate validation is performed through `rustls` rather
than through the direct `rustls-webpki` dependency, this should
hopefully resolve issues with issuer certs that contain name constraints
--- these were not fixed by linkerd/linkerd2-proxy#2465, because the
failure with certs containing name constraints occurred inside of the
*`webpki` version depended on by `rustls`*, rather than inside of the
proxy's direct dep. See [this comment][2] for details.

In addition, it was necessary to update `rustls-webpki` to v0.101.6,
since v0.101.5 was yanked due to an accidental API breaking change.

<details>

<summary>Verifying that we no longer depend on `webpki`:</summary>

Before:

```console
$ cargo tree -p webpki -i
webpki v0.22.1
├── rustls v0.20.8
│   └── tokio-rustls v0.23.4
│       ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
│       └── linkerd-meshtls-rustls v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/meshtls/rustls)
│           ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound)
│           │   ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app)
│           │   │   ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
│           │   │   └── linkerd2-proxy v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd2-proxy)
│           │   ├── linkerd-app-admin v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/admin)
│           │   │   └── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
│           │   │   [dev-dependencies]
│           │   │   └── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
│           │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway)
│           │       └── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
│           │   [dev-dependencies]
│           │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
│           ├── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound)
│           │   ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
│           │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
│           │   [dev-dependencies]
│           │   └── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
│           └── linkerd-meshtls v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/meshtls)
│               ├── linkerd-app-core v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/core)
│               │   ├── linkerd-app v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app) (*)
│               │   ├── linkerd-app-admin v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/admin) (*)
│               │   ├── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
│               │   ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
│               │   ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
│               │   ├── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
│               │   └── linkerd-app-test v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/test)
│               │       ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
│               │       ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
│               │       └── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
│               │       [dev-dependencies]
│               │       ├── linkerd-app-gateway v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/gateway) (*)
│               │       ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
│               │       └── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
│               ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
│               ├── linkerd-proxy-tap v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/proxy/tap)
│               │   └── linkerd-app-core v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/core) (*)
│               └── linkerd2-proxy v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd2-proxy)
│               [dev-dependencies]
│               ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
│               ├── linkerd-app-integration v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/integration)
│               └── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
│           [dev-dependencies]
│           ├── linkerd-app-inbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/inbound) (*)
│           └── linkerd-app-outbound v0.1.0 (/home/eliza/Code/linkerd2-proxy/linkerd/app/outbound) (*)
└── tokio-rustls v0.23.4 (*)
```

After:

```console
$ cargo tree -p webpki -i
error: package ID specification `webpki` did not match any packages
```

</details>

[1]:
    https:/linkerd/linkerd2-proxy/blob/8afc72258b8ced868fbd0bde0235955c0adf4ccd/Cargo.lock#L2450-L2460C2
[2]:
    #9299 (comment)

---

* meshtls: use published `rustls-webpki` v0.101.5 (linkerd/linkerd2-proxy#2470)
* Replace `procinfo` with `procfs` (linkerd/linkerd2-proxy#2433)
* meshtls: update to `rustls` v0.21.7 (linkerd/linkerd2-proxy#2472)

Signed-off-by: Eliza Weisman <[email protected]>
mateiidavid pushed a commit to linkerd/linkerd2 that referenced this pull request Sep 22, 2023
Currently, the proxy [depends on an outdated version of `rustls`][1],
v0.20.8. The `rustls` dependency is via our dependency on `tokio-rustls`
v0.23.4; we don't have a direct `rustls` dependency, in order to ensure
that the version of `rustls` is always the same version as used by
`tokio-rustls`. `rustls` also has a dependency on `webpki`, and v0.20.x
of `rustls` uses the original `webpki` crate, rather than the
`rustls-webpki` crate. So, unfortunately, because we have a transitive
dep on `webpki` via `rustls`, PR linkerd/linkerd2-proxy#2465 did not
remove _all_ `webpki` deps from our dependency tree, only the direct
dependency.

This branch updates to `rustls` v0.21.x, which depends on
`rustls-webpki` rather than `webpki`, removing the `webpki` dependency.
This is accomplished by updating `tokio-rustls` to v0.24.x, implicitly
updating the transitive `rustls` dep. In order to update to the
semver-incompatible version of `rustls`, it was necessary to modify our
code in order to track some breaking API changes. I've also added a
`cargo-deny` ban for `webpki` to our `deny.toml`, to ensure that we
always use the actively-maintained `rustls-webpki` crate rather than
`webpki` classic.

Since peer certificate validation is performed through `rustls` rather
than through the direct `rustls-webpki` dependency, this should
hopefully resolve issues with issuer certs that contain name constraints
--- these were not fixed by linkerd/linkerd2-proxy#2465, because the
failure with certs containing name constraints occurred inside of the
*`webpki` version depended on by `rustls`*, rather than inside of the
proxy's direct dep. See [this comment][2] for details.

In addition, it was necessary to update `rustls-webpki` to v0.101.6,
since v0.101.5 was yanked due to an accidental API breaking change.

[1]:
    https:/linkerd/linkerd2-proxy/blob/8afc72258b8ced868fbd0bde0235955c0adf4ccd/Cargo.lock#L2450-L2460C2
[2]:
    #9299 (comment)

---

* meshtls: use published `rustls-webpki` v0.101.5 (linkerd/linkerd2-proxy#2470)
* Replace `procinfo` with `procfs` (linkerd/linkerd2-proxy#2433)
* meshtls: update to `rustls` v0.21.7 (linkerd/linkerd2-proxy#2472)

Signed-off-by: Eliza Weisman <[email protected]>
mateiidavid pushed a commit to linkerd/linkerd2 that referenced this pull request Sep 25, 2023
Currently, the proxy [depends on an outdated version of `rustls`][1],
v0.20.8. The `rustls` dependency is via our dependency on `tokio-rustls`
v0.23.4; we don't have a direct `rustls` dependency, in order to ensure
that the version of `rustls` is always the same version as used by
`tokio-rustls`. `rustls` also has a dependency on `webpki`, and v0.20.x
of `rustls` uses the original `webpki` crate, rather than the
`rustls-webpki` crate. So, unfortunately, because we have a transitive
dep on `webpki` via `rustls`, PR linkerd/linkerd2-proxy#2465 did not
remove _all_ `webpki` deps from our dependency tree, only the direct
dependency.

This branch updates to `rustls` v0.21.x, which depends on
`rustls-webpki` rather than `webpki`, removing the `webpki` dependency.
This is accomplished by updating `tokio-rustls` to v0.24.x, implicitly
updating the transitive `rustls` dep. In order to update to the
semver-incompatible version of `rustls`, it was necessary to modify our
code in order to track some breaking API changes. I've also added a
`cargo-deny` ban for `webpki` to our `deny.toml`, to ensure that we
always use the actively-maintained `rustls-webpki` crate rather than
`webpki` classic.

Since peer certificate validation is performed through `rustls` rather
than through the direct `rustls-webpki` dependency, this should
hopefully resolve issues with issuer certs that contain name constraints
--- these were not fixed by linkerd/linkerd2-proxy#2465, because the
failure with certs containing name constraints occurred inside of the
*`webpki` version depended on by `rustls`*, rather than inside of the
proxy's direct dep. See [this comment][2] for details.

In addition, it was necessary to update `rustls-webpki` to v0.101.6,
since v0.101.5 was yanked due to an accidental API breaking change.

[1]:
    https:/linkerd/linkerd2-proxy/blob/8afc72258b8ced868fbd0bde0235955c0adf4ccd/Cargo.lock#L2450-L2460C2
[2]:
    #9299 (comment)

---

* meshtls: use published `rustls-webpki` v0.101.5 (linkerd/linkerd2-proxy#2470)
* Replace `procinfo` with `procfs` (linkerd/linkerd2-proxy#2433)
* meshtls: update to `rustls` v0.21.7 (linkerd/linkerd2-proxy#2472)

Signed-off-by: Eliza Weisman <[email protected]>
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.

3 participants