Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

Update component usage and add action pattern #85

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GuessWhoSamFoo
Copy link

This PR introduces the following changes:

  • A proof of concept for using buttons to trigger an action. This is done by creating a wrapper around the respective starboard command.
    image

  • Refactoring old markdown components to directly use links and icons

  • Increase visibility to vulnerabilities by changing icon status color (in this case, only critical) and at-a-glance view from a signpost
    image

  • Show how to create multiple tabs from a single plugin.

@danielpacak danielpacak self-requested a review September 9, 2021 20:01
Copy link
Contributor

@danielpacak danielpacak left a comment

Choose a reason for hiding this comment

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

👋 @GuessWhoSamFoo This PR is amazing! Thank you for all work that you've done.

I tested in my env and everything seems to be fine. However, I do have a couple of questions before we'll merge the code.


func startKubeHunterScan(ctx context.Context) error {
configFlags := genericclioptions.NewConfigFlags(true)
restconfig, err := configFlags.ToRESTConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this in general may be a different client config than the one used by octant process. Is that correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is a good point and leaves more to be desired in the plugin interface.

A workaround is to see if $KUBECONFIG is set and see if it matches the one from config flags since we can assume Octant looks at the same spot by default via clientcmd.NewDefaultClientConfigLoadingRules().GetDefaultFilename().

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarification. I think it's a good start and it should work in most cases. We can also leave a TODO comment explaining when this approach may not work.

return plugin.TabResponse{}, errors.New("object is nil")
}

workload, err := getWorkloadFromObject(request.Object)
Copy link
Contributor

@danielpacak danielpacak Sep 10, 2021

Choose a reason for hiding this comment

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

We generate ConfigAuditReports for a subset of K8s resources (GVKs). For example, we'll never generate it for Nodes because for Nodes we generate CISKubeBenchReports.
That said, I was wondering if we can skip printing a Tab for certain GVKs?

Copy link
Author

Choose a reason for hiding this comment

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

Checking if the object type is a node and returning an empty tab response is one way, but I don't like having that error message.

Going to patch upstream to skip empty tab responses without errors.

https://github.com/vmware-tanzu/octant/blob/5a8648921cc2779eb62a0ac11147f12aa29f831c/pkg/plugin/grpc.go#L557-L559

Copy link
Contributor

Choose a reason for hiding this comment

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

An empty tab response sounds good. We can improve error handling by upgrading Octant whenever a new version is available.

@GuessWhoSamFoo
Copy link
Author

Let me know if the proposed fixes are acceptable and I can make another patch. Otherwise we can break up this PR into items that are considered merge-able while fixing upstream octant

@danielpacak
Copy link
Contributor

Let me know if the proposed fixes are acceptable and I can make another patch. Otherwise we can break up this PR into items that are considered merge-able while fixing upstream octant

@GuessWhoSamFoo Let's wrap up this PR according to your suggestions and merge it. Once you fix upstream Octant we can update this plugin to take advantage of such enhancements.

@CLAassistant
Copy link

CLAassistant commented Sep 14, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Sam Foo seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants