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

Add checks for CNIBinDir ownership/permissions. #606

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

aznashwan
Copy link
Contributor

@aznashwan aznashwan commented Aug 14, 2024

When the Cilium feature is enabled, add simple user/group/permissions checks on the configured CNIBinDir (/opt/cni/bin) before installing the Cilium Helm chart.

Fixes: #567

@aznashwan aznashwan requested a review from a team as a code owner August 14, 2024 08:55
Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

Nice work! Some comments

src/k8s/pkg/k8sd/features/cilium/network.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/features/cilium/network.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/features/cilium/network.go Outdated Show resolved Hide resolved
Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

In a more general note, this does not prevent or address the issue, it only raises the error message in k8s status rather than getting cilium deployed.

I could see this causing more issues (e.g. gateway or ingress also failing to enable with errors). Perhaps we should just amend the required state? e.g. try to chown and chmod the directory?

@aznashwan aznashwan force-pushed the check-cnibin-dir-permissions branch 2 times, most recently from dfb54da to f2ec833 Compare August 14, 2024 13:32
@aznashwan
Copy link
Contributor Author

In a more general note, this does not prevent or address the issue, it only raises the error message in k8s status rather than getting cilium deployed.

Correct, that was what was suggested in this comment on the original issue, so I just went with it.

Perhaps we should just amend the required state? e.g. try to chown and chmod the directory?

I had considered it, but was unsure whether the snap should liberally change the state of files on the host system.

If you think this is something the k8s snap should/could be able to do, I can gladly update the PR to perform the chown/chmod's directly...

@aznashwan
Copy link
Contributor Author

Perhaps it's best to move this as part of setup.EnsureAllDirectories() which runs on all cluster nodes when they join the cluster.

Just updated, thanks!

I initially made it part of the Cilium network setup since it was "closer" to where the Cilium chart deployment which failed due to the wrong ownership/permissions was being deployed, but it does indeed make more sense to handle this in the main k8sd setup.

Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

It's OK, left some comments about the logging

src/k8s/pkg/k8sd/setup/directories.go Show resolved Hide resolved
src/k8s/pkg/k8sd/setup/directories.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/setup/directories.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

Please fix the Go test, then we are good to merge

Add checks/remediation steps for the ownership/permissions of the
CNIBinDir during initial k8sd directories setup.

Fixes: canonical#567

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
@aznashwan
Copy link
Contributor Author

@bschimke95 I have managed to fix the previous logic bug but the tests are apparently being run as 1001:127 so the chown isn't permitted.

The only workaround I can think of is running the tests as root from the workflow (sudo make go.unit), but I'm open to better ideas.

@bschimke95
Copy link
Contributor

Hey @aznashwan
Just talked with @berkayoz about this and we also don't really have a better idea right now. Let's just run this as root.

Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>
@bschimke95 bschimke95 merged commit f269968 into canonical:main Aug 28, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants