Webhook checking for KubeFedConfig #941
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Please rebase. |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
rebase done and basic test passed. I'm adding test now |
xunpan
force-pushed
the
webhook
branch
4 times, most recently
from
d4dea93 to
5cb33ac
Compare
xunpan
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
CI failure looks like a potential flake. |
|
Not sure details but make CI passed by adding |
|
webhook reject scope is empty case. This is because we had no default value setting in webhook and the old default setting does not set value for add scope can make it work. |
charts/kubefed/charts/controllermanager/templates/clusterrolebindings.yaml
Show resolved
Hide resolved
| } | ||
| if elect.RenewDeadline.Duration <= time.Duration(float64(elect.RetryPeriod.Duration)*leaderelection.JitterFactor) { | ||
| allErrs = append(allErrs, field.Invalid(electPath.Child("renewDeadline"), elect.RenewDeadline, | ||
| "renewDeadline must be greater than retryPeriod*JitterFactor")) |
There was a problem hiding this comment.
Please include the jitter factor in the message.
Also, is there a reason the jitter factor isn't configurable?
cc: @shashidharatd
There was a problem hiding this comment.
Seems JitterFactor is agreed upon constant https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go#L72 and is not exposed by the leader election library. lets keep it like that for now.
There was a problem hiding this comment.
Ok, so it should remain non-configurable. Is there a reason it should not be exposed in the error message though? I think the value should either be explicitly mentioned in the error message or mention of JitterFactor removed.
|
|
||
| gates := spec.FeatureGates | ||
| gatesPath := specPath.Child("featureGates") | ||
| for _, gate := range gates { |
There was a problem hiding this comment.
Please ensure that a gate appears at most once.
move clusterrole to the corresponding yaml file
|
Done:
|
There was a problem hiding this comment.
Functionally this looks fine, and I'm going to merge it. Please ensure that variable names in the unit test reflect their contents (i.e. if a variable is going to be configured to contain an invalid value, it should be prefixed with invalid rather than valid).
/approve
/lgtm
| validElectorResourceLock.Spec.LeaderElect.ResourceLock = "NeitherConfigmapsOrEndpoints" | ||
| errorCases["spec.leaderElect.resourceLock: Unsupported value"] = validElectorResourceLock | ||
|
|
||
| validFeatureGateName := validKubeFedConfig() |
There was a problem hiding this comment.
This is a bad feature gate name.
|
|
||
| errorCases := map[string]*v1beta1.KubeFedConfig{} | ||
|
|
||
| validScope := validKubeFedConfig() |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marun, xunpan 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 |
k8s-ci-robot
added
the
approved
This is not ready to review until #914 is merged and I rebase the code with UT developed.
I also fix a minor non-function helm chart related issue: make clusterrole in clusterrole.yaml. It is not functional change, so I use a single commit instead of a new PR.
Fixes #916