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.

Sign up for GitHub

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

Jump to bottom

fix: capture commands should respect --namespace #568

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+34 −36
Open

fix: capture commands should respect --namespace #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions cli/cmd/capture/capture.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import (
"k8s.io/cli-runtime/pkg/genericclioptions"
)

var name string
var opts = struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can borrow what kubectl cli has been achieved here? Like https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/create/create_cronjob.go

Copy link
Contributor

Choose a reason for hiding this comment

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

^^ it's for this change, we can improve later.
Thanks for fixing missing namespace flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's the direction I am moving with this change, but it is just the first small step

genericclioptions.ConfigFlags
Name *string
}{}

var capture = &cobra.Command{
Use: "capture",
Expand All @@ -18,8 +21,8 @@ var capture = &cobra.Command{

func init() {
cmd.Retina.AddCommand(capture)
configFlags = genericclioptions.NewConfigFlags(true)
configFlags.AddFlags(capture.PersistentFlags())
capture.PersistentFlags().StringVar(&name, "name", "", "The name of the Retina Capture")
opts.ConfigFlags = *genericclioptions.NewConfigFlags(true)
opts.AddFlags(capture.PersistentFlags())
capture.PersistentFlags().StringVar(opts.Name, "name", "", "The name of the Retina Capture")
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

I know it's a small thing here, but this might be better as a method of opts. You can inject whatever the value of capture.PersistentFlags() is and let opts add Flags to it. It'd make it a little more modular by letting the opts say "I know what my flags should be".

Not a blocker, but just an idea.

_ = capture.MarkPersistentFlagRequired("name")
}
38 changes: 17 additions & 21 deletions cli/cmd/capture/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,12 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/kubernetes"
"k8s.io/kubectl/pkg/util/i18n"
"k8s.io/kubectl/pkg/util/templates"
)

var (
configFlags *genericclioptions.ConfigFlags

duration time.Duration
maxSize int
packetSize int
Expand All @@ -52,7 +49,6 @@ var (
excludeFilter string
includeFilter string
includeMetadata bool
namespace string
jobNumLimit int

nowait bool
Expand Down Expand Up @@ -101,7 +97,7 @@ var createCapture = &cobra.Command{
Short: "create a Retina Capture",
Example: createExample,
RunE: func(*cobra.Command, []string) error {
kubeConfig, err := configFlags.ToRESTConfig()
kubeConfig, err := opts.ToRESTConfig()
if err != nil {
return errors.Wrap(err, "failed to compose k8s rest config")
}
Expand All @@ -124,10 +120,10 @@ var createCapture = &cobra.Command{
if nowait {
retinacmd.Logger.Info("Please manually delete all capture jobs")
if capture.Spec.OutputConfiguration.BlobUpload != nil {
retinacmd.Logger.Info("Please manually delete capture secret", zap.String("namespace", namespace), zap.String("secret name", *capture.Spec.OutputConfiguration.BlobUpload))
retinacmd.Logger.Info("Please manually delete capture secret", zap.String("namespace", *opts.Namespace), zap.String("secret name", *capture.Spec.OutputConfiguration.BlobUpload))
timraymond marked this conversation as resolved.
Show resolved Hide resolved
}
if capture.Spec.OutputConfiguration.S3Upload != nil && capture.Spec.OutputConfiguration.S3Upload.SecretName != "" {
retinacmd.Logger.Info("Please manually delete capture secret", zap.String("namespace", namespace), zap.String("secret name", capture.Spec.OutputConfiguration.S3Upload.SecretName))
retinacmd.Logger.Info("Please manually delete capture secret", zap.String("namespace", *opts.Namespace), zap.String("secret name", capture.Spec.OutputConfiguration.S3Upload.SecretName))
}
printCaptureResult(jobsCreated)
return nil
Expand All @@ -144,19 +140,19 @@ var createCapture = &cobra.Command{
retinacmd.Logger.Info("Deleting jobs as all jobs are completed")
jobsFailedToDelete := deleteJobs(kubeClient, jobsCreated)
if len(jobsFailedToDelete) != 0 {
retinacmd.Logger.Info("Please manually delete capture jobs failed to delete", zap.String("namespace", namespace), zap.String("job list", strings.Join(jobsFailedToDelete, ",")))
retinacmd.Logger.Info("Please manually delete capture jobs failed to delete", zap.String("namespace", *opts.Namespace), zap.String("job list", strings.Join(jobsFailedToDelete, ",")))
}

err = deleteSecret(kubeClient, capture.Spec.OutputConfiguration.BlobUpload)
if err != nil {
retinacmd.Logger.Error("Failed to delete capture secret, please manually delete it",
zap.String("namespace", namespace), zap.String("secret name", *capture.Spec.OutputConfiguration.BlobUpload), zap.Error(err))
zap.String("namespace", *opts.Namespace), zap.String("secret name", *capture.Spec.OutputConfiguration.BlobUpload), zap.Error(err))
}

err = deleteSecret(kubeClient, &capture.Spec.OutputConfiguration.S3Upload.SecretName)
if err != nil {
retinacmd.Logger.Error("Failed to delete capture secret, please manually delete it",
zap.String("namespace", namespace),
zap.String("namespace", *opts.Namespace),
zap.String("secret name", capture.Spec.OutputConfiguration.S3Upload.SecretName),
zap.Error(err),
)
Expand All @@ -170,7 +166,7 @@ var createCapture = &cobra.Command{

retinacmd.Logger.Info("Not all job are completed in the given time")
retinacmd.Logger.Info("Please manually delete the Capture")
return getCaptureAndPrintCaptureResult(kubeClient, capture.Name, namespace)
return getCaptureAndPrintCaptureResult(kubeClient, capture.Name, *opts.Namespace)
},
}

Expand All @@ -189,7 +185,7 @@ func createSecretFromBlobUpload(kubeClient kubernetes.Interface, blobUpload, cap
captureConstants.CaptureOutputLocationBlobUploadSecretKey: []byte(blobUpload),
},
}
secret, err := kubeClient.CoreV1().Secrets(namespace).Create(context.TODO(), secret, metav1.CreateOptions{})
secret, err := kubeClient.CoreV1().Secrets(*opts.Namespace).Create(context.TODO(), secret, metav1.CreateOptions{})
if err != nil {
return "", err
}
Expand All @@ -208,7 +204,7 @@ func createSecretFromS3Upload(kubeClient kubernetes.Interface, s3AccessKeyID, s3
captureConstants.CaptureOutputLocationS3UploadSecretAccessKey: []byte(s3SecretAccessKey),
},
}
secret, err := kubeClient.CoreV1().Secrets(namespace).Create(context.TODO(), secret, metav1.CreateOptions{})
secret, err := kubeClient.CoreV1().Secrets(*opts.Namespace).Create(context.TODO(), secret, metav1.CreateOptions{})
if err != nil {
return "", fmt.Errorf("failed to create s3 upload secret: %w", err)
}
Expand All @@ -220,14 +216,14 @@ func deleteSecret(kubeClient kubernetes.Interface, secretName *string) error {
return nil
}

return kubeClient.CoreV1().Secrets(namespace).Delete(context.TODO(), *secretName, metav1.DeleteOptions{})
return kubeClient.CoreV1().Secrets(*opts.Namespace).Delete(context.TODO(), *secretName, metav1.DeleteOptions{}) //nolint:wrapcheck //internal return
}

func createCaptureF(kubeClient kubernetes.Interface) (*retinav1alpha1.Capture, error) {
capture := &retinav1alpha1.Capture{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Name: *opts.Name,
Namespace: *opts.Namespace,
},
Spec: retinav1alpha1.CaptureSpec{
CaptureConfiguration: retinav1alpha1.CaptureConfiguration{
Expand Down Expand Up @@ -304,15 +300,15 @@ func createCaptureF(kubeClient kubernetes.Interface) (*retinav1alpha1.Capture, e

if len(blobUpload) != 0 {
// Mount blob url as secret onto the capture pod for security concern if blob url is not empty.
secretName, err := createSecretFromBlobUpload(kubeClient, blobUpload, name)
secretName, err := createSecretFromBlobUpload(kubeClient, blobUpload, *opts.Name)
if err != nil {
return nil, err
}
capture.Spec.OutputConfiguration.BlobUpload = &secretName
}

if s3Bucket != "" {
secretName, err := createSecretFromS3Upload(kubeClient, s3AccessKeyID, s3SecretAccessKey, name)
secretName, err := createSecretFromS3Upload(kubeClient, s3AccessKeyID, s3SecretAccessKey, *opts.Name)
if err != nil {
return nil, fmt.Errorf("failed to create s3 upload secret: %w", err)
}
Expand Down Expand Up @@ -361,12 +357,12 @@ func createJobs(kubeClient kubernetes.Interface, capture *retinav1alpha1.Capture

jobsCreated := []batchv1.Job{}
for _, job := range jobs {
jobCreated, err := kubeClient.BatchV1().Jobs(namespace).Create(context.TODO(), job, metav1.CreateOptions{})
jobCreated, err := kubeClient.BatchV1().Jobs(*opts.Namespace).Create(context.TODO(), job, metav1.CreateOptions{})
if err != nil {
return nil, err
}
jobsCreated = append(jobsCreated, *jobCreated)
retinacmd.Logger.Info("Packet capture job is created", zap.String("namespace", namespace), zap.String("capture job", jobCreated.Name))
retinacmd.Logger.Info("Packet capture job is created", zap.String("namespace", *opts.Namespace), zap.String("capture job", jobCreated.Name))
}
return jobsCreated, nil
}
Expand Down Expand Up @@ -410,7 +406,7 @@ func waitUntilJobsComplete(kubeClient kubernetes.Interface, jobs []batchv1.Job)

if len(jobsIncompleted) != 0 {
retinacmd.Logger.Info("Not all jobs are completed",
zap.String("namespace", namespace),
zap.String("namespace", *opts.Namespace),
zap.String("Completed jobs", strings.Join(jobsCompleted, ",")),
zap.String("Uncompleted packet capture jobs", strings.Join(jobsIncompleted, ",")),
)
Expand Down
12 changes: 6 additions & 6 deletions cli/cmd/capture/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var deleteCapture = &cobra.Command{
Short: "Delete a Retina capture",
Example: deleteExample,
RunE: func(*cobra.Command, []string) error {
kubeConfig, err := configFlags.ToRESTConfig()
kubeConfig, err := opts.ToRESTConfig()
if err != nil {
return errors.Wrap(err, "")
}
Expand All @@ -42,7 +42,7 @@ var deleteCapture = &cobra.Command{

captureJobSelector := &metav1.LabelSelector{
MatchLabels: map[string]string{
label.CaptureNameLabel: name,
label.CaptureNameLabel: *opts.Name,
label.AppLabel: captureConstants.CaptureAppname,
},
}
Expand All @@ -51,12 +51,12 @@ var deleteCapture = &cobra.Command{
LabelSelector: labelSelector.String(),
}

jobList, err := kubeClient.BatchV1().Jobs(namespace).List(context.TODO(), jobListOpt)
jobList, err := kubeClient.BatchV1().Jobs(*opts.Namespace).List(context.TODO(), jobListOpt)
if err != nil {
return errors.Wrap(err, "failed to list capture jobs")
}
if len(jobList.Items) == 0 {
return errors.Errorf("capture %s in namespace %s was not found", name, namespace)
return errors.Errorf("capture %s in namespace %s was not found", *opts.Name, *opts.Namespace)
}

for _, job := range jobList.Items {
Expand All @@ -70,13 +70,13 @@ var deleteCapture = &cobra.Command{

for _, volume := range jobList.Items[0].Spec.Template.Spec.Volumes {
if volume.Secret != nil {
if err := kubeClient.CoreV1().Secrets(namespace).Delete(context.TODO(), volume.Secret.SecretName, metav1.DeleteOptions{}); err != nil {
if err := kubeClient.CoreV1().Secrets(*opts.Namespace).Delete(context.TODO(), volume.Secret.SecretName, metav1.DeleteOptions{}); err != nil {
return errors.Wrap(err, "failed to delete capture secret")
}
break
}
}
retinacmd.Logger.Info(fmt.Sprintf("Retina Capture %q delete", name))
retinacmd.Logger.Info(fmt.Sprintf("Retina Capture %q delete", *opts.Name))

return nil
},
Expand Down
4 changes: 2 additions & 2 deletions cli/cmd/capture/download.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ var downloadCapture = &cobra.Command{
splitPath := strings.SplitN(containerPath, "/", 2) //nolint:gomnd // TODO string splitting probably isn't the right way to parse this URL?
containerName := splitPath[0]

params := storage.ListBlobsParameters{Prefix: name}
params := storage.ListBlobsParameters{Prefix: *opts.Name}
blobList, err := blobService.GetContainerReference(containerName).ListBlobs(params)
if err != nil {
return errors.Wrap(err, "failed to list blobstore ")
}

if len(blobList.Blobs) == 0 {
return errors.Errorf("no blobs found with prefix: %s", name)
return errors.Errorf("no blobs found with prefix: %s", *opts.Name)
}

for _, v := range blobList.Blobs {
Expand Down
5 changes: 2 additions & 3 deletions cli/cmd/capture/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var listCaptures = &cobra.Command{
Short: "List Retina Captures",
Example: listExample,
RunE: func(*cobra.Command, []string) error {
kubeConfig, err := configFlags.ToRESTConfig()
kubeConfig, err := opts.ToRESTConfig()
if err != nil {
return errors.Wrap(err, "failed to compose k8s rest config")
}
Expand All @@ -36,7 +36,7 @@ var listCaptures = &cobra.Command{
return errors.Wrap(err, "failed to initialize kubernetes client")
}

captureNamespace := namespace
captureNamespace := *opts.Namespace
if allNamespaces {
captureNamespace = ""
}
Expand All @@ -46,7 +46,6 @@ var listCaptures = &cobra.Command{

func init() {
capture.AddCommand(listCaptures)
listCaptures.Flags().StringVarP(&namespace, "namespace", "n", "default", "Namespace to host capture job")
listCaptures.Flags().BoolVarP(&allNamespaces, "all-namespaces", "A", allNamespaces,
"If present, list the requested object(s) across all namespaces. Namespace in current context is ignored even if specified with --namespace.")
}
Loading