Use NodeAuthorizer config options instead of soely hard-codes #7211
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @jacksontj. Thanks for your PR. I'm waiting for a kubernetes 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
@jacksontj thank you for the PR! There's a few small updates that will need doing before this can move through:
- Please can you additionally update the files
pkg/apis/kops/v1alpha1/cluster.goandpkg/apis/kops/v1alpha2/cluster.gowith theIntervalinclusion - Run
make apimachinery
cc @gambol99
k8s-ci-robot
added
size/M
|
@KashifSaadat thanks, I have updated the PR -- hopefully it all passes :) |
|
Unfortunately all the e2e tests are still blocked on kubernetes/kubernetes#79305 -- it made an error fatal which every test run hits (details in slack ) |
|
Tests should be sorted now, although there's a few conflicts, once they're sorted I'll LG and we can get this merged :) |
Without this patch the timeout is 5m and the interval is 10m -- hard-coded with no mechanism to change, even though the config struct already had a timeout option (which was completely unused)
jacksontj
force-pushed
the
node_authorizer_options
branch
from
2f2b71a to
0a4b18e
Compare
|
@KashifSaadat all set (rebased and tests are passing)! |
| @@ -189,6 +189,8 @@ type NodeAuthorizerSpec struct { | |||
| NodeURL string `json:"nodeURL,omitempty"` | |||
| // Port is the port the service is running on the master | |||
| Port int `json:"port,omitempty"` | |||
| // Interval the time between retires for authorization request | |||
| @@ -188,6 +188,8 @@ type NodeAuthorizerSpec struct { | |||
| NodeURL string `json:"nodeURL,omitempty"` | |||
| // Port is the port the service is running on the master | |||
| Port int `json:"port,omitempty"` | |||
| // Interval the time between retires for authorization request | |||
| @@ -189,6 +189,8 @@ type NodeAuthorizerSpec struct { | |||
| NodeURL string `json:"nodeURL,omitempty"` | |||
| // Port is the port the service is running on the master | |||
| Port int `json:"port,omitempty"` | |||
| // Interval the time between retires for authorization request | |||
| timeout := 5 * time.Minute | ||
| if na.Timeout != nil { |
There was a problem hiding this comment.
Ah - you're right - it looks like we simply weren't using it! Thanks for fixing!
|
Minor typo (retires instead of retries) but I'm inclined to merge it and we can fix it later. /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jacksontj, justinsb 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
…1-upstream-release-1.12 Automated cherry pick of #7211: Use NodeAuthorizer config options instead of soely
…1-upstream-release-1.14 Automated cherry pick of #7211: Use NodeAuthorizer config options instead of soely
…1-upstream-release-1.13 Automated cherry pick of #7211: Use NodeAuthorizer config options instead of soely
Without this patch the timeout is 5m and the interval is 10m --
hard-coded with no mechanism to change, even though the config struct
already had a timeout option (which was completely unused)