search

ansible-collections / kubernetes.core

The collection includes a variety of Ansible content to help automate the management of applications in Kubernetes and OpenShift clusters, as well as the provisioning and maintenance of clusters themselves.
Other
215 stars 134 forks source link

k8s task treats null values differently if creating vs updating resources #161

Open jmazzitelli opened 3 years ago

jmazzitelli commented 3 years ago
SUMMARY

A field (such as a metadata label, or service selector) may or may not exist on a resource - I want to make sure that field does NOT exist (i.e. the field should be removed if it does exist; if it does not exist, do nothing).

A null value should tell k8s to remove the field if it exists but do nothing if it does not exist. If a resource itself does not exist, you would expect the behavior to be the same as if the field does not already exist (hence, do nothing).

However, if k8s is creating the resource, a null value is translated to an empty string ("") and the field is erroneously created. This does not happen if the resource exists - k8s behaves correctly in this case.

ISSUE TYPE
COMPONENT NAME

k8s

ANSIBLE VERSION
$ ansible --version
ansible 2.9.19
  config file = None
  configured module search path = ['/home/jmazzite/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/jmazzite/source/ansible/lib/ansible
  executable location = /home/jmazzite/source/ansible/bin/ansible
  python version = 3.6.8 (default, Mar 18 2021, 08:58:41) [GCC 8.4.1 20200928 (Red Hat 8.4.1-1)]
COLLECTION VERSION
$ ansible-galaxy collection list community.kubernetes
usage: ansible-galaxy collection [-h] COLLECTION_ACTION ...
ansible-galaxy collection: error: argument COLLECTION_ACTION: invalid choice: 'list' (choose from 'init', 'build', 'publish', 'install')

$ cat ~/.ansible/collections/ansible_collections/community/kubernetes/MANIFEST.json | grep version
  "version": "2.0.0",
CONFIGURATION
$ ansible-config dump --only-changed
(empty results)
OS / ENVIRONMENT

RHEL 8.4

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.0", GitCommit:"cb303e613a121a29364f75cc67d3d580833a7479", GitTreeState:"clean", BuildDate:"2021-04-08T16:31:21Z", GoVersion:"go1.16.1", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.2", GitCommit:"faecb196815e248d3ecfb03c680a4507229c2a56", GitTreeState:"clean", BuildDate:"2021-01-13T13:20:00Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}

$ minikube version
minikube version: v1.20.0
commit: c61663e942ec43b20e8e70839dcca52e44cd85ae
STEPS TO REPRODUCE

ansible-test.yaml:

- hosts: localhost
  gather_facts: no
  collections:
  - community.kubernetes
  tasks:
  - k8s:
      state: present
      definition:
        apiVersion: v1
        kind: Namespace
        metadata:
          name: testns
          labels:
            one: "value1-A"
            two: "value2"
            three: null # namespace does not exist; label three does not exist - k8s incorrectly adds label three with value empty string. Run this playbook again when namespace DOES exist and k8s correctly removes label three
  - k8s:
      state: present
      definition:
        apiVersion: v1
        kind: Namespace
        metadata:
          name: testns
          labels:
            one: "value1-B"
            two: null # namespace exists; label two exists - k8s correctly removes label two
            four: null # namespace exists; label four does not exist - k8s correctly does nothing here
EXPECTED RESULTS

When a field is set to null in the k8s task config, that means that field should not exist on the resource (this is true regardless if the resource exists and is being updated, or if the resource does not already exist and k8s will create it).

For any field whose value is specified as null in the k8s task config, that field should be removed from an existing resource. If the field does NOT exist on the resource (or the resource itself does not exist), that field should not be created.

ACTUAL RESULTS

This works UNLESS k8s creates the resource. If k8s has state: present and the resource does not exist, it will create that resource and any field set to null will be created and its value will be set to empty string (""). If, however, the resource already does exist (i.e. k8s will not create the resource but will instead update/patch it) then it behaves as expected - if the field exists, it is removed; if the field does not exist, k8s does nothing.

So, in short, k8s behaves differently if the resource needs to be created versus updated. If a field is set to null and k8s creates the resource, it will create the field with empty string (""). This is unexpected.

# show that we do not have the namespace yet
$ kubectl get ns testns -o jsonpath={.metadata.labels} | jq
Error from server (NotFound): namespaces "testns" not found

# run the ansible playbook - this creates the namespace, then updates it
$ ansible-playbook ansible-test.yaml 
...normal output...

# look at the labels - why is there a label `three` with an empty string (`""`) value here?
$ kubectl get ns testns -o jsonpath={.metadata.labels} | jq
{
  "one": "value1-B",
  "three": ""
}

# run the ansible playbook again - this updates the namespace two times
$ ansible-playbook ansible-test.yaml 
...normal output...

# look at the labels again - notice the "three" label is removed now
$ kubectl get ns testns -o jsonpath={.metadata.labels} | jq
{
  "one": "value1-B"
}
jmazzitelli commented 3 years ago

Here's a description of the real-world problem that I am having that is caused by this k8s issue: https://github.com/kiali/kiali-operator/pull/348#issuecomment-873191750

abikouo commented 3 years ago

@jmazzitelli thanks for taking the time to report and fully described the issue, I will work on this.

abikouo commented 3 years ago

@jmazzitelli I just check how this will work using kubectl and it seems working as ansible does

Namespace yaml definition

apiVersion: v1
kind: Namespace
metadata:
  name: kubectl-ns
  labels:
    one: "value1-A"
    two: "value2"
    three: null

create namespace using kubectl create -f ./namespace.yaml --validate=false and describe it 

apiVersion: v1
kind: Namespace
metadata:
  creationTimestamp: "2021-07-06T16:59:45Z"
  labels:
    kubernetes.io/metadata.name: kubectl-ns
    one: value1-A
    three: ""
    two: value2
  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
(...)

Then when running apply command with the same definition the namespace is updated as on ansible

apiVersion: v1
kind: Namespace
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Namespace","metadata":{"annotations":{},"labels":{"one":"value1-A","three":null,"two":"value2"},"name":"kubectl-ns"}}
  creationTimestamp: "2021-07-06T16:59:45Z"
  labels:
    kubernetes.io/metadata.name: kubectl-ns
    one: value1-A
    two: value2
  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
(...)
jmazzitelli commented 3 years ago

Regardless of the behavior of kubectl, I would think a simple solution would be for the k8s task to remove any fields with a value of "null" if it knows it is going to create a resource (I assume k8s knows if it is going to create a new resource as opposed to updating an existing one??). In this way, ansible will work better than kubectl :)

