travisghansen / kubernetes-client-php

No nonsense PHP Client for the Kubernetes API
Apache License 2.0
33 stars 10 forks source link

Fixed JSONPath lib handling. #6

Closed data219 closed 3 years ago

data219 commented 3 years ago

Accessing array element '0' results in error on empty result sets, while ->first() just returns null in that case.

travisghansen commented 3 years ago

Should we be throwing errors in various places here instead of just returning? if the token/expiry key are set but we get empty values should that throw an error?

data219 commented 3 years ago

I refactored my knowledge from observing kubectl and how it handles 2 kinds of config files generated by Google Cloud SDK. And judging from that everything under the auth-provider key must be optional (since Google can generate a config file where just the auth provider name is set and nothing else and it works with only the certificate-authority-data.) If no access token found than it tryies with only the certificate, if no expiry key it will not try to update (which requires the command anyways), if no command also no try to update the token. if no arguments command is run without arguments.

Also the updated auth token (and new expiry timestamp) should be written back to the config file. But since this is just a minor performace improvement (not running the update command if not needed) I prefer to leave it for another day.

data219 commented 3 years ago

With this latest commit I fixed the command running even if a valid, not expired token can be found in the config file. Which now also is read from config file first (previously it was only tried to find in in update commands output.)

data219 commented 3 years ago

So sorry, but that is still not correct. I need to refactor kubectl even more to see how that config data is to be intrepreted. I guess the only remaining error is that I thought the JSONPath expression are used to describe where to find the token and expiry in the configfile itself too, but thoose path only describe where to find the token and expiry in the commands output while the keys in the yaml config file are different ones, most likeley fixed/defined ones. I will keep testing the stuff in my current project and update this PR as soon as I can see it working in the same ways as kubectl

travisghansen commented 3 years ago

I think fundamentally both the auth and exec providers are implemented correctly currently. https://banzaicloud.com/blog/kubeconfig-security/

data219 commented 3 years ago

Yes, my ideas about the logic in reading the config where somehow misled. But the change in usage of array access vs that iterator ->first() method still make sense, since array access raises PHP errors on empty results. And I had that one configfile that was generated with a totally empty "auth-provider>config" section, only the name "gpc" configured under "auth-provider>name", that was raising thoose errors... Still it seems that kubectl intends to read token and expiry from the yaml config first, only executing the command if that token already expired, and yet if running the command also updates the yaml config.

I guess it makes sense to roll back all changes in this PR, leaving only the swapping of array access for the ->first() method. Which was my initial intention anyway.

travisghansen commented 3 years ago

Yes, agreed about the ->first() changes. I prefer to not write to the config file with this project as that seems like it could get quite messy managing that, so yes, the config is essentially in-memory after startup. Do you mind sharing what your config file looks like (cleanse any sensitive info)? I'd like to better understand what the failure is.

data219 commented 3 years ago

I can gladly share the config file, but I will need to regenerate it again (since that was born as a result of one of my experiments that led into a wrong direction for our project and is overwritten by now). Not a big deal, but I am about to call it a day and catch an espresso and some leftover sun. Thought of tidying up my project and this PR tomorrow.

data219 commented 3 years ago

This is the config-file generated by gcloud if using a service account locally:

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: LS0tLS........<cert-data>
    server: https://123.123.123.123
  name: gke_<project>_europe-west3-a_<cluster-name>
contexts:
- context:
    cluster: gke_<project>_europe-west3-a_<cluster-name>
    user: gke_<project>_europe-west3-a_<cluster-name>
  name: gke_<project>_europe-west3-a_<cluster-name>
current-context: gke_<project>_europe-west3-a_<cluster-name>
kind: Config
preferences: {}
users:
- name: gke_<project>_europe-west3-a_<cluster-name>
  user:
    auth-provider:
      name: gcp
data219 commented 3 years ago

PR is updated/squashed/tidied up. Only the real JSONPath API usage improvements left.