utils and utils.create_from_yaml improvement suggestions

[oz123]

create_from_yaml should accept a string like object, like yaml.load and safe_safe do.
Then, it's the user's task to read the file.
To help users we can add a function create_from_file which accept a file path. create_from_file will than call create_from_yaml.

The new signatures should be:

def create_from_yaml(k8s_client, yaml_str, verbose=False, **kwargs):
      """
      load yaml string and apply using `create_from_map`
      """
      stream = yaml.safe_load_all(yaml_str)
      for obj in stream:
            create_from_map(k8s_client, obj, verbos)

      ...

def create_from_file(k8s_client, path_str, verbose=False, **kwargs):
      """
      read a file and continue with create_from_yaml
      """
      ...

Additionally, we should add:

 def create_from_json(k8s_client, json_str, verbose=False, **kwargs):
       """
       read a json like string  and apply by using create_from_map
       """
       ...

 def create_from_map(k8s_client, dict_like, verbose=False, **kwargs):
       """
       does all the logic previously in create_from_file.
       """

This approach brings the python client a little bit closer to

kubectl create -f <filename>

which accepts either files and streams from stdin, file, formatted as json and yaml).

I will happily prepare patch for this.

[micw523]

The YAML loaders should parse (most) JSON correctly especially those created by kubectl so I don’t think the JSON part is needed.

I don’t think you use the -f flag if you’re parsing from stdin though.

[oz123]

It's not usual to do this, but I have seen people do this in scripts, for example:

dnscheck_pod='{"apiVersion":"v1","kind":"Pod","metadata":{"labels":{"k8s-app":"dnscheck"},"name":"dnscheck","namespace":"default"},"spec":{"containers":[{"image":"tutum/dnsutils","name":"dnscheck","command":["sleep"],"args":["86400"]}],"securityContext":{}}}'; 
echo ${dnscheck_pod} | kubectl --kubeconfig=${KUBECONFIG} apply -f -; 

In any case, would you accept a PR with my suggestions?

[micw523]

I take 50% of what I said back - create -f - takes input from stdin. It’s not super unusual either - this kind of usage appears on the official documentation. It would require an extra parameter if we implement in Python: file name as - and a string input.

I’m conflicted on how useful this would be. It would be simple enough to implement though on top of the create_from_yaml we have now. Please do note that the function is not yet complete in the master branch. Probably we can wait a little bit to see how many users want this feature.

[oz123]

Currently, the function create_from_yaml requires a file. Would you consider accepting the PR
with just my first two suggestions ?

If so, my PR will include only these changes:

• change create_from_yaml to accept yaml string
• add a function which explicitly reads from file: create_from_file

[micw523]

I'd rather not do it since your suggestion will break existing API while not necessary. If the addition of feature is deemed necessary, I'd rather make the change inside create_from_yaml and make create_from_file an alias.

[oz123]

@micw523
Actually, I like your suggestion! I can do this as you suggested.

create_from_yaml can check if the string passed to it is a file system path, if it is we'll read it as as yaml. If it's not we can can directly load the yaml.

[juliantaylor]

The function should also support file handles instead of path strings, so if you pass it a StringIO object, stdin or an already open file with valid yaml it will also work.

something like the suggested create_from_map would indeed be very useful. It is currently quite inconvenient if you want to create multiple objects with minor changes (like different namespace)

[eagle9527]

I think the Python interface, using JSON is very common, I create applications are the transfer of JSON format data for application creation, easy to use, integration into their own platform is also very convenient.

[dee-me-tree-or-love]

That would be a very convenient feature, has anyone started working on it?

Also wouldn't it be useful to expand the API with providing custom resovlers and constructors for the yaml loader? It would be possible then to use the environment variables when loading the file. If there is any better way to do it already, would appreciate a suggestion, otherwise I could pick that up :)

https://pyyaml.org/wiki/PyYAMLDocumentation#constructors-representers-resolvers
@oz123 @micw523 what do you think?

[oz123]

@dee-me-tree-or-love I have a branch where this is already partially implemented. I need to clean it and make a PR. Will do so this week.

[oz123]

@dee-me-tree-or-love I pushed my branch. I agree that custom resovlers and constructor could be useful, but in order to enhance the chance that this PR is accepted I think the scope should be minimum.
A follow up request can enhance this.

[micw523]

@dee-me-tree-or-love I'm not sure if we need a custom class for loading YAML since

1. the whole YAML body should be passed into the function calls for performing an action specified in the YAML file
2. we pretty much know what kind of things we need to extract from the YAML file anyway...?
   Let me know if I misunderstood.

[micw523]

/assign
/assign @oz123

[oz123]

