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.

Sign up for GitHub

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

Jump to bottom

genpolicy: support dynamic SMB storage class options #245

Merged
merged 2 commits into from
Nov 6, 2024
Merged

genpolicy: support dynamic SMB storage class options #245

merged 2 commits into from
Nov 6, 2024

Conversation

arc9693
Copy link

@arc9693 arc9693 commented Oct 22, 2024 •
edited by Redent0r
Loading

Merge Checklist
  • Followed patch format from upstream recommendation: https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format
    • Included a single commit in a given PR - at least unless there are related commits and each makes sense as a change on its own.
  • Aware about the PR to be merged using "create a merge commit" rather than "squash and merge" (or similar)
  • The upstream/missing label (or upstream/not-needed) has been set on the PR.
Summary

work item: https://microsoft.visualstudio.com/OS/_workitems/edit/54514252

Test Methodology

danmihai1
danmihai1 approved these changes Oct 22, 2024
View reviewed changes
src/tools/genpolicy/rules.rego Outdated Show resolved Hide resolved
src/tools/genpolicy/rules.rego Outdated Show resolved Hide resolved
src/tools/genpolicy/src/mount_and_storage.rs Outdated Show resolved Hide resolved
@danmihai1 danmihai1 requested a review from Redent0r October 22, 2024 17:50
Redent0r
Redent0r approved these changes Oct 22, 2024
View reviewed changes
Copy link

@Redent0r Redent0r left a comment

Choose a reason for hiding this comment

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

LGTM once feedback has been resolved :)

@arc9693 arc9693 force-pushed the archana1/azfile-genpolicy-refactor branch from e4b7f7d to 1ee16b0 Compare October 23, 2024 09:04
@arc9693 arc9693 added the upstream/not-needed PRs that will not be upstreamed (e.g. internal) label Oct 23, 2024
@arc9693 arc9693 force-pushed the archana1/azfile-genpolicy-refactor branch from 1ee16b0 to f1fb8f1 Compare October 23, 2024 09:24
@arc9693 arc9693 marked this pull request as ready for review October 23, 2024 09:28
@arc9693 arc9693 requested review from a team as code owners October 23, 2024 09:28
@arc9693
genpolicy: support dynamic SMB storage class options
dd6342c 
- Update mount_and_storage.rs and stateful_set.rs to support dynamic SMB storage class options.
- Update SMB storage class options in genpolicy-settings.json.
- Update rules.rego to support dynamic SMB storage class options.

Signed-off-by: Archana Choudhary <[email protected]>
@arc9693 arc9693 force-pushed the archana1/azfile-genpolicy-refactor branch from a9c0dac to dd6342c Compare October 23, 2024 17:37
@Redent0r
samples: update samples
e3d3e04 
Adapt samples for dynamic SMB changes.

Signed-off-by: Saul Paredes <[email protected]>
@danmihai1
Copy link

Looks good, thanks @arc9693 !

sprt
sprt reviewed Nov 4, 2024
View reviewed changes
src/tools/genpolicy/genpolicy-settings.json
"cc-azurefile-csi",
"cc-azurefile-premium-csi"
{
"name": "azurefile-csi-kata-cc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to use azurefile-csi and azurefile-premium-csi since we want the official driver to support conf pods?

Copy link
Author

Choose a reason for hiding this comment

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

The default storage drivers do not have enableKataCCMount set to true.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @sprt, it would be good to include the default Azure File storage classes. For example, our team creates a custom storage class with name azurefile-csi and set the enableKataCCMount flag to true; we use the same name for compatibility with non-confidential deployments. Adding them would also align with the plan of removing the flag from the storage class and automatically enabling kata mount for confidential pods in the Azure File CSI driver.

Copy link
Author

Choose a reason for hiding this comment

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

To be added, post migration discussion with Andy. As of now, it's not true by default for azurefile-csi. So, to reduce conflict we will keep as it is for now.

@sprt
Copy link
Collaborator

sprt commented Nov 4, 2024

Did we ensure that this change does not regress our Azure Disk and Azure Local drivers?

@arc9693
Copy link
Author

arc9693 commented Nov 5, 2024

Did we ensure that this change does not regress our Azure Disk and Azure Local drivers?

image
image

@arc9693
Copy link
Author

arc9693 commented Nov 6, 2024

Closing this after discussion yesterday @sprt.

@arc9693 arc9693 merged commit 396e0d2 into msft-main Nov 6, 2024
82 of 103 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andpiccione andpiccione andpiccione left review comments

@sprt sprt sprt left review comments

@Redent0r Redent0r Redent0r approved these changes

@danmihai1 danmihai1 danmihai1 approved these changes

Assignees
No one assigned
Labels
upstream/not-needed PRs that will not be upstreamed (e.g. internal)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arc9693 @danmihai1 @sprt @Redent0r @andpiccione