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

Support specifying control plane firewall rules when creating or updating DOKS clusters #696

Conversation

llDrLove
Copy link
Contributor

@llDrLove llDrLove commented Jun 4, 2024

Add the new control plane permission property for creating or updating control plane firewalled clusters. By default, the zero value is nil which won't break existing doks godo users since they won't be using this new feature.

@llDrLove llDrLove marked this pull request as ready for review June 4, 2024 14:54
…trol-plane-firewall-rules-in-godo-and-doctl' into CON-10347-support-scecifying-control-plane-firewall-rules-in-godo-and-doctl
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@andrewsomething andrewsomething merged commit 1b9fcb0 into digitalocean:main Jun 4, 2024
8 checks passed
MaintenancePolicy *KubernetesMaintenancePolicy `json:"maintenance_policy,omitempty"`
AutoUpgrade *bool `json:"auto_upgrade,omitempty"`
SurgeUpgrade bool `json:"surge_upgrade,omitempty"`
ControlPlanePermission *KubernetesControlPlanePermission `json:"control_plane_permission,omitempty"`
Copy link
Contributor

@timoreimann timoreimann Jun 5, 2024

Choose a reason for hiding this comment

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

Sanity check: will omitempty semantics combined with the KubernetesControlPlanePermission type not being pointerized work for our case? That is, will be see the desired behavior / serialization when the field is omitted for a given cluster configuration (firewall disabled vs enabled)?

Genuinely asking because this part of the Go JSON library is always a bit scary, and mistakes can be very hard to correct once the API is in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Timo, this is fine since I had been using my godo fork using this new struct for cluster that don't use control plane permission and e2e tests for clusters that use the feature. Also, we do have some logic if the field is not provided it won't be considered for storing or updating the record.

Copy link
Member

Choose a reason for hiding this comment

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

Since Enabled takes a pointer and does not use omitempty, you should get the behavior (I'm assuming) you want.

Enabled          *bool    `json:"enabled"`

https://go.dev/play/p/nSgNNF_c1Lx

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

Successfully merging this pull request may close these issues.

3 participants