[infra-proxy-service] Client list using search API #4660

kvivek1115 · 2021-01-28T05:05:56Z

🔩 Description: What code changed, and why?

Client list using search API with pagination support.
It would not break the existing list API.
It will fetch all clients available in the system if no params are given.
Search query works with the following request params
- search_query.q: Search query string have to in the format like *:*, name:* or name:node-* etc.
- search_query.page: page number. [Updated 04-Feb-2020]
- search_query.per_page: number of records per page. [Updated 04-Feb-2020]
gRPC API

chef-automate dev grpcurl infra-proxy-service chef.automate.domain.infra_proxy.service.InfraProxyService.GetClients -- -d '{"server_id": "chef-server-dev-test", "org_id": "chef-org-dev", "search_query": {"q": "name:*", "page": 0, "per_page": 1}}'
{
  "clients": [
    {
      "name": "chef-load-25"
    }
  ],
  "page": 0,
  "total": 56
}

NOTE: integration test cases written based on prepopulated data using scripts.

⛓️ Related Resources

Fixes: #4628

👍 Definition of Done

Client list API is now available with search and pagination.

👟 How to Build and Test the Change

Steps to build:

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

Steps to test:

To add data using 3rd internal steps https://github.com/chef/automate/blob/master/dev-docs/adding-data/adding_test_data.md#adding-data-to-infra-views
Client list API without a search query

curl -sSkH "api-token: $(get_admin_token)" https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients?pretty
{
  "clients": [
    {
      "name": "chef-load-27"
    },
    {
      "name": "chef-load-25"
    },
    {
      "name": "chef-load-29"
    },
   ...
   
  ],
  "page": 0,
  "total": 56
}

Client list API with a search query. [Update 04-Feb-2020]

curl -sSkH "api-token: $(get_admin_token)" "https://a2-dev.test/api/v0/infra/servers/chef-server-dev-test/orgs/chef-org-dev/clients?search_query.q=name:node-*&search_query.page=0&search_query.per_page=5"
{
  "clients": [
    {
      "name": "node-learn_chef_apache2"
    },
    {
      "name": "node-git"
    },
    {
      "name": "node-push-job"
    },
   ...
   
  ],
  "page": 0,
  "total": 7
}

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

📷 Screenshots, if applicable

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

Aha! Link: https://chef.aha.io/epics/SH-E-393

netlify · 2021-01-28T05:06:00Z

Deploy preview for chef-automate processing.

Building with commit 0372e2e

https://app.netlify.com/sites/chef-automate/deploys/602540ed5a19eb0007787efc

vivek-yadav

Please let me know if any doubts

vivek-yadav · 2021-01-29T12:23:08Z

components/infra-proxy-service/server/clients.go

 // GetClients get clients list
 func (s *Server) GetClients(ctx context.Context, req *request.Clients) (*response.Clients, error) {
 	c, err := s.createClient(ctx, req.OrgId, req.ServerId)
 	if err != nil {
 		return nil, err
 	}

-	clients, err := c.client.Clients.List()
+	res, err := c.SearchClients(req.SearchQuery)


can this be not like this : c.client.Clients.Search()

making c.client.Clients.Search() is like adding the changes into https://github.com/go-chef/chef/blob/master/client.go but here we need to handle some cases based on our need, so added like this.

It would be some refactoring needed for making search API call for other objects like environments, roles, and data bag items.

Although I don't believe in creating Tech Debut for future. But if still you want to only add code in this repo, then

You could create a separate file to extend features of ApiClientService and use that function which will still be c.client.Clients.Search()
Also, I could not see any unit tests.
Please let me know if I am mistaken.

Sure will take a look how we can achieve it, thanks

@vivek-yadav now, using query, err := c.client.Search.NewQuery("INDEX", "SEARCH_TERM") for searching please review it again, Thanks!

punitmundra · 2021-01-29T14:15:04Z

this is the curl request for search
curl --location --request GET 'https://a2-dev.test/api/v0/infra/servers//orgs//clients?search_query.q=name:*&search_query.start=1&search_query.rows=3'
--header 'api-token: '
--data-raw ''

jaym · 2021-01-29T14:37:27Z

components/infra-proxy-service/server/clients.go

+				return result, ParseAPIError(err)
+			}
+
+			defer res1.Body.Close() // nolint: errcheck


I think these defers are going to get stacked until the function returns. This is probably not good style, but can have some real consequences, such as this code wouldn't be able to reuse any of the http connections while in the loop

Thanks for information @jaym , will rectify this

Remove search all looping entirely, not using res1.Body.Close().

alexpop · 2021-01-29T16:21:05Z

Hey @vsingh-msys is it possible to add some integration tests for these APIs?

kmacgugan · 2021-01-29T18:28:34Z

From a product perspective I think this functionality could be potentially confusing. It is getting a list of clients from the infra-server but it might differ from the list of clients in automate. Would it make more sense to have automate be the source of truth for clients and pull any missing clients in from infra server?

kvivek1115 · 2021-02-04T05:21:19Z

@kmacgugan, yeah it is a bit of confusing terms with reference to Automate client runs.

