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

Revert "kubeadm: Create control plane with ClusterFirstWithHostNet dns policy" #71010

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

neolit123
Copy link
Member

Reverts #68890

This is causing issues as described in the linked issue:
fixes kubernetes/kubeadm#1236

@kubernetes/sig-cluster-lifecycle-pr-reviews
/priority critical-urgent
/kind bug
/assign @timothysc @chuckha
/hold

@k8s-ci-robot
Copy link
Contributor

@neolit123: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2018
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubeadm labels Nov 13, 2018
@neolit123
Copy link
Member Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 13, 2018
@timothysc timothysc added this to the v1.13 milestone Nov 13, 2018
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

/cc @andrewrynhard - we needed to revert this!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, timothysc

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 Nov 13, 2018
@neolit123
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2018
@andrewrynhard
Copy link
Contributor

andrewrynhard commented Nov 13, 2018

@timothysc I see. Sorry for the troubles. I would still like to see DNSClusterFirstWithHostNet used. Any ideas here? I would gladly submit the PR. We could put DNSClusterFirstWithHostNet behind a flag/config option.

@mauilion
Copy link

@andrewrynhard Since oidc url has to be https anyway in tectonic we did this by standing up an ingress controller and configuring that as part of our install. Being able to reference internal svc urls for this stuff is kind of brittle.

@andrewrynhard
Copy link
Contributor

andrewrynhard commented Nov 14, 2018

@mauilion Thanks for the suggestion. I actually ran it like that for quite some time. In my current use case I am using cert-manager and some rewrite rules in CoreDNS:

rewrite stop {
    name regex dex.example.com dex.security.svc.cluster.local
    answer name dex.security.svc.cluster.local dex.example.com
}

I am able to handle OIDC using only a service. It has worked out well so far. I can get valid certs for my internal service, but this whole setup is dependent on DNSClusterFirstWithHostNet.

@mauilion
Copy link

nice! this is a neat idea. This is a hard thing to solve since the apiserver comes in so early in the process. The solution to this is to ensure that the underlying node uses some dns server with dns stub capability or to ensure that before the apiserver starts a hosts entry like

10.96.0.100 dex.example.com
is added to the /etc/hosts file on the master nodes.
You must do this before the apiserver is started cause on start the apiserver will receive a copy of the /etc/hosts file of the node.

TL;DR it's better to solve this problem without ClusterFirstWithHostNet because of the circular dependency.

Another possible and interesting solution would be to extend kubeadm to populate hostAliases on the control plane.
This would enable you to set the /etc/hosts file for the apiserver for the endpoint directly I can see how this would be useful for a number of edge cases.

as an example here is a modified /etc/kubernetes/manifests/kube-apiserver.yaml

~~ SNIP ~~
spec:
  hostAliases:
  - ip: "10.96.0.100"
    hostnames:
    - "dex.example.org"
  containers:
  - command:
    - kube-apiserver
    - --authorization-mode=Node,RBAC
    - --feature-gates=MountPropagation=true
~~ SNIP ~~

This results in a /etc/hosts on the apiserver pod that can resolv dex.example.org without resorting to ClusterFirstWithHostNet

@andrewrynhard
Copy link
Contributor

@mauilion Awesome, those suggestions look like some potentially good alternatives, but I don't have a way of know the IP of the internal dex service.

@k8s-ci-robot k8s-ci-robot merged commit 9029564 into kubernetes:master Nov 14, 2018
@mauilion
Copy link

You can predefine it. You can specify the cluster ip when defining the Dex service.
Example:

kubectl create service clusterip dex --tcp=3000 --clusterip=10.96.0.5  -l k8s-app=dex -n kube-system --dry-run -o yaml

@hansenwg
Copy link

@andrewrynhard Did you find a way to resolve this? I have a similiar scenario where I want to extend kube-apiserver by using the webhook with a service. However, DNS cannot resolve the webhook.ns and I could not fix an IP address due to potential conflicts

@andrewrynhard
Copy link
Contributor

@hansenwg I kinda gave up on the idea unfortunately because I got pulled into other things, but I still would like to run it this way.

@hansenwg
Copy link

@andrewrynhard Thanks for the reply, I will look for some other workarounds then, thanks anyways!

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/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubeadm: Control Plane with "ClusterFirstWithHostNet" is a circular dependency.
7 participants