-
Notifications
You must be signed in to change notification settings - Fork 198
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 master authorized network config #1004
Support master authorized network config #1004
Conversation
|
Welcome @yuecong! |
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 Once the patch is verified, the new status will be reflected by the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please sign the CLA
/ok-to-test
thanks for this PR :D
Thanks and I just signed the CLA |
/assign @richardcase |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: MasterAuthorizedNetworksConfig repsesents configuration | |
description: MasterAuthorizedNetworksConfig represents configuration |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can remove this empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @yuecong , thank you.
} | ||
|
||
return &containerpb.MasterAuthorizedNetworksConfig{ | ||
// Enabled specifies whether master authorized networks is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure you really need these comments when creating an instance of the struct.
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use cmp.Equal
instead of reflect.DeepEqual
?
// 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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt @cpanato ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would prefer that we make MasterAuthorizedNetworksConfig == nil
to mean disabled.
CidrBlocks: []*containerpb.MasterAuthorizedNetworksConfig_CidrBlock{}, | ||
GcpPublicCidrsAccessEnabled: new(bool), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we just omit these and leave Enabled: false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if a.Enabled != b.Enabled { | ||
return false | ||
} | ||
if *a.GcpPublicCidrsAccessEnabled != *b.GcpPublicCidrsAccessEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
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
4b5ebde
to
682457a
Compare
Thanks for squashing the commits. From my side: /approve For the lgtm: /assign cpanato |
[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:
Approvers can indicate their approval by writing |
Actually it doesn't require both (just lgtm), so: /lgtm |
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:
Release note: