-
Notifications
You must be signed in to change notification settings - Fork 152
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 repositoryServerUserHostname to the kopia repository based functions #2177
Add repositoryServerUserHostname to the kopia repository based functions #2177
Conversation
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
docs/functions.rst
Outdated
@@ -1607,6 +1607,7 @@ Arguments: | |||
`container`, Yes, `string`, name of the kanister sidecar container | |||
`includePath`, Yes, `string`, path of the data to be backed up | |||
`snapshotTags`, No, `string`, custom tags to be provided to the kopia snapshots | |||
`userHostname`, No, `string`, user's hostname from user access map to be provided to the kopia repository server for performing backup |
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.
We should change it to RepositoryServerUserHostname
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.
What is user access map? We will have to simplify this for users.
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 !
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.
We should rename userHostname
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.
LGTM
docs/functions.rst
Outdated
@@ -1607,6 +1607,7 @@ Arguments: | |||
`container`, Yes, `string`, name of the kanister sidecar container | |||
`includePath`, Yes, `string`, path of the data to be backed up | |||
`snapshotTags`, No, `string`, custom tags to be provided to the kopia snapshots | |||
`userHostname`, No, `string`, user's hostname from user access map to be provided to the kopia repository server for performing backup |
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.
What is user access map? We will have to simplify this for users.
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
@@ -1607,6 +1607,7 @@ Arguments: | |||
`container`, Yes, `string`, name of the kanister sidecar container | |||
`includePath`, Yes, `string`, path of the data to be backed up | |||
`snapshotTags`, No, `string`, custom tags to be provided to the kopia snapshots | |||
`repositoryServerUserHostname`, No, `string`, user's hostname to access the kopia repository server. Hostname would be available in the user access credential secret |
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.
does second sentence mean, user should figure out the hostname by looking at the user access credential secret?
What does it mean?
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.
Yes ! User could provide the hostname which he/she wants to use by looking at user access credentials secret. If it's not provided we'll get the last entry from the user access map which is present inside the secret by default.
if err != nil { | ||
return "", "", errors.Wrap(err, "Failed to find Hostname in User Access Map") | ||
} | ||
hostName = hostname |
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 really need this assignment?
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.
Yes Vivek, so if we look at this function, we're taking hostname
as an argument. So what we're doing is we're initially assigning the last value of hostname from user access map to hostName
. That is the default case.
Later on we're checking if user has provided any hostname
and if it exists in the user access map. If it's present, we're updating the hostName
's value with the hostname
provided by the user and returning hostName
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.
aah, I got confused between the casing of the variables, didn't see difference between hostName
and hostname
.
I don't think we need hostName
right? Even if we just remove hostName
I think we can work with hostname
easily. Let me know if you have questions about this.
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.
Removed the hostName
variable
@@ -254,3 +267,11 @@ func userCredentialsAndServerTLS(tp *param.TemplateParams) (string, string, erro | |||
} | |||
return string(userCredJSON), string(certJSON), nil | |||
} | |||
|
|||
func checkHostnameExistsInUserAccessMap(userAccessMap map[string]string, hostname string) error { | |||
// check if hostname is provided in the repository server exists in the user access map |
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.
this is the hostname is provided
or it exists in the user access map?
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.
multiple mapping of hostnames and passwords could be present in User Access Map inside Repository Server User Access Credentials Secret, So the end user has to provide one of the hostname which he/she wants to use for connecting with kopia repository server
Co-authored-by: Vivek Singh <vsingh.ggits.2010@gmail.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Co-authored-by: Vivek Singh <vsingh.ggits.2010@gmail.com>
@@ -223,24 +229,36 @@ func backupDataUsingKopiaServer( | |||
return kopiacmd.ParseSnapshotCreateOutput(stdout, stderr) | |||
} | |||
|
|||
func hostNameAndUserPassPhraseFromRepoServer(userCreds string) (string, string, error) { | |||
func hostNameAndUserPassPhraseFromRepoServer(userCreds, hostname string) (string, string, error) { |
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.
func hostNameAndUserPassPhraseFromRepoServer(userCreds, hostname string) (string, string, error) { | |
func hostNameAndUserPassPhraseFromRepoServer(userCreds, hostName string) (string, string, error) { |
should hostName
be preferred?
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.
I see hostname
is used as directly or as substring in other var name. I think we should change them to hostName
or HostName
. Whatever applicable.
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.
Yeah, seems good. Will make the changes !
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 !
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Signed-off-by: Rajat Gupta <rajat.gupta@veeam.com>
Looks good to me, let's 🚢 it. |
Change Overview
Add
repositoryServerUserHostname
as an Optional Argument to the Kopia Repository Based functions.It enables to users to pass the hostname from the user access map which they want to use while performing kopia repository server based operations
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
Manual Testing Steps
1) Create Images for Kanister and Repo Server controller
2) Install Kanister
3) Apply Repo Server CRD
4) Create Test Application [Time Logger]
5) Create OpenSSL Certificate
6) Create S3 Location and Location Secret Config Files
7) Apply Secrets
8) Create Repository
9) Create Repository Server CR
Wait till the status of Repository Server CR gets to
ServerReady
, You could check it by running following command10) Create Blueprint
11) Build kanctl with latest changes
12) Take Backup of the Application
Check Status of the actionset
13) Update Blueprint with userHostname with value not in user access map
14) Take Backup of the Application
Check Status of the actionset