Update KubeFedConfig API fields to be optional #966

font · 2019-06-04T18:39:12Z

For fields we plan to provide a default such that we won't require the user to specify them, we need to make them optional in order to pass kubectl client side validation. CRD client side validation will be enabled by default in k8s v1.15. See https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#publish-validation-schema-in-openapi-v2 for more details.

Additionally, optional fields are made pointers. This is to distinguish the user not specifying a value (since it's not required), from the user specifying an invalid value of 0 (for Duration and time) or "" for the empty string (should produce a validation error). It also makes serialized objects cleaner.

k8s-ci-robot · 2019-06-04T18:39:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: font

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~~ [font]

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

marun · 2019-06-05T17:04:08Z

pkg/apis/core/v1beta1/kubefedconfig_types.go

@@ -80,19 +91,24 @@ const (

 type ClusterHealthCheckConfig struct {
 	// How often to monitor the cluster health (in seconds).
-	PeriodSeconds int64 `json:"periodSeconds"`


Not the fault of this PR, but why should time in the health check config be specified as an integer representing seconds but in the leader election config as a duration? This seems inconsistent to me.

cc: @shashidharatd

yes, there is inconsistency. it was thought like if we explicitly put unit in the option/flag name itself there will be no confusion while providing the config. Otherwise user has to explicitly provide something like "10s" to specify 10 seconds. If user misses the "s" it is still a valid configuration (in nanoseconds) which leads to undesired value getting configured.

Fair enough. Why, though, is this important for cluster health check but not leader election? And which is more consistent with user expectations?

Mostly, it is like that because leader election module uses the Duration type and if we change them to int we have to convert and pass the parameters https://github.com/kubernetes-sigs/kubefed/blob/master/cmd/controller-manager/app/leaderelection/leaderelection.go#L72

Leader election module is used by multiple components and i think whoever configures them might already be aware of such an issue :) Now we need to make a choice ;)

If leader election configuration uses duration across components, is there a reason we shouldn't be consistent and also use duration for leader election? And if we use duration for that, why wouldn't we also use duration for the health check config?

I'm +1 on using duration and removing the unit from the name. I'm less clear on why we'd need to validate that the value was in seconds, is that a CRD thing? Whatever duration is provided could be converted to seconds.

It's not a CRD thing. It's more of a granularity thing. I figured the reason for requiring seconds is to allow a sane amount of interval delay granularity between cluster health checks as anything less than 1 second may not be reasonable.

I think it's entirely dependent on context. Maybe partial seconds (e.g. 1.5) would be useful in some circumstances? I think setting sane defaults and documenting how and why to change those defaults is a reasonable alternative to strict validation.

I have no issues with being less opinionated about the level of granularity we allow. We can always enforce a certain level in validation. I think this change should be done in a follow-up PR though since this PR focuses on optional fields.

Please file an issue in the milestone that tracks the transition to use duration for all time fields.

marun · 2019-06-07T15:06:40Z

/lgtm

marun · 2019-06-07T15:44:35Z

@font It looks like merging this broke master due to linting errors:

+echo Linting
Linting
+golangci-lint run
WARN [runner/megacheck] Can't run megacheck because of compilation errors in packages [sigs.k8s.io/kubefed/pkg/controller/webhook sigs.k8s.io/kubefed/pkg/controller/webhook/federatedtypeconfig sigs.k8s.io/kubefed/pkg/controller/webhook/kubefedconfig sigs.k8s.io/kubefed/pkg/webhook sigs.k8s.io/kubefed/pkg/apis/core/v1beta1/validation [sigs.k8s.io/kubefed/pkg/apis/core/v1beta1/validation.test]]: pkg/apis/core/v1beta1/validation/validation.go:174: cannot convert elect.ResourceLock (variable of type *v1beta1.ResourceLockType) to string and 32 more errors: run `golangci-lint run --no-config --disable-all -E typecheck` to see all errors 
pkg/apis/core/v1beta1/validation/validation.go:174:88: cannot convert elect.ResourceLock (variable of type *v1beta1.ResourceLockType) to string (typecheck)
	allErrs = append(allErrs, validateEnumStrings(electPath.Child("resourceLock"), string(elect.ResourceLock),
	                                                                                      ^
pkg/apis/core/v1beta1/validation/validation.go:198:84: cannot use health.PeriodSeconds (variable of type *int64) as int64 value in argument to validateGreaterThan0 (typecheck)
	allErrs = append(allErrs, validateGreaterThan0(healthPath.Child("periodSeconds"), health.PeriodSeconds)...)
	                                                                                  ^
pkg/apis/core/v1beta1/validation/validation.go:199:87: cannot use health.FailureThreshold (variable of type *int64) as int64 value in argument to validateGreaterThan0 (typecheck)
	allErrs = append(allErrs, validateGreaterThan0(healthPath.Child("failureThreshold"), health.FailureThreshold)...)
	                                                                                     ^
pkg/apis/core/v1beta1/validation/validation_test.go:341:59: cannot convert "NeitherConfigmapsOrEndpoints" (untyped string constant) to *v1beta1.ResourceLockType (typecheck)
	validElectorResourceLock.Spec.LeaderElect.ResourceLock = "NeitherConfigmapsOrEndpoints"
	                                                         ^
pkg/apis/core/v1beta1/validation/validation_test.go:361:73: cannot convert 0 (untyped int constant) to *int64 (typecheck)
	validPeriodSecondsGreaterThan0.Spec.ClusterHealthCheck.PeriodSeconds = 0
	                                                                       ^
pkg/apis/core/v1beta1/validation/validation_test.go:377:59: cannot convert "NeitherEnableOrDisable" (untyped string constant) to *v1beta1.ResourceAdoption (typecheck)
	validAdoptResources.Spec.SyncController.AdoptResources = "NeitherEnableOrDisable"

I'm very much confused as to how CI managed to pass for this PR.

marun · 2019-06-07T16:03:35Z

Lint errors are addressed in #973

Fix lint errors introduced by #966 and #941 merging without rebase

font added 3 commits June 4, 2019 08:28

Make most KubeFedConfig API fields optional

811d440

Code changes for KubeFedConfig API field changes

f5c5ee8

Update generated code

b2771b3

font requested review from pmorie, marun, shashidharatd, irfanurrehman and xunpan June 4, 2019 18:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 4, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2019

k8s-ci-robot requested a review from gyliu513 June 4, 2019 18:39

marun reviewed Jun 5, 2019

View reviewed changes

abhat mentioned this pull request Jun 6, 2019

Create default FeatureGateConfig for KubeFedConfig #971

Closed

k8s-ci-robot assigned marun Jun 7, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2019

k8s-ci-robot merged commit 1edbf56 into kubernetes-retired:master Jun 7, 2019

k8s-ci-robot added a commit that referenced this pull request Jun 7, 2019

Merge pull request #973 from marun/fix-lint-errors

34ba2a0

Fix lint errors introduced by #966 and #941 merging without rebase

font mentioned this pull request Jun 8, 2019

Update KubeFedConfig API time fields #975

Closed

marun mentioned this pull request Jun 9, 2019

Fix KubeFedConfig validation of pointer fields #976

Merged

font deleted the kc-api branch June 13, 2019 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update KubeFedConfig API fields to be optional #966

Update KubeFedConfig API fields to be optional #966

font commented Jun 4, 2019

k8s-ci-robot commented Jun 4, 2019

marun Jun 5, 2019

shashidharatd Jun 6, 2019

marun Jun 6, 2019

shashidharatd Jun 6, 2019

marun Jun 6, 2019 •

edited

Loading

marun Jun 6, 2019

font Jun 6, 2019

marun Jun 6, 2019

font Jun 7, 2019

marun Jun 7, 2019

marun commented Jun 7, 2019

marun commented Jun 7, 2019

marun commented Jun 7, 2019

Update KubeFedConfig API fields to be optional #966

Update KubeFedConfig API fields to be optional #966

Conversation

font commented Jun 4, 2019

k8s-ci-robot commented Jun 4, 2019

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

marun Jun 6, 2019 • 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

Choose a reason for hiding this comment

marun commented Jun 7, 2019

marun commented Jun 7, 2019

marun commented Jun 7, 2019

marun Jun 6, 2019 •

edited

Loading