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

chore(helm): enhance securityContext #6523

Merged
merged 3 commits into from
Sep 24, 2024
Merged

chore(helm): enhance securityContext #6523

merged 3 commits into from
Sep 24, 2024

Conversation

blanchardma
Copy link
Contributor

What type of PR is this?

/kind feature
/kind cleanup

What this PR does / why we need it:

This pull request improves the securityContext example (commented out) within the Cluster Autoscaler Helm chart values.
These modifications are based on widely accepted security best practices like this.

The runAsUser and runAsGroup will be adjusted from 1001 to 65534, like the other examples in this repo to deploy the Cluster Autoscaler.

Which issue(s) this PR fixes:

Fixes # NONE

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 9, 2024
@blanchardma blanchardma changed the title chore(helm): enhance securityContext [DRAFT] chore(helm): enhance securityContext Feb 9, 2024
@Shubham82
Copy link
Contributor

@blanchardma, Please address the failed test, which is required to pass to merge the PR.
It failed because you didn't bump the Chart version, so you need to bump the version to pass the failed test.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 12, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2024
@blanchardma blanchardma changed the title [DRAFT] chore(helm): enhance securityContext chore(helm): enhance securityContext Feb 12, 2024
@Shubham82
Copy link
Contributor

Thanks @blanchardma
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2024
@blanchardma
Copy link
Contributor Author

@Shubham82 I just squash commits into one.

@Shubham82
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2024
@Shubham82
Copy link
Contributor

@blanchardma, please resolve the merge conflicts.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 3, 2024
@Shubham82
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2024
@Shubham82
Copy link
Contributor

cc @gjtempleton @mwielgus
could you please approve this PR so that it will merge?

Thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2024
@Shubham82
Copy link
Contributor

@blanchardma please resolve the merge conflict.

@Shubham82
Copy link
Contributor

@blanchardma, is there any update on the merge conflict error, please resolve it, so that it will be merged
Thanks!

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 21, 2024
@blanchardma
Copy link
Contributor Author

blanchardma commented Jun 21, 2024 •
edited
Loading

@gjtempleton @mwielgus @MaciekPytel any chance to get a review?

@Shubham82 merge conflicts are solved again.

Shubham82
Shubham82 reviewed Jun 24, 2024
View reviewed changes
charts/cluster-autoscaler/Chart.yaml Outdated
@@ -11,4 +11,4 @@ name: cluster-autoscaler
sources:
- https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler
type: application
version: 9.37.0
version: 9.37.1
Copy link
Contributor

Choose a reason for hiding this comment

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

@blanchardma, As this is a new feature request, please bump this into a minor version, not a patch version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only bump the patch version because the improvements are commented out and therefore not active, so this is more of a recommendation. But I can adapt it if you stick with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blanchardma, Hi, we update the patch version when there is some bug, but you mention this PR as a feature so I think it should be a minor bump version. So that was my only concern, other than that changes look good to me.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shubham82 I fixed the version.

Copy link
Member

Choose a reason for hiding this comment

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

I'd agree with @blanchardma here, it's an example documentation change only, we're not changing the functionality exposed at all, so a patch release increment makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd agree with @blanchardma here, it's an example documentation change only, we're not changing the functionality exposed at all, so a patch release increment makes more sense.

Thanks, @gjtempleton for clarification.

@Shubham82
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2024
@Shubham82
Copy link
Contributor

Shubham82 commented Jul 4, 2024 •
edited
Loading

Hi @gjtempleton, Needs your approval to merge this PR.
PTAL

Thanks!

gjtempleton
gjtempleton requested changes Sep 23, 2024
View reviewed changes
Copy link
Member

@gjtempleton gjtempleton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and sorry for being so slow to get to it.

One thing to fix, and then we can get this merged.

charts/cluster-autoscaler/Chart.yaml Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2024
@Shubham82
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2024
@gjtempleton gjtempleton removed the kind/feature Categorizes issue or PR as related to a new feature. label Sep 24, 2024
@gjtempleton
Copy link
Member

/kind documentation

@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Sep 24, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2024
@gjtempleton
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: blanchardma, gjtempleton

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5e1d752 into kubernetes:master Sep 24, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gjtempleton gjtempleton gjtempleton approved these changes

@mwielgus mwielgus Awaiting requested review from mwielgus

@Shubham82 Shubham82 Awaiting requested review from Shubham82

Assignees

@Shubham82 Shubham82

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/helm-charts cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blanchardma @Shubham82 @gjtempleton @k8s-ci-robot