woodpecker-ci / woodpecker

Woodpecker is a simple, yet powerful CI/CD engine with great extensibility.
https://woodpecker-ci.org
Apache License 2.0
4.29k stars 370 forks source link

Broken / incomplete implementation of from_secrets #840

Closed anbraten closed 2 years ago

anbraten commented 2 years ago

Currently the docs suggest following code to set a setting with a secret:

pipeline:
  docker:
    image: my-plugin
    settings:
+     token:
+       from_secret: secret_token

But this is missing that you still need to add secrets to the step:

pipeline:
  docker:
    image: my-plugin
    settings:
      token:
        from_secret: secret_token
    secrets:
      - secret_token

Best would be to not need to redefine secrets with values used with from_secrets, but that would require a lot of refactoring.

The easiest and best for now could be to throw an error if a from_secrets is used but not defined in secrets and add a hint to the docs to not forget it.

jolheiser commented 2 years ago

Admittedly I haven't looked if something changed, but when I made the PR you didn't need to redefine anything.

jolheiser commented 2 years ago

I currently have working pipelines that don't define secrets anywhere. If this no longer works, it's a regression.

anbraten commented 2 years ago

Weird. Im am currently trying to get this step to work:

pipeline:
  build:
    image: alpine
    commands:
      - mkdir -p docs/build/
      - echo "Hello world" > docs/build/index.html

  deploy-preview:
    image: woodpeckerci/plugin-surge-preview:next
    settings:
      path: "docs/build/"
      surge_token:
        from_secret: SURGE_TOKEN
      forge_type: github
      forge_url: "https://github.com"
      forge_repo_token:
        from_secret: GITHUB_TOKEN_SURGE

And my secrets look like:

Screenshot from 2022-03-17 21-23-43 Screenshot from 2022-03-17 21-23-28

But in the debugger the env variables passed to the docker container only contain: PLUGIN_PATH=docs/build/ and PLUGIN_SURGE_TOKEN=.

jolheiser commented 2 years ago

That is strange...any errors? Looks like not even all the settings are getting injected.

My pipeline has

pipeline:
...
  release-main:
    image: ...
    settings:
      base: ...
      token:
        from_secret: gitea_token
      files:
        - "..."
        - "..."

And it has been working for me.

anbraten commented 2 years ago

I added some "e2e" test for the whole config parsing and compiling and debugged it a bit. Seems the problem is upper / lower case related 😂

I will add some more data to the test and maybe introduce some error handling for the compiling function. Currently it only logs errors.

CC @6543

6543 commented 2 years ago

I would make this completly case insensitive and also let the backend fail if it exists ... so first a migration fo make all lowercase and then make sure in imput validation to do lowercase

or we go the upercase only road - doesnt matter

anbraten commented 2 years ago

I did a tiny fix for the mapping in #842. While doing that I noticed that we currently do not check the container-image name for from_secret I will try to fix that in a separate PR as well.

luwol03 commented 2 years ago

What is the reason why this secrets are not like expression like github uses?

anbraten commented 2 years ago

I am not 100% sure, but I think its based on the idea to have additional security restrictions like an image filter per secret. Which would be pretty hard to implement in a expression replacing preprocessing step.

6543 commented 2 years ago

(https://github.com/woodpecker-ci/woodpecker/pull/925)