-
Notifications
You must be signed in to change notification settings - Fork 89
Switch to pykube-ng to handle arbitrary resources (pods, jobs, etc) #110
Conversation
🤖 zincr found 0 problems , 2 warnings
Details on how to resolve are provided below Large CommitsChecks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of
Dependency LicensingAll dependencies specified in package manager files must be reviewed, banned dependency licenses will block the merge, all new dependencies introduced in this pull request will give a warning, but not block the merge Please ensure that only dependencies with licenses compatible with the license of this project is included in the pull request.
|
I tried this PR locally on K3s and i was having connectivity problems. Thanks @nolar for your support! |
kopf/k8s/classes.py
Outdated
from kopf.k8s import config | ||
|
||
|
||
# TODO: this mixin has to be migrated into pykube-ng itself. |
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.
feel free to open a PR for pykube-ng 😄
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 will for sure. First of all, let's see how it works in this mode. And second:
With the official client, there was an issue when the switch the Content-Type from merge-patch format (dicts of fields) to JSON-patch format (lists of ops) — see #134 and kubernetes-client/python#866.
For Kopf, merge-patch is needed and is sufficient. But for a generic k8s client, both formats should be supported.
kopf/k8s/classes.py
Outdated
kind = "CustomResourceDefinition" | ||
|
||
|
||
def _make_cls(resource) -> Type[APIObject]: |
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 guess this can be part of pykube-ng, too
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.
@hjacobs In theory, there is already an implementation in pykube.object_factory()
, and it is an officially documented way of creating the classes.
But I had issues with it. First, because of the self-made mixins with the patch()
method (now solved). Second, because it requires kind
as a search criterion, and Kopf uses the canonical plural names from the name
field (as in the K8s API URLs, and as in the original Kubernetes client library).
The automatic implicit mapping kubectl-style (i.e. any form is accepted, including the short-names) is planned in #57, and it will be more convenient for the users, but it is not yet there.
It would be nice if pukube-ng could search by any name field (kind, plural, singular, all shortnames) — same as kubectl does. But it can be done as a separate PR in Kopf.
kopf/k8s/config.py
Outdated
|
||
def login_pykube(): | ||
global _pykube_cfg | ||
try: |
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 guess we can add some "auto.." method for pykube-ng itself to not repeat the logic everywhere (I do the same as you in my projects: first try "in-cluster" then try kubeconfig file)
All pre-requisites are now in master. This branch is rebased on top of master, with few fixes here & there. The implementation is mainly the same. Ready for review — to be released as a pre-release version (presumably |
@nolar I created a PR to move reasonable changes upstream to |
kopf/clients/auth.py
Outdated
raise AccessError("Cannot authenticate to the Kubernetes API. " | ||
"Please login or configure the tokens.") | ||
else: | ||
raise |
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 was surprised to get a 403
from there the other day. I eventually discovered it was because my operator's service account was missing the following role:
nonResourceURLs: ["/"]
verbs: ["get"]
(This was not a problem when Kopf was using the kubernetes
client.)
Do you think it would be worth catching and mentioning here (not necessarily in this PR)?
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.
Good catch, I think requesting /
might be tricky to not forget in RBAC setups
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.
Good point. Maybe, there should be no verification at all. It should fail on the actual operations attempted.
Looking wider, it can be so that authentication succeeds, but the authorization fails on specific resources or specific REST operations of those resources, such as PATCH
(because of RBAC). The operator should perform a full self-check on startup. It will only cover the permissions needed by the framework (watching-patching), but will not cover the permissions needed by the operator's logic (e.g. creating pods/jobs).
See also: #49 for such self-checks (against a RBAC yaml, not against the real cluster, but it can be extended).
Meanwhile, I will make it here to ignore all errors except for HTTP 401 and TCP/HTTPS connection errors — since the purpose is to verify authentication, not authorization.
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 in 2118b8e. Now, only connection errors and 401 do raise (and few tests added). All other errors are ignored.
kopf/clients/classes.py
Outdated
|
||
|
||
# TODO: this mixin has to be migrated into pykube-ng itself. | ||
class APIObject(pykube.objects.APIObject): |
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 created hjacobs/pykube#29 to have this upstream
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.
kopf/clients/classes.py
Outdated
pass | ||
|
||
|
||
class CustomResourceDefinition(APIObject): |
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 created hjacobs/pykube#29 to have this upstream
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.
Codecov Report
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
- Coverage 83.81% 83.65% -0.16%
==========================================
Files 29 30 +1
Lines 1433 1542 +109
Branches 211 226 +15
==========================================
+ Hits 1201 1290 +89
- Misses 194 208 +14
- Partials 38 44 +6
Continue to review full report at Codecov.
|
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.
Just a potential simplification, but everything looks great.
""" | ||
try: | ||
api = get_pykube_api() | ||
rsp = api.get(version="", base="/") | ||
rsp.raise_for_status() | ||
api.raise_for_status(rsp) # replaces requests's HTTPError with its own. |
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 think the rsp.raise_for_status()
can be removed, because api.raise_for_status(rsp)
already does it and you are already catching everything.
Basically if the first line raises, the second would as well (but will not be run because the first raised), and if the first line doesn't raise, then the second won't either, so nothing is gained by having both.
Released as Note: it is a pre-release, so it requires either |
Description
Switch the internal K8s client library from the official library
kubernetes
topykube-ng
.This simplifies handling of arbitrary resource types, such as pods, jobs, etc — for any handlers: both regular handlers (
@kopf.on.create/update/delete/resume
), and spy-handles (@kopf.on.event
).The previous Kubernetes client library is supported optionally: if present, it will be auto-logged in, to keep compatibility with the previous behaviour. Otherwise, it is ignored. And it is not used anywhere. It will be removed from 1.0 release.
Types of Changes
PLEASE NOTE: The K8s client library used inside of Kopf is Kopf's internal implementation detail. Only the native data structures (dicts, lists, etc) are exposed from the framework to the operator and accepted back. The operators MUST NOT rely on how Kopf talks to the K8s APIs. It can be
requests
/requests-async
in the future (just as an example). The only dependency is auto-login — to be removed with #59 (the operators must explicitly authenticate themselves; now they can do this on the module-level code only).Todos
Review
List of tasks the reviewer must do to review the PR