terraform-provider-kubernetes icon indicating copy to clipboard operation
terraform-provider-kubernetes copied to clipboard

storage_class_name = "" is treated as nil, preventing Google Filestore use

Open dpkirchner opened this issue 3 years ago • 14 comments

Terraform Version, Provider Version and Kubernetes Version

Terraform version: Terraform v1.0.5 on darwin_amd64
Kubernetes provider version: 2.4.1
Kubernetes version: v1.19.9-gke.1900

Affected Resource(s)

  • kubernetes_persistent_volume
  • kubernetes_persistent_volume_claim

Terraform Configuration Files

resource "kubernetes_persistent_volume" "tmp" {
  metadata {
    name = "tmp"
  }
  spec {
    capacity = {
      storage = "10Gi"
    }

    access_modes       = ["ReadWriteMany"]
    storage_class_name = ""

    persistent_volume_source {
      nfs {
        path   = "/whatever"
        server = var.filestore_nfs_server_ip
      }
    }
  }
}

resource "kubernetes_persistent_volume_claim" "tmp" {
  metadata {
    name      = "tmp"
    namespace = "default"
  }

  spec {
    access_modes       = ["ReadWriteMany"]
    storage_class_name = ""
    volume_name        = kubernetes_persistent_volume.tmp.metadata.0.name

    resources {
      requests = {
        storage = "10Gi"
      }
    }
  }
}

Debug Output

Can't paste the whole thing, contains proprietary information. Here's the relevant bits though: https://gist.github.com/dpkirchner/76447ded8e97ae7286a83faf034420df

Steps to Reproduce

  1. terraform apply
  2. kubectl get events -w

Expected Behavior

The PVC should have been created with an empty string for the storage class name.

Actual Behavior

The PVC ends up being created with the default ("standard") storage class name.

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

dpkirchner avatar Aug 20 '21 23:08 dpkirchner

Hello,

I was able to reproduce your issue and even though I do not have any working knowledge of this library, here are the possible culprits

  1. https://github.com/hashicorp/terraform-provider-kubernetes/blob/b5770d4b51a1e1314c0f64b9f2c92cac9be7021a/kubernetes/structure_persistent_volume_claim.go#L82 2.https://github.com/hashicorp/terraform-provider-kubernetes/blob/b5770d4b51a1e1314c0f64b9f2c92cac9be7021a/kubernetes/structure_persistent_volume_spec.go#L388

This here is tricky.

https://github.com/hashicorp/terraform-provider-kubernetes/blob/b5770d4b51a1e1314c0f64b9f2c92cac9be7021a/kubernetes/structure_persistent_volume_spec.go#L1230

This checks if the old string is empty and emits addop or replaceop accordingly. However, the zero-state for storageClass is not empty string ( as it has a special meaning ) but nil which, I think, unearths another can of worms:

// PVC spec
// Name of the StorageClass required by the claim.
// More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#class-1
// +optional
StorageClassName *string `json:"storageClassName,omitempty" protobuf:"bytes,5,opt,name=storageClassName"`
// PV spec
// Name of StorageClass to which this persistent volume belongs. Empty value
// means that this volume does not belong to any StorageClass.
// +optional
StorageClassName string `json:"storageClassName,omitempty" protobuf:"bytes,6,opt,name=storageClassName"`

I have so several questions here:

  1. Why is storageClassName a *string in one spec and string in another?
  2. Why is there an omitempty in json encoding tag even though this field being empty has a special meaning?
  3. Why is Dynamic Provision suppression bound to storageClassName being empty even though empty/nil strings are known to be a pain point in interoperability?

Akaame avatar Aug 22 '21 17:08 Akaame

To emphasize the validity of this bug here is a reference to the official kubernetes docs , where you can see the comment

Empty string must be explicitly set otherwise default StorageClass will be set

steveizzle avatar Sep 27 '21 12:09 steveizzle

Using an empty storageClassName is something that is required for some of our on-premise use cases.

I had to patch the provider in the meantime with a quick fix.

Linked #590 and #872.

davidfischer-ch avatar Oct 08 '21 08:10 davidfischer-ch

This makes TF unable to handle static configured PV's for the EFS CSI controller too.

Lillecarl avatar Feb 03 '22 16:02 Lillecarl

This is an OLD problem that keeps getting kicked down the road. https://github.com/hashicorp/terraform-provider-kubernetes/pull/590 https://github.com/hashicorp/terraform-provider-kubernetes/issues/567 and others. I wouldn't hold out much hope.

sarg3nt avatar May 11 '22 16:05 sarg3nt

Yup, actually any class-less PVCs/PVs can't be described as resources currently.

I raised this as a core terraform bug, but they redirected to the provider.

Here's the relevant part:

I'm trying to specify a kubernetes_persistent_volume_claim to a manually defined (i.e. not automatically provisioned) Kubernetes persistent volume. Part of this requires that the storage_class_name attribute should be to be an empty string, as per the K8S docs:

A PVC with its storageClassName set equal to "" is always interpreted to be requesting a PV with no class, so it can only be bound to PVs with no class (no annotation or one set equal to ""). A PVC with no storageClassName is not quite the same and is treated differently by the cluster, depending on whether the DefaultStorageClass admission plugin is turned on.

However, it looks like Terraform K8S provider interprets that as "use the default", which in this case, means that it looks up and uses the default storage class as the field value, and so it does not match with a class-less PV as expected.

A literal empty string is required to tell Kubernetes that it should match against class-less persistent volumes.

It'd be awesome to have a way to set the storage_class_name to be an explicit empty string and disable the defaulting behavior from the provider.

dinibu avatar May 31 '22 13:05 dinibu

@dinibu https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs/resources/manifest Requires kubernetes access during planning, other than that it'll work just fine. You can convert existing specs with k2tf.

Lillecarl avatar Jun 15 '22 09:06 Lillecarl

Marking this issue as stale due to inactivity. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. This helps our maintainers find and focus on the active issues. Maintainers may also remove the stale label at their discretion. Thank you!

github-actions[bot] avatar Jun 16 '23 00:06 github-actions[bot]

still broken

Lillecarl avatar Jun 19 '23 19:06 Lillecarl

Still broken. Trying to work around it by providing a kubernetes_manifest directly...

mike-sol avatar Sep 28 '23 05:09 mike-sol

Yes, something like this works to get around this for now:

resource "kubernetes_manifest" "persistentvolumeclaim" {

  manifest = yamldecode(<<-EOF
    apiVersion: v1
    kind: PersistentVolumeClaim
    metadata:
      name: whatever-pvc-name
      namespace: whatever
    spec:
      accessModes:
      - ReadWriteMany
      resources:
        requests:
          storage: 1Mi
      volumeName: existing-pv-name-goes-here
      storageClassName: ""
  EOF
  )
}

mike-sol avatar Sep 28 '23 05:09 mike-sol