Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Kubernetes 1.10.0 #1876

Merged
merged 11 commits into from
Apr 19, 2018
Merged

Kubernetes 1.10.0 #1876

merged 11 commits into from
Apr 19, 2018

Conversation

nilebox
Copy link
Contributor

@nilebox nilebox commented Mar 27, 2018

As usual, review individual commits and skip "Update vendor" commits.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 27, 2018
@nilebox nilebox changed the title [WIP] Kubernetes 1.10.0 Kubernetes 1.10.0 Apr 9, 2018
@nilebox
Copy link
Contributor Author

nilebox commented Apr 9, 2018

All tests are passing now, but the helm chart install times out. Probably some flags were changed and we need to update a helm chart, will check.

@nilebox
Copy link
Contributor Author

nilebox commented Apr 10, 2018

Had to copy-paste some code from(a *AdmissionOptions) ApplyTo since the admission initialization has changed and it was blowing up.
We should switch to RecommendedOptions as a follow-up to reduce copy-paste forked code.

@nilebox
Copy link
Contributor Author

nilebox commented Apr 10, 2018

Temporarily disabled webhook admission plugins to localize the issue.

@nilebox
Copy link
Contributor Author

nilebox commented Apr 10, 2018

The build succeeds now, but need to look whether disabling default admission plugins causes a regression (i.e. missing support for initializer or something).
Once there is no regression, we can fix additional admission controllers support in the follow-up PR.

@nilebox
Copy link
Contributor Author

nilebox commented Apr 10, 2018

The issue with webhook admission controllers is caused by RBAC permissions:

I0410 01:46:46.875133       1 request.go:874] Response Body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"validatingwebhookconfigurations.admissionregistration.k8s.io is forbidden: User \"system:serviceaccount:catalog:service-catalog-apiserver\" cannot list validatingwebhookconfigurations.admissionregistration.k8s.io at the cluster scope","reason":"Forbidden","details":{"group":"admissionregistration.k8s.io","kind":"validatingwebhookconfigurations"},"code":403}
E0410 01:46:46.875202       1 reflector.go:205] github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/client-go/informers/factory.go:87: Failed to list *v1beta1.ValidatingWebhookConfiguration: validatingwebhookconfigurations.admissionregistration.k8s.io is forbidden: User "system:serviceaccount:catalog:service-catalog-apiserver" cannot list validatingwebhookconfigurations.admissionregistration.k8s.io at the cluster scope

We need to grant additional permissions to service-catalog-apiserver.

@nilebox
Copy link
Contributor Author

nilebox commented Apr 10, 2018

Travis build times out (the limit is 50 minutes). We might have to fix the problem with t.Parallel() I suppose... (see #1848 (comment))

@MHBauer
Copy link
Contributor

MHBauer commented Apr 16, 2018

I'll give it a rebase on the commit with faster clients. #1929

@MHBauer
Copy link
Contributor

MHBauer commented Apr 16, 2018

well, the rebase made it faster, but it's now 47 minutes.

@n3wscott
Copy link
Contributor

LGTM, thanks for doing the work

@n3wscott n3wscott added the LGTM1 label Apr 16, 2018
Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

LGTM, I think we should merge and do any other changes in follow ups, such as 1.10.1.

@@ -138,6 +138,7 @@ node {
}
} catch (Exception e) {
echo 'Run failed.'
print e
Copy link
Contributor

Choose a reason for hiding this comment

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

still need this?

verbs: ["get", "list", "watch"]
- apiGroups: ["admissionregistration.k8s.io"]
resources: ["mutatingwebhookconfigurations"]
verbs: ["get", "list", "watch"]
Copy link
Contributor

Choose a reason for hiding this comment

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

yay rbac

@@ -161,20 +164,46 @@ func buildGenericConfig(s *ServiceCatalogServerOptions) (*genericapiserver.Recom
}

// buildAdmission constructs the admission chain
func buildAdmission(s *ServiceCatalogServerOptions,
// TODO nilebox: Switch to RecommendedOptions and use method (a *AdmissionOptions) ApplyTo
Copy link
Contributor

Choose a reason for hiding this comment

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

follow up to solve #1851, methinks

// enabledPluginNames makes use of RecommendedPluginOrder, DefaultOffPlugins,
// EnablePlugins, DisablePlugins fields
// to prepare a list of ordered plugin names that are enabled.
// TODO nilebox: remove this method once switched to RecommendedOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

same #1861 I think

// methods were removed.
// Remove the resource loop after we make sure that we don't have
// overrides for resources in our config.
return nil, fmt.Errorf("resource overrides are not supported anymore: %v", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

good error, no idea what any of this code is for.

@@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// Code generated by client-gen. DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure anyone would actually see these comments, but code gen, whatever.

@@ -39,7 +41,7 @@ func init() {
//
// import (
// "k8s.io/client-go/kubernetes"
// clientsetscheme "k8s.io/client-go/kuberentes/scheme"
// clientsetscheme "k8s.io/client-go/kubernetes/scheme"
Copy link
Contributor

Choose a reason for hiding this comment

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

wut

@@ -14,11 +14,13 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// This file was automatically generated by informer-gen
// Code generated by informer-gen. DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

at least it's consistent now?

@MHBauer MHBauer added the LGTM2 label Apr 17, 2018
@MHBauer
Copy link
Contributor

MHBauer commented Apr 19, 2018

rebased to pick up fixes in #1951

@MHBauer MHBauer merged commit 2c22fc1 into kubernetes-retired:master Apr 19, 2018
@carolynvs carolynvs added this to the 1.0.0 milestone May 21, 2018
@ash2k ash2k deleted the k8s-1.10 branch January 21, 2019 03:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants