ericchiang / k8s

A simple Kubernetes Go client
Apache License 2.0
599 stars 108 forks source link

No default namespace for resource #87

Closed gsf closed 6 years ago

gsf commented 6 years ago

When I attempt to create a Job without Namespace: k8s.String("default") specified in the Metadata I get a no resource namespace provided error. Is this the expected behavior?

ericchiang commented 6 years ago

No. Can you copy your full Job constructor? This has tests for other resources:

https://github.com/ericchiang/k8s/blob/677cf3318ef83bf681a38821f81a233a9be09641/client_test.go#L170

gsf commented 6 years ago

I think I wasn't clear. I mean I get the error if I try to create a Job without a Namespace specified, expecting it to default to the "default" namespace. Here's my job constructor (error if I remove the Namespace field):

  k8sClient, err = k8s.NewInClusterClient()
  if err != nil {
    log.Fatal(err)
  }

  job := &batchv1.Job{
    Metadata: &metav1.ObjectMeta{
      GenerateName: k8s.String(name + "-update-"),
      Namespace:    k8s.String("default"),
    },
    Spec: &batchv1.JobSpec{
      Template: &corev1.PodTemplateSpec{
        Spec: &corev1.PodSpec{
          Containers: []*corev1.Container{
            {
              Name:  k8s.String("main"),
              Image: k8s.String(image),
              EnvFrom: []*corev1.EnvFromSource{
                {
                  SecretRef: &corev1.SecretEnvSource{
                    LocalObjectReference: &corev1.LocalObjectReference{
                      Name: k8s.String("testdb"),
                    },
                  },
                  Prefix: k8s.String("DB"),
                },
              },
              VolumeMounts: []*corev1.VolumeMount{
                {
                  Name:      k8s.String("config"),
                  MountPath: k8s.String("/config"),
                },
              },
            },
          },
          RestartPolicy: k8s.String("Never"),
          Volumes: []*corev1.Volume{
            {
              Name: k8s.String("config"),
              VolumeSource: &corev1.VolumeSource{
                ConfigMap: &corev1.ConfigMapVolumeSource{
                  LocalObjectReference: &corev1.LocalObjectReference{
                    Name: k8s.String(name),
                  },
                },
              },
            },
          },
        },
      },
    },
  }
  if err := k8sClient.Create(ctx, job); err != nil {
    log.Fatal(err)
  }
  fmt.Printf("Job created: %q\n", *job.Metadata.Name)
ericchiang commented 6 years ago

At the least I'd expect it to default to Client.Namespace. If that's not specified falling back to "default" seems odd, though I guess that's what kubectl does?

Regards being explicit feels better than being implicit. Is it tedious to specify a namespace? I could imagine it making code harder to read by obfuscating which objects are namespaced or what namespace is being defaulted to.

gsf commented 6 years ago

Yes, kubectl assumes "default" namespace unless a different namespace is set in the context. So the analogous behavior would be setting Client.Namespace to "default" unless otherwise specified.

gsf commented 6 years ago

On a closer look, no namespace preference is set in the context unless specified manually, so the behavior is as you noted, with commands falling back to "default".

ericchiang commented 6 years ago

On closer inspection the comment for client.Namespace is:

    // Default namespaces for objects that don't supply a namespace in
    // their object metadata.
    Namespace string

Which I don't think actually gets applied anywhere. That does default to "default" if you don't supply one in your kubeconfig.

I think the fix would be to propagate that value correctly on a create and other actions.

It's going to be a second before I can write up a fix. Please feel free to send one if you'd like to.

ericchiang commented 6 years ago

Tried to do this today, but there are some subtitles in the API that I don't think justify making "" mean "use the default namespace"

For now users can continue to supply the namespace as a value:

client, err := k8s.NewClient(config)
if err != nil {
    // handle error
}
cm := v1.ConfigMap{
    Metadata: &metav1.ObjectMeta{
        Name:      &k8s.String("my-configmap"),
        Namespace: &client.Namespace,
    },
    Data: map[string]string{"foo": "bar", "spam": "eggs"},
}
err := client.Create(ctx, cm)

Some more thoughts in https://github.com/ericchiang/k8s/pull/97

For now, I'm going to go with a docs only "fix"

If we want to something more clever later, we still can. For now going to close this.