Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify kube-apiserver patches #24178

Merged
merged 3 commits into from Dec 4, 2019
Merged

Simplify kube-apiserver patches #24178

merged 3 commits into from Dec 4, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Nov 20, 2019

use the config poststarthooks to further simplify the kube-apiserver patches.

/assign @p0lyn0mial

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 20, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Nov 20, 2019

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2019
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 20, 2019
@mfojtik
Copy link
Member

mfojtik commented Nov 21, 2019

/retest

@mfojtik
Copy link
Member

mfojtik commented Nov 21, 2019

The gcp failure was interesting:

I1120 22:38:53.916977       1 log.go:172] http2: panic serving [::1]:53440: open /etc/kubernetes/secrets/etcd-client.crt: no such file or directory
goroutine 455847 [running]:
github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/server/filters.(*timeoutHandler).ServeHTTP.func1.1(0xc01652c1e0)

@stlaz
Copy link
Member

stlaz commented Nov 22, 2019

As for the above error, it seems that the goroutine is being restarted in a loop which causes about 21k lines of panics in the bootstrap-control-plane kube-apiserver logs

sttts
sttts reviewed Nov 25, 2019
View reviewed changes
vendor/k8s.io/kubernetes/cmd/kube-apiserver/app/aggregator.go Outdated
@@ -104,6 +104,8 @@ func createAggregatorConfig(
EnableAggregatedDiscoveryTimeout: utilfeature.DefaultFeatureGate.Enabled(kubefeatures.EnableAggregatedDiscoveryTimeout),
},
}
// we need to clear the poststarthooks so we don't add them multiple times to all the servers (that fails)
aggregatorConfig.GenericConfig.PostStartHooks = map[string]genericapiserver.PostStartHookConfigEntry{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an upstream PR already? Please rename the commit.

sttts
sttts reviewed Nov 25, 2019
View reviewed changes
vendor/k8s.io/kubernetes/openshift-kube-apiserver/openshiftkubeapiserver/patch_handlerchain.go Outdated
if err != nil {
return nil, err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange to have file loading in a handler chain builder. Why not pass in a []byte?

sttts
sttts reviewed Nov 25, 2019
View reviewed changes
vendor/k8s.io/kubernetes/cmd/kube-apiserver/app/server.go Outdated
@@ -30,7 +30,8 @@ import (
"strings"
"time"

"k8s.io/kubernetes/openshift-kube-apiserver/configdefault"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/admissionenablement"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no empty line

@deads2k deads2k force-pushed the simplify-more branch 2 times, most recently from 3d45015 to cbc317a Compare November 26, 2019 14:51
@deads2k
Copy link
Contributor Author

deads2k commented Nov 26, 2019

/retest

@deads2k deads2k force-pushed the simplify-more branch 2 times, most recently from 3539297 to 51fc2c7 Compare November 27, 2019 12:47
stlaz
stlaz reviewed Nov 27, 2019
View reviewed changes
Copy link
Member

@stlaz stlaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bunch of nits, othwerwise looks good to me

...k8s.io/kubernetes/openshift-kube-apiserver/admission/admissionenablement/admission_config.go

import (
"time"

"github.com/openshift/library-go/pkg/apiserver/admission/admissiontimeout"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this should have stayed separate

vendor/k8s.io/kubernetes/pkg/kubeapiserver/authenticator/patch_authenticator.go
Comment on lines +7 to +22
oauthclient "github.com/openshift/client-go/oauth/clientset/versioned"
oauthinformer "github.com/openshift/client-go/oauth/informers/externalversions"
userclient "github.com/openshift/client-go/user/clientset/versioned"
userinformer "github.com/openshift/client-go/user/informers/externalversions"
bootstrap "github.com/openshift/library-go/pkg/authentication/bootstrapauthenticator"
"k8s.io/apiserver/pkg/authentication/authenticator"
"k8s.io/apiserver/pkg/authentication/group"
genericapiserver "k8s.io/apiserver/pkg/server"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/util/keyutil"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/authorization/restrictusers/usercache"
oauthvalidation "k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/oauth"
"k8s.io/kubernetes/openshift-kube-apiserver/authentication/oauth"
"k8s.io/kubernetes/openshift-kube-apiserver/enablement"
"k8s.io/kubernetes/pkg/serviceaccount"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: split imports into openshift/kube groups

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: split imports into openshift/kube groups

this is our file. I really see no benefit to keeping them separate now that we have auto-importing capability (didn't exist 5 years ago)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's a good habit to keep separate import contexts visibly separate, like syslibs/imports from elsewhere/imports from us

vendor/k8s.io/kubernetes/pkg/kubeapiserver/authenticator/patch_authenticator.go Outdated
panic(err)
}

if enablement.IsOpenShift() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do any of the above if this is false, do we?

@stlaz
Copy link
Member

stlaz commented Nov 27, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@stlaz
Copy link
Member

stlaz commented Nov 29, 2019

/hold

Kube request header auth was put higher in the eval stack than the token auth, which causes the OAuth-server has the correct token and certificate fallback semantics to fail.

We probably still want our token auth to precede kube req headers?

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 29, 2019
@deads2k deads2k removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019
@stlaz
Copy link
Member

stlaz commented Dec 3, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Dec 3, 2019

/hold

We have a fun pickle regarding --token now. I need to think about this.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Dec 3, 2019

added release note openshift/openshift-docs#18426 after clearing it with Clayton.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019
@stlaz
Copy link
Member

stlaz commented Dec 4, 2019

/lgtm
The release note looks good and we need this change for further work.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, stlaz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit a2333a6 into openshift:master Dec 4, 2019
deads2k added a commit to deads2k/origin that referenced this pull request Dec 13, 2019
@deads2k
Revert "Merge pull request openshift#24178 from deads2k/simplify-more"
aac186c 
This reverts commit a2333a6, reversing
changes made to 7cca2df.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stlaz stlaz stlaz left review comments

@sttts sttts sttts left review comments

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@stlaz stlaz

@p0lyn0mial p0lyn0mial

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@deads2k @mfojtik @stlaz @openshift-bot @openshift-ci-robot @sttts @p0lyn0mial @openshift-merge-robot