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

fix: add correct binary for manager and set runAsUser: 1000 in deployment.yaml #313

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jokestax
Copy link
Contributor

@jokestax jokestax commented Dec 3, 2024

Description:

This PR addresses the issue related to the missing manager binary in the Dockerfile and an error encountered due to user permissions in the deployment.yaml.

  1. Dockerfile Update:

  2. Deployment Update:

    • Added runAsUser: 1000 to the deployment.yaml to resolve the permission error shown below:

      Permission Error

Testing

To test this change:

  1. Clone the repository.
  2. Run make build.image.controlplane.
  3. Create a Kubernetes cluster using either Kind or K3d.
  4. Run kubectl apply -k config/default to deploy the changes.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jokestax
Once this PR has been reviewed and has the lgtm label, please assign astoycos for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 3, 2024
build/Containerfile.controlplane Show resolved Hide resolved
build/Containerfile.controlplane Show resolved Hide resolved
config/manager/manager.yaml Show resolved Hide resolved
@shaneutt
Copy link
Member

shaneutt commented Dec 5, 2024

/cc @aryan9600

build/Containerfile.controlplane Show resolved Hide resolved

COPY --from=builder /workspace/manager /manager

RUN chmod +x /manager
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 don't need this line?

Copy link
Member

Choose a reason for hiding this comment

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

we do not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the late reply,been travelling to attend Kubecon India 😅,and since we have used ./manger to start the controller,not sure why we dont need this line

Copy link
Member

Choose a reason for hiding this comment

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

Because the manager binary is already executable

Copy link
Member

@aryan9600 aryan9600 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 sending this PR! needs a couple of changes.

config/manager/manager.yaml Show resolved Hide resolved

COPY --from=builder /workspace/manager /manager

RUN chmod +x /manager
Copy link
Member

Choose a reason for hiding this comment

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

we do not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants