-
Notifications
You must be signed in to change notification settings - Fork 6
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
pb-7504: make NFS job pod to use root for resource backup #391
Conversation
OSS Scan Results:
Total issues: 142 |
License Evaluation Results:
Total License Issues: 17 |
pkg/drivers/utils/utils.go
Outdated
// Checks if the cluster is GCP hosted cluster. | ||
func IsGcpHostedCluster() (bool, error) { | ||
// Any GCP hosted cluster be it vanilla , OCP or GKE | ||
// it is expected to have a ProviderId in its spec witha prefix of "gce" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo witha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/drivers/nfsrestore/nfsrestore.go
Outdated
|
||
// check the cluster is GCP based or not | ||
isGcpBasedCluster, err := utils.IsGcpHostedCluster() | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this fix for restore as well? for restore, we need only read permission rt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch we don't need it.
pkg/drivers/utils/utils.go
Outdated
|
||
for _, node := range nodes.Items { | ||
providerID := node.Spec.ProviderID | ||
if strings.HasPrefix(providerID, "gce://") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we make the "gce://" as string constant definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
c898e5e
to
6ef74cc
Compare
OSS Scan Results:
Total issues: 142 |
License Evaluation Results:
Total License Issues: 17 |
- When we use GCP based file store as NFS backup location, the job pod using that doesn't have write permission for group user, this causes the non-root user permission denied error during backup and restore. - This is GKE specific behaviour hence a check added to force all job pod to run as a root user eradicating the permission denied error. Signed-off-by: Lalatendu Das <[email protected]>
6ef74cc
to
e893f55
Compare
OSS Scan Results:
Total issues: 142 |
License Evaluation Results:
Total License Issues: 17 |
When we use GCP based file store as NFS backup location, the job pod using that doesn't have write permission for group user, this causes the non-root user permission denied error during backup and restore.
This is GKE specific behaviour hence a check added to force all job pod to run as a root user eradicating the permission denied error.
What this PR does / why we need it: This PR fixes the GKE resource backup and restore path wherein if NFS backup location is used then it will not use non-root user in any case. Hence no security context will be added.
Which issue(s) this PR fixes (optional)
Closes # pb-7504
Special notes for your reviewer:
Tested with kdmp volume + resource and then resource only
restore of the same