Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method to dynamically set namespace in yaml files #782

Merged

Conversation

johscheuer
Copy link
Contributor

Fixes: #781

This PR allows a user to set a custom namespace if creating API objects from a yaml file. This is useful if you generate your namespaces dynamically in the Python code (e.g. e2e tests).

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 18, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 18, 2019
@johscheuer
Copy link
Contributor Author

I will add a test case for the new method.

@johscheuer johscheuer changed the title Add method to dynamically set namespace in yaml files WIP: Add method to dynamically set namespace in yaml files Mar 18, 2019
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 18, 2019
@johscheuer johscheuer changed the title WIP: Add method to dynamically set namespace in yaml files Add method to dynamically set namespace in yaml files Mar 18, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2019
@micw523
Copy link
Contributor

micw523 commented Mar 19, 2019

I think this could be done easily outside of create_from_yaml when we have #783.

@johscheuer
Copy link
Contributor Author

@micw523 yes with the MR it's possible since you can pass complete objects to the method. I still think that the possibility to set the namespace dynamically adds value to the end user, it's easier to set the namespace directly while iterating over the objects in: https://github.com/kubernetes-client/python/pull/783/files#diff-561089da9975de9999b6638341c50914R75

Otherwise I need to read all objects by myself patch them with the desired namespace and then pass them to create_from_yaml with a method like create_namespaced_from_yaml I don't have any further work as a user.

@micw523
Copy link
Contributor

micw523 commented Mar 20, 2019

I think without this function you still have to have a list of the yaml files you're going to load anyway and load one by one... I don't think patching it will be too much.

On a side note, I don't think overriding namespace should be encouraged. I just checked the behavior of the typed client. If you attempt to override the namespace in the yaml file, you get this error:

(400) Reason: Bad Request HTTP response headers: HTTPHeaderDict({'Content-Type': 'application/json', 'Date': 'Wed, 20 Mar 2019 04:16:58 GMT', 'Content-Length': '200'}) HTTP response body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"the namespace of the provided object does not match the namespace sent on the request","reason":"BadRequest","code":400} 

I think it's important that the create_from_yaml utility does exactly what it is told to do in the yaml file to avoid confusion by the client, and also to comply with the behavior from the typed client.

@johscheuer
Copy link
Contributor Author

Currently one benefit would be that the create_from_yaml could load multi object yaml file(s) for me and do all the handling around this (e.g. boilerplate) vs. doing this all by myself.

You actually can overwrite the namespace you just need to modify the namespace of the loaded object see: https://github.com/kubernetes-client/python/pull/782/files#diff-561089da9975de9999b6638341c50914R67 but I think your concern is correct and we shouldn't overwrite the namespace of an API object (if defined).

I still think that there is a good use case for create_namespaced_from_yaml this would be the equivalent of kubectl -n my-fancy-ns apply -f my-multi-object.yaml. This would be super useful for at least these use cases:

  • e2e testing where the namespace is dynamically created but the manifests are always the same (that's who some of the Kubernetes e2e tests are implemented)
  • Using the same manifests to deploy the same stack in different namespaces (actually this is similar to use case 1)

Without a create_namespaced_from_yaml I would need to read all API objects from the yaml files modify their namespace and then pass it to create_from_yaml so there's still a bunch of boilerplate which which could be avoided with the according method.

If you like I would adjust my PR to match the behaviour of kubectl apply -f ...:

For the following manifest the behavior would be:

apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: web
  name: web
spec:
  replicas: 1
  selector:
    matchLabels:
      app: web
  template:
    metadata:
      labels:
        app: web
    spec:
      containers:
      - image: nginx
        name: nginx

If there is no namespace defined in the yaml file I can pass kubectl any namespace I want (must exist):

$ kubectl -n test apply -f test.yaml     
deployment.apps/web created
$ kubectl -n test get deployments
NAME   READY   UP-TO-DATE   AVAILABLE   AGE
web    1/1     1            1           6s

If I add a namespace to the yaml file I get the error message you mentioned above:

apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: web
  name: web
  namespace: test # defined a namespace
spec:
  replicas: 1
  selector:
    matchLabels:
      app: web
  template:
    metadata:
      labels:
        app: web
    spec:
      containers:
      - image: nginx
        name: nginx

See:

kubectl -n default apply -f test.yaml 
error: the namespace from the provided object "test" does not match the namespace "default". You must pass '--namespace=test' to perform this operation.

TLDR: My goal would it be to mimic the behavior of kubectl e.g. let's create resources from the same manifest in different namespaces without the need for any additional boilerplate.

@micw523
Copy link
Contributor

micw523 commented Mar 20, 2019

To be honest, we’re not getting into apply -f behavior. We’re doing create only. See discussion in #504

@micw523
Copy link
Contributor

micw523 commented Mar 20, 2019

In another thought, if the yaml file didn’t set a namespace, providing an option for setting it should be fine and is consistent with the offering by the current client. I think we should be consistent with what the client does - instead of changing the namespace of the yaml file, we could pass the namespace as a parameter to subsequent calls of create_namespaced_*. What do you think?

@johscheuer
Copy link
Contributor Author

That's actually the main use case I would like to support. I will start working on this at the end of the week.

Thanks for your great feedback!

@micw523
Copy link
Contributor

micw523 commented Mar 20, 2019

/assign
You might need a rebase though, since we just merged a feature enhance PR for this function.

@johscheuer
Copy link
Contributor Author

@micw523 PTAL :)

@micw523
Copy link
Contributor

micw523 commented Mar 26, 2019

In my opinion we do not need a second function for this. Why not add namespace as an optional parameter to the original function since our typed client already does this?

Copy link
Contributor

@micw523 micw523 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 26, 2019
@johscheuer
Copy link
Contributor Author

Actually I just didn't thought on using an optional parameter. Makes everything much easier/clearer. I adjusted the code.

@micw523
Copy link
Contributor

micw523 commented Mar 26, 2019

The CI is not working due to a new release of kubernetes. I'll retest the PR after our CI is back up again.

@johscheuer
Copy link
Contributor Author

@micw523 I added some more docs to the namespace parameter

@johscheuer
Copy link
Contributor Author

ping @micw523

@micw523
Copy link
Contributor

micw523 commented May 3, 2019

Hi @johscheuer, this lgtm to me and I already sent it to Roy to review. He might be a little too busy...

@micw523
Copy link
Contributor

micw523 commented May 3, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2019
@roycaihw
Copy link
Member

This is useful if you generate your namespaces dynamically in the Python code (e.g. e2e tests).

I'd expect the namespace field in yaml blobs to be generated, but this won't hurt

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johscheuer, roycaihw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2019
@k8s-ci-robot k8s-ci-robot merged commit c86e489 into kubernetes-client:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create from yaml should support overwrite namespaces
4 participants