refactor 🔨 : implement client factory for DNS client [#155] #156

sudiptob2 · 2023-05-23T13:11:59Z

Description

closes #155

Changes Made

implemented client factory folder structure
implemented creation of DNSClient using factory
refactored get_managed_zones and test_dns_policies with DNSClient
updated unit test to call get_managed_zones and test_dns_policies with new method signature.

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

under-hill · 2023-05-24T18:34:35Z

src/gcp_scanner/scanner.py

-                                                                  credentials)
+        project_result['managed_zones'] = crawl.get_managed_zones(
+          project_id,
+          ClientFactory.get_client('dns').get_service(credentials),


Eventually I'd like to move each of these resource-specific client creation calls behind a function that takes the resource string (e.g. "dns") and creds, and leverages a dictionary mapping from resource string to service object. Then we can dynamically update the dict as we loop through the scan_config: either set the newly created object or re-use if it's already there for the resource key.

I'm open to doing this now, or revisiting after the we've implemented each of the clients. WDYT?

Hi, I'm trying to gain a complete understanding of the motivation behind this approach. Is it because calling ClientFactory.get_client('dns').get_service(credentials) multiple times creates redundant objects in memory? If so I can think of an alternative approach also. 💡

For me, it would be more convenient to incorporate the above refactoring when we implement the double-loop approach. At this moment, I'm having difficulty visualizing the overall implementation, and I think it would be easier for me to grasp if I can start by implementing the simpler parts. Once I have a better understanding of those, I believe I will be able to comprehend this approach more easily 🎯

Yes, it helps us keep only one client in memory for each resource.

That sounds perfectly reasonable, let's revisit the idea later and introduce it as an isolated PR if we decide to do it.

under-hill · 2023-05-24T18:35:38Z

src/gcp_scanner/client/interface_client.py

+  @staticmethod
+  @abstractmethod
+  def get_service(credentials: Credentials) -> discovery.Resource:
+    """Create a client.


Nit: let's update the description here

This should be "Get a discovery resource for a client", not "Create a client", right?

thanks, lets make it more verbose.

src/gcp_scanner/client/interface_client.py

src/gcp_scanner/client/dns_client.py

src/gcp_scanner/client/client_factory.py

under-hill

Some spelling / documentation nits, and discussion about resource->client map

sudiptob2 · 2023-05-24T18:46:16Z

Thanks a ton! I am extremely sorry that I missed some of the basic docs and spelling. will be addressing your comments one by one as soon as possible.

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

under-hill

More comment nits, and request to add another unit test

under-hill · 2023-05-30T00:31:44Z

src/gcp_scanner/client/interface_client.py

+      An object of discovery.Resource
+    """
+
+    raise NotImplementedError("Child class must implement create_client")


do you mean "must implement get_service" here?

under-hill · 2023-05-30T00:36:13Z

src/gcp_scanner/client/interface_client.py

+  @staticmethod
+  @abstractmethod
+  def get_service(credentials: Credentials) -> discovery.Resource:
+    """Create a client.


This should be "Get a discovery resource for a client", not "Create a client", right?

under-hill · 2023-05-30T00:39:42Z

src/gcp_scanner/scanner.py

-                                                                  credentials)
+        project_result['managed_zones'] = crawl.get_managed_zones(
+          project_id,
+          ClientFactory.get_client('dns').get_service(credentials),


Yes, it helps us keep only one client in memory for each resource.

That sounds perfectly reasonable, let's revisit the idea later and introduce it as an isolated PR if we decide to do it.

under-hill · 2023-05-30T00:42:53Z

src/gcp_scanner/test_unit.py

Let's add a test for calling ClientFactory.get_client() with a gibberish string argument, and ensuring that it logs an error and returns None as expected.

added test. Also organized tests related to client factory in a Class, later on we can add at least one test case for each client factory here.

under-hill · 2023-05-30T00:51:16Z

src/gcp_scanner/crawl.py

  Args:
    project_name: A name of a project to query info about.
-    credentials: An google.oauth2.credentials.Credentials object.
+    service: A resource object for interacting with the Compute API.


This should say "DNS API", not "Compute API" right?

yes, or can we say GCP API?

under-hill · 2023-05-30T00:51:24Z

src/gcp_scanner/crawl.py

  Args:
    project_id: An id of a project to query info about.
-    credentials: An google.oauth2.credentials.Credentials object.
+    service: A resource object for interacting with the Compute API.


This should say "DNS API", not "Compute API" right?

fix:sparkles:: implement client factory for DNS client [google#155]

a503910

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

sudiptob2 changed the title ~~fix:sparkles:: implement client factory for DNS client [#155]~~ refactor:sparkles:: implement client factory for DNS client [#155] May 24, 2023

sudiptob2 changed the title ~~refactor:sparkles:: implement client factory for DNS client [#155]~~ refactor 🔨 : implement client factory for DNS client [#155] May 24, 2023

sudiptob2 added 2 commits May 24, 2023 22:06

refactor:hammer:: refactor list_dns_policies

5f8986c

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

refactor:hammer:: refactor managed_zones

247fa2d

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

sudiptob2 marked this pull request as ready for review May 24, 2023 16:36

under-hill reviewed May 24, 2023

View reviewed changes

src/gcp_scanner/client/interface_client.py Outdated Show resolved Hide resolved

under-hill reviewed May 24, 2023

View reviewed changes

src/gcp_scanner/client/dns_client.py Outdated Show resolved Hide resolved

under-hill reviewed May 24, 2023

View reviewed changes

src/gcp_scanner/client/client_factory.py Outdated Show resolved Hide resolved

under-hill reviewed May 24, 2023

View reviewed changes

sudiptob2 marked this pull request as draft May 24, 2023 19:29

sudiptob2 added 4 commits May 25, 2023 01:31

refactor:hammer:: update docstring for Clientfactory class

a7890eb

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

refactor:hammer:: update docstring for DNSClient

06d441f

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

refactor:hammer:: fix spelling.

50abebd

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

refactor:hammer:: fix spelling in docstring.

9156ec2

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

sudiptob2 requested a review from under-hill May 25, 2023 15:58

sudiptob2 marked this pull request as ready for review May 28, 2023 15:16

feat: update docstring.

b377588

Signed-off-by: Sudipto Baral <sudiptobaral.me@gmail.com>

under-hill reviewed May 30, 2023

View reviewed changes

sudiptob2 added 3 commits May 30, 2023 10:44

refactor:hammer:: fix docstring for IClient interface.

7661441

refactor:hammer:: fix docstring.

c1e8594

test:rotating_light:: add unit test for client factory.

be75456

sudiptob2 requested a review from under-hill May 30, 2023 05:11

under-hill approved these changes May 30, 2023

View reviewed changes

under-hill merged commit fce767f into google:main May 30, 2023

refactor 🔨 : implement client factory for DNS client [#155] #156

refactor 🔨 : implement client factory for DNS client [#155] #156

Uh oh!

Conversation

sudiptob2 commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

under-hill left a comment

Choose a reason for hiding this comment

Uh oh!

sudiptob2 commented May 24, 2023

Uh oh!

under-hill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sudiptob2 commented May 23, 2023 •

edited

Loading