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

Update cert-manager dependency libraries to 1.11.2 #981

Merged
merged 1 commit into from
May 25, 2023

Conversation

mihaialexandrescu
Copy link
Contributor

@mihaialexandrescu mihaialexandrescu commented May 24, 2023

Description

We had to use cert-manager 1.11 for RHOS support but our libraries are still on older versions. This PR aims to update cert-manager libraries and its dependencies.

The main problematic update that came with this is the controller-runtime update.
The table of dependencies between cert-manager and controller-runtime looks like this:

cert-manager controller-runtime
v1.9.1 v0.11.2
v1.11.0 & .1 v0.14.1
v1.11.2 v0.14.6

The update to pkg/webhooks/kafkatopic_validator_test.go is there because KafkaTopic validating webhook unit tests started failing with the same error repeated on all err checks : (I’m pasting only one instance)

=== RUN   TestValidateTopic
    kafkatopic_validator_test.go:247: err should be nil, got: failed to connect to Kubernetes API server: List on GroupVersionKind [kafka.banzaicloud.io/v1alpha1](http://kafka.banzaicloud.io/v1alpha1), Kind=KafkaTopic specifies selector on field spec.name , but no index with name spec.name  has been registered for GroupVersionKind [kafka.banzaicloud.io/v1alpha1](http://kafka.banzaicloud.io/v1alpha1), Kind=KafkaTopic
    kafkatopic_validator_test.go:250: Expected not allowed due to invalid replication factor, got allowed

That error starts from here and links to using field selectors (for "spec.name" in this particular case) during checkExistingKafkaTopicCRs().

In more detail, the error after validateKafkaTopic() when called during unit testing would be :

    kafkatopic_validator_test.go:260:
        err = List on GroupVersionKind kafka.banzaicloud.io/v1alpha1, Kind=KafkaTopic specifies selector on field spec.name, but no index with name spec.name has been registered for GroupVersionKind kafka.banzaicloud.io/v1alpha1, Kind=KafkaTopic
        failed to connect to Kubernetes API server
        github.com/banzaicloud/koperator/pkg/webhooks.(*KafkaTopicValidator).checkExistingKafkaTopicCRs
        	/Users/mihalexa/development/mygopath/src/github.com/banzaicloud/koperator/pkg/webhooks/kafkatopic_validator.go:231
        github.com/banzaicloud/koperator/pkg/webhooks.(*KafkaTopicValidator).validateKafkaTopic
        	/Users/mihalexa/development/mygopath/src/github.com/banzaicloud/koperator/pkg/webhooks/kafkatopic_validator.go:130
        github.com/banzaicloud/koperator/pkg/webhooks.TestValidateTopic
        	/Users/mihalexa/development/mygopath/src/github.com/banzaicloud/koperator/pkg/webhooks/kafkatopic_validator_test.go:253
        testing.tRunner
        	/opt/homebrew/Cellar/go/1.20.4/libexec/src/testing/testing.go:1576
        runtime.goexit
        	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/asm_arm64.s:1172

The fake client we use in the test didn't have the field indexers that are required for using field selectors in queries and that we normally add to our manager in main.go using AddKafkaTopicIndexers() (including "spec.name"). The "real" client and the "fake" client are too far apart for us to be able to apply the same function. Also, the underlying structs don't allow the same backing structures to even try.

The relevant PR from controller-runtime about this (thank you, Darren!) was kubernetes-sigs/controller-runtime#2025.

I decided to only include spec.name for now in the test because it was the only field indexer needed for the moment out of the 3 in AddKafkaTopicIndexers().

What's up with the update to pkg/resources/kafka/mocks/Client.go ?

The update to pkg/resources/kafka/mocks/Client.go is the result of the needed make mock-generate. The Client interface in controller-runtime changed in v0.14.0 and we need to generate a new mock Client.

What's up with the changes to CRDs in charts/ and config/ ?

No idea. They were generated after make test, I think and I would attribute them to us not having run a make target in a previous PR.

Steps taken

For me, it was :

go clean -testcache

PATH="$(go1.19.9 env GOROOT)/bin:${PATH}" GOWORK=off make test   <<< tests run fine here with older versions

cd api
go get -u github.com/cert-manager/cert-manager@v1.11.2
go mod tidy
cd ..
go get -u github.com/cert-manager/cert-manager@v1.11.2
go mod tidy

PATH="$(go1.19.9 env GOROOT)/bin:${PATH}" GOWORK=off make mock-generate

<update tests with the field indexer considerations explained above>

PATH="$(go1.19.9 env GOROOT)/bin:${PATH}" GOWORK=off make test

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change
  • Refactor
  • Documentation
  • Other (please describe): dependency update

Checklist

  • I have read the contributing guidelines
  • Existing issues have been referenced (where applicable)
  • I have verified this change is not present in other open pull requests
  • Functionality is documented
  • All code style checks pass
  • New code contribution is covered by automated tests
  • All new and existing tests pass

@mihaialexandrescu mihaialexandrescu requested a review from a team May 24, 2023 12:10
@mihaialexandrescu mihaialexandrescu marked this pull request as ready for review May 24, 2023 12:10
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Kuvesz Kuvesz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@mihaialexandrescu mihaialexandrescu merged commit f0d8fec into master May 25, 2023
@mihaialexandrescu mihaialexandrescu deleted the STB-1114/update_cert-manager_deplibraries branch May 25, 2023 08:19
ctrlaltluc added a commit to adobe/koperator that referenced this pull request Jun 8, 2023
…herrypick-from-banzai-master

* banzai/master:
  Add new CruiseControlTaskOperation to represent Status Cruise Control Operation (banzaicloud#975)
  Use permanent Slack link in README (banzaicloud#985)
  update cert-manager dependency libraries to 1.11.2 (banzaicloud#981)
  Adding test cases for kafka user issuerRef group  (banzaicloud#967)
  fix(cc): re-creation of CC metrics topic (banzaicloud#976)
  Revert "Enabling k8s-csr for PKIbackend in kafka cluster spec (banzaicloud#970)" (banzaicloud#972)
  Using gomock to mock sigs.k8s.io/controller-runtime/pkg/client If (banzaicloud#973)

# Conflicts:
#	go.mod
#	go.sum
#	pkg/resources/kafka/kafka_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants