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

Run pods needing control-plane instance credentials on hostNetwork #14913

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

johngmyers
Copy link
Member

Allows setting IMDS hop limit to 1 on control-plane nodes.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/addons labels Dec 30, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 30, 2022
@johngmyers
Copy link
Member Author

many-addons broken due to cluster-autoscaler

@hakman
Copy link
Member

hakman commented Dec 31, 2022

/lgtm
/assign @olemarkus

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 31, 2022
@johngmyers johngmyers force-pushed the hostnetwork-no-irsa branch from a7b30fb to e1716d2 Compare January 3, 2023 17:08
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2023
@rifelpet
Copy link
Member

rifelpet commented Jan 3, 2023

I believe this has a side effect of preventing CiliumNetworkPolicies from restricting the network access for these pods

@johngmyers johngmyers force-pushed the hostnetwork-no-irsa branch 4 times, most recently from 0cec781 to 3a00712 Compare January 3, 2023 19:23
@johngmyers
Copy link
Member Author

@olemarkus What is with the upgrade-ab test? It's failing because the PDB is not satisfied 5 seconds after the new manifest was applied. It takes time for a Deployment to roll out.

We probably need a loop around the ${CHANNELS} apply channel command, waiting for it to reconcile.

@johngmyers
Copy link
Member Author

I don't see a problem with CiliumNetworkPolicies not applying to these pods. They are privileged, after all. If they care that much about restricting their access, they can enable IRSA.

@johngmyers johngmyers force-pushed the hostnetwork-no-irsa branch from 3a00712 to 78fbaa1 Compare January 5, 2023 07:15
@johngmyers
Copy link
Member Author

upgrade-ab had been failing due to port conflicts.

@johngmyers johngmyers force-pushed the hostnetwork-no-irsa branch 2 times, most recently from f06f787 to ceb0cb3 Compare January 5, 2023 14:06
@johngmyers
Copy link
Member Author

/retest

@johngmyers johngmyers force-pushed the hostnetwork-no-irsa branch from ceb0cb3 to f035d75 Compare January 7, 2023 01:51
@johngmyers
Copy link
Member Author

/test pull-kops-e2e-aws-upgrade-126-ko126-to-klatest-kolatest-many-addons

Comment on lines 88 to 89
// CloudCSIDriverPort is reserved for the cloud provider's primary CSI driver.
CloudCSIDriverPort = 9808
Copy link
Member

Choose a reason for hiding this comment

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

This is only needed for AWS, no need to complicate other providers with it.

Suggested change
// CloudCSIDriverPort is reserved for the cloud provider's primary CSI driver.
CloudCSIDriverPort = 9808
// AWSCSIDriverPort is reserved for the cloud provider's primary CSI driver.
AWSCSIDriverPort = 9808

Copy link
Member Author

Choose a reason for hiding this comment

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

The OpenStack cinder daemonset runs on host network and uses a port.

The GCP PD controller also runs on host network, though it uses three ports.

@justinsb should we not try to share port assignments across the primary CSI drivers for cloud providers?

Copy link
Member

Choose a reason for hiding this comment

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

We should diverge from cloud provider default as little as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should be more proactive in avoiding port conflicts. We don't have the coverage to find them all through testing.

Copy link
Member

Choose a reason for hiding this comment

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

We probably should have a map of the ports in wellknownports.go and use that map in manifests that are using host networking. That would make it easier to discover any conflicts.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023
@johngmyers
Copy link
Member Author

Making this PR minimal and leaving the avoidance of other port conflicts to future work.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 Jan 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit f288311 into kubernetes:master Jan 11, 2023
@johngmyers johngmyers deleted the hostnetwork-no-irsa branch January 11, 2023 06:27
k8s-ci-robot added a commit that referenced this pull request Jan 11, 2023
…4913-upstream-release-1.26

Automated cherry pick of #14913: Run pods needing control-plane instance credentials on
Shimiazoulai pushed a commit to spotinst/kubernetes-kops that referenced this pull request Jul 13, 2023
…pick-of-#14913-upstream-release-1.26

Automated cherry pick of kubernetes#14913: Run pods needing control-plane instance credentials on
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. area/addons 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants