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

runtime: Fix configmap/secrets updates with FS sharing disabled #8239

Conversation

Sumynwa
Copy link
Contributor

@Sumynwa Sumynwa commented Oct 16, 2023

This PR fixes k8's configmap/secrets etc update propagation when filesystem sharing is disabled.
The commit introduces below changes with some limitations:

  • creates new timestamped directory in guest
  • updates the '..data' symlink
  • creates user visible symlinks to newly created secrets.
  • Limitation: The older timestamped directory and stale user visible symlinks exist in guest due to missing DELETE api in agent.

Fixes: #7398

@katacontainersbot
Copy link
Contributor

Can one of the admins verify this patch?

@katacontainersbot katacontainersbot added the size/large Task of significant size label Oct 16, 2023
@Sumynwa Sumynwa force-pushed the sumsharma/fix_configmap_update_propagation branch from 5cd7410 to 43f7ed6 Compare October 17, 2023 15:49
@danmihai1
Copy link
Contributor

/test

@Sumynwa Sumynwa force-pushed the sumsharma/fix_configmap_update_propagation branch 2 times, most recently from 01d08b6 to a537a62 Compare October 20, 2023 06:03
@danmihai1
Copy link
Contributor

/test

@danmihai1 danmihai1 added the area/confidential-containers Issues related to confidential containers (see also CCv0 branch) label Oct 20, 2023
Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

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

Code-wise it looks good to me.

You have my "lgtm", and will have my approval once someone can have this one tested on the peer-pods side. Maybe @stevenhorsman, in case he has a handy cluster around.

src/runtime/virtcontainers/fs_share_linux.go Show resolved Hide resolved
@stevenhorsman
Copy link
Member

You have my "lgtm", and will have my approval once someone can have this one tested on the peer-pods side. Maybe @stevenhorsman, in case he has a handy cluster around.

So the issue is that peer pods can't run against main here. I did create #8015 to cover adding tests for this scenario, but I don't want to block this change waiting for that.

@bpradipt
Copy link
Contributor

I just had a minor comment. Rest looks good. As @stevenhorsman already mentioned, it's not possible to test this with peer-pods.

…abled

This PR fixes k8's configmap/secrets etc update propagation when filesystem sharing is disabled.
The commit introduces below changes with some limitations:
- creates new timestamped directory in guest
- updates the '..data' symlink
- creates user visible symlinks to newly created secrets.
- Limitation: The older timestamped directory and stale user visible symlinks exist in guest
  due to missing DELETE api in agent.

Fixes: kata-containers#7398

Signed-off-by: Sumedh Alok Sharma <sumsharma@microsoft.com>
@Sumynwa Sumynwa force-pushed the sumsharma/fix_configmap_update_propagation branch from a537a62 to 4aaf54b Compare November 17, 2023 07:35
@stevenhorsman
Copy link
Member

/test

@danmihai1
Copy link
Contributor

Sumedh is out on vacation. I will remind him to address the feedback from this PR when he comes back to work, next week.

@danmihai1
Copy link
Contributor

Sumedh is out on vacation. I will remind him to address the feedback from this PR when he comes back to work, next week.

Nevermind - looks like he is back already.

@Sumynwa
Copy link
Contributor Author

Sumynwa commented Nov 20, 2023

Dan, I have addressed the PR feedbacks.

Copy link
Member

@fidencio fidencio left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @Sumynwa!

@danmihai1 danmihai1 self-requested a review November 23, 2023 14:50
@danmihai1 danmihai1 merged commit 7560227 into kata-containers:main Nov 23, 2023
137 of 144 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/confidential-containers Issues related to confidential containers (see also CCv0 branch) ok-to-test size/large Task of significant size
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CCv0: deleting a K8s Secret doesn't delete that Secret from the Guest, when !IsFsSharingSupported
6 participants