open-cluster-management-io / registration

hub / spoke registration controllers
Apache License 2.0
42 stars 58 forks source link

Add V1beta1CSRAPICompatibility feature gate for registration-controller #259

Closed promacanthus closed 2 years ago

promacanthus commented 2 years ago

In some cases, our hub cluster's kubernetes version is less than 1.19, and its certificates.k8s.io is v1beta1. So we need to enable the feature gate V1beta1CSRAPICompatibility. Otherwise, the registration-controller will get the following error message.

W0630 07:07:55.361239       1 reflector.go:324] k8s.io/client-go@v0.23.0/tools/cache/reflector.go:167: failed to list *v1.CertificateSigningRequest: the server could not find the requested resource
E0630 07:07:55.361260       1 reflector.go:138] k8s.io/client-go@v0.23.0/tools/cache/reflector.go:167: Failed to watch *v1.CertificateSigningRequest: failed to list *v1.CertificateSigningRequest: the server could not find the requested resource

Signed-off-by: Promacanthus promacanthus@gmail.com

qiujian16 commented 2 years ago

/assign @skeeey

skeeey commented 2 years ago

@Promacanthus thanks for your contribution, I think you need run go mod tidy and go mod vendor to make the verify deps pass

promacanthus commented 2 years ago

@Promacanthus thanks for your contribution, I think you need run go mod tidy and go mod vendor to make the verify deps pass

Thanks for your review. I will rerun the go mod tidy and go mod vendor to fix the verify-deps problem and follow your suggestions above.

promacanthus commented 2 years ago

@skeeey I have modify the code based on your comment, please review it again when you have time.

I think this PR needs to be merged first.

When I rerun the go mod vendor command, there is no V1beta1CSRAPICompatibility in DefaultHubRegistrationFeatureGates at vendor/open-cluster-management.io/api/feature/feature.go file.

This will break integration and e2e test, and get the error message as following.

feature "V1beta1CSRAPICompatibility" is not registered in FeatureGate 

"open-cluster-management.io/registration/pkg/features/feature.go:15"
skeeey commented 2 years ago

@Promacanthus the integration test failed

panic: feature "V1beta1CSRAPICompatibility" is not registered in FeatureGate "open-cluster-management.io/registration/pkg/features/feature.go:15"

you need register the feature gate to the management.io/registration/pkg/features/feature.go

promacanthus commented 2 years ago

@Promacanthus the integration test failed

panic: feature "V1beta1CSRAPICompatibility" is not registered in FeatureGate "open-cluster-management.io/registration/pkg/features/feature.go:15"

you need register the feature gate to the management.io/registration/pkg/features/feature.go

The previous understanding of DefaultHubRegistrationFeatureGates is not correct, I will close the PR and register the V1beta1CSRAPICompatibility feature gate to pkg/features/feature.go.

skeeey commented 2 years ago

@Promacanthus the integration test failed

panic: feature "V1beta1CSRAPICompatibility" is not registered in FeatureGate "open-cluster-management.io/registration/pkg/features/feature.go:15"

you need register the feature gate to the management.io/registration/pkg/features/feature.go

The previous understanding of DefaultHubRegistrationFeatureGates is not correct, I will close the PR and register the V1beta1CSRAPICompatibility feature gate to pkg/features/feature.go.

Hi, @Promacanthus I think for this case, it make sense to add the V1beta1CSRAPICompatibility to the DefaultHubRegistrationFeatureGates in the api repo, so you do not need the close the pr, we can wait for the api pr is merged, then we update the api dependency in this pr to use the new DefaultHubRegistrationFeatureGates, what's your opinion?

promacanthus commented 2 years ago

@Promacanthus the integration test failed

panic: feature "V1beta1CSRAPICompatibility" is not registered in FeatureGate "open-cluster-management.io/registration/pkg/features/feature.go:15"

you need register the feature gate to the management.io/registration/pkg/features/feature.go

The previous understanding of DefaultHubRegistrationFeatureGates is not correct, I will close the PR and register the V1beta1CSRAPICompatibility feature gate to pkg/features/feature.go.

Hi, @Promacanthus I think for this case, it make sense to add the V1beta1CSRAPICompatibility to the DefaultHubRegistrationFeatureGates in the api repo, so you do not need the close the pr, we can wait for the api pr is merged, then we update the api dependency in this pr to use the new DefaultHubRegistrationFeatureGates, what's your opinion?

Agreed with you. Let me reopen the PR.

skeeey commented 2 years ago

/approve

openshift-ci[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Promacanthus, skeeey

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](https://github.com/open-cluster-management-io/registration/blob/main/OWNERS)~~ [skeeey] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
skeeey commented 2 years ago

This pr is fine to me

@qiujian16 would you also take a look this? if there are no more comments, I think we can merge this

qiujian16 commented 2 years ago

/lgtm Thanks!