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 go to 1.20 and k8s deps to 0.27.2 #586

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

dgrisonnet
Copy link
Member

@dgrisonnet dgrisonnet commented Jun 5, 2023

This PR bumps the various dependency and add support for OpenAPI v3.

Fixes #581

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 5, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2023
@dgrisonnet
Copy link
Member Author

This requires kubernetes/test-infra#29683.

@booleanbetrayal
Copy link

@dgrisonnet - Built an image off of this branch to test K8s v1.27 compatibility. Seems to work, except I see this regression again: #525

Had to hot-patch the Helm produced manifest from the project to remove the '--logtostderr=true' line it generates.

Possibly broken in a prior commit?

go.mod Show resolved Hide resolved
@dgrisonnet
Copy link
Member Author

dgrisonnet commented Jun 6, 2023

This flag alongside others were removed from Kubernetes components in 1.26 and since prometheus-adapter is reusing code from the kube-apiserver, the same applies here. kubernetes/enhancements#2845

I'll update the manifests in this repo to reflect that, but the helm chart will have to be fixed separately.

@booleanbetrayal
Copy link

This flag alongside others were removed from Kubernetes components in 1.26 and since prometheus-adapter is reusing code from the kube-apiserver, the same applies here. kubernetes/enhancements#2845

I'll update the manifests in this repo to reflect that, but the helm chart will have to be fixed separately.

Thanks for that explanation!

Copy link
Member

@olivierlemasle olivierlemasle left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, olivierlemasle

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

@olivierlemasle
Copy link
Member

/retest

1 similar comment
@olivierlemasle
Copy link
Member

/retest

@justenwalker
Copy link

FWIW I confirmed this change worked for me. I did my own build of the docker images and re-packaged the helm chart (removing the --logtostderr=true flag)

@dgrisonnet
Copy link
Member Author

In this PR's state, it shouldn't work on 1.27. Both when I tried it locally and in CI the pod is crashlooping because I need to update metrics-server to support 1.27 before this can go in.

@logicalhan
Copy link

/triage accepted
/assign

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 15, 2023
Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>
Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2023
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 20, 2023
@dgrisonnet
Copy link
Member Author

#589 is required to support k8s 1.27

Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>
@dgrisonnet
Copy link
Member Author

I added the OpenAPI v3 support to this PR directly since it requires client updates, but they require OpenAPI v3 support for the tests to pass.

@dgrisonnet dgrisonnet force-pushed the k8s-1.27 branch 3 times, most recently from 86e5dcd to a4aa32e Compare June 23, 2023 13:58
@dgrisonnet
Copy link
Member Author

/retest

@justenwalker
Copy link

@dgrisonnet

In this PR's state, it shouldn't work on 1.27. Both when I tried it locally and in CI the pod is crashlooping because I need to update metrics-server to support 1.27 before this can go in.

Not sure why it worked for me then. I'm running v0.6.3 (Helm Chart v3.10.0)

I updated to the latest commit today and it continues to work.

@booleanbetrayal
Copy link

Not sure why it worked for me then. I'm running v0.6.3 (Helm Chart v3.10.0)

I updated to the latest commit today and it continues to work.

Have also been running this branch's changes for the past couple of weeks without crash.

@dgrisonnet
Copy link
Member Author

Now it should be all good, but before my change, if you didn't touch you the OpenAPI v3 feature gate on a 1.27 cluster, it wasn't working properly.

@justenwalker
Copy link

@dgrisonnet

before my change, if you didn't touch you the OpenAPI v3 feature gate on a 1.27 cluster, it wasn't working properly.

I'm deploying on EKS so I do not have access to these feature gates (so they are likely the defaults) - maybe that's why it worked?

@justenwalker
Copy link

@dgrisonnet is there any additional feedback or testing I can provide to validate this PR? Or is this waiting on approval from someone else?

@colinrgodsey
Copy link

also stuck on an EKS 1.27 upgrade here. any new developments here on what's needed to move forward with this PR ?

@dgrisonnet
Copy link
Member Author

I am just waiting for a review from @olivierlemasle if possible

@Aym3nTN
Copy link

Aym3nTN commented Jul 20, 2023

Can @booleanbetrayal review it?

@dgrisonnet
Copy link
Member Author

I'd prefer having someone from kubernetes-sig signing it off, but if it still hasn't made any move next week, I'll merge it and create a new release.

@justenwalker
Copy link

@dgrisonnet seems unlikely we'll get that review from @olivierlemasle ; IMO since we've got some independent tests from myself and @booleanbetrayal and its relatively straight forward my vote would be to merge.

Only one note is that we're currently on Go 1.20.6 so it might be good to make that small tweak just to include the security fixes if its an easy enough change. https://github.com/kubernetes-sigs/prometheus-adapter/pull/586/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R11

@dgrisonnet dgrisonnet merged commit f733e2f into kubernetes-sigs:master Jul 25, 2023
@dgrisonnet dgrisonnet deleted the k8s-1.27 branch July 25, 2023 09:37
@justenwalker
Copy link

@dgrisonnet thanks for closing this out! How does the helm chart for this get built; is that automatic?

The current app version available is v0.10.0

$ helm search hub prometheus-adapter
URL                                                     CHART VERSION   APP VERSION     DESCRIPTION
https://artifacthub.io/packages/helm/prometheus...      4.2.0           v0.10.0         A Helm chart for k8s prometheus adapter
...

@dgrisonnet
Copy link
Member Author

I don't know, the chart is maintained separately.

@colinrgodsey
Copy link

@justenwalker helm chart PR here: prometheus-community/helm-charts#3641

@justenwalker
Copy link

@justenwalker helm chart PR here: prometheus-community/helm-charts#3641

Nice! Thanks @colinrgodsey

@colinrgodsey
Copy link

@justenwalker merged!

$ helm search hub prometheus-adapter
URL                                               	CHART VERSION	APP VERSION	DESCRIPTION                            
https://artifacthub.io/packages/helm/prometheus...	4.3.0        	v0.11.0    	A Helm chart for k8s prometheus adapter

monitoring-commit-bot bot pushed a commit to rhobs/k8s-prometheus-adapter that referenced this pull request Aug 10, 2023
Changes since v0.10.0

Logging flags that are klog specific (`--log-dir`, `--log-file`, `--logtostderr`, `--alsologtostderr`, `--one-output`, `--stderrthreshold`, `--log-file-max-size`, `--skip-log-headers`, `--add-dir-header`, `--skip-headers`, `--log-backtrace-at`) were deprecated in v0.10.0 and are now **removed**.

Improvements

* Bump go to 1.20 and k8s deps to 0.27.2 (kubernetes-sigs#586, @dgrisonnet)
* Add support for OpenAPI v3 (kubernetes-sigs#586, @dgrisonnet)
* Update registry location to registry.k8s.io in the manifests (kubernetes-sigs#529, @dgrisonnet)
* Update manifests in deploy/manifests to be in sync with the one in kube-prometheus (kubernetes-sigs#531, @JoaoBraveCoding)
* Use golangci-lint to lint the codebase (kubernetes-sigs#540, @olivierlemasle)
* Set MinVersion: tls.VersionTLS12 in prometheus client's TLSClientConfig (kubernetes-sigs#544, @olivierlemasle)
* Refactor adding logging flags (kubernetes-sigs#546, @olivierlemasle)

Bug fixes

* Fix segfault when using --prometheus-token-file (kubernetes-sigs#538, @olivierlemasle)

Documentation

* Fix broken links in README.md (kubernetes-sigs#518, @sillyfrog)
* Fix yaml in sample config & docs (kubernetes-sigs#559, @asherf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaks v1.27.x clusters
8 participants