Move cluster-cidr proxy argument to the node config#9863
Conversation
openshift-ci-robot
added
the
size/XS
openshift-ci-robot
added
the
needs-ok-to-test
|
Can one of the admins verify this patch?
|
| bindAddress: 0.0.0.0:10250 | ||
| bindNetwork: tcp4 | ||
| clientCA: client-ca.crt | ||
| {% if openshift_use_calico | default(False) | bool %} |
There was a problem hiding this comment.
While OpenShift SDN might not need it for this particular case, there are a handful of edge cases that kube-proxy uses the cluster-cidr argument for, so it's probably best to set it regardless of network plugin.
There was a problem hiding this comment.
That's fair. I originally kept it specific to Calico since I wasn't sure what the effect would be with other network plugins. I can make those changes.
mgleung
force-pushed
the
calico-pass-cluster-cidr-to-proxy
branch
2 times, most recently
from
8360914 to
eddf564
Compare
|
@danwinship Can I get you to take another look and see if these changes make sense? |
|
/ok-to-test |
openshift-ci-robot
removed
the
needs-ok-to-test
| bindAddress: 0.0.0.0:10250 | ||
| bindNetwork: tcp4 | ||
| clientCA: client-ca.crt | ||
| {% if osm_cluster_network_cidr is defined and not openshift_use_openshift_sdn | default(False) | bool %} |
There was a problem hiding this comment.
er, no, I meant to say before, we should always set the variable. Just drop the conditional check entirely
There was a problem hiding this comment.
The variable we should use is openshift_cluster_network_cidr
There was a problem hiding this comment.
Sorry, misunderstood.
| bindAddress: 0.0.0.0:10250 | ||
| bindNetwork: tcp4 | ||
| clientCA: client-ca.crt | ||
| {% if osm_cluster_network_cidr is defined and not openshift_use_openshift_sdn | default(False) | bool %} |
There was a problem hiding this comment.
The variable we should use is openshift_cluster_network_cidr
mgleung
force-pushed
the
calico-pass-cluster-cidr-to-proxy
branch
from
eddf564 to
aa85376
Compare
|
@danwinship @michaelgugino Can I get you to take another look at these changes? |
michaelgugino
left a comment
michaelgugino
left a comment
There was a problem hiding this comment.
This looks fine to me.
|
@danwinship ptal |
|
lgtm |
|
/lgtm |
|
/cherrypick release-3.11 |
|
@michaelgugino: once the present PR merges, I will cherry-pick it on top of release-3.11 in a new PR and assign it to you. DetailsIn response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mgleung, michaelgugino The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/cherrypick release-3.10 |
|
@michaelgugino: once the present PR merges, I will cherry-pick it on top of release-3.10 in a new PR and assign it to you. DetailsIn response to this:
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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@michaelgugino: new pull request created: #10197 DetailsIn response to this:
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. |
|
@michaelgugino: new pull request created: #10198 DetailsIn response to this:
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. |
9 participants
As per #8099, "connections to nodeports only succeed if the pod backing the nodeport service is running on the host being hit. So requests are not forwarded to other nodes if the pod is running there.
This PR resolves the issue by passing Cluster CIDR to kube-proxy, which implements the nececssary iptables rules."
This PR replicates those changes in the new location of the node config template.