calico_pool_blocksize must be cast as well in assertion when defined #8321
Conversation
k8s-ci-robot
added
kind/bug
|
Hi @emiran-orange. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
| @@ -53,7 +53,7 @@ | |||
| - name: "Check if inventory match current cluster configuration" | |||
| assert: | |||
| that: | |||
| - calico_pool_conf.spec.blockSize|string == (calico_pool_blocksize | default(kube_network_node_prefix | string)) | |||
| - calico_pool_conf.spec.blockSize|string == (calico_pool_blocksize | default(kube_network_node_prefix) | string) | |||
There was a problem hiding this comment.
Shouldn't these be compared as int instead of string ?
There was a problem hiding this comment.
Well, #8120 changed the comparison format from int to string.
To answer your question, if so, the referenced PR should be reverted and mine changed so that calico_pool_conf.spec.blockSize is cast as int as both kube_network_node_prefix and calico_pool_blocksize are int
There was a problem hiding this comment.
To answer your question, if so, the referenced PR should be reverted and mine changed so that..
Actually, I take back what I posted in my last comment, if so, the referenced PR should be reverted and mine deleted.
I took a look at #8119. I think the issue is a false positive as ansible key/val parser there does not do typing and passing -e kube_network_node_prefix=26 will make kube_network_node_prefix be a string whereas it is expected t be an int (in inventory)
There was a problem hiding this comment.
@cristicalin What do you think ?
There was a problem hiding this comment.
You can always do the right thing ™️ in this PR and fix both the order and they type cast.
I have to admit that #8120 dropped off my radar after the premature merge.
PS: everything you pass to ansible via -e var=value ends up as a string, to pass different types like int or bool you have to use the json syntax like -e '{bool_var: True, my_int_var: 23}'.
There was a problem hiding this comment.
@cristicalin
My point wasn't not to pass the parameter but to pass it the right way:
-e '{"kube_network_node_prefix": 26}' instead of -e kube_network_node_prefix=26
There was a problem hiding this comment.
There are valid situations where you cannot control the type ansible assigns to this variable, especially if you embed kubespray as part of a larger project and call cluster.yml with include_playbook and your variables are calculated or extracted from some other data source.
It is a good practice to ensure types a properly set when variables are compared to avoid issues propagating silently. As was the case with the original PR #8120 this should be fixed by setting the proper types when comparing the variables since at least kube_network_node_prefix is user supplied input and all user input should be sanitized.
There was a problem hiding this comment.
Fine by me. So my PR stands, it defaults calico_pool_blocksize to kube_network_node_prefix and then applies type cast.
LGTY?
There was a problem hiding this comment.
As said above, I think the typecast should be to int instead of string but the change in logic is correct.
There was a problem hiding this comment.
Sorry I misunderstood your point enforcing int type. That should fixed now
k8s-ci-robot
added
ok-to-test
emiran-orange
changed the title
k8s-ci-robot
added
the
approved
| @@ -53,7 +53,7 @@ | |||
| - name: "Check if inventory match current cluster configuration" | |||
| assert: | |||
| that: | |||
| - calico_pool_conf.spec.blockSize|string == (calico_pool_blocksize | default(kube_network_node_prefix | string)) | |||
| - calico_pool_conf.spec.blockSize|int == (calico_pool_blocksize | default(kube_network_node_prefix) | int) | |||
There was a problem hiding this comment.
calico_pool_conf.spec.blockSize is an integer as the following, the change itself seems good for me.
# calicoctl.sh get ipPool default-pool -o json
{
...
},
"spec": {
"cidr": "10.233.64.0/18",
"vxlanMode": "Always",
"ipipMode": "Never",
"natOutgoing": true,
"blockSize": 24,
"nodeSelector": "all()"
}
}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: emiran-orange, floryut 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 |
…ubernetes-sigs#8321) * calico_pool_blocksize must be cast as string in assertion when defined * Cast as int rather than string
…ubernetes-sigs#8321) * calico_pool_blocksize must be cast as string in assertion when defined * Cast as int rather than string
What type of PR is this?
/kind bug
What this PR does / why we need it:
When calico default pool already exists and
calico_pool_blocksizeis defined in inventory, the assertion on blocksize equality wrongly fails because a string cast is missingWhich issue(s) this PR fixes:
None that I know of
Special notes for your reviewer:
N/A
Does this PR introduce a user-facing change?:
None