The current master branch has utils.create_from_yaml_single_item which is basically like create_from_map I suggested. utils.create_from_yaml still accepts a file. I guess this would surprised a few more users, but after getting an error once, and remembering that the function expects a file this is OK.

[micw523]

Can you reopen this, @oz123? We could still get something done here.

[oz123]

@micw523 I appreciate you not giving up on this. I will need more guidance as for what can be still added.

[micw523]

As a start, the e2e_test folder now contains a few extra yaml files that weren’t there before. There are a few files that are List kind or contain multiple resources in them. Those cannot be handled by the create_from_yaml_single_itemfunction, since that function only supports an explicit kind.

What we can do is approximately the same thing you did it before and still change the create_from_yaml function to handle a dict input or a string input. What do you think?

[micw523]

Another reason the create_from_yaml_single_item get segmented out is that if we should have a dynamic client in the future this function will likely become obsolete and be replaced by an implementation based on the dynamic client. Be assured, it’s not rendering anything that you did obsolete.

[dee-me-tree-or-love]

hello @oz123 @micw523, sorry I have been not responsive, great to see the progress, thanks!
@micw523 what I meant with the custom resolvers is to provide the functionality to easier support variable substitution.

# nginx-deployment.yaml (silly example file)
apiVersion: v1
kind: Service
metadata:
  name: ${SERVICE_NAME}
spec:
  ports:
  - port: ${SERVICE_PORT}
    targetPort: ${TARGET_PORT}
    name: web-app
  - port: 80
    targetPort: 80
    name: nginx
  type: ${SERVICE_PORT_TYPE}

and then

# Define the resolvers and the constructors
#   see https://pyyaml.org/wiki/PyYAMLDocumentation#constructors-representers-resolvers
path_matcher = re.compile(r'\$\{([^}^{]+)\}')
def path_constructor(loader, node):
  ''' Extract the matched value, expand env variable, and replace the match '''
  value = node.value
  match = path_matcher.match(value)
  env_var = match.group()[2:-1]
  return os.environ.get(env_var) + value[match.end():]

def main():
  # Configure the utils to use the resolver and constructor
  utils.add_implicit_yaml_resolver('!path', path_matcher)
  utils.add_yaml_constructor('!path', path_constructor)

  # Now all the environment variables will be substituted on the flight
  utils.create_from_yaml(k8s_client, "nginx-deployment.yaml")
  ...

Thought of this from this discussion https://stackoverflow.com/questions/26712003/pyyaml-parsing-of-the-environment-variable-in-the-yaml-configuration-file

Maybe there is a better way to do this already, which I am not aware of already, would very much appreciate a suggestion if so, sorry for a long message and if I misunderstood something

[juliantaylor]

@dee-me-tree-or-love adding such functionality to the kubernetes python client is a bad idea, you are mixing features of a specific yaml deserializer implementation with a generic api client and these two things should have no connection besides that the output of one (which is an generic python object) can be the input of the other.
Besides that you are mixing two orthogonal APIs what about other serialization formats like json, msgbuf, xml etc.? the pyyaml API won't work on these formats.

But your feature will be more easily possible with the proposed improvements as as it decouples the data format from the input to the function so you can use these features more freely, e.g. your code could then look like this:

yaml.add_yaml_constructor('!path', path_constructor)
# ....
with open(data) as f:
   utils.create_from_mapping(k8s_client, yaml.load(f))

(you can already do that right now by writing the parsed yaml to a NamedTempfile, but that is very ugly which is why this issue exists)

[dee-me-tree-or-love]

@juliantaylor thanks a lot for the suggestion, indeed, I definitely didn't take the big picture into the consideration and made quite a mistake in mixing the boundaries... thanks a lot for pointing out this fact, it is a very good learning!

[oz123]

@micw523 please see my PR #795 . This is my suggestion on how to allow creating of k8s objects
from python dicts, and also allow passing of strings without breaking the existing API.

A later version could deprecate usage of create_from_yaml with a file, and require direct usage
via create_from_file. But I leave this now out of the PR.

Also, I still have not exposed create_from_map by importing it in kuberentes/utils/__init__.py.
As far as I understand from the discussion here, there is a desire by the participants here that this would be done, but I would like to hear a second opinion.

[dee-me-tree-or-love]

Friendly bump, is this still on the review? :)

[oz123]

@dee-me-tree-or-love @micw523 hasn't had any public activity since April, and I would appreciate it if we could merge this PR. It's been rotting here, and since I can't merge this one, I put many other contributions on hold.

Also, there is another PR with a similar intent #770, which has been rotting. This functionality is really wanted (and needed), so please merge this. It would be a good community service.

[oz123]

This issue is finally fixed.