-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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 RBAC, healthchecks, autoscalers and update Calico to v2.5.1 #51237
Conversation
Hi @gunjan5. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with 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. I understand the commands that are listed here. |
@@ -0,0 +1,87 @@ | |||
kind: ClusterRole | |||
apiVersion: rbac.authorization.k8s.io/v1beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rbac has updated to v1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xilabao we're planning to port this with k8s 1.7.x, so keeping it v1beta1
for this PR
@@ -136,3 +151,68 @@ spec: | |||
tolerations: | |||
- key: "CriticalAddonsOnly" | |||
operator: "Exists" | |||
|
|||
# Create all the required CustomResourceDefinitions needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each CRD should be in its own yaml file in order to work with the add-on manager.
7c5ba70
to
f2ed269
Compare
/ok-to-test |
/test pull-kubernetes-e2e-gce-gpu |
/test pull-kubernetes-e2e-gce-gpu |
4bbdd66
to
96cc364
Compare
5533dfe
to
16dc434
Compare
/assign dnardo |
@@ -103,7 +115,7 @@ spec: | |||
{ | |||
"type": "portmap", | |||
"capabilities": {"portMappings": true}, | |||
"snat": false | |||
"snat": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false right - ip-masq-agent will be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's because "snat": true
only applies to local host traffic. See https://github.com/containernetworking/plugins/tree/master/plugins/meta/portmap#snat
@caseydavenport spotted that if we try to reach hostport from the same node as the hostport pod with localhost:<hostport>
it won't work without snat: true
This shouldn't affect ip-masq-agent since this is only for the localhost traffic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is just for the hostport plugin, not general masquerade (which will still be done by the ip-masq-agent).
The hostport plugin needs this setting in order to allow users to do curl localhost:hostport
successfully.
@@ -1796,20 +1748,9 @@ function start-kube-addons { | |||
if [[ "${NETWORK_POLICY_PROVIDER:-}" == "calico" ]]; then | |||
setup-addon-manifests "addons" "calico-policy-controller" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should ENABLE_IP_MASQ_AGENT=true be added where network policy provider is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are thinking of doing that in a separate PR later, since it might need more changes for example node labels, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I think I was confusing that with "ipMasq": true
/lgtm except for the comments on SNAT and ip-masq-agent |
containers: | ||
- image: calico/typha:v0.2.3 | ||
- image: calico/typha:v0.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gunjan5 this should be v0.4.1
/lgtm @gunjan5 one comment re: the typha verison |
16dc434
to
05661b6
Compare
/lgtm |
/lgtm |
/approve no-issue |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caseydavenport, dnardo, gunjan5, thockin Associated issue requirement bypassed by: caseydavenport, thockin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
@gunjan5: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue |
@caseydavenport - it seems like bug-fixes, which I think should be fine to cherrypick. But I would like to see a user-friendly release-note on this PR first. |
@wojtek-t @caseydavenport I've added the release note, someone needs to add the label |
@gunjan5 @caseydavenport - I changed the label. I tried doing automated cherrypick, but this generated conflict. Can one of you prepare the cherrypick then? |
Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
What this PR does / why we need it:
v2.5
v2.5.1
v1.10.0
v0.4.1
Release note: