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.

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

Bump controller-tools #2497

Merged
merged 19 commits into from
Mar 2, 2020
Merged

Bump controller-tools #2497

merged 19 commits into from
Mar 2, 2020

Conversation

anyasabo
Copy link
Contributor

Fix #2490

Controller tools 0.2.5 includes k8s 1.17 deps, and also includes some of the new openapi properties to support new CRD features. This causes a few issues:

  • x-kubernetes-int-or-string seems to cause the 1.15 api server to interpret the schema as structural, and so is sad that there is no type: object property (which we intentionally remove for Openshift 3.11 support). Removing the x-kubernetes-int-or-string property allows the API server to accept the manifests on both 1.11 and 1.15, though kubectl validation fails so you need to pass it --validation=false.

  • Kubectl validation fails because there are other new properties that are added (x-kubernetes-list-type and x-kubernetes-list-map-keys). I removed these properties too to make applying the CRDs easier, even though they are not strictly required by the API server.

I believe these properties will be required in k8s 1.19 when v1beta1 CRD support is removed and structural schema is required, but that is a bit away. All of this stems from supporting 1.11 which has a bug that requires us to remove type:object from the root. Once we stop supporting 1.11 this all becomes much simpler.

Other options than the approach in this PR that may be worthwhile:

  • Add a flag to crd-gen to disable adding these markers, though since it is only an issue for <=1.11 support I am not sure how much support it would get
  • Pin an older version of controller-tools (looks like 0.2.2 is the latest we could use just browsing release notes)

Also added the namespace to some unit tests that were passing but don't seem like they should have been in older versions (and were failing now).

It's also not clear to me if we should target v1.0.1 with this.

@anyasabo anyasabo added >enhancement Enhancement of existing functionality v1.1.0 labels Jan 31, 2020
@@ -36,11 +36,11 @@ func Test_podsToUpgrade(t *testing.T) {
args: args{
statefulSets: sset.StatefulSetList{
sset.TestSset{
Name: "masters", Replicas: 2, Master: true,
Name: "masters", Namespace: TestEsNamespace, Replicas: 2, Master: true,
Copy link
Contributor

@sebgl sebgl Feb 3, 2020

Choose a reason for hiding this comment

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

I'm surprised to see that in the PR.
Does it fail if not set, or is it completely unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was surprising to me that this ever passed. The NS was added in
9d3aaa8 so if you check out any of the ones before that you should be able to see.

@sebgl
Copy link
Contributor

sebgl commented Feb 4, 2020

We may want to wait for kubernetes-sigs/controller-tools#392 to be resolved first?

@anyasabo
Copy link
Contributor Author

anyasabo commented Feb 4, 2020

Ah I missed that issue somehow, yes that would definitely make the kustomize part of this PR simpler. I could see this going either way though -- we could merge this as is then use the flag once it is available, or we could just close this and wait for the flag.

Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review decision.
I think I'm ok with including these changes and following the most up-to-date controller-tools, as long as we clearly reference controller-tools bugs in our patches.

config/crds/patches/apm-kibana-patches.yaml Outdated Show resolved Hide resolved
@charith-elastic
Copy link
Contributor

Jenkins test this please

@anyasabo
Copy link
Contributor Author

This is pending #2611 as there are some oddities in the license generator on OSX vs Linux that should be resolved in that PR. Right now the CI check is failing for the notice file

@anyasabo anyasabo merged commit 80e3a62 into elastic:master Mar 2, 2020
@anyasabo anyasabo deleted the bumpctrltools branch March 2, 2020 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v1.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump controller-tools to 0.2.5
3 participants