Add fields to the GCPManagedControlPlane types #936

aniruddha2000 · 2023-05-31T04:26:12Z

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR contains the necessary API changes for GCPManagedControlPlane types to support additional fields.

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

Special notes for your reviewer:

Do we need to rename ClusterNetwork.Pod to a more reasonable name? Because it is quite ambiguous to the upstream k8s Pod.
CidrBlock map[string]string `json:"cidrBlock,omitempty" must not be greater than 10. What should be the kubebuilder validation?

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

TODOs:

squashed commits
includes documentation

Release note:

Add necessary API changes for GCPManagedControlPlane types to support additional fields.

/cc @akshay196 @richardcase @cpanato

k8s-ci-robot · 2023-05-31T04:26:15Z

@aniruddha2000: GitHub didn't allow me to request PR reviews from the following users: akshay196.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR contains the necessary API changes for GCPManagedControlPlane types to support additional fields.

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

Special notes for your reviewer:

Do we need to rename ClusterNetwork.Pod to a more reasonable name? Because it is quite ambiguous to the upstream k8s Pod.

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 necessary API changes for GCPManagedControlPlane types to support additional fields.
/cc @akshay196 @richardcase @cpanato

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.

k8s-ci-robot · 2023-05-31T04:26:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aniruddha2000
Once this PR has been reviewed and has the lgtm label, please assign neolit123 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

- Description - Cluster Network - PrivateCluster - UseIPAliases - AuthorizedNetwork - Pod - Service - Cluster Security - WorkloadIdentityConfig - AuthenticatorGroupConfig - EnableLagacyAuthorization - IssueClientCertificate - AddonsConfig - LoggingConfig - MonitoringConfig Signed-off-by: Aniruddha Basak <[email protected]>

akshay196

@aniruddha2000 Few minor comments.

Do we need to rename ClusterNetwork.Pod to a more reasonable name? Because it is quite ambiguous to the upstream k8s Pod.

ClusterNetwork.Pod and ClusterNetwork.Service names are inspired by Cluster.ClusterNetwork section in Cluster-API. However we can rename it to something like PodConfig and ServiceConfig as it encapsulates pod and service networking configuration.

CidrBlock map[string]string `json:"cidrBlock,omitempty" must not be greater than 10. What should be the kubebuilder validation?

See if +kubebuilder:validation:MaxProperties is useful here? (ref: https://book.kubebuilder.io/reference/markers/crd-validation.html)
Otherwise we may add webhook validation.

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

k8s-ci-robot · 2023-06-04T11:33:09Z

@akshay196: changing LGTM is restricted to collaborators

In response to this:

@aniruddha2000 Few minor comments.

Do we need to rename ClusterNetwork.Pod to a more reasonable name? Because it is quite ambiguous to the upstream k8s Pod.

ClusterNetwork.Pod and ClusterNetwork.Service names are inspired by Cluster.ClusterNetwork section in Cluster-API. However we can rename it to something like PodConfig and ServiceConfig as it encapsulates pod and service networking configuration.

CidrBlock map[string]string `json:"cidrBlock,omitempty" must not be greater than 10. What should be the kubebuilder validation?

See if +kubebuilder:validation:MaxProperties is useful here? Otherwise we may add webhook validation. (ref: https://book.kubebuilder.io/reference/markers/crd-validation.html)

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.

akshay196 · 2023-06-04T11:38:30Z

Regarding apidiff CI check,

   Incompatible changes:
  - GCPManagedControlPlaneSpec: old is comparable, new is not

The struct is comparable if all fields are comparable (go spec). So converting fields like ClusterNetwork to pointer (*ClusterNetwork) would fix it. However DefaultNodeLocation is a slice. Slice types are not comparable.

@richardcase Any idea how to resolve this?

richardcase · 2023-06-05T08:53:11Z

So we don't merge too early, without the controller changes (which will be done in another PR):

/hold

richardcase · 2023-06-05T08:54:10Z

As this is for GKE and it's experimental, we can ignore apidiff changes:

/override pull-cluster-api-provider-gcp-apidiff

k8s-ci-robot · 2023-06-05T08:54:12Z

@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-gcp-apidiff

In response to this:

As this is for GKE and it's experimental, we can ignore apidiff changes:

/override pull-cluster-api-provider-gcp-apidiff

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.

richardcase · 2023-06-05T09:17:46Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+type PrivateCluster struct {
+	// Enabled defines weather a cluster is private or not. If disabled cluster network is public.
+	// +optional
+	Enabled bool `json:"enabled,omitempty"`


Should we split this into nodes and endpoint?

If a user declares privateCluster (which is optional) in their yaml then we can assume they want a private network, so we could add a validation to the webhook to check that one of private nodes or private endpoint is true.

We could also add a validating webhook, that is

Good Idea!

Just to clarify, are you talking about

// ControlPlaneCidrBlock is the IP range in CIDR notation to use for the hosted master network. This range must not // overlap with any other ranges in use within the cluster's network. Honored when enabled is true. // +optional ControlPlaneCidrBlock string `json:"controlPlaneCidrBlock,omitempty"` // ControlPlaneGlobalAccess is whenever master is accessible globally or not. Honored when enabled is true. // +optional ControlPlaneGlobalAccess bool `json:"controlPlaneGlobalAccess,omitempty"`

these two fields in the Node and last two ControlPlanePrivateEndpoint & DisableDefaultSNAT in the Network, if I am not wrong.

@richardcase Sorry I don't quite understand what you mean by splitting into nodes and endpoint. Can you please explain further or give an example?
The private cluster has 4 properties shown in picture out of which ContolPlaneCidrBlock is mandatory. I clubbed them into private cluster group.

I'm talking about EnablePrivateNodes and EnablePrivateEndpoint on the API call: https://cloud.google.com/go/docs/reference/cloud.google.com/go/container/latest/apiv1/containerpb#cloud_google_com_go_container_apiv1_containerpb_PrivateClusterConfig

richardcase · 2023-06-05T09:20:17Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+}
+
+// AuthorizedNetworkCidrBlock defines the CIDR block configuration.
+type AuthorizedNetworkCidrBlock struct {


If someone is specifying a cidr block i'm guessing that at least one of Name or CidrBlock is required.

Yes, adding a MinProperties make sense right?

+ // +kubebuilder:validation:MinProperties=1 // +kubebuilder:validation:MaxProperties=10 // +optional CidrBlock map[string]string `json:"cidrBlock,omitempty"`

GKE allows enabling authorized network with no cidr blocks. The spec with enable and empty cidr block would just behave same as this.

Thanks @akshay196 👍

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

richardcase · 2023-06-05T09:22:27Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+
+	// CidrBlocks defines the CIDR block configuration.
+	// +optional
+	CidrBlocks AuthorizedNetworkCidrBlock `json:"cidrBlocks,omitempty"`


If we have an authorized network then i assume its required to specify cidr blocks?

The CIDR block is independent in itself, there are multiple CIDR blocks in the same file that's why I prefixed it. Should I change it to CidrBlock?

richardcase · 2023-06-05T09:24:14Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+	// (in CIDR notation) within a network range, a mask, or leave this field blank to use a default range.
+	// This setting is permanent.
+	// +optional
+	CidrBlock string `json:"cidrBlock,omitempty"`


How does this differ from the pod cidrblock on the CAPI Cluster?

In the spreadsheet it is string and in the CAPI it is []string, does it count the diff? or using the CAPI one make more sense?

Note that GKE allows defining only single pod address range.

People may get confused if they can supply the pod cidr block on the CAPI Cluster and here.

I wonder if you just take the first if there are multiple and warn the user?

richardcase · 2023-06-05T09:32:22Z

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

+}
+
+// AddonsConfig defines the enabled Cluster Addons.
+type AddonsConfig struct {


Do we need to consider the other options:

https://cloud.google.com/go/docs/reference/cloud.google.com/go/container/latest/apiv1/containerpb#cloud_google_com_go_container_apiv1_containerpb_WorkloadIdentityConfig

Sorry, I don't understand this :(

Sorry i pasted the wrong link, i should've pasted this: https://cloud.google.com/go/docs/reference/cloud.google.com/go/container/latest/apiv1/containerpb#cloud_google_com_go_container_apiv1_containerpb_AddonsConfig

exp/api/v1beta1/gcpmanagedcontrolplane_types.go

Signed-off-by: Aniruddha Basak <[email protected]>

k8s-ci-robot · 2023-06-05T15:39:23Z

@aniruddha2000: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-gcp-apidiff	`40ee48f`	link	false	`/test pull-cluster-api-provider-gcp-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

richardcase · 2023-06-09T05:31:10Z

This is great work 👍

k8s-ci-robot · 2023-09-02T00:35:30Z

PR needs rebase.

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.

akshay196 · 2023-11-28T12:39:12Z

Hey @aniruddha2000 Hope you're doing good.
Are you still working on this PR? Me and @richardcase had a discussion while ago. It would be nice if we add new fields and also work on reconciliation logic of those fields in the same PR.

aniruddha2000 · 2023-11-29T11:38:31Z

@akshay196 hello, yes I am doing good, can you take this over from now if you have the bandwidth? because I don't have many windows to focus on it right now :(

k8s-triage-robot · 2024-02-27T12:02:27Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

akshay196 · 2024-03-20T08:02:13Z

New PR: #1186
We may close this.

/close

k8s-ci-robot · 2024-03-20T08:02:17Z

@akshay196: Closed this PR.

In response to this:

New PR: #1186
We may close this.

/close

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.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 31, 2023

k8s-ci-robot requested a review from richardcase May 31, 2023 04:26

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 31, 2023

k8s-ci-robot requested a review from cpanato May 31, 2023 04:26

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2023

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

aniruddha2000 mentioned this pull request May 31, 2023

Support additional fields for GKE #906

Closed

aniruddha2000 force-pushed the ani/add-gcpmanagedcontrolplane-additional-type branch from 497c7f0 to e19a9b6 Compare May 31, 2023 06:12

aniruddha2000 force-pushed the ani/add-gcpmanagedcontrolplane-additional-type branch from e19a9b6 to 08bc033 Compare May 31, 2023 06:37

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2023

akshay196 suggested changes Jun 4, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2023

richardcase reviewed Jun 5, 2023

View reviewed changes

Add generated CRD in the config

db54842

Signed-off-by: Aniruddha Basak <[email protected]>

aniruddha2000 force-pushed the ani/add-gcpmanagedcontrolplane-additional-type branch from 08bc033 to db54842 Compare June 5, 2023 10:14

aniruddha2000 added 5 commits June 5, 2023 16:27

Remove enable fields for optional struct

65f7639

Signed-off-by: Aniruddha Basak <[email protected]>

Add individual fields to the Logging & Monitoring

6a66012

Signed-off-by: Aniruddha Basak <[email protected]>

Remove enable field from authorized network

2575cf4

Signed-off-by: Aniruddha Basak <[email protected]>

Remove Enable from LoggingConfig & MonitoringConfig

87d767d

Signed-off-by: Aniruddha Basak <[email protected]>

Update crd and generated file

40ee48f

Signed-off-by: Aniruddha Basak <[email protected]>

akshay196-rafay mentioned this pull request Aug 17, 2023

Add additional fields to GCPManagedMachinePool #944

Merged

2 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2023

richardcase mentioned this pull request Oct 26, 2023

GCPManagedControlPlane changes to support various GKE features #1024

Closed

3 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 27, 2024

k8s-ci-robot closed this Mar 20, 2024

Add fields to the GCPManagedControlPlane types #936

Add fields to the GCPManagedControlPlane types #936

Uh oh!

Conversation

aniruddha2000 commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 31, 2023

Uh oh!

k8s-ci-robot commented May 31, 2023

Uh oh!

akshay196 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 4, 2023

Uh oh!

akshay196 commented Jun 4, 2023

Uh oh!

richardcase commented Jun 5, 2023

Uh oh!

richardcase commented Jun 5, 2023

Uh oh!

k8s-ci-robot commented Jun 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardcase commented Jun 9, 2023

Uh oh!

k8s-ci-robot commented Sep 2, 2023

Uh oh!

akshay196 commented Nov 28, 2023

Uh oh!

aniruddha2000 commented Nov 29, 2023

Uh oh!

aniruddha2000 commented May 31, 2023 •

edited

Loading

akshay196 left a comment •

edited

Loading

k8s-ci-robot commented Jun 5, 2023 •

edited

Loading