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

Bind Workspaces for PipelineRun #2459

Closed
wants to merge 2 commits into from
Closed

Conversation

marniks7
Copy link
Contributor

@marniks7 marniks7 commented Sep 12, 2022

Changes

Implements #1283

  • UI: Workspaces are displayed as Dropdown with all possible kinds together: secrets, configmaps, pvc, emptyDir

Workspaces for:

  • PipelineRun
  • TaskRun
  • Run

VolumeSources:

Features:

Notes:

  • K8s proxy & Security: tekton-backend will not allow to retrieve internals (sensitive data) of configmaps, secrets or persistentvolumeclaim data by modifying response from k8s: only minimal required amount of data is preserved. Such filtration is applied to all urls with secret or configmap or persistentvolumeclaim in the path.

ANY request (not only to secrets or configmaps or persistentvolumeclaim) to namespace that contains such strings are NOT allowed as well. (this is false positive...)

  • RBAC: configmaps, secrets, persistentvolumeclaims (list and watch verbs) were added to the ClusterRole tekton-dashboard-backend.

Note, this allows to read secret data, such issue is handled by limiting what backend tekton-dashboard returns to UI. (see above)

  • E2E tests with Cypress

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

See the contribution guide
for more details.

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 12, 2022
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign alangreene after the PR has been reviewed.
You can assign the PR to them by writing /assign @alangreene in a comment when ready.

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

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 12, 2022
@tekton-robot
Copy link
Contributor

Hi @marniks7. Thanks for your PR.

I'm waiting for a tektoncd 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.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2022
@marniks7 marniks7 changed the title Early alpha version for Workspaces binding Partial: Workspaces binding for PipelineRun Sep 25, 2022
@marniks7 marniks7 changed the title Partial: Workspaces binding for PipelineRun Partial: Bind Workspaces for PipelineRun Sep 25, 2022
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2022
UI: Add workspaces dropdown with all possible volume sources for PipelineRun
Support volume sources: configmap, secret, pvc, emptydir
Backend: intercept k8s response for configmap, secret or pvc and modify response to contain only non-sensitive data
RBAC: allow to list \ watch those volume sources

Support Optional Workspaces
@marniks7 marniks7 marked this pull request as ready for review September 25, 2022 23:26
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2022
@marniks7 marniks7 changed the title Partial: Bind Workspaces for PipelineRun Bind Workspaces for PipelineRun Sep 25, 2022
Copy link
Member

@AlanGreene AlanGreene 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 @marniks7. Overall it looks like a pretty good start but as discussed on Slack could do with being broken into a series of smaller changes so we can iterate on it more easily and do a more complete review.

I have some general comments:

  • we only need to filter Secrets, the other resources do not contain sensitive information and we will actually be using ConfigMaps for other functionality in the near future and will require their full content
  • when filtering the data it should only affect resources of the correct kind, it shouldn't affect other resources that happen to have 'secret' elsewhere in the API path (e.g. namespace, resource name, etc.)
  • need to confirm that any websocket messages are also filtered, or block websocket connections for Secrets
  • the user should be able to differentiate between types of resource when configuring the workspace, e.g. identify which resource named 'foo' is a Secret vs. ConfigMap vs. PVC. We could include the type in the option text as you suggested on the issue but I think it's worth exploring other approaches here to see what works best
  • test resources should be no larger / more complex than needed to verify functionality, e.g. no need to include large cert values in secrets
  • it should be possible to run the Cypress E2E tests in isolation and against any environment (e.g. local dev environment), shouldn't rely on setup from e2e-tests.sh creating specific resources etc.
  • new files should just have current year in copyright header

I think the next step would be to open a PR with just the back end changes providing the new resources with filtering etc.

Let me know if you have any questions.

@AlanGreene AlanGreene removed the request for review from skaegi October 4, 2022 13:26
@marniks7
Copy link
Contributor Author

marniks7 commented Oct 4, 2022

Hi @AlanGreene

  • ConfigMaps, PVC are internals, it is not always a good idea to provide ability to see what is inside. for example, even here author suggests to create ConfigMap https://hub.tekton.dev/tekton/task/kubernetes-actions
  • based on kind - ok
  • websockets - ok
  • Alternative is https://github.com/redhat-developer/vscode-tekton see "Pipeline with Workspace", they provide separate field with "Type = ConfigMap \ PVC \ Secret", but i don't like this idea - making it more complex.
    ** use can filter by text
    ** there should be some separate configuration to be able to show only target type and target list of values (e.g. filtered by label) for pipeline or group of pipelines
  • large test resources - ok
  • We need to upload resources to cluster, and in case of PVC we need to wait for them to become "bound".
    So, where do I put them and how to upload to cluster?
    Alternative is to use "Import resources from repository", but not sure that it will work.
  • current year - ok

@AlanGreene
Copy link
Member

author suggests to create ConfigMap https://hub.tekton.dev/tekton/task/kubernetes-actions

ConfigMaps should not be used for sensitive info, I don't know why the author suggests that

We need to upload resources to cluster, and in case of PVC we need to wait for them to become "bound".
So, where do I put them and how to upload to cluster?

The e2e tests can execute commands on the host if needed, e.g. run kubectl to apply resources etc. See https://docs.cypress.io/api/commands/exec

If the Dashboard already has permissions to access / modify the resources in question you can also use cy.request to access the APIs directly.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@tekton-robot
Copy link
Contributor

@marniks7: PR needs rebase.

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.

@triThirty
Copy link

Hi @AlanGreene , is there any plan to merge this pr?

@AlanGreene
Copy link
Member

Thanks for your interest @triThirty. There are a number of outstanding issues that need to be addressed (some outlined in the description and comments above) and we're working through them at the moment. This PR will not be merged in its current form but we do plan to enable this functionality in a future release, keep an eye on #1283 for updates.

@AlanGreene AlanGreene closed this Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants