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

new: [BREAKING] Add support for LKE Control Plane ACL #495

Merged
merged 17 commits into from
May 8, 2024

Conversation

lgarber-akamai
Copy link
Contributor

@lgarber-akamai lgarber-akamai commented Apr 29, 2024

📝 Description

This pull request adds support for the upcoming LKE Control Plane ACL feature, including the following changes:

Updated Endpoints

  • GET /lke/clusters - Updated to include ACL under control_plane field
  • GET /lke/clusters/{cluster_id} - Updated to include ACL under control_plane field
  • POST /lke/clusters - Updated to include ACL under control_plane field
  • PUT /lke/clusters/{cluster_id} - Updated to include ACL under control_plane field

New Endpoints

  • GET /lke/clusters/{cluster_id}/control_plane_acl
  • PUT /lke/clusters/{cluster_id}/control_plane_acl
  • DELETE /lke/clusters/{cluster_id}/control_plane_acl

New Event Action Constants

  • lke_control_plane_acl_create
  • lke_control_plane_acl_update
  • lke_control_plane_acl_delete

✔️ How to Test

The following test steps assume you have pulled down this repo locally and have configured your Linode account according to the guidelines commented on TPT-2854.

Integration Testing

make fixtures ARGS="-run TestLKECluster_withACL"

Manual Testing

In a linodego sandbox environment (e.g. dx-devenv), run the following:

package main

import (
	"context"
	"fmt"
	"github.com/sanity-io/litter"
	"log"
	"os"

	"github.com/linode/linodego"
)

func main() {
	ctx := context.Background()

	client := linodego.NewClient(nil)
	client.SetToken(os.Getenv("LINODE_TOKEN"))

	valueTrue := true

	// Create an LKE cluster with an allowed control plane IP
	cluster, err := client.CreateLKECluster(
		ctx,
		linodego.LKEClusterCreateOptions{
			Label:      "linodego-manual-test",
			Region:     "us-mia",
			K8sVersion: "1.29",
			NodePools: []linodego.LKENodePoolCreateOptions{
				{
					Type:  "g6-standard-1",
					Count: 1,
				},
			},
			ControlPlane: &linodego.LKEClusterControlPlaneOptions{
				ACL: &linodego.LKEClusterControlPlaneACLOptions{
					Enabled: &valueTrue,
					Addresses: &linodego.LKEClusterControlPlaneACLAddressesOptions{
						IPv4: &[]string{"10.0.0.1/32"},
					},
				},
			},
		},
	)
	if err != nil {
		log.Fatal(err)
	}

	// Manually retrieve the ACL since it isn't yet returned
	// from the LKE cluster response endpoint
	acl, err := client.GetLKEClusterControlPlaneACL(ctx, cluster.ID)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Println("Cluster ID:", cluster.ID)
	fmt.Println("Cluster ACL Configuration:", litter.Sdump(acl))

	acl, err = client.UpdateLKEClusterControlPlaneACL(
		ctx,
		cluster.ID,
		linodego.LKEClusterControlPlaneACLUpdateOptions{
			ACL: linodego.LKEClusterControlPlaneACLOptions{
				Addresses: &linodego.LKEClusterControlPlaneACLAddressesOptions{
					IPv4: &[]string{"10.0.0.5/32"},
				},
			},
		},
	)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println("Updated Cluster ACL Configuration:", litter.Sdump(acl))
}
  1. Ensure the output looks similar to the following:
Cluster ID: 173797
Cluster ACL Configuration: &linodego.LKEClusterControlPlaneACLResponse{
  ACL: linodego.LKEClusterControlPlaneACL{
    Enabled: &true,
    Addresses: &linodego.LKEClusterControlPlaneACLAddresses{
      IPv4: []string{
        "10.0.0.1/32",
      },
      IPv6: nil,
    },
  },
}
Updated Cluster ACL Configuration: &linodego.LKEClusterControlPlaneACLResponse{
  ACL: linodego.LKEClusterControlPlaneACL{
    Enabled: &true,
    Addresses: &linodego.LKEClusterControlPlaneACLAddresses{
      IPv4: []string{
        "10.0.0.5/32",
      },
      IPv6: nil,
    },
  },
}

@lgarber-akamai lgarber-akamai added the new-feature for new features in the changelog. label Apr 29, 2024
@@ -6,6 +6,7 @@ require (
github.com/jarcoal/httpmock v1.3.1
github.com/linode/linodego v1.30.0
github.com/linode/linodego/k8s v0.0.0-00010101000000-000000000000
github.com/stretchr/testify v1.9.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was originally added in the parent/child project branch

@lgarber-akamai lgarber-akamai marked this pull request as ready for review April 29, 2024 17:34
@lgarber-akamai lgarber-akamai requested a review from a team as a code owner April 29, 2024 17:34
@lgarber-akamai lgarber-akamai requested review from jriddle-linode and zliang-akamai and removed request for a team April 29, 2024 17:34
@lgarber-akamai lgarber-akamai marked this pull request as draft April 29, 2024 17:34
@lgarber-akamai lgarber-akamai added breaking-change for breaking changes in the changelog. do-not-merge PRs that should not be merged until the commented issue is resolved labels Apr 29, 2024
@lgarber-akamai lgarber-akamai changed the title new: Add support for LKE Control Plane ACL new: [BREAKING] Add support for LKE Control Plane ACL Apr 29, 2024
@lgarber-akamai lgarber-akamai marked this pull request as ready for review May 2, 2024 15:56
@lgarber-akamai lgarber-akamai removed the do-not-merge PRs that should not be merged until the commented issue is resolved label May 2, 2024
@lgarber-akamai
Copy link
Contributor Author

As a team we decided to go ahead with this breaking change 🙂

@lgarber-akamai lgarber-akamai marked this pull request as draft May 2, 2024 16:09
Comment on lines +33 to +34
IPv4 *[]string `json:"ipv4,omitempty"`
IPv6 *[]string `json:"ipv6,omitempty"`
Copy link
Contributor Author

@lgarber-akamai lgarber-akamai May 2, 2024

Choose a reason for hiding this comment

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

A pointer to a slice might be considered an anti-pattern but in this case we need omitempty to only apply to nil values.


// LKEClusterControlPlaneACLResponse represents the response structure
// for the Client.GetLKEClusterControlPlaneACL(...) method.
type LKEClusterControlPlaneACLResponse struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't quite know what to name this response struct. For reference, this response body is unique to GetLKEClusterControlPlaneACL(...) and UpdateLKEClusterControlPlaneACL(...).

@lgarber-akamai lgarber-akamai marked this pull request as ready for review May 2, 2024 16:18
Comment on lines +34 to +35
Enabled *bool `json:"enabled,omitempty"`
Addresses *LKEClusterControlPlaneACLAddressesOptions `json:"addresses,omitempty"`
Copy link
Contributor Author

@lgarber-akamai lgarber-akamai May 2, 2024

Choose a reason for hiding this comment

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

I'm slightly concerned about the UX impact of making all update fields pointers but I don't think there's a better way to do it.

Maybe we could introduce a helper like this to make defining pointers to literals bit cleaner?

func Optional[T any](value T) *T {
	return &value
}

// Usage: linodego.Optional("test-string")

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding this helper is a good idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yec-akamai Sounds good! I'll track adding that helper as a separate ticket/PR just to keep the scope of this PR reasonable 🙂

@lgarber-akamai lgarber-akamai requested review from a team, ykim-1 and yec-akamai and removed request for a team May 2, 2024 16:22
Copy link
Contributor

@yec-akamai yec-akamai left a comment

Choose a reason for hiding this comment

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

Great work! Tests passed locally 🎉

Comment on lines +34 to +35
Enabled *bool `json:"enabled,omitempty"`
Addresses *LKEClusterControlPlaneACLAddressesOptions `json:"addresses,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding this helper is a good idea 👍

lke_clusters_control_plane.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ykim-1 ykim-1 left a comment

Choose a reason for hiding this comment

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

LGTM, Tests pass locally. Should we add the required customer tag to all the test account?

@lgarber-akamai
Copy link
Contributor Author

LGTM, Tests pass locally. Should we add the required customer tag to all the test account?

@ykim-1 Great idea, yeah we should definitely add the customer tag until this reaches GA

@lgarber-akamai lgarber-akamai merged commit 7c73ad6 into linode:main May 8, 2024
4 checks passed
renovate bot added a commit to anza-labs/lke-operator that referenced this pull request May 23, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/linode/linodego](https://github.com/linode/linodego) |
`v1.33.1` -> `v1.34.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2flinode%2flinodego/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2flinode%2flinodego/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2flinode%2flinodego/v1.33.1/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2flinode%2flinodego/v1.33.1/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>linode/linodego (github.com/linode/linodego)</summary>

###
[`v1.34.0`](https://github.com/linode/linodego/releases/tag/v1.34.0)

[Compare
Source](https://github.com/linode/linodego/compare/v1.33.1...v1.34.0)

#### What's Changed

##### ⚠️  Breaking Change

- \[BREAKING] Add support for LKE Control Plane ACL by
[@&#8203;lgarber-akamai](https://github.com/lgarber-akamai) in
[linode/linodego#495

##### 🐛 Bug Fixes

- Prevent unexpected warning from Resty when calling
`Client.SetDebug(false)` by
[@&#8203;lgarber-akamai](https://github.com/lgarber-akamai) in
[linode/linodego#508

##### 💡 Improvements

- Mask the value of the Authorization header if debug is enabled by
[@&#8203;rosskirkpat](https://github.com/rosskirkpat) in
[linode/linodego#501
- Expose region capabilities enum by
[@&#8203;yec-akamai](https://github.com/yec-akamai) in
[linode/linodego#507

##### ⚙️  Repo/CI Improvements

- replace test execution handler with conditional by
[@&#8203;ykim-1](https://github.com/ykim-1) in
[linode/linodego#502

##### 📦 Dependency Updates

- bump golang.org/x/net from 0.24.0 to 0.25.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[linode/linodego#500
- bump github.com/go-resty/resty/v2 from 2.12.0 to 2.13.1 by
[@&#8203;dependabot](https://github.com/dependabot) in
[linode/linodego#505
- bump golangci/golangci-lint-action from 5 to 6 by
[@&#8203;dependabot](https://github.com/dependabot) in
[linode/linodego#506

#### New Contributors

- [@&#8203;rosskirkpat](https://github.com/rosskirkpat) made their
first contribution in
[linode/linodego#501

**Full Changelog**:
linode/linodego@v1.33.1...v1.34.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/anza-labs/lke-operator).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNjguMTAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4zNjguMTAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbImFyZWEvZGVwZW5kZW5jeSJdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change for breaking changes in the changelog. new-feature for new features in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants