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

Service export should handle securityContext field #1788

Open
dsimansk opened this issue Mar 17, 2023 · 12 comments · May be fixed by #1954
Open

Service export should handle securityContext field #1788

dsimansk opened this issue Mar 17, 2023 · 12 comments · May be fixed by #1954
Assignees
Labels
good first issue Denotes an issue ready for a new contributor. kind/feature New feature or request triage/accepted Issues which should be fixed (post-triage)
Milestone

Comments

@dsimansk
Copy link
Contributor

Feature request

With more Pod Security Standards enabled clusters we need to address the securityContext being present in PodSpec and Containers definitions. Since the two cluster with different PSS config might not be compatible for the same export.

I'd suggest that we remove it in default behavior and add a flag to include it optionally

UI Example


kn service export <ksvc> --with-securityContext
or
kn service export <ksvc> --with-sc

/kind good-first-issue
/kind proposal

/cc @rhuss @mvinkler

@dsimansk dsimansk added kind/feature New feature or request good first issue Denotes an issue ready for a new contributor. labels Mar 17, 2023
@dsimansk dsimansk added this to the Knative 1.10 milestone Mar 23, 2023
@dsimansk dsimansk moved this to Ready To Work in Client Planning Mar 23, 2023
@github-project-automation github-project-automation bot moved this from Ready To Work to Backlog in Client Planning Mar 23, 2023
@dsimansk dsimansk moved this from Backlog to Ready To Work in Client Planning Mar 23, 2023
@taniaduggal
Copy link

I want to work on this issue, can you please tell from where i can start?

@rhuss
Copy link
Contributor

rhuss commented May 12, 2023

@taniaduggal, that is great! Before we start, I would like to discuss this update's background briefly.

I think this story is part of a larger story, where we allow user to filter out certain information when doing an export so that it can be later used in a GitOps scenario.

Besides the securityContext, I can also see fields we should not export by default, e.g. the namespace.

So I suggest that we make a new option flexible so that we can fine granularly specify what to include (if excluded by default) and what to included (if included by default).

My suggestion would be to have an option like

kn service export myservice --include securityContext,namespace

or

kn service export myservice --include securityContext --include namespace

@dsimansk what is your take on this ?

(using multiple options here).

@taniaduggal for starting its best to look at https://github.com/knative/client/blob/main/pkg/kn/commands/service/export.go which has all the logic for an export. This would be the place where we would evaluate that option and do the filtering.

Thanks again @taniaduggal that you want to jump on the train, greatly appreciated!

@dsimansk dsimansk modified the milestones: Knative 1.10 , Knative 1.12 Aug 3, 2023
@dsimansk dsimansk added the triage/accepted Issues which should be fixed (post-triage) label Aug 10, 2023
@Rahul-Kumar-prog
Copy link

@rhuss I read you idea and you want that users can filter certain info... that good idea.
I have some questions:
like: why we should not use

kn service export <ksvc> --with-securityContext
or
kn service export <ksvc> --with-sc

@dsimansk
Copy link
Contributor Author

@Rahul-Kumar-prog I'd go with @rhuss's suggestion, the --include flag might be more flexible in the term of adding new known variables, instread of adding the whole new flag, with near infinite growth (hopefully not).

I'd go with this variant. And the valid values are predefined and listed in the help message.

kn service export myservice --include securityContext,namespace

@rhuss
Copy link
Contributor

rhuss commented Aug 17, 2023

+1 for not use options inflationary but have an easy way to extend this feature without changing the "option API surface". In addition to provide the options as a comma separated list, multiple --include options should also work, like in --include securityContext --include namespace (it then also easy to add shortcuts like --include sc,ns without blowing up the options)

@Rahul-Kumar-prog
Copy link

Rahul-Kumar-prog commented Aug 17, 2023

Ok, @rhuss @dsimansk
I got your points and rhuss suggestion is perfect to go with.
Is there anything else I need to know?

@Rahul-Kumar-prog
Copy link

I want to work on this but i am already working on knative-kafka-broker repo issue so after finishing that i would love to work on this if its ok then please let me know.

@rhuss
Copy link
Contributor

rhuss commented Aug 21, 2023

Thanks! It's totally fine if you want to jump on this issue later, and we can then work on a PR as soon as you have something to share.

@dsimansk
Copy link
Contributor Author

+1 @rhuss

Assigning to you @Rahul-Kumar-prog to indicate it's being looked at some point. It's still ~2 months until next release v1.12. There's no time pressure right now. Feel free to proceed at your convenience.

/assign @Rahul-Kumar-prog

@EraKin575
Copy link

I would like to take this one

@Rahul-Kumar-prog
Copy link

Feel free to start working or you can assign it to yourself.

@EraKin575
Copy link

/assign

@EraKin575 EraKin575 linked a pull request Jun 22, 2024 that will close this issue
@Rahul-Kumar-prog Rahul-Kumar-prog removed their assignment Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor. kind/feature New feature or request triage/accepted Issues which should be fixed (post-triage)
Projects
Status: Ready To Work
Development

Successfully merging a pull request may close this issue.

5 participants