Support master authorized network config #1004

yuecong · 2023-08-15T01:31:14Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR is to add CAPG to add authorized networks for the control plane access feature provided by GKE.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes ##1003

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Add  authorized network support for the control plane access feature provided by GKE.

linux-foundation-easycla · 2023-08-15T01:31:18Z

The committers listed above are authorized under a signed CLA.

✅ login: yuecong / name: Cong Yue (682457a)

k8s-ci-robot · 2023-08-15T01:31:22Z

Welcome @yuecong!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-gcp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-gcp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-08-15T01:31:22Z

Hi @yuecong. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cpanato

please sign the CLA

/ok-to-test

thanks for this PR :D

yuecong · 2023-08-15T16:54:27Z

please sign the CLA

/ok-to-test

thanks for this PR :D

Thanks and I just signed the CLA

cpanato · 2023-08-16T07:15:38Z

/assign @richardcase

cpanato · 2023-08-16T07:16:53Z

config/crd/bases/infrastructure.cluster.x-k8s.io_gcpmanagedcontrolplanes.yaml

@@ -91,6 +91,35 @@ spec:
                description: Location represents the location (region or zone) in
                  which the GKE cluster will be created.
                type: string
+              master_authorized_networks_config:
+                description: MasterAuthorizedNetworksConfig repsesents configuration


Suggested change

description: MasterAuthorizedNetworksConfig repsesents configuration

description: MasterAuthorizedNetworksConfig represents configuration

cpanato · 2023-08-16T07:17:20Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

@@ -53,6 +53,9 @@ type GCPManagedControlPlaneSpec struct {
 	// Endpoint represents the endpoint used to communicate with the control plane.
 	// +optional
 	Endpoint clusterv1.APIEndpoint `json:"endpoint"`
+	// MasterAuthorizedNetworksConfig repsesents configuration options for master authorized networks feature of the GKE cluster.


Suggested change

// MasterAuthorizedNetworksConfig repsesents configuration options for master authorized networks feature of the GKE cluster.

// MasterAuthorizedNetworksConfig represents configuration options for master authorized networks feature of the GKE cluster.

cpanato · 2023-08-16T07:17:44Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+
+// MasterAuthorizedNetworksConfigCidrBlock contains an optional name and one CIDR block.
+type MasterAuthorizedNetworksConfigCidrBlock struct {
+


nit: can remove this empty line

richardcase

Great work @yuecong , thank you.

richardcase · 2023-08-17T06:30:19Z

cloud/services/container/clusters/reconcile.go

+	}
+
+	return &containerpb.MasterAuthorizedNetworksConfig{
+		// Enabled specifies whether master authorized networks is enabled.


nit: not sure you really need these comments when creating an instance of the struct.

richardcase · 2023-08-17T06:37:29Z

cloud/services/container/clusters/reconcile.go

+	// When desiredMasterAuthorizedNetworksConfig is nil, we will not update the existing config because this will casuse always reconcile.
+	// The GCP SDK is not allowed to pass nil to the update request either.
+	desiredMasterAuthorizedNetworksConfig := convertToSdkMasterAuthorizedNetworksConfig(s.scope.GCPManagedControlPlane.Spec.MasterAuthorizedNetworksConfig)
+	if desiredMasterAuthorizedNetworksConfig != nil && !reflect.DeepEqual(desiredMasterAuthorizedNetworksConfig, existingCluster.MasterAuthorizedNetworksConfig) {


Could we use cmp.Equal instead of reflect.DeepEqual?

richardcase · 2023-08-17T06:40:20Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+// Google Compute Engine Public IPs and Google Prod IPs.
+type MasterAuthorizedNetworksConfig struct {
+	// Whether or not master authorized networks is enabled.
+	Enabled bool `json:"enabled,omitempty"`


As MasterAuthorizedNetworksConfig is optional on GCPManagedControlPlaneSpec then i would say that if someone specifies a non-nil MasterAuthorizedNetworksConfig then this means they want to enable master authorized networks. So i feel that the Enabled field is not needed.

wdyt @cpanato ?

Good question and I agree we should decide the behavior here.
Right now, MasterAuthorizedNetworksConfig == nil means we will not reconcile it from CAPG. This means even if the users somehow manually changed the config for their cluster, we will not change it. So we would need this enabled field to let the users explicitly disable this feature.

If we want to make MasterAuthorizedNetworksConfig == nil means disable, we would need to also make sure do not to reconcile if MasterAuthorizedNetworksConfig == nil and the existing config is false.

I agree with both of the options here. :)

Personally I would prefer that we make MasterAuthorizedNetworksConfig == nil to mean disabled.

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

richardcase · 2023-08-29T11:17:37Z

cloud/services/container/clusters/reconcile.go

+			CidrBlocks:                  []*containerpb.MasterAuthorizedNetworksConfig_CidrBlock{},
+			GcpPublicCidrsAccessEnabled: new(bool),


nit: could we just omit these and leave Enabled: false ?

it does not work per my test. for GcpPublicCidrsAccessEnabled, it will be Nil instead of false as it is a *bool rather than bool in the GKE SDK. similarly to CidrBlocks, it will be Nil if we do not put some empty values. And it is a Nil to the GKE SDK, it will not overwrite current values and cause us to always have diff on those two fields when reconciling. Let me know if you have different opinions on this.

richardcase · 2023-08-29T11:28:16Z

cloud/services/container/clusters/reconcile.go

+	if a.Enabled != b.Enabled {
+		return false
+	}
+	if *a.GcpPublicCidrsAccessEnabled != *b.GcpPublicCidrsAccessEnabled {


If either of the GcpPublicCidrsAccessEnabled is nil then this will panic. Do we need to do some additional checks on this (or is this handled elsewhere)?

richardcase · 2023-08-31T04:05:48Z

This looks good to me, thanks for looking at the feedback @yuecong ❤️

Can you squash the commits? Then i think we are good to merge.

* add MasterAuthorizedNetworksConfig filed into GCPManagedControlPlane CRD * fix * disable it if no desired specified in CR * change to do not reconcile if the desired CR is nil * add comment add nil handling fo GcpPublicCidrsAccessEnabled change nil as enabled fix display_name not to be optional address comments fix typo and remove empty line

richardcase · 2023-09-01T07:32:44Z

Thanks for squashing the commits. From my side:

/approve

For the lgtm:

/assign cpanato

k8s-ci-robot · 2023-09-01T07:32:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase, yuecong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [richardcase]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

richardcase · 2023-09-01T07:33:29Z

Actually it doesn't require both (just lgtm), so:

/lgtm

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 15, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 15, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2023

k8s-ci-robot requested review from cpanato and dims August 15, 2023 01:31

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2023

yuecong changed the title ~~Support master authorized network config (#65)~~ Support master authorized network config Aug 15, 2023

cpanato reviewed Aug 15, 2023

View reviewed changes

k8s-ci-robot assigned richardcase Aug 16, 2023

cpanato reviewed Aug 16, 2023

View reviewed changes

yuecong requested a review from cpanato August 16, 2023 17:50

richardcase reviewed Aug 17, 2023

View reviewed changes

yuecong requested a review from richardcase August 17, 2023 16:34

richardcase reviewed Aug 29, 2023

View reviewed changes

yuecong requested a review from richardcase August 29, 2023 17:41

yuecong force-pushed the support-master-authorized-network-config branch from 4b5ebde to 682457a Compare August 31, 2023 16:57

k8s-ci-robot assigned cpanato Sep 1, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2023

k8s-ci-robot merged commit 01af3be into kubernetes-sigs:main Sep 1, 2023
5 checks passed

k8s-ci-robot added this to the v1.1.0 milestone Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support master authorized network config #1004

Support master authorized network config #1004

yuecong commented Aug 15, 2023 •

edited by richardcase

Loading

linux-foundation-easycla bot commented Aug 15, 2023 •

edited

Loading

k8s-ci-robot commented Aug 15, 2023

k8s-ci-robot commented Aug 15, 2023

cpanato left a comment

yuecong commented Aug 15, 2023

cpanato commented Aug 16, 2023

cpanato Aug 16, 2023

cpanato Aug 16, 2023

cpanato Aug 16, 2023

richardcase left a comment

richardcase Aug 17, 2023

richardcase Aug 17, 2023

richardcase Aug 17, 2023

richardcase Aug 17, 2023

yuecong Aug 17, 2023 •

edited

Loading

richardcase Aug 22, 2023

richardcase Aug 29, 2023

yuecong Aug 29, 2023

richardcase Aug 29, 2023

richardcase commented Aug 31, 2023

richardcase commented Sep 1, 2023

k8s-ci-robot commented Sep 1, 2023

richardcase commented Sep 1, 2023 •

edited

Loading

	description: MasterAuthorizedNetworksConfig repsesents configuration
	description: MasterAuthorizedNetworksConfig represents configuration

	// MasterAuthorizedNetworksConfig repsesents configuration options for master authorized networks feature of the GKE cluster.
	// MasterAuthorizedNetworksConfig represents configuration options for master authorized networks feature of the GKE cluster.


		// MasterAuthorizedNetworksConfigCidrBlock contains an optional name and one CIDR block.
		type MasterAuthorizedNetworksConfigCidrBlock struct {

		CidrBlocks: []*containerpb.MasterAuthorizedNetworksConfig_CidrBlock{},
		GcpPublicCidrsAccessEnabled: new(bool),

Support master authorized network config #1004

Support master authorized network config #1004

Conversation

yuecong commented Aug 15, 2023 • edited by richardcase Loading

linux-foundation-easycla bot commented Aug 15, 2023 • edited Loading

k8s-ci-robot commented Aug 15, 2023

k8s-ci-robot commented Aug 15, 2023

cpanato left a comment

Choose a reason for hiding this comment

yuecong commented Aug 15, 2023

cpanato commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuecong Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase commented Aug 31, 2023

richardcase commented Sep 1, 2023

k8s-ci-robot commented Sep 1, 2023

richardcase commented Sep 1, 2023 • edited Loading

yuecong commented Aug 15, 2023 •

edited by richardcase

Loading

linux-foundation-easycla bot commented Aug 15, 2023 •

edited

Loading

yuecong Aug 17, 2023 •

edited

Loading

richardcase commented Sep 1, 2023 •

edited

Loading