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

Explicitly set `kubelet-preferred-address-types` flag for apiserver #2777

Merged
merged 3 commits into from Feb 8, 2018

Conversation

Projects
None yet
9 participants
@justaugustus
Copy link
Contributor

commented Jan 19, 2018

Explicitly sets --kubelet-preferred-address-types=InternalDNS,InternalIP,Hostname,ExternalDNS,ExternalIP on the apiserver.

This allows API --> kubelet communication to succeed in instances where a DNS search domain is not present in /etc/resolv.conf

Fixes: https://jira.coreos.com/browse/TECREL-150

@coreosbot

This comment has been minimized.

Copy link

commented Jan 19, 2018

Can one of the admins verify this patch?

@justaugustus justaugustus force-pushed the justaugustus:kubelet-preferred-address branch from 5acb469 to df328d7 Jan 21, 2018

@justaugustus justaugustus changed the title bootkube: Add `kubelet-preferred-address-types` flag to apiserver Explicitly set `kubelet-preferred-address-types` flag for apiserver Jan 21, 2018

@diegs

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

cc @thorfour in case this ends up being cherry-picked into lithium and requires a migration in the KVO.

@coresolve

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

I think the order should be:
InternalIP,Hostname,InternalDNS,ExternalDNS,ExternalIP
cause in the AWS case the InternalDNS is only resolvable by AWS domain name servers. One of the assumptions that lead to this PR is that the cluster admin has change the resolvers and presumably the searchpath.

If we use InternalIP then that also saves us a dns call.

One interesting note. The kubelet generates a cert and places this cert in /var/lib/kubelet/pki It's a self signed cert and it has a SAN matching the hostname.
In Azure this cert looks like:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 1 (0x1)
    Signature Algorithm: sha256WithRSAEncryption
        Issuer: CN=kube-lego-master-0@1516654753
        Validity
            Not Before: Jan 22 20:59:13 2018 GMT
            Not After : Jan 22 20:59:13 2019 GMT
        Subject: CN=kube-lego-master-0@1516654753
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
                Modulus:
                      Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment, Certificate Sign
            X509v3 Extended Key Usage:
                TLS Web Server Authentication
            X509v3 Basic Constraints: critical
                CA:TRUE
            X509v3 Subject Alternative Name:
                DNS:kube-lego-master-0
    Signature Algorithm: sha256WithRSAEncryption

So I wanted to ensure that we can connect to it without a problem. Turns out the apiserver doesn't validate the cert when connecting to the node for calls like exec logs attach cp

Looks like InternalIP may be the winner here. Until we get to bootstrap tls and then we need to ensure that the SAN for the kubelet includes the InternalIP.

Here are the command I used to test this out:

# straight curl no auth:  
$ curl  https://10.0.16.4:10250/healthz  -k -w " %{http_code}\n"
Unauthorized 401

# curl with apiserver crt as auth using hostname.
$ curl --cert /etc/kubernetes/bootstrap-secrets/secrets/kube-apiserver/apiserver.crt --key /etc/kubernetes/bootstrap-secrets/secrets/kube-apiserver/apiserver.key https://kube-lego-worker-0:10250/healthz  -k -w " %{http_code}\n"
ok 200

# curl with apiserver.crt as auth using IP.
$ curl --cert /etc/kubernetes/bootstrap-secrets/secrets/kube-apiserver/apiserver.crt --key /etc/kubernetes/bootstrap-secrets/secrets/kube-apiserver/apiserver.key https://10.0.16.4:10250/healthz  -k -w " %{http_code}\n"
ok 200
@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2018

@sym3tri / @alexsomesan / @squat --
Can we get eyes on this tomorrow? @coresolve and I have done some validation, but we'd like to be sure this is a reasonable change for all platforms.

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2018

To Duffie's point (#2777 (comment)), if we use InternalIP,Hostname,InternalDNS,ExternalDNS,ExternalIP, theoretically, we should be able to ALWAYS return InternalIP, right?

In that case, why not just have the stringSlice as --kubelet-preferred-address-types=InternalIP?

If InternalIP is broken, we're in a bad state anyway, right?

cc: @ericchiang / @diegs / @rphillips

@justaugustus justaugustus force-pushed the justaugustus:kubelet-preferred-address branch from df328d7 to e0407d1 Jan 24, 2018

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

Setting --kubelet-preferred-address-types=InternalIP and updated the discussion in https://jira.coreos.com/browse/TECREL-150

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@sym3tri / @mxinden / @cpanato --
Can we have someone apply all of the platform labels and run smoke tests, so that we can get some initial signal on this?

@coreosbot

This comment has been minimized.

Copy link

commented Jan 25, 2018

Can one of the admins verify this patch?

@coreosbot

This comment has been minimized.

Copy link

commented Jan 25, 2018

Can one of the admins verify this patch?

@diegs

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

ok to test

@diegs

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

retest this please

@@ -51,6 +51,7 @@ spec:
- --tls-private-key-file=/etc/kubernetes/secrets/apiserver.key
- --kubelet-client-certificate=/etc/kubernetes/secrets/apiserver.crt
- --kubelet-client-key=/etc/kubernetes/secrets/apiserver.key
- --kubelet-preferred-address-types=InternalIP

This comment has been minimized.

This comment has been minimized.

Copy link
@diegs

diegs Jan 26, 2018

Contributor

@justaugustus I would feel slightly better copying what kubeadm does...

This comment has been minimized.

Copy link
@justaugustus

justaugustus Jan 26, 2018

Author Contributor

@diegs I'm down for that too.
Honestly, any method that prefers InternalIP over Hostname should address the issue.

I'll push that change. Thanks for the feedback!!

This comment has been minimized.

Copy link
@justaugustus

justaugustus Jan 26, 2018

Author Contributor

Updated!

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

retest this please

@cpanato

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

retest this please. metal only

@justaugustus justaugustus force-pushed the justaugustus:kubelet-preferred-address branch from e0407d1 to 5c31a02 Jan 26, 2018

@cpanato cpanato removed the platform/aws label Jan 27, 2018

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2018

retest this please. azure only

@justaugustus justaugustus changed the base branch from track-1 to 1.8.7 Jan 29, 2018

@justaugustus justaugustus force-pushed the justaugustus:kubelet-preferred-address branch from 8d0238c to e101377 Jan 29, 2018

@sudhaponnaganti

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

@justaugustus @robszumski -a temp branch has been created for you to submit this PR 1.8.7-cust. We will make a build once your PR gets merged.
/cc @coverprice

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

@sudhaponnaganti / @robszumski -- here's the PR to merge to 1.8.7-cust: #2849

@sudhaponnaganti

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

@justaugustus - can you merge it - I do not see any option to merge it by myself. Also custom_tls check is failing but you can delete it from checks as this is only temp branch. This suite has been passed on 1.8.7 branch.

@cpanato

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@justaugustus do we need to merge this pr? if yes can you rebase? otherwise please close

@diegs

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@cpanato I believe this is on hold until further testing is done in the custom branch (#2849). Adding a do not merge label.

@diegs diegs added the do-not-merge label Feb 2, 2018

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2018

@cpanato Yup, what Diego said. Testing in progress on the custom branch.
Thanks @diegs for updating labels on this!!

@justaugustus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

I took a look at using --kubelet-preferred-address-types=InternalIP,ExternalIP,Hostname on the following platforms:

  • AWS
  • AWS GovCloud
  • Azure

Confirming that QA checks out on each platform.

@cpanato

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

retest this please

@cpanato

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

retest this please. aws/govcloud/metal

@squat

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

@diegs @thorfour just clarifying one of Diego’s comment and towards the top of this issue; 1.8.7 is Lithium so we will need a migration once this merges even if there is no cherry-pick

@diegs

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

Yes, @thorfour will have to make a migration for existing clusters if this goes in with Lithium. Thanks @cpanato !

@alexsomesan
Copy link
Contributor

left a comment

I'm fine with the technical aspects of this change.
The migration aspects need to be managed together with core services.

@sym3tri sym3tri merged commit 07c3741 into coreos:1.8.7 Feb 8, 2018

17 checks passed

Jenkins-Tectonic-Installer Build finished.
Details
basic-tests
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
smoke-tests/aws/basic
Details
smoke-tests/aws/ca
Details
smoke-tests/aws/custom_tls
Details
smoke-tests/aws/exp
Details
smoke-tests/aws/network_canal
Details
smoke-tests/aws/vpc_internal
Details
smoke-tests/azure/basic
Details
smoke-tests/azure/custom_tls
Details
smoke-tests/azure/example
Details
smoke-tests/azure/external
Details
smoke-tests/azure/private_external
Details
smoke-tests/govcloud/vpc_internal
Details
smoke-tests/metal/basic
Details
smoke-tests/metal/custom_tls
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.