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

Add variable expansion to URLs for values #169

Merged
merged 2 commits into from
Nov 23, 2021
Merged

Add variable expansion to URLs for values #169

merged 2 commits into from
Nov 23, 2021

Conversation

jkroepke
Copy link
Owner

@jkroepke jkroepke commented Nov 22, 2021

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@github-actions
Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors
'shellcheck -x' found no issues.

shfmt errors

'shfmt ' returned error 1 finding the following formatting issues:

----------
--- scripts/lib/common.sh.orig
+++ scripts/lib/common.sh
@@ -81,16 +81,16 @@
 
 # https://stackoverflow.com/a/40167919
 expand_vars_strict() {
-  while IFS= read -r line || [ -n "${line}" ]; do  # the `||` clause ensures that the last line is read even if it doesn't end with \n
-    # Escape ALL chars. that could trigger an expansion..
-    lineEscaped=$(
-        printf %s "$line" | \
-        tr '`([$' '\1\2\3\4' | \
-        # ... then selectively reenable ${ references
-        sed -e 's/'"$(printf '\x4')"'{/\${/g' | \
-        # Finally, escape embedded double quotes to preserve them.
-        sed -e 's/"/\\\"/g' \
-    )
-    eval "printf '%s\n' \"$lineEscaped\"" | tr '\1\2\3\4' '`([$'
-  done
+    while IFS= read -r line || [ -n "${line}" ]; do # the `||` clause ensures that the last line is read even if it doesn't end with \n
+        # Escape ALL chars. that could trigger an expansion..
+        lineEscaped=$(
+            printf %s "$line" |
+                tr '`([$' '\1\2\3\4' |
+                # ... then selectively reenable ${ references
+                sed -e 's/'"$(printf '\x4')"'{/\${/g' |
+                # Finally, escape embedded double quotes to preserve them.
+                sed -e 's/"/\\\"/g'
+        )
+        eval "printf '%s\n' \"$lineEscaped\"" | tr '\1\2\3\4' '`([$'
+    done
 }
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename


@jkroepke jkroepke changed the title Add variable expansion to URLs Add variable expansion to URLs for values Nov 22, 2021
@github-actions
Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors
'shellcheck -x' found no issues.

shfmt errors

'shfmt ' returned error 1 finding the following formatting issues:

