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

Split repo trusted setting #4025

Merged
merged 41 commits into from
Nov 1, 2024
Merged

Split repo trusted setting #4025

merged 41 commits into from
Nov 1, 2024

Conversation

qwerty287
Copy link
Contributor

related to #3758

@qwerty287 qwerty287 added the feature add new functionality label Aug 11, 2024
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Aug 16, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-4025.surge.sh

@qwerty287 qwerty287 marked this pull request as ready for review August 18, 2024 18:11
@qwerty287 qwerty287 requested a review from a team August 18, 2024 18:11
@qwerty287
Copy link
Contributor Author

Actually you can start taking a look here. There are probably still some issues but for the general concept.

This is currently a breaking change (changes API), we could either get it into 3.0 or make it non-breaking, what do you prefer?

Copy link
Contributor

@zc-devs zc-devs left a comment

Choose a reason for hiding this comment

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

get it into 3.0

server/store/datastore/migration/035_split_trusted.go Outdated Show resolved Hide resolved
@qwerty287 qwerty287 added this to the 3.0.0 milestone Aug 29, 2024
@qwerty287 qwerty287 added the breaking will break existing installations if no manual action happens label Aug 29, 2024
@anbraten
Copy link
Member

I am not really a fan of this split. Although I kinda see the general idea behind it, it complicates the CI (not really interested to move towards Jenkins and similar tools) and it shifts the security guidelines from the agent admin to the repo admin. Therefore multiple agents aren't able to provide different settings.

@qwerty287
Copy link
Contributor Author

it shifts the security guidelines from the agent admin to the repo admin.

Repo admins can't change this setting if they're not a server admin and usually the server admin is also the agent admin I think.

Therefore multiple agents aren't able to provide different settings.

That's a general issue, but it's impossible to deal differently with our current structure. You would need to change the whole system and give the agents more capabilities (for example, the server must not parse the YAML anymore. This must be done by agents as well).

Also, this issue is not different from how it works now. Enabling trusted is currently also on repo-level and this PR does not give any way to get more privileges than before.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@anbraten anbraten left a comment

Choose a reason for hiding this comment

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

Not confined by the benefit of splitting the setting.

@anbraten
Copy link
Member

anbraten commented Sep 6, 2024

Repo admins can't change this setting if they're not a server admin and usually the server admin is also the agent admin I think.

Isn't that the reason why this is currently an agent config (at least for the kubernetes options). It would only make sense for smaller deployments where server admins are agent admins as well I guess. For larger teams / orgs with or when supporting #267 those options would be quite unimportant.

@zc-devs
Copy link
Contributor

zc-devs commented Sep 6, 2024

this is currently an agent config (at least for the kubernetes options)

Where?

this issue is not different from how it works now. Enabling trusted is currently also on repo-level and this PR does not give any way to get more privileges than before

@qwerty287
Copy link
Contributor Author

Where?

You mean the "feature gates"?

Tbh I think this somehow would be a mess. Or we refactor our whole structure, e.g. the agent would have to parse the YAML and not the server. Also, having it on a repo level additionally can still be helpful as you might have repos that shouldn't be able to use some features.

@zc-devs
Copy link
Contributor

zc-devs commented Sep 6, 2024

You mean the "feature gates"?

I was asking. @anbraten, what did you mean by agent config? Could you provide an example?

Agent's feature gates != settings per a repo.

having it on a repo level additionally can still be helpful as you might have repos that shouldn't be able to use some features


it complicates the CI

The same way as trusted does.

not really interested to move towards Jenkins and similar tools

Could you elaborate?

it shifts the security guidelines from the agent admin to the repo admin

To the server/instance admin, if I understand correctly. The same as trusted. And doesn't shift, but adds per repo settings.

cli/exec/flags.go Outdated Show resolved Hide resolved
cli/exec/flags.go Outdated Show resolved Hide resolved
@anbraten
Copy link
Member

anbraten commented Sep 30, 2024

As we moved resource settings #3174 to the agent and as we will have #3539, I still think we should move such settings to the agent. My current idea would be to use filters like allows.volumes=true instead, so only agents supporting volumes would pick up your workflow.

@qwerty287
Copy link
Contributor Author

I understand your point, but it is not as flexible as it is currently. If you currently have a few repos that are trusted and some not, you was able to do that with one agent. With this new system, this wouldn't be possible anymore.

@qwerty287
Copy link
Contributor Author

@anbraten maybe we can just merge this now as it's ready and discuss the general approach again?

@anbraten
Copy link
Member

@anbraten maybe we can just merge this now as it's ready and discuss the general approach again?

Still not really convinced how this would really help users and think it mainly ignores UX while complicating existing options. Especially as it doesn't consider org level agents. Anyways will trust you making this helpful at some point.

@anbraten anbraten dismissed their stale review October 30, 2024 06:38

wont block

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

migration ...

@6543
Copy link
Member

6543 commented Nov 1, 2024

@qwerty287 lint err

@6543 6543 enabled auto-merge (squash) November 1, 2024 20:28
@6543 6543 merged commit 29474fc into woodpecker-ci:main Nov 1, 2024
6 of 7 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request Nov 1, 2024
1 task
@qwerty287 qwerty287 deleted the trusted-split branch November 2, 2024 05:58
@woodpecker-bot woodpecker-bot mentioned this pull request Dec 14, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking will break existing installations if no manual action happens feature add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants