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

packages/nixos: add IMDS setup script #988

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Nov 11, 2024

Azure needs special care for enabling IMDS within Peerpods. This adds a script to setup IMDS through Proxy ARP (from Peerpods upstream, see https://github.com/confidential-containers/cloud-api-adaptor/blob/main/src/cloud-api-adaptor/podvm/files/usr/local/bin/setup-nat-for-imds.sh), so that all requests to the IMDS from within the pod are routed through an interface that is peered to the Pod VM. Verified to work in 2 distinct Azure peer pods.

It also makes a simplification by excluding basic.target in the wantedBy list of the Azure readiness report, as it's already contained in the default dependencies.

@msanft msanft requested a review from katexochen as a code owner November 11, 2024 07:43
@msanft msanft added the no changelog PRs not listed in the release notes label Nov 11, 2024
@msanft msanft force-pushed the msanft/podvm-image/imds-setup branch from 692bfe7 to 86784fe Compare November 11, 2024 07:46
@msanft msanft force-pushed the msanft/podvm-image/imds-setup branch from 86784fe to f48bf81 Compare November 11, 2024 09:39
@msanft msanft requested a review from katexochen November 11, 2024 09:39
Comment on lines 94 to 102
# TODO: Find out why just ordering this after network-online.target
# isn't sufficient. (Errors with saying that the network is unreachable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what step produces these errors? One dependency that's not declared is the podns existence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would make sense, since iirc, this fails

Copy link
Contributor

Choose a reason for hiding this comment

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

If the netns exists but routing is not set up yet, that would explain it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verifying that hypothesis right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still fails at the very same line. Ordering is correct though:

...
 195ms setup-nat-for-imds.service
 193ms systemd-udevd.service
 190ms systemd-oomd.service
 154ms [email protected]
...

Copy link
Contributor

Choose a reason for hiding this comment

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

setup-nat should go after netns, is that actually what your snippet shows?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, having the podns is not enough, it also needs routes set up, which might only happen in the agent-protocol-forwarder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, it doesn't! systemd-analyze blame does not order by execution but by time the unit took to execute.

However, the ordering still seems to be correct. [email protected] executes (exits) 10s before setup-nat-for-imds.service starts.

Copy link
Contributor

Choose a reason for hiding this comment

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

As stated above, this requires route setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate? This works as-is. I don't know what I should change.

@msanft msanft force-pushed the msanft/podvm-image/imds-setup branch from f48bf81 to e96d1cb Compare November 12, 2024 07:52
@katexochen
Copy link
Member

We should add an e2e test step to check imds functionality before merging this.

@katexochen katexochen added the needs: test This can only be merged after a test was added label Nov 12, 2024
@msanft msanft force-pushed the msanft/podvm-image/imds-setup branch 4 times, most recently from 0ad96aa to 63cabf1 Compare December 2, 2024 14:13
@msanft msanft requested a review from burgerdev December 2, 2024 14:13
@msanft msanft removed the needs: test This can only be merged after a test was added label Dec 2, 2024
packages/nixos/azure.nix Outdated Show resolved Hide resolved
Comment on lines 94 to 102
# TODO: Find out why just ordering this after network-online.target
# isn't sufficient. (Errors with saying that the network is unreachable)
Copy link
Contributor

Choose a reason for hiding this comment

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

As stated above, this requires route setup.

@msanft msanft force-pushed the msanft/podvm-image/imds-setup branch from 63cabf1 to 130137e Compare December 2, 2024 14:42
packages/nixos/azure.nix Outdated Show resolved Hide resolved
Azure needs special care for enabling IMDS within Peerpods. This adds a script to setup IMDS through Proxy ARP (from Peerpods upstream, see https://github.com/confidential-containers/cloud-api-adaptor/blob/main/src/cloud-api-adaptor/podvm/files/usr/local/bin/setup-nat-for-imds.sh), so that all requests to the IMDS from within the pod are routed through an interface that is peered to the Pod VM. Verified to work in 2 distinct Azure peer pods.
This adds a verification of IMDS functionality to the peer-pods smoke test.
@msanft msanft force-pushed the msanft/podvm-image/imds-setup branch from 130137e to b6abc8d Compare December 3, 2024 07:59
@msanft msanft requested a review from katexochen December 3, 2024 08:03
@msanft msanft merged commit d39a0d0 into main Dec 3, 2024
12 checks passed
@msanft msanft deleted the msanft/podvm-image/imds-setup branch December 3, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants