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

(feat) add new validator for deprecated apis #5216

Merged

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Sep 16, 2021

Description of the change:
Allow users checks their bundle against the specific criteria over deprecation of APIs in k8s

Motivation for the change:
The deprecation of APIs in k8s 1.22 impacts the operator's projects

When found the APIs:

Screen Shot 2021-09-16 at 21 16 41

When does not find the APIs:

Screen Shot 2021-09-16 at 22 09 49

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci openshift-ci bot requested a review from estroz September 16, 2021 19:10
@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 Sep 16, 2021
@camilamacedo86 camilamacedo86 changed the title feat; add new validator for deprecated apis (feat) add new validator for deprecated apis Sep 16, 2021
@camilamacedo86 camilamacedo86 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2021
@camilamacedo86 camilamacedo86 requested review from rashmigottipati and removed request for estroz September 16, 2021 21:20
k8s.io/cli-runtime v0.21.0
k8s.io/client-go v0.21.2
k8s.io/client-go v0.22.1
Copy link
Member

@varshaprasad96 varshaprasad96 Sep 16, 2021

Choose a reason for hiding this comment

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

we haven't bumped the dependencies to 1.22 yet, since kb didn't bump it yet. Though this just bumps the deps in SDK binary and not in the plugin, do we still wait till that is done?

Copy link
Member

Choose a reason for hiding this comment

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

This is certainly different than what we've done in the past right? @camilamacedo86 is this really needed for this change?

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Sep 17, 2021

Choose a reason for hiding this comment

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

The API is bumped. So, when we bump the API that is automatic.
KB does not use these deps in the CLI tool as well.
It has only k8s.io/apimachinery v0.21.3 which is used for the alpha-gen.

If I am not wrong, we needed first to ensure the controller runtime was updated.
Then, we could bump the scaffolds in KB and then here.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Sep 17, 2021

Choose a reason for hiding this comment

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

Copy link
Member

@joelanford joelanford Sep 17, 2021

Choose a reason for hiding this comment

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

As long as we're:

  • not somehow using the deps of operator-sdk itself in the code that SDK scaffolds
  • able to pin our test environments (kind, envtest, etc.) to use the versions we want, not the versions of SDK's deps

I don't see how this API bump would be a problem. Since no one should be able to import SDK code anymore, this should have no affect on anything except the operator-sdk binary itself.

Is there a specific reason we know of that forces us to keep aligned with kubebuilder and/or our scaffolded code?

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Sep 17, 2021

Choose a reason for hiding this comment

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

Thank you for your input and help 🥇
I think the concern only comes because usually, we bumped KB first to also get the scaffolds updated as well.

@jmrodri @varshaprasad96 could we move forward?

@camilamacedo86 camilamacedo86 changed the title (feat) add new validator for deprecated apis WIP; (feat) add new validator for deprecated apis Sep 16, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021
@camilamacedo86 camilamacedo86 changed the title WIP; (feat) add new validator for deprecated apis (feat) add new validator for deprecated apis Sep 16, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021
@joelanford
Copy link
Member

This PR itself looks good to me. I'm more curious about the validator logic in the API repo.

What's with the WARN ... checking APIs against Kubernetes version : 1.22 log message?

Seems like that should be either INFO level or not logged at all (the caller explicitly asked for it, so IMO it just adds noise to the output)

Also, how does this validator work. Can I pass any kubernetes version I want? Can you link the PR where the validator was merged?

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Sep 17, 2021

Hi @joelanford,

Thank you for your time here and help. Following the answers to your comment inline:

This PR itself looks good to me. I'm more curious about the validator logic in the API repo.

This implementation has been used for a while in many places such as audit,iib, CVP. Also, it was QE tested as well and provided by SDK since its 1.18.0 version. It has been proved very helpful to check the usage of the removal APIs in 1.22 since the most likely Operators projects to be affected are shipped on the bundle (CRD/RBAC).

Users have been able to take advantage of these checks via the optional operatorhub validator already. This PR/new validator only allows them to test their bundles exclusively against this criteria when it is not desirable to check the whole criteria provided by operatorhub optional validator.

What's with the WARN ... checking APIs against Kubernetes version: 1.22 log message?
Seems like that should be either INFO level or not logged at all (the caller explicitly asked for it, so IMO it just adds noise to the output) +1000

That is not necessary at all. The user informed the version already. I did PR to address this nit: operator-framework/api#157

Also, how does this validator work? Can I pass any kubernetes version I want? Can you link the PR where the validator was merged?

Currently, it only has implemented checks for the k8s version 1.22, but you can pass any version. We will add checks for 1.25 as well (probably, least effective in this case). Would be nice we tell what are the supported versions. That would be easier after we are able to address something like operator-framework/api#158. I'd not like to add to the consumer its seems to make more sense to be in the validator itself.

The code is here: https://github.com/operator-framework/api/blob/master/pkg/validation/internal/removed_apis.go, and it has a hall for improvements as well, which should get done soon when we will add the checks for 1.25 or as its follow up. Brett and/or I will be working to improve its maintainability.

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2021
@camilamacedo86 camilamacedo86 merged commit 4f2dc20 into operator-framework:master Sep 21, 2021
@camilamacedo86 camilamacedo86 deleted the new-validator branch September 21, 2021 13:42
@asmacdo asmacdo added this to the v1.13.0 milestone Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants