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

fix(vendor/scoped): bump k8s version to 1.24, go version to 1.18 and fix scoped client #2794

Merged
merged 5 commits into from
Jun 17, 2022
Merged

fix(vendor/scoped): bump k8s version to 1.24, go version to 1.18 and fix scoped client #2794

merged 5 commits into from
Jun 17, 2022

Conversation

dinhxuanvu
Copy link
Member

@dinhxuanvu dinhxuanvu commented Jun 8, 2022 •
edited
Loading

Signed-off-by: Vu Dinh [email protected]

Description of the change:

  • Update k8s version to the latest 1.24
  • Update go version to latest minor version 1.18
  • Fix scoped client to handle token secret under new k8s 1.24 changes

Motivation for the change:

Architectural changes:

Testing remarks:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

Closes #2792
Closes #2762

@openshift-ci openshift-ci bot requested review from benluddy and perdasilva June 8, 2022 14:30
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2022
timflannagan
timflannagan reviewed Jun 8, 2022
View reviewed changes
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

Bump the GHA workflow's go-version in the checkout action too?

@dinhxuanvu
Copy link
Member Author

Bump the GHA workflow's go-version in the checkout action too?

Will do

@dinhxuanvu dinhxuanvu force-pushed the k8s-1.24 branch 6 times, most recently from 4cd6a15 to 7e1fdfe Compare June 8, 2022 20:16
@timflannagan timflannagan force-pushed the k8s-1.24 branch 2 times, most recently from 6594913 to 3490f9b Compare June 9, 2022 15:01
@timflannagan
Copy link
Member

Those (upcoming) e2e failures are known issues. The latest k8s minor version release has changed to SA's generating secret tokens, and OLM relies on 1.) that secret being generated and 2.) parsing that token's value.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2022
@dinhxuanvu dinhxuanvu changed the title fix(vendor): bump k8s version to 1.24 and go version to 1.18 fix(vendor/scoped): bump k8s version to 1.24, go version to 1.18 and fix scoped client Jun 13, 2022
@dinhxuanvu
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2022
@dinhxuanvu
Copy link
Member Author

/retest

@perdasilva
Copy link
Collaborator

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jun 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, perdasilva

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:
  • OWNERS [dinhxuanvu,perdasilva]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dinhxuanvu and others added 4 commits June 17, 2022 10:11
@dinhxuanvu
fix(vendor): bump k8s version to 1.24 and go version to 1.18
21edf69 
Signed-off-by: Vu Dinh <[email protected]>
@dinhxuanvu
Update clock package from apimachinery to utils
289833e 
The apimachinery/pkg/util/clock is deprecated and all clock utils is
on k8s.io/utils/clock repo instead.

Signed-off-by: Vu Dinh <[email protected]>
@dinhxuanvu @timflannagan
fix(go.mod): pin opentelemetry packages into specific versions
b14f97d 
Signed-off-by: Vu Dinh <[email protected]>
Co-authored-by: timflannagan <[email protected]>
@dinhxuanvu
fix(scoped): update scoped client library to handle token secret
03758a9 
In k8s 1.24, token secret is no longer referenced in ServiceAccount.
By listing all secrets in the namespace and then filter them with
SA name via kubernetes.io/service-account.name annotation, the
token secret can be retrieved successfully.

Signed-off-by: Vu Dinh <[email protected]>
@dinhxuanvu
fix(e2e): Fix several RBAC-related e2e test cases
2dee09d 
Create token secret for ServiceAccount to ensure those SA
is valid for scoped client use.

Signed-off-by: Vu Dinh <[email protected]>
timflannagan
timflannagan reviewed Jun 17, 2022
View reviewed changes
pkg/lib/scoped/token_retriever.go
Comment on lines +59 to +64
for _, ref := range sa.Secrets {
if _, ok := secrets[ref.Name]; !ok {
logger.Warnf("skipping secret %s: secret not found", ref.Name)
continue
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We're iterating over the list of SA secrets, purely for logging purposes, here? Is that right?

timflannagan
timflannagan reviewed Jun 17, 2022
View reviewed changes
pkg/lib/scoped/token_retriever.go
secretMap := make(map[string]*corev1.Secret)
for _, ref := range secrets.Items {
annotations := ref.GetAnnotations()
value := annotations[corev1.ServiceAccountNameKey]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to nil check the annotations map here before indexing for the SA name key, or do maps handle this instruction fine?

timflannagan
timflannagan reviewed Jun 17, 2022
View reviewed changes
go.mod
Comment on lines 59 to 71
replace google.golang.org/grpc => google.golang.org/grpc v1.40.0

replace (
go.opentelemetry.io/contrib => go.opentelemetry.io/contrib v0.20.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp => go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.20.0
go.opentelemetry.io/otel/exporters/otlp => go.opentelemetry.io/otel/exporters/otlp v0.20.0
go.opentelemetry.io/otel/metric => go.opentelemetry.io/otel/metric v0.20.0
go.opentelemetry.io/otel/oteltest => go.opentelemetry.io/otel/oteltest v0.20.0
go.opentelemetry.io/otel/sdk/export/metric => go.opentelemetry.io/otel/sdk/export/metric v0.20.0
go.opentelemetry.io/otel/sdk/metric => go.opentelemetry.io/otel/sdk/metric v0.20.0
go.opentelemetry.io/otel/trace => go.opentelemetry.io/otel/trace v0.20.0
go.opentelemetry.io/proto/otlp => go.opentelemetry.io/proto/otlp v0.7.0
)
Copy link
Member

@timflannagan timflannagan Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note for our future selves: this was needed due to Kubernetes 1.24 requiring a new version of helm, which was problematic as we ran into module issues when bumping that dependency:

go: finding module for package go.opentelemetry.io/otel/internal/metric
go: finding module for package go.opentelemetry.io/otel/unit
go: finding module for package go.opentelemetry.io/otel/semconv
go: finding module for package rsc.io/letsencrypt
go: found rsc.io/letsencrypt in rsc.io/letsencrypt v0.0.3
go: found go.opentelemetry.io/otel/internal/metric in go.opentelemetry.io/otel/internal/metric v0.27.0
go: finding module for package go.opentelemetry.io/otel/metric/registry
go: finding module for package go.opentelemetry.io/otel/semconv
github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/server imports
	k8s.io/apiserver/pkg/server/options imports
	go.opentelemetry.io/otel/semconv: module go.opentelemetry.io/otel@latest found (v1.7.0), but does not contain package go.opentelemetry.io/otel/semconv
github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/server imports
	k8s.io/apiserver/pkg/server/options imports
	go.opentelemetry.io/otel/exporters/otlp/otlpgrpc imports
	go.opentelemetry.io/otel/exporters/otlp imports
	go.opentelemetry.io/otel/sdk/metric/controller/basic imports
	go.opentelemetry.io/otel/metric/registry: module go.opentelemetry.io/otel/metric@latest found (v0.30.0), but does not contain package go.opentelemetry.io/otel/metric/registry

It appears the issue here is that helm uses an incompatible containerd version which the opentelemetry module doesn't play well with. After searching github for how others were tackling this through the filename:go.mod replace opentelemetry search query, and filtering by the newest matches, we found projects replace pinning the opentelemtry module version. Adding those replace pins, and re-generating the vendor directory seemed to solve those CI failures we saw pop up before.

@timflannagan
Copy link
Member

Feel free to remove the hold.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@timflannagan
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2022
@openshift-ci openshift-ci bot merged commit fd90173 into operator-framework:master Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan left review comments

@benluddy benluddy Awaiting requested review from benluddy

@perdasilva perdasilva Awaiting requested review from perdasilva

Assignees

@timflannagan timflannagan

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump dependencies to support Kubernetes 1.24 Bump repository to Go 1.18
3 participants
@dinhxuanvu @timflannagan @perdasilva