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

Add Kyma Access command to CLI #2067

Merged
merged 11 commits into from
May 13, 2024
Merged

Add Kyma Access command to CLI #2067

merged 11 commits into from
May 13, 2024

Conversation

Cortey
Copy link
Contributor

@Cortey Cortey commented May 8, 2024

Description
Adds a command that creates a kubeconfig that includes token and certificate for specially crafted service account with admin access rights

Related issue(s)

@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024
@kyma-bot
Copy link

kyma-bot commented May 8, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2024
@Cortey Cortey changed the title Accesscmd Add Kyma Access command to CLI May 10, 2024
@Cortey Cortey added area/cli Related to all activities around CLI kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 10, 2024
@Cortey Cortey marked this pull request as ready for review May 10, 2024 12:05
@Cortey Cortey requested a review from a team as a code owner May 10, 2024 12:05
@pPrecel pPrecel self-assigned this May 13, 2024
Copy link
Contributor

@pPrecel pPrecel left a comment

Choose a reason for hiding this comment

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

In general, looks really ok. The only doubt I have is if it's not better to extract most functions that are creating Kubeconfig to the separate cluster and test it. But I don't want to force it in this PR. Maybe it would be better to create a separate PR.

cmd.Flags().StringVar(&cfg.name, "name", "", "Name of the Service Account to be created")
cmd.Flags().StringVar(&cfg.clusterrole, "clusterrole", "", "Name of the cluster role to bind the Service Account")
cmd.Flags().StringVar(&cfg.output, "output", "", "Path to the output kubeconfig file")
cmd.Flags().StringVar(&cfg.namespace, "namespace", "default", "Namespace ")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to improve the --namespace flag :)

},
}
_, err := cfg.KubeClient.Static().RbacV1().ClusterRoles().Create(cfg.Ctx, &cRole, metav1.CreateOptions{})
if client.IgnoreNotFound(err) == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to replace client.IngoreNotFound with errors.IsAlreadyExists from the k8s.io/apimachinery/pkg/api/errors. I think the Create func will never return a not found error because in this case this func will just create it

@kyma-bot kyma-bot added the lgtm Looks good to me! label May 13, 2024
@Cortey Cortey merged commit 9b8381f into kyma-project:v3 May 13, 2024
3 checks passed
pPrecel pushed a commit to pPrecel/cli that referenced this pull request May 21, 2024
* v1

* v12

* print

* workingv1

* accesscommand

* cut function into pieces

this is my last resort

* typo

* cleanup

* long

* gomodtidy

* review

---------

Co-authored-by: Piotr Halama <[email protected]>
pPrecel pushed a commit that referenced this pull request May 21, 2024
* v1

* v12

* print

* workingv1

* accesscommand

* cut function into pieces

this is my last resort

* typo

* cleanup

* long

* gomodtidy

* review

---------

Co-authored-by: Piotr Halama <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to all activities around CLI cla: yes Indicates the PR's author has signed the CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants