-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Generate Kube should not print default structs #12021
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8b284e0
to
949e9db
Compare
allowPrivilegeEscalation: true | ||
capabilities: {} | ||
privileged: false | ||
readOnlyRootFilesystem: false | ||
tty: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea what is the default for allowPrivilegeEscalation? The Kubernetes documentation is unclear - kubernetes/website#30104 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is allowed by default. @umohnani8 @haircommander @mrunalp Correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's on by default, though there is a PSP field that allows you to turn it off for any pod that uses the PSP (DefaultAllowPrivilegeEscalation)
allowPrivilegeEscalation: true | ||
privileged: false | ||
readOnlyRootFilesystem: false | ||
drop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation here is funky
libpod/kube.go
Outdated
@@ -489,15 +484,23 @@ func containerToV1Container(ctx context.Context, c *Container) (v1.Container, [] | |||
kubeContainer.Command = nil | |||
} | |||
|
|||
if c.WorkingDir() != "/" && !reflect.DeepEqual(imgData.Config.WorkingDir, c.WorkingDir()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what type is imgData.Config.WorkingDir that requires a deep equal here? can't it be cast to a string, which I assume will be less expensive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just cut and paste, I will fix it.
If podman uses Workdir="/" or the workdir specified in the image, it should not add it to the yaml. If Podman find environment variables in the image, they should not get added to the yaml. If the container or pod do not have changes to SELinux we should not print seLinuxOpt{} If the container or pod do not change any dns options the yaml should not have a dnsOption={} If the container is not privileged it should not have privileged=false in the yaml. Fixes: containers#11995 Signed-off-by: Daniel J Walsh <[email protected]>
@jwhonce Do you know an easy way to drop the remaining {} fields from podman generate kube.
In the print out above I would want to remove I think this is something we would need to do in the filters. Within code, I don't believe we can change them to nil. |
@containers/podman-maintainers PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/lgtm |
If podman uses Workdir="/" or the workdir specified in the image, it
should not add it to the yaml.
If Podman find environment variables in the image, they should not
get added to the yaml.
If the container or pod do not have changes to SELinux we should not
print seLinuxOpt{}
If the container or pod do not change any dns options the yaml should
not have a dnsOption={}
If the container is not privileged it should not have privileged=false
in the yaml.
Fixes: #11995
Signed-off-by: Daniel J Walsh [email protected]
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer: