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

[infra-proxy-service] reset node client key, create client & delete client API #4592

Merged
merged 9 commits into from
Feb 11, 2021

Conversation

kvivek1115
Copy link

@kvivek1115 kvivek1115 commented Jan 14, 2021

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

  • Infra proxy service is now able to respond create client, delete client and reset client chef server APIs.

👟 How to Build and Test the Change

  • rebuild components/infra-proxy-service
  • rebuild components/automate-gateway

Steps to test:

Client detail API with appended default key data:

curl -sSkH "api-token: $(get_admin_token)" https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients/rhsm-rhel8?pretty
{
  "name": "rhsm-rhel8",
  "client_name": "rhsm-rhel8",
  "org_name": "4thcoffee",
  "validator": false,
  "json_class": "Chef::ApiClient",
  "chef_type": "client",
  "client_key": {
    "name": "default",
    "public_key": "-----BEGIN PUBLIC KEY-----...-----END PUBLIC KEY-----\n",
    "expiration_date": "infinity"
  }
}

Create Client API

curl -sSkH "api-token: $(get_admin_token)" -X POST https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients?pretty -d '{"name": "abctes6t", "create_key": true}'
{
  "name": "abctes6t",
  "client_key": {
    "name": "default",
    "public_key": "-----BEGIN PUBLIC KEY-----...-----END PUBLIC KEY-----\n\n",
    "expiration_date": "infinity",
    "private_key": "-----BEGIN RSA PRIVATE KEY-----...-----END RSA PRIVATE KEY-----"
  }
}

Delete Client API

curl -sSkH "api-token: $(get_admin_token)" -X DELETE https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients/abctes6t?pretty                                              
{
  "name": "abctes6t",
  "client_name": "",
  "org_name": "",
  "validator": false,
  "json_class": "",
  "chef_type": ""
}

Reset Client Key API

curl -sSkH "api-token: $(get_admin_token)" -X PUT https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients/node2/reset?pretty
{
  "name": "default",
  "public_key": "",
  "expiration_date": "",
  "private_key": "-----BEGIN RSA PRIVATE KEY-----...-----END RSA PRIVATE KEY-----"

✅ Checklist

📷 Screenshots, if applicable

Signed-off-by: Vivek Singh [email protected]

@kvivek1115 kvivek1115 self-assigned this Jan 14, 2021
@kvivek1115 kvivek1115 requested a review from a team as a code owner January 14, 2021 12:13
@netlify
Copy link

netlify bot commented Jan 14, 2021

Deploy preview for chef-automate processing.

Building with commit 929d43d

https://app.netlify.com/sites/chef-automate/deploys/6024a0efed69810007630c19

@kvivek1115 kvivek1115 changed the title [WIP] [infra-proxy-service] reset node client key, create client & delete client API [infra-proxy-service] reset node client key, create client & delete client API Jan 15, 2021
@shaik80
Copy link
Member

shaik80 commented Jan 18, 2021

Facing some trouble in Create Client API
{
“error”: “no server found with ID \“chef-server-dev-test\“”,
“code”: 5,
“message”: “no server found with ID \“chef-server-dev-test\“”,
“details”: [
]
}

@kvivek1115
Copy link
Author

@shaik80 hope you have added the chef-server entry provided in the "Steps to test".

Could you please verify with providing the output of:

curl -sSkH "api-token: $(get_admin_token)" https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test?pretty

@kvivek1115
Copy link
Author

Updated the client detail API containing public key data:

curl -sSkH "api-token: $(get_admin_token)" https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients/rhsm-rhel8?pretty
{
  "name": "rhsm-rhel8",
  "client_name": "rhsm-rhel8",
  "org_name": "4thcoffee",
  "validator": false,
  "json_class": "Chef::ApiClient",
  "chef_type": "client",
  "client_key": {
    "name": "default",
    "public_key": "-----BEGIN PUBLIC KEY-----...-----END PUBLIC KEY-----\n",
    "expiration_date": "infinity"
  }
}

@kvivek1115 kvivek1115 added this to the Automate 2021 Q1 SP1 milestone Jan 25, 2021
rpc DeleteClient (infra_proxy.request.Client) returns (infra_proxy.response.Client) {
option (google.api.http).delete = "/api/v0/infra/servers/{server_id}/orgs/{org_id}/clients/{name}";
option (chef.automate.api.iam.policy).resource = "infra:servers:{server_id}:orgs:{org_id}:clients";
option (chef.automate.api.iam.policy).action = "infra:infraServers:update";
Copy link
Collaborator

Choose a reason for hiding this comment

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

check the other proto file, in case of get/update/delete this policy has differerent way to handle.
option (chef.automate.api.iam.policy).action = "infra:infraServers:delete";

Copy link
Author

Choose a reason for hiding this comment

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

@punitmundra, here we handled it differently, in all action only get & update for all third-level objects like chef server >> chef organizations >> cookbooks| roles| clients | data bags etc.

Please check the detailed discussion in PR #3550

Copy link
Collaborator

Choose a reason for hiding this comment

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

fine

Comment on lines 28 to 31
string name = 3;
// Boolean indicates client type is validator or not.
bool validator = 4;
// Boolean indicates whether it's required to create a key or not.
Copy link
Collaborator

Choose a reason for hiding this comment

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

make the proper alinement

Copy link
Author

Choose a reason for hiding this comment

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

I guess alignment looks okay here, any specific ask?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks OK in some editors and it looks wrong in some editors--that is because you are mixing spaces and tabs. Please change to all spaces.
(This applies to several files.)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, Due to some misconfiguration in my local VSCode. Have fixes for some files, will keep it for future in relevant files.
Thanks!

Comment on lines 28 to 30
string name = 3;
// Boolean indicates client type is validator or not.
bool validator = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

make proper formatting

// Deletes the existing key
_, err = c.client.Clients.DeleteKey(req.Name, key)
chefError, _ := chef.ChefError(err)
if err != nil && chefError.StatusCode() != 404 {
Copy link
Author

Choose a reason for hiding this comment

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

Ignore if the key already got deleted

Copy link
Collaborator

@punitmundra punitmundra left a comment

Choose a reason for hiding this comment

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

I have test the code locally and it gives the output as expected.

rpc DeleteClient (infra_proxy.request.Client) returns (infra_proxy.response.Client) {
option (google.api.http).delete = "/api/v0/infra/servers/{server_id}/orgs/{org_id}/clients/{name}";
option (chef.automate.api.iam.policy).resource = "infra:servers:{server_id}:orgs:{org_id}:clients";
option (chef.automate.api.iam.policy).action = "infra:infraServers:update";
Copy link
Collaborator

Choose a reason for hiding this comment

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

fine

Comment on lines 28 to 31
string name = 3;
// Boolean indicates client type is validator or not.
bool validator = 4;
// Boolean indicates whether it's required to create a key or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks OK in some editors and it looks wrong in some editors--that is because you are mixing spaces and tabs. Please change to all spaces.
(This applies to several files.)

}, nil
}

// ResetClientKey resets an infra client
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ResetClientKey resets an infra client
// ResetClientKey resets an infra client key

Also, could we explain what it means to reset a client key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still: what does it mean to "reset a client key"? Does that mean set the key to null? ...to empty string? ...to a random string? or something else.

Copy link
Author

Choose a reason for hiding this comment

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

In chef infra context, resetting the client key, deletes the associated key pair and generates new key pair again, and then attached it to the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. That should be in the code comment, too.

@kvivek1115
Copy link
Author

@msorens have made the tabs to spaces changes it due to vscode issue microsoft/vscode-go#2871 good to review it again.

@kvivek1115 kvivek1115 force-pushed the VSingh/infra-proxy-service-node-reset-key-api branch from 0e959a9 to 2d638a1 Compare February 10, 2021 23:55
}

// ResetClientKey resets the client key
// Deletes the associated key pair and generates new key pair again, and then attached it to the client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Deletes the associated key pair and generates new key pair again, and then attached it to the client.
// Deletes the associated key pair and generates new key pair again, and then attaches it to the client.

Signed-off-by: Vivek Singh <[email protected]>
@kalroy kalroy merged commit 96800f0 into master Feb 11, 2021
@kalroy kalroy deleted the VSingh/infra-proxy-service-node-reset-key-api branch February 11, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[infra-proxy-service] API to node reset key
5 participants