gravesm commented 3 years ago

Just to shed some more light on this. The null (or None by the time the yaml gets loaded into python) value is being passed to the K8S api, but converting that to an empty string is just how the api works as best I can tell. Fixing this is a bit complicated because null values are specifically allowed in CRDs. If we just removed null fields we'd break CRDs. I think CRDs are the only resource that allows nullable fields, but I'm honestly not sure. I see a few possible solutions, but feel free to suggest others:

  1. We remove null fields on everything except CRDs. This assumes we can determine if a resources is a CRD. Can't think of how to do this off the top of my head, but it's likely possible. We'd need to confirm first that only CRDs allow nullable fields.
  2. We do nothing and recommend that if this is an issue, you should use the validate param.
  3. We can discuss the possibility of making validation the default behavior. This is what kubectl does.
jmazzitelli commented 3 years ago

BTW: to me this is a result of having k8s support this state field with "present". Granted, the nice thing with that feature is that the ansible developer is not required to know (well, is not "supposed to be" required to know) if the resource exists or not -- k8s is going to create a new object or just patch an existing one. The burden is lifted off the developer. But, the dev needs to prepare the yaml so it can support either create or update. But in this case, I can't find out how to do that. There is no way for me to give a yaml to k8s that supports both creating a new resource AND updating an existing one because of the different treatment of the "null" fields. I fear I am almost forced to do something like this in my ansible template:

{% if ...lookup to see if resource exists... %}
some yaml that supports updates
{% else %}
some other yaml that supports creates
{% endif %}

ewwww... if someone can propose another workaround that is less hacky, please let me know.

FWIW: here's what I am going to have to do in my ansible template in order to support upgrades in my ansible operator where a previous version of the operand's service had one set of selector fields but a new version of the operand service has a new set of selector fields -- the old ones, if they exist, need to be removed. See the {% if... endif %}, from https://github.com/kiali/kiali-operator/pull/348/files

apiVersion: v1
kind: Service
metadata:
  name: {{ kiali_vars.deployment.instance_name }}
  namespace: {{ kiali_vars.deployment.namespace }}
...
spec:
...
  selector:
{% if query('k8s', kind='Service', resource_name=kiali_vars.deployment.instance_name, namespace=kiali_vars.deployment.namespace) | length > 0 %}
    app: null
    version: null
{% endif %}
    app.kubernetes.io/name: kiali
    app.kubernetes.io/instance: {{ kiali_vars.deployment.instance_name }}
bmaupin commented 7 months ago

I believe I'm running into the same problem. With this task:

- kubernetes.core.k8s:
    state: present
    definition:
      kind: ResourceQuota
      apiVersion: v1
      metadata:
        name: compute-resources
        namespace: deleteme
      spec:
        hard:
          limits.cpu: null

  1. Before running it, no quotas are set

    $ kubectl describe quota
No resources found in deleteme namespace.

  2. After running it the first time, the property is created with a value of 0:

    $ kubectl describe quota
Name:       compute-resources
Namespace:  deleteme
Resource    Used  Hard
--------    ----  ----
limits.cpu  0     0

  3. After running it again, the value is removed:

    $ kubectl describe quota
Name:       compute-resources
Namespace:  deleteme
Resource    Used  Hard
--------    ----  ----

So it's not idempotent.

... and as @abikouo mentioned, this is the same behaviour as kubectl:

$ kubectl describe quota
No resources found in deleteme namespace.

$ kubectl apply -f - <<EOF
kind: ResourceQuota
apiVersion: v1
metadata:
  name: compute-resources
  namespace: deleteme
spec:
  hard:
    limits.cpu: null
EOF
resourcequota/compute-resources created

$ kubectl describe quota
Name:       compute-resources
Namespace:  deleteme
Resource    Used  Hard
--------    ----  ----
limits.cpu  0     0

$ kubectl apply -f - <<EOF
kind: ResourceQuota
apiVersion: v1
metadata:
  name: compute-resources
  namespace: deleteme
spec:
  hard:
    limits.cpu: null
EOF
resourcequota/compute-resources configured

$ kubectl describe quota
Name:       compute-resources
Namespace:  deleteme
Resource    Used  Hard
--------    ----  ----

So I guess it's a k8s API bug?

In my case since I'm setting limits.cpu directly in an Ansible task, I had to replace my task with something like this:

- kubernetes.core.k8s:
    state: present
    definition:
      kind: ResourceQuota
      apiVersion: v1
      metadata:
        name: compute-resources
        namespace: "{{ k8s_namespace }}"
      spec:
        hard: "{{ spec_hard }}"
  vars:
    _spec_hard:
      # Other quotas set as needed here
    spec_hard: "{{ _spec_hard | combine({'limits.cpu': None if (query('k8s', kind='ResourceQuota', resource_name='compute-resources', namespace=k8s_namespace ) | length > 0) else omit}) }}"