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

WIP set service account for dev container via component attributes #6111

Closed

Conversation

vinny-sabatini
Copy link
Contributor

@vinny-sabatini vinny-sabatini commented Sep 9, 2022

Signed-off-by: Vinny Sabatini vincent.sabatini@kohls.com

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Allow users to specify what Kubernetes service account the pod should run as for an odo dev session.
This is generally useful if you want your workload to have additional permissions within a Kubernetes cluster
and you do not want to grant additional access to the default service account.

If the attribute is not set, the default service account will be used.

Which issue(s) this PR fixes:

Fixes #5977

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

  • In your Kubernetes cluster, create a service account
  • In your devfile, set .components.attributes.serviceAccountName for your container component to the name of the service account you created
    components:
    - attributes:
        serviceAccountName: my-service-account
      container:
        dedicatedPod: false
        endpoints:
        - name: http
          secure: false
          targetPort: 8080
        image: quay.io/devfile/golang:latest
        memoryLimit: 1024Mi
        mountSources: true
      name: runtime
  • Start odo dev
  • Check .spec.template.spec.serviceAccountName on the deployment, or .spec.serviceAccountName of the running pod

Today, the devfile API does not allow you to set the Service Account for
a Container component, so attributes are being used instead.

Signed-off-by: Vinny Sabatini <vincent.sabatini@kohls.com>
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Sep 9, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 9, 2022

Hi @vinny-sabatini. Thanks for your PR.

I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@netlify
Copy link

netlify bot commented Sep 9, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 63e11ca
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/631b999e361db200086d25a8
😎 Deploy Preview https://deploy-preview-6111--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@openshift-ci
Copy link

openshift-ci bot commented Sep 9, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kadel for approval by writing /assign @kadel in a comment. 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

@odo-robot
Copy link

odo-robot bot commented Sep 9, 2022

Unit Tests on commit finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 9, 2022

Validate Tests on commit finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 9, 2022

Kubernetes Tests on commit finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 9, 2022

OpenShift Tests on commit finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Sep 9, 2022

Windows Tests (OCP) on commit finished with errors.
View logs: TXT HTML

@valaparthvi
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Sep 12, 2022
@kadel
Copy link
Member

kadel commented Sep 14, 2022

Hi @vinny-sabatini, earlier this week, there was a discussion on one of the devfile issues. And the solution that was proposed there would also address the "ServiceAccount" issue as well but in a more generic way.

You can see what is being proposed here devfile/api#920 (comment)

@vinny-sabatini
Copy link
Contributor Author

@kadel thanks for the info! That solution looks much more flexible than trying to key every single customization and overloading the devfile API spec. I can work on updating this PR to follow the convention mentioned in that issue

@openshift-ci
Copy link

openshift-ci bot commented Sep 15, 2022

@vinny-sabatini: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v4.10-integration-e2e 63e11ca link true /test v4.10-integration-e2e
ci/prow/v4.11-integration-e2e 63e11ca link true /test v4.11-integration-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@kadel
Copy link
Member

kadel commented Sep 16, 2022

@kadel thanks for the info! That solution looks much more flexible than trying to key every single customization and overloading the devfile API spec. I can work on updating this PR to follow the convention mentioned in that issue

odo uses devfile/library for generating k8s resources from devfile. I think that this logic should be implemented in the devfile/library. There is a set of functions in the generator package that odo uses. Those functions should be updated to use the information from the annotations.

@vinny-sabatini
Copy link
Contributor Author

This was done in #6512
Closing this PR

@vinny-sabatini vinny-sabatini deleted the issue-5977 branch January 23, 2023 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to specify ServiceAccount for odo dev
3 participants