In Automate infra views, clients mean an API client which used to provide auth to communicate between nodes and infra clients as it falls under infra-proxy-service might we can distinguish

it named based on chef-server API https://docs.chef.io/server/api_chef_server/#get-10 & manage also naming the same.

@jonong1972 any suggestions ^^^

jonong1972 · 2021-02-04T15:53:19Z

@vsingh-msys @kmacgugan I'm confused on the question. I'm reading two different items here. One is a back-end question the other is a naming question?

kvivek1115 · 2021-02-11T05:58:50Z

@jonong1972 yes, one is for backend and another is for UI in chef manage, but both pointed the object, so in order to maintain the parity, keeping the same name.

kalroy · 2021-02-11T10:55:49Z

components/infra-proxy-service/integration_test/clients_test.go

+		res, err := infraProxy.GetClients(ctx, req)
+		assert.NoError(t, err)
+		assert.NotNil(t, res)
+		assert.Equal(t, int(res.Page), 0)


These tests are too specific to the sample data loaded. Let's make sure that we modify them when we are going to add edit/create apis.

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

kvivek1115 requested a review from a team as a code owner January 28, 2021 05:05

kvivek1115 self-assigned this Jan 28, 2021

kvivek1115 added the automate-infra-proxy label Jan 28, 2021

kvivek1115 added this to the Automate 2021 Q1 SP1 milestone Jan 28, 2021

punitmundra self-requested a review January 29, 2021 10:22

vivek-yadav requested changes Jan 29, 2021

View reviewed changes

jaym reviewed Jan 29, 2021

View reviewed changes

kvivek1115 marked this pull request as draft February 2, 2021 11:58

kvivek1115 modified the milestones: Automate 2021 Q1 SP1, Automate 2021 Q1 SP2 Feb 5, 2021

kvivek1115 force-pushed the VSingh/infra-proxy-service-clients-search-api branch from 18072fe to 34cf711 Compare February 11, 2021 05:53

kvivek1115 marked this pull request as ready for review February 11, 2021 06:08

kvivek1115 requested review from vivek-yadav and jaym February 11, 2021 06:13

kalroy self-requested a review February 11, 2021 10:54

kalroy approved these changes Feb 11, 2021

View reviewed changes

Vivek Singh added 7 commits February 11, 2021 16:39

client list using search API

3edd743

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

default rows count if not provided

5e2352a

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

Use search query interface

f502826

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

fixes tabs to spaces

8ecdb38

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

tabs to spaces

ec465fc

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

Renamed start & rows with page and per_page search params

f599cf8

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

remove untracked file pushed

8bc33a1

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

Vivek Singh added 3 commits February 11, 2021 16:55

Added client list search test cases

2ae2e23

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

proto comments update

000845a

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

minor updates

5b98415

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

punitmundra requested a review from kalroy February 11, 2021 12:27

kalroy approved these changes Feb 11, 2021

View reviewed changes

kvivek1115 force-pushed the VSingh/infra-proxy-service-clients-search-api branch from ffedd67 to 5b98415 Compare February 11, 2021 14:08

punitmundra approved these changes Feb 11, 2021

View reviewed changes

proto changes

0372e2e

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

vivek-yadav approved these changes Feb 11, 2021

View reviewed changes

kalroy merged commit 566e077 into master Feb 11, 2021

kalroy deleted the VSingh/infra-proxy-service-clients-search-api branch February 11, 2021 16:02

kvivek1115 added the acceptance: verified label Feb 18, 2021

AnkurMundhra added Epic and removed automate-infra-proxy acceptance: verified labels Aug 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[infra-proxy-service] Client list using search API #4660

[infra-proxy-service] Client list using search API #4660

kvivek1115 commented Jan 28, 2021 •

edited by AnkurMundhra

Loading

netlify bot commented Jan 28, 2021 •

edited

Loading

vivek-yadav left a comment

vivek-yadav Jan 29, 2021

kvivek1115 Jan 29, 2021

vivek-yadav Jan 29, 2021

kvivek1115 Jan 29, 2021

kvivek1115 Feb 11, 2021

punitmundra commented Jan 29, 2021

jaym Jan 29, 2021

kvivek1115 Jan 29, 2021

kvivek1115 Feb 11, 2021

alexpop commented Jan 29, 2021

kmacgugan commented Jan 29, 2021

kvivek1115 commented Feb 4, 2021

jonong1972 commented Feb 4, 2021

kvivek1115 commented Feb 11, 2021

kalroy Feb 11, 2021

[infra-proxy-service] Client list using search API #4660

[infra-proxy-service] Client list using search API #4660

Conversation

kvivek1115 commented Jan 28, 2021 • edited by AnkurMundhra Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

netlify bot commented Jan 28, 2021 • edited Loading

vivek-yadav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punitmundra commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexpop commented Jan 29, 2021

kmacgugan commented Jan 29, 2021

kvivek1115 commented Feb 4, 2021

jonong1972 commented Feb 4, 2021

kvivek1115 commented Feb 11, 2021

Choose a reason for hiding this comment

kvivek1115 commented Jan 28, 2021 •

edited by AnkurMundhra

Loading

netlify bot commented Jan 28, 2021 •

edited

Loading