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

feat: add configuration for file permission #751

Merged
merged 26 commits into from
Jan 19, 2022

Conversation

nilekhc
Copy link
Contributor

@nilekhc nilekhc commented Jan 12, 2022

Reason for Change:

This PR adds support for file permission for secrets being written to the pod.

Requirements

  • squashed commits
  • included documentation
  • added unit tests and e2e tests (if applicable).

Issue Fixed:

fixes #712

Does this change contain code from or inspired by another project?

  • Yes
  • No

If "Yes," did you notify that project's maintainers and provide attribution?

Special Notes for Reviewers:

@nilekhc nilekhc requested a review from aramase January 12, 2022 12:18
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2022

Codecov Report

Merging #751 (0d0cf19) into master (6fc6b53) will decrease coverage by 0.07%.
The diff coverage is 53.57%.

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
- Coverage   62.09%   62.02%   -0.08%     
==========================================
  Files           7        7              
  Lines         765      782      +17     
==========================================
+ Hits          475      485      +10     
- Misses        257      263       +6     
- Partials       33       34       +1     

@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 13, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@aramase aramase changed the title File permission feat: add configuration for file permission Jan 13, 2022
Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

Added few comments! To summarize,

  1. If no file permission is provided, we should default to the permission provided by the driver (same as today). This can be done when the objects to retrieve are unmarshalled.
  2. The validation for file permission should happen before actual call to keyvault to avoid making keyvault API calls when final result is an error.

Let's move the validation to a function so we can also unit test it

pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 14, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilekhc nilekhc requested a review from aramase January 14, 2022 09:28
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 14, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@aramase aramase added this to the v1.1.0 milestone Jan 14, 2022
Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

Could we also an e2e test to validate this change?

pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 19, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 19, 2022

/azp run pr-e2e-azure

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 19, 2022

Could we also an e2e test to validate this change?

I have added e2e tests for kind cluster.

@nilekhc nilekhc requested a review from aramase January 19, 2022 15:49
Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

The changes look good! Could you update the documentation here to add the new field? You can add available for version > v1.1.0 so users know it's not yet released.

Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@nilekhc
Copy link
Contributor Author

nilekhc commented Jan 19, 2022

The changes look good! Could you update the documentation here to add the new field? You can add available for version > v1.1.0 so users know it's not yet released.

added.

@nilekhc nilekhc requested a review from aramase January 19, 2022 18:24
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
@nilekhc nilekhc requested a review from aramase January 19, 2022 20:07
@nilekhc nilekhc merged commit c06bc12 into Azure:master Jan 19, 2022
@nilekhc nilekhc deleted the file-permission branch January 19, 2022 20:18
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.

Set file-system permissions for mounted secrets
3 participants