search

ComplianceAsCode / auditree-arboretum

The Auditree common fetchers, checks and harvest reports library.
https://auditree.github.io/
Apache License 2.0
17 stars 10 forks source link

cluster resource fetcher for IBM Cloud #54

Closed tmishina closed 3 years ago

tmishina commented 3 years ago

What

Add functionality of fetching resources from IBM Cloud Kubernetes clusters.

Why

Kubernetes resources (e.g., output of kubectl get pod) can be used as evidence. For example, spec of Pod, custom resource of an operator, and ConfigMap shows whether applications (pod) and kubernetes infrastructure (operator) run with correct (expected) configuration. Managed clusters have their own mechanism to manage multiple clusters, and therefore a specific fetcher for each managed cluster is required. This PR contains a fetcher for IBM Cloud clusters.

How

Test

Context

tmishina commented 3 years ago

I'd like to get reviewers' comments for the following items. Thanks in advance.

  1. Are these handling (raising a RuntimeError when a fatal error happens) suitable?
  2. Can I fixup two commits in this PR? I split it because they modify different category (ibm_cloud and kubernetes), but I know that PR should contain one commit.

I will bump version in and add a line to CHANGES.md when we are ready to merge this PR.

alfinkel commented 3 years ago

The PR can have as many commits as you want. The repo is set to squash all commits to one as part of a PR merge anyway.

alfinkel commented 3 years ago

If the fetcher is going to error anyway then I wouldn't bother raising a RuntimeError. Historically we've been pretty sparse with raising errors in fetchers and checks if the result produces an error anyway. If processing won't stop for some reason and you want it to then raise the error otherwise, I would let Python raise the error which should hopefully produce a useful stack trace.

tmishina commented 3 years ago

@alfinkel Thank you for your comment, I checked "raise" lines and removed them because error and its cause can be found at the error message that Python runtime generates. Also, I rebased the branch to v0.11.1.

tmishina commented 3 years ago

@alfinkel Thank you very much for your detailed review. I updated the code and the document as commented above. Please check them and tell me if further update is required. Thanks again.

tmishina commented 3 years ago

@alfinkel I have updated README and example configration. Could you review this? Thanks in advance.

alfinkel commented 3 years ago

It looks like your build is failing due to the code formatter. You should have the pre-commit hook set up locally to catch such things before they get here. In any case, it looks like this https://github.com/ComplianceAsCode/auditree-arboretum/pull/54/commits/31f04141be7d64ac2921c8532c8e94b2055dd0e2#diff-bcd498e6b9c862fab4f20772411039245ac766ae4cab4186d5c9285b8835064aR140-R142 is the culprit. I ran the formatter locally and the suggested change that it makes is terrible IMO. Here's what I would do instead:

                url = f'/global/v1/clusters/{cluster["id"]}/config'
                resp = self.session().get(url)

This will keep things easy to read while keeping the formatter happy.

tmishina commented 3 years ago

@alfinkel sorry for bothering, I'm still working using "commit suggestion" feature in addition to the local change. The "commit suggestion" feature automatically triggers test while I haven't complete entire fix process including code formatting. I will mention to you when I complete the fix, thanks.

alfinkel commented 3 years ago

@tmishina no problem at all. I just figured that I'd chime in with a better suggested change to the code than what the code formatter is making at the moment.

tmishina commented 3 years ago

@alfinkel I've finished updating the code and the readme. Please review the changes. Thank you so much for your support. I have done git rebase main; bumping the version string remains.

tmishina commented 3 years ago

@alfinkel I have applied your suggestions, bumped version, and updated CHANGES.md. Please tell me if further changes are required, thanks.

tmishina commented 3 years ago

@alfinkel thank you again and again, I updated the code.