----------
--- scripts/lib/expand_vars_strict.sh.orig
+++ scripts/lib/expand_vars_strict.sh
@@ -5,15 +5,15 @@
 # https://stackoverflow.com/a/40167919
 expand_vars_strict() {
     _x4=$(printf '\x4')
-    while IFS= read -r line || [ -n "${line}" ]; do  # the `||` clause ensures that the last line is read even if it doesn't end with \n
+    while IFS= read -r line || [ -n "${line}" ]; do # the `||` clause ensures that the last line is read even if it doesn't end with \n
         # Escape ALL chars. that could trigger an expansion..
         lineEscaped=$(
-            printf %s "$line" | \
-            tr '`([$' '\1\2\3\4' | \
-            # ... then selectively reenable ${ references
-            sed -e "s/$_x4{/\${/g" | \
-            # Finally, escape embedded double quotes to preserve them.
-            sed -e 's/"/\\\"/g' \
+            printf %s "$line" |
+                tr '`([$' '\1\2\3\4' |
+                # ... then selectively reenable ${ references
+                sed -e "s/$_x4{/\${/g" |
+                # Finally, escape embedded double quotes to preserve them.
+                sed -e 's/"/\\\"/g'
         )
         eval "printf '%s\n' \"$lineEscaped\"" | tr '\1\2\3\4' '`([$'
     done
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename


@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #169 (30cc2ea) into main (ad8787a) will decrease coverage by 0.27%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
- Coverage   84.77%   84.50%   -0.28%     
==========================================
  Files          23       24       +1     
  Lines         611      626      +15     
==========================================
+ Hits          518      529      +11     
- Misses         93       97       +4     
Impacted Files Coverage Δ
scripts/lib/expand_vars_strict.sh 60.00% <60.00%> (ø)
scripts/lib/file/http.sh 84.61% <100.00%> (+6.83%) ⬆️
scripts/run.sh 95.60% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad8787a...30cc2ea. Read the comment docs.

@github-actions
Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors
'shellcheck -x' found no issues.

shfmt errors

'shfmt ' returned error 1 finding the following formatting issues:

----------
--- scripts/lib/expand_vars_strict.sh.orig
+++ scripts/lib/expand_vars_strict.sh
@@ -5,15 +5,15 @@
 # https://stackoverflow.com/a/40167919
 expand_vars_strict() {
     _x4=$(printf '\x4')
-    while IFS= read -r line || [ -n "${line}" ]; do  # the `||` clause ensures that the last line is read even if it doesn't end with \n
+    while IFS= read -r line || [ -n "${line}" ]; do # the `||` clause ensures that the last line is read even if it doesn't end with \n
         # Escape ALL chars. that could trigger an expansion..
         lineEscaped=$(
-            printf %s "$line" | \
-                tr '`([$' '\1\2\3\4' | \
+            printf %s "$line" |
+                tr '`([$' '\1\2\3\4' |
                 # ... then selectively reenable ${ references
-                sed -e "s/$_x4{/\${/g" | \
+                sed -e "s/$_x4{/\${/g" |
                 # Finally, escape embedded double quotes to preserve them.
-                sed -e 's/"/\\\"/g' \
+                sed -e 's/"/\\\"/g'
         )
         eval "printf '%s\n' \"$lineEscaped\"" | tr '\1\2\3\4' '`([$'
     done
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename


@github-actions
Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors
'shellcheck -x' found no issues.

shfmt errors

'shfmt ' returned error 1 finding the following formatting issues:

----------
--- scripts/lib/expand_vars_strict.sh.orig
+++ scripts/lib/expand_vars_strict.sh
@@ -5,15 +5,15 @@
 # https://stackoverflow.com/a/40167919
 expand_vars_strict() {
     _x4=$(printf '\x4')
-    while IFS= read -r line || [ -n "${line}" ]; do  # the `||` clause ensures that the last line is read even if it doesn't end with \n
+    while IFS= read -r line || [ -n "${line}" ]; do # the `||` clause ensures that the last line is read even if it doesn't end with \n
         # Escape ALL chars. that could trigger an expansion..
         lineEscaped=$(
-            printf %s "$line" | \
-                tr '`([$' '\1\2\3\4' | \
+            printf %s "$line" |
+                tr '`([$' '\1\2\3\4' |
                 # ... then selectively reenable ${ references
-                sed -e "s/$_x4{/\${/g" | \
+                sed -e "s/$_x4{/\${/g" |
                 # Finally, escape embedded double quotes to preserve them.
-                sed -e 's/"/\\\"/g' \
+                sed -e 's/"/\\\"/g'
         )
         eval "printf '%s\n' \"$lineEscaped\"" | tr '\1\2\3\4' '`([$'
     done
----------

You can reformat the above files to meet shfmt's requirements by typing:

  shfmt  -w filename


@jkroepke
Copy link
Owner Author

Hey @jmclean-starburst, is it possible that you can checkout this and test it on your setup?

@jmclean-starburst
Copy link

getting the below

error loading config: no matching creation rules found
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x5401918]

@jkroepke
Copy link
Owner Author

jkroepke commented Nov 22, 2021

Hm, it looks good in general here, because

  • no errors from curl, it means fetch the file was successful.

  • panic: runtime error: invalid memory address or nil pointer dereference
    This is expectable. helm throws a gopanic, if loading the file was not successfully.
    go panic with plugin downloader helm/helm#8935

  • error loading config: no matching creation rules found. This comes from sops maybe after fetching the content. I have no idea, why this comes. This error is expectable at encryption process, but not on the decryption process.

Set HELM_SECRETS_DEBUG=true help here to get deeper into this. This enable the shell builtin debugging.

@jmclean-starburst
Copy link

jmclean-starburst commented Nov 22, 2021

Not sure...the error is kicked right after sops:

+ _sops --decrypt --input-type yaml --output-type yaml /var/folders/pr/wrsgr_fd57bcnht422blbwq40000gn/T/tmp.9i0DFDTh/7unlRm
+ set -- --decrypt --input-type yaml --output-type yaml /var/folders/pr/wrsgr_fd57bcnht422blbwq40000gn/T/tmp.9i0DFDTh/7unlRm
+ sops --decrypt --input-type yaml --output-type yaml /var/folders/pr/wrsgr_fd57bcnht422blbwq40000gn/T/tmp.9i0DFDTh/7unlRm
error loading config: no matching creation rules found

FWIW - i pass in the url string being surrounded by single quotes (ie '), so something like:

HELM_SECRETS_URL_VARIABLE_EXPANSION=true HELM_SECRETS_DEBUG=true helm template mychart ./mychart -f 'secrets://https://${GITHUB_TOKEN}@raw.githubusercontent.com/org/repo/ref/pathtofile.yml'

I do this because I am trying to mimic what ArgoCD passes in.

@jkroepke
Copy link
Owner Author

Looks like getsops/sops#884

@jmclean-starburst
Copy link

seems like the only time you leverage the expansion is via the below, which doesnt actually update the $file variable used


    if ! _file_exists "$file"; then
        error 'File does not exist: %s\n' "${file}"
    fi

    real_file=$(_file_get "${file}")

I would have expected the real_file to fetch from the newly created url

@jkroepke
Copy link
Owner Author

jkroepke commented Nov 22, 2021

_file_get is calling _file_http_get and the functions eval the URL, download the file and returns the path of the temporary file which holds the content of the curl command.

Then, the temporary file path real_file is passed to sops.

What happens, if you are using a invalid GITHUB_TOKEN? same error?

@jmclean-starburst
Copy link

a bad token gives the below after a curl command

curl: (22) The requested URL returned error: 404 

Then I get the:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x5401918]

@jkroepke
Copy link
Owner Author

This validates that the evaluation works in general.

Maybe we are hitting getsops/sops#884 here. I do a commit here which introduce a workaround for that case.

Otherwise, the encrypted yaml file from org/repo/ref/pathtofile.yml is strange.

@jmclean-starburst
Copy link

@jkroepke definitely seems like the issue is the sops tag above....i had a local .sops.yaml file with more specific rules. When I deleted it, it likely defaulted to your file which is more of a catch al, and it worked! thanks for all the work on this. we plan on using it HEAVILY ;)

@jkroepke
Copy link
Owner Author

Thanks for the feedback. Great to know that it works now.

and it worked!

I would like to know, if I the empty sops.yaml file is still needed, because it worked years fine without it.

Before merging this, I still needs to write some documentation about this feature.

@jkroepke jkroepke merged commit 0dcbc7e into main Nov 23, 2021
@jkroepke jkroepke deleted the var_expansion branch November 23, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants