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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support empty TLS blocks in ingress_v1 #2344

Merged
merged 2 commits into from Dec 5, 2023

Conversation

andremarianiello
Copy link
Contributor

@andremarianiello andremarianiello commented Nov 18, 2023

Description

Fix an issue where the empty tls attribute in the configuration does not generate the corresponding Ingress object without any TLS configuration.

The following TF code:

resource "kubernetes_ingress_v1" "this" {
  metadata {
    name = "this"
  }
  spec {
    default_backend {
      service {
        name = "app"
        port {
          number = 443
        }
      }
    }
    tls {}
  }
}

Should produce the following Ingress object:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  creationTimestamp: "2023-12-04T08:57:33Z"
  generation: 1
  name: this
  namespace: default
  resourceVersion: "491"
  uid: 97330b8c-5ebf-4037-a206-8ca2928b242f
spec:
  defaultBackend:
    service:
      name: app
      port:
        number: 443
  tls:
  - {}
status:
  loadBalancer: {}

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

# make testacc TESTARGS='-run=TestAccKubernetesIngressV1_'
==> Checking that code complies with gofmt requirements...
go vet .
TF_ACC=1 go test "/root/terraform-provider-kubernetes/kubernetes" -v -vet=off -run=TestAccKubernetesIngressV1_ -parallel 8 -timeout 3h
=== RUN   TestAccKubernetesIngressV1_serviceBackend
=== PAUSE TestAccKubernetesIngressV1_serviceBackend
=== RUN   TestAccKubernetesIngressV1_resourceBackend
=== PAUSE TestAccKubernetesIngressV1_resourceBackend
=== RUN   TestAccKubernetesIngressV1_TLS
=== PAUSE TestAccKubernetesIngressV1_TLS
=== RUN   TestAccKubernetesIngressV1_emptyTLS
=== PAUSE TestAccKubernetesIngressV1_emptyTLS
=== RUN   TestAccKubernetesIngressV1_InternalKey
=== PAUSE TestAccKubernetesIngressV1_InternalKey
=== RUN   TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud
=== PAUSE TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud
=== RUN   TestAccKubernetesIngressV1_hostOnlyRule
=== PAUSE TestAccKubernetesIngressV1_hostOnlyRule
=== RUN   TestAccKubernetesIngressV1_multipleRulesDifferentHosts
=== PAUSE TestAccKubernetesIngressV1_multipleRulesDifferentHosts
=== RUN   TestAccKubernetesIngressV1_defaultIngressClass
=== PAUSE TestAccKubernetesIngressV1_defaultIngressClass
=== CONT  TestAccKubernetesIngressV1_serviceBackend
=== CONT  TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud
=== CONT  TestAccKubernetesIngressV1_multipleRulesDifferentHosts
=== CONT  TestAccKubernetesIngressV1_hostOnlyRule
=== CONT  TestAccKubernetesIngressV1_defaultIngressClass
=== CONT  TestAccKubernetesIngressV1_emptyTLS
=== CONT  TestAccKubernetesIngressV1_TLS
=== CONT  TestAccKubernetesIngressV1_InternalKey
=== NAME  TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud
    provider_test.go:261: The Kubernetes endpoint must come from GKE for this test to run - skipping
--- SKIP: TestAccKubernetesIngressV1_WaitForLoadBalancerGoogleCloud (0.10s)
=== CONT  TestAccKubernetesIngressV1_resourceBackend
--- PASS: TestAccKubernetesIngressV1_resourceBackend (20.73s)
--- PASS: TestAccKubernetesIngressV1_emptyTLS (20.91s)
--- PASS: TestAccKubernetesIngressV1_multipleRulesDifferentHosts (21.23s)
--- PASS: TestAccKubernetesIngressV1_hostOnlyRule (21.27s)
--- PASS: TestAccKubernetesIngressV1_defaultIngressClass (27.42s)
--- PASS: TestAccKubernetesIngressV1_serviceBackend (34.11s)
--- PASS: TestAccKubernetesIngressV1_TLS (34.41s)
--- PASS: TestAccKubernetesIngressV1_InternalKey (45.46s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   45.832s

Release Note

Release note for CHANGELOG:

resource/kubernetes_ingress: Fix an issue where the empty `tls` attribute in the configuration does not generate the corresponding Ingress object without any TLS configuration.

resource/kubernetes_ingress_v1: Fix an issue where the empty `tls` attribute in the configuration does not generate the corresponding Ingress object without any TLS configuration.

References

Fix: #2343

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

@andremarianiello andremarianiello requested a review from a team as a code owner November 18, 2023 06:53
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 18, 2023

CLA assistant check
All committers have signed the CLA.

@andremarianiello
Copy link
Contributor Author

Ready for review

Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Hi @andremarianiello,

Thank you for fixing this!

There are a few things I would like to ask you to fix before we merge your PR:

  1. Could you please make the same change for the kubernetes_ingress resource too?

  2. Please add the following change file .changelog/2344.txt:

```release-note:bug
`resource/kubernetes_ingress`: Fix an issue where the empty `tls` attribute in the configuration does not generate the corresponding Ingress object without any TLS configuration.
```

```release-note:bug
`resource/kubernetes_ingress_v1`: Fix an issue where the empty `tls` attribute in the configuration does not generate the corresponding Ingress object without any TLS configuration.
```
  1. Run make tests-lint-fix to fix the TF code used in tests.

I will keep an eye on this PR.

Thank you!

@andremarianiello
Copy link
Contributor Author

@arybolovlev Thanks for the review! I have pushed those changes your requested.

@andremarianiello
Copy link
Contributor Author

The kind acceptance tests appear to be attempting to run without kind actually being installed. I'm sure it's unrelated to my changes.

Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Thank you for addressing this, @andremarianiello!

@arybolovlev
Copy link
Contributor

The kind acceptance tests appear to be attempting to run without kind actually being installed. I'm sure it's unrelated to my changes.

That is ok, this is due to some GH limitations. Thank you for addressing this problem!

@arybolovlev arybolovlev merged commit a7809cd into hashicorp:main Dec 5, 2023
12 of 13 checks passed
@andremarianiello andremarianiello deleted the allow-empty-tls-blocks branch December 5, 2023 15:19
@andremarianiello
Copy link
Contributor Author

@arybolovlev Thank you for your responsiveness! I am trying to figure out when this fix will be released. Do you have a published release schedule or release criteria?

@arybolovlev
Copy link
Contributor

We don't have a schedule for this provider but looking at the number of fixes we currently have unreleased, I think we will issue a new release this or next week.

@andremarianiello
Copy link
Contributor Author

We don't have a schedule for this provider but looking at the number of fixes we currently have unreleased, I think we will issue a new release this or next week.

Awesome! Can't wait 馃憤

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