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

Adding traffic_distribution field #2625

Open
wants to merge 4 commits into
base: v3-major-release
Choose a base branch
from

Conversation

JaylonmcShan03
Copy link
Contributor

@JaylonmcShan03 JaylonmcShan03 commented Nov 18, 2024

Description

Addresses #2581
This PR introduces the traffic_distribution field to the kubernetes_service_v1 resource. This field, introduced in Kubernetes v1.31 provides a way to influence traffic routing within a Kubernetes Service by specifying a preference for traffic distribution.

  • PreferClose: prioritizes routing to topologically closer endpoints for improved performance, cost, or reliability.

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=TestAccXXX'

...
2024-11-21T08:51:03.941-0600 [DEBUG] sdk.helper_resource: Finished TestCase: test_name=TestAccKubernetesServiceV1_trafficDistribution
--- PASS: TestAccKubernetesServiceV1_trafficDistribution (1.44s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   2.238s


jaylon.mcshan@jaylon issue-testing % terraform apply
╷
│ Error: expected spec.0.traffic_distribution to be one of ["PreferClose"], got InvalidValue
│ 
│   with kubernetes_service_v1.test,
│   on main.tf line 68, in resource "kubernetes_service_v1" "test":
│   68:     traffic_distribution = "InvalidValue"
│ 
╵
jaylon.mcshan@jaylon issue-testing % 

Release Note

Release note for CHANGELOG:

...

References

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

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.

All in all, great job! 🚀

I have left some cosmetic suggestions. 😊

Thanks for making the test function flexible with the additional attribute, once the Kubernetes team adds more traffic distribution values, it can be reused to add more tests.

@@ -0,0 +1,3 @@
```release-note:enhancement
Added the `traffic_distribution` field to the `kubernetes_service_v1` resource
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to use imperative voice here and add kubernetes_service too, since both resources refer to the same code:

```release-note:enhancement
`resource/kubernetes_service`:  add a new field `traffic_distribution`.
```

```
`resource/kubernetes_service_v1`:  add a new field `traffic_distribution`.
```

Description: "Specifies the preferred strategy for distributing traffic to Service endpoints. When set to PreferClose, the Kubernetes will prioritize routing traffic to endpoints that are topologically closer",
Optional: true,
ValidateFunc: validation.StringInSlice([]string{
"PreferClose",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest importing such predefined words from Kubernetes code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arybolovlev Wow, that's a clever approach! Would this be considered best practice for the future? Due to the k8s api were to update / expand on the traffic_distrubtion field with additional options, our code would automatically align with those changes?

resourceName := "kubernetes_service_v1.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add here skipIfClusterVersionLessThan to make sure these test runs on Kubernetes 1.31+ only.

@JaylonmcShan03 JaylonmcShan03 marked this pull request as ready for review November 22, 2024 13:38
@JaylonmcShan03 JaylonmcShan03 requested a review from a team as a code owner November 22, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants