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

✨ LFX: Extend use-case of detecting deprecated Kubernetes API usage #441

Merged
merged 12 commits into from
Jan 12, 2024

Conversation

Parthiba-Hazra
Copy link
Contributor

Added 'yq' provider.
Added rules to detect use of deprecated APIs in k8s yaml files.

@Parthiba-Hazra
Copy link
Contributor Author

@JonahSussman @eemcmullan still need to add the test for those new function. Will try to complete that in few days.

Copy link
Contributor

@JonahSussman JonahSussman left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for submitting a contribution! Just a few changes

@@ -27,9 +30,12 @@ RUN python3 -m pip install python-lsp-server

COPY --from=jaeger-builder /go/bin/all-in-one-linux /usr/bin/

COPY --from=yq-builder /usr/bin/yq /usr/bin/yq
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud here, but I think there was talk of doing some dependency management for external providers a while back. It's fine for now, but this method could bloat the Dockerfile when adding more providers.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will for sure, I think we should ask @dymurray to prioritize this work for the next version.

@@ -217,7 +217,7 @@ func main() {
if err != nil {
log.Error(err, "error writing output file", "file", outputViolations)
os.Exit(1) // Treat the error as a fatal error
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This extra space has been bugging me for a while now, thanks! 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if this is where we want to put these k8s rules. Perhaps we should add it to the https://github.com/konveyor/rulesets/ repo instead? WDYT @shawn-hurley


lspServerPath, ok := c.ProviderSpecificConfig[provider.LspServerPathConfigKey].(string)
if !ok || lspServerPath == "" {
return nil, fmt.Errorf("invalid lspServerPath provided, unable to init go provider")
Copy link
Contributor

Choose a reason for hiding this comment

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

this log mentions a go provider

const KIND = "kind"
const GITHUB_K8S_API_URL = "https://api.github.com/repos/kubernetes/kubernetes/releases"

var default_k8s_version = "v1.28.4"
Copy link
Contributor

@eemcmullan eemcmullan Nov 29, 2023

Choose a reason for hiding this comment

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

may want to think of a way to make this more dynamic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add a config struct and then assign those values form a json file or yaml file?

@@ -0,0 +1,1034 @@
- message: Deprecated/removed Kubernetes API version 'extensions/v1beta1' is used for 'Deployment'. Consider using 'apps/v1'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we maybe get these rules separated into rulesets by the api group? i.e. rules for apps, rules for networking, etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will look after it. need some time

removedIn: "v1.16.0"
replacementAPI: "apps/v1"
- message: Deprecated/removed Kubernetes API version 'apps/v1beta2' is used for 'Deployment'. Consider using 'apps/v1'.
ruleID: k8s-deprecated-api-002
Copy link
Contributor

Choose a reason for hiding this comment

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

If the above is done, we could then have more descriptive ruleIDs


RUN make build

# Add yq to the build stage
FROM docker.io/mikefarah/yq as yq-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

note to us, we probably need to fork this and build it out of konveyor in the future

@@ -0,0 +1,1034 @@
- message: Deprecated/removed Kubernetes API version 'extensions/v1beta1' is used for 'Deployment'. Consider using 'apps/v1'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these rules be generated or did you do this by hand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah it's not generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can find a way to get this automated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, that must have taken a long time, thank you. This is going to be a nice feature!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trust me, that didn't take much time 😅

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

I think this makes a lot of sense; I wonder if we could make it an external provider in its own repo, though maybe in the konveyor-externsions and make a flag to build an image with it built in or something?

Just wondering, if it is too much work right now I am cool with adding it and doing the messy work after :)

@shawn-hurley
Copy link
Contributor

@Parthiba-Hazra This is awesome work! Thank you so much for taking the time to work on this and contributing!

@shawn-hurley
Copy link
Contributor

@Parthiba-Hazra can you rebase then I think we can merge

@JonahSussman JonahSussman self-requested a review December 4, 2023 19:09
Copy link
Contributor

@JonahSussman JonahSussman left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Parthiba-Hazra <parthibahazra02@gmail.com>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
@eemcmullan
Copy link
Contributor

@Parthiba-Hazra Do you mind updating the demo testing output?

@Parthiba-Hazra
Copy link
Contributor Author

@Parthiba-Hazra Do you mind updating the demo testing output?

yes sure,

Signed-off-by: Parthiba-Hazra <parthibahazra@gmail.com>
k8s-rule.yaml Show resolved Hide resolved
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.

5 participants