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

kubelet anonymousAuth not being passed to kubelet #256

Closed
rmgpinto opened this issue Jul 2, 2021 · 20 comments
Closed

kubelet anonymousAuth not being passed to kubelet #256

rmgpinto opened this issue Jul 2, 2021 · 20 comments

Comments

@rmgpinto
Copy link
Contributor

rmgpinto commented Jul 2, 2021

kubelet and kube-api-server are not receiving the anonymous_auth parameter from terraform.

config:

kubelet {
  anonymous_auth = false
}
kube_api_server {
  anonymous_auth = false
}

kubelet cmdline on node

/usr/local/bin/kubelet --authentication-token-webhook=true
--authorization-mode=Webhook--cgroup-driver=systemd
--cgroup-root=/
--client-ca-file=/srv/kubernetes/ca.crt
--cloud-provider=aws
--cluster-dns=10.0.0.10
--cluster-domain=cluster.local
--enable-debugging-handlers=true
--evictionhard=memory.available<100Mi,nodefs.available<10%,nodefs.inodesFree<5%,imagefs.available<10%,imagefs.inodesFree<5%
--hostname-override=ip-10-0-157-188.eu-west-1.compute.internal
--kubeconfig=/var/lib/kubelet/kubeconfig
--max-pods=17
--network-plugin=cni
--non-masquerade-cidr=10.0.0.0/16
--pod-manifest-path=/etc/kubernetes/manifests
--register-schedulable=true
--register-with-taints=node-role.kubernetes.io/master=:NoSchedule
--resolv-conf=/run/systemd/resolve/resolv.conf
--v=2
--volume-plugin-dir=/usr/libexec/kubernetes/kubelet-plugins/volume/exec/
--cloud-config=/etc/kubernetes/cloud.config
--node-ip=10.0.157.188
--container-runtime=remote
--runtime-request-timeout=15m
--container-runtime-endpoint=unix:///run/containerd/containerd.sock
--tls-cert-file=/srv/kubernetes/kubelet-server.crt
--tls-private-key-file=/srv/kubernetes/kubelet-server.key

There should be an --anonymous-auth=false

Can you check this please? Thanks

edit: just check now, and if I pass yes then the flag --anonymous-auth=true is added to kubelet and to kube-api-server. Maybe something wrong with the code processing the false values?

@eddycharly
Copy link
Owner

Hello @rmgpinto 👋
Thanks for reporting. I'll look at it but i'm pretty sure i'm not catching the difference between null and false.
It's a common problem when translation from hcl config to go struct.

@eddycharly
Copy link
Owner

eddycharly commented Jul 2, 2021

See the expansion code here

if reflect.DeepEqual(in, reflect.Zero(reflect.TypeOf(in)).Interface()) {
return nil
}

One hack to fix this to make the parameter mandatory but it's not silver bullet as null gets unsupported.

@eddycharly
Copy link
Owner

I think a solution would be to make the argument a struct, something like this:

  kubelet{
    anonymous_auth {
      value = true
    }
  }

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 3, 2021 via email

@eddycharly
Copy link
Owner

There are two possible solutions, the one above or:

  kubelet{
    anonymous_auth = false
    anonymous_auth_specified = true
  }

I'm not a big fan of adding a _specified argument for every prop that is a pointer in kOps.

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 3, 2021 via email

@eddycharly
Copy link
Owner

I agree, I'm making this change, i had this in mind for some time.

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 3, 2021 via email

@eddycharly
Copy link
Owner

WIP here #258
It's gonna be a huge change though.

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 5, 2021

I think a solution would be to make the argument a struct, something like this:

  kubelet{
    anonymous_auth {
      value = true
    }
  }

Hi, just my two cents regarding the PR and the code above.

I thought only kubelet -> anonymous_auth was going to be this way, but I saw the PR and every value will change. IMO this is not a very terraform way of specifying values, and it's a significant drift from standards.

I'd simplify things and set a sensible default for anonymous_auth and other boolean values, if the value is not specified.

Just my opinion of course.

@eddycharly
Copy link
Owner

I thought only kubelet -> anonymous_auth was going to be this way, but I saw the PR and every value will change.

The issue happens with every prop that is a pointer in kOps code, not only kubelet anonymous_auth unfortunately.

For some props, setting a sensible default value will do the trick but in some cases where null, false and true can have a different meaning.

Going through all props and finding a sensible value is gonna be a pain and I feel like a lot of bugs are introduced because of this.

IMO this is not a very terraform way of specifying values, and it's a significant drift from standards.

I agree but i find the syntax not that hard to understand and it would fix the issue once for all (leaving complete freedom to the end user).

This is a bit of a pain that terraform has no solution for this (see hashicorp/terraform-plugin-sdk#261).

What do you think ? What are the drawbacks you see in applying it to all pointer props ?

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 5, 2021

does this: https://stackoverflow.com/questions/43351216/check-if-boolean-value-is-set-in-go help?

I understand the issue with nil vs false but on the PR there are changes like:

member {
      name = "master-0"
      instance_group {
        value = "master-0"
      }
    }

Which are odd to me... Why do string values need to be changed as well?
Trying to push for a better solution here :)

@eddycharly
Copy link
Owner

eddycharly commented Jul 5, 2021

I wonder why it is a pointer in kOps in the first place (https://github.com/kubernetes/kops/blob/559b57ea4c85931987da2bb1de85f7d9cd81f880/pkg/apis/kops/cluster.go#L586)

TBH it's probably safe to interpret "" as null here but in some cases interpreting zero as null is not correct (see https://github.com/kubernetes/kops/blob/559b57ea4c85931987da2bb1de85f7d9cd81f880/pkg/apis/kops/instancegroup.go#L224 for example), in this case if we interpret 0 as null it will be equivalent to 100% instead of 0% :(

I feel not comfortable making choices in the back of kOps, I feel a lot more comfortable with the approach if kOps expects a pointer let's give it a pointer this way i can almost be sure that i don't introduce unexpected behavior as kOps configuration can sometimes be tricky.

@eddycharly
Copy link
Owner

But i agree it could be nuanced for some props.

@eddycharly
Copy link
Owner

eddycharly commented Jul 5, 2021

@rmgpinto second try #259

Maybe safer 🤷

Only applied on kubelet.anonymous_auth and kube_api_server.anonymous_auth for now.

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 5, 2021

Amazing @eddycharly, looking forward to a new release!

@eddycharly
Copy link
Owner

Ok, let's merge the pr and see how things go 🤞

@eddycharly
Copy link
Owner

eddycharly commented Jul 5, 2021

v1.20.0-alpha.6 should be available in a couple of minutes.

Thanks for your great feedback, let me know if it fixes your issue.

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 5, 2021 via email

@rmgpinto
Copy link
Contributor Author

rmgpinto commented Jul 5, 2021

Just tested this, looks good to me. Thanks for the quick response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants