fix(actions/shared): fix action inputs, security issues and add READMEs#215
Open
borislavr wants to merge 4 commits into
Open
fix(actions/shared): fix action inputs, security issues and add READMEs#215borislavr wants to merge 4 commits into
borislavr wants to merge 4 commits into
Conversation
- Modify `create_restricted_resources` action to make inputs optional and adjust kubectl config command. - Update `get_certs` action to make `service_name` input optional. - Enhance `get_crds` action with a description and specify input type. - Revamp `helm_deploy` action documentation to clarify features, inputs, and outputs; improve usage examples. - Revise `verify_installation` action to streamline error flag initialization and enhance documentation. - Introduce `collect_diag_info` action to gather Kubernetes diagnostics and upload as artifacts. - Add `create_ingress` action for setting up ingress-nginx in kind clusters with detailed documentation. - Implement `get_certs` action to extract TLS certificate data from Kubernetes secrets with comprehensive usage notes.
There was a problem hiding this comment.
Pull request overview
This PR updates several shared composite GitHub Actions under actions/shared/ to fix correctness/security issues in action implementations (primarily around inputs and execution conditions) and to add/expand missing READMEs so callers have accurate usage + behavior documentation.
Changes:
- Relaxed/adjusted action inputs (e.g., making previously-unused inputs optional) and improved shell safety by using step env vars and quoting.
- Fixed action execution behavior issues (e.g., removing a duplicate
ERROR_FLAGinit step; ensuring monitoring diagnostics run underalways(); replacing unusablematrix.versionaccess with an explicitversioninput). - Added or significantly expanded README documentation for multiple shared actions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| actions/shared/verify_installation/README.md | Expanded documentation: inputs/behavior breakdown and usage guidance. |
| actions/shared/verify_installation/action.yml | Removes duplicated “Initialize error flag” step. |
| actions/shared/helm_deploy/README.md | Expanded documentation for behavior, inputs, and usage examples. |
| actions/shared/get_crds/action.yaml | Adds description/type and refactors to use env var for path; minor quoting improvements. |
| actions/shared/get_certs/README.md | New README documenting cert extraction behavior and usage. |
| actions/shared/get_certs/action.yml | Makes service_name optional; improves quoting/uses env vars for safer shell usage. |
| actions/shared/create_restricted_resources/README.md | Expanded documentation including side-effects, phases, and operational notes. |
| actions/shared/create_restricted_resources/action.yml | Makes some inputs optional; improves quoting; avoids kubectl config view --raw. |
| actions/shared/create_ingress/README.md | New README documenting kind ingress setup and CoreDNS changes. |
| actions/shared/collect_diag_info/README.md | New README documenting diagnostic collection behavior and artifact naming. |
| actions/shared/collect_diag_info/action.yml | Adds version input; runs monitoring diagnostics under always(); uses inputs.version instead of matrix.version. |
Comments suppressed due to low confidence (2)
actions/shared/get_crds/action.yaml:32
CRDS_ARRAYis initialised as an array, butCRDS_ARRAY+=$crd_nameconcatenates into element 0 rather than appending a new entry. If multiple CRDs exist, the output becomes a single concatenated string (no separator), which will break callers that expect a space-separated list. Use proper array append (e.g.CRDS_ARRAY+=("$crd_name")) or buildCRDSwith an explicit delimiter.
CRDS_ARRAY=()
for yaml in ${yamls}; do
yml_kind=$(yq e '.kind' $yaml)
if [ "$yml_kind" == "CustomResourceDefinition" ]; then
crd_name=$(yq e '.metadata.name' $yaml)
CRDS_ARRAY+=$crd_name
actions/shared/get_certs/action.yml:36
- The null/empty check won’t catch missing keys:
yq eval '.data["ca.crt"]'returns the literal stringnullwhen the field is absent, so-zis false and the action will writenullinto the output files. If the intent is to fail when any field is missing, either useyq -e(and check exit codes) or explicitly treatnullas an error value.
ca_crt=$(echo "$secret" | yq eval '.data["ca.crt"]' -)
tls_crt=$(echo "$secret" | yq eval '.data["tls.crt"]' -)
tls_key=$(echo "$secret" | yq eval '.data["tls.key"]' -)
if [ -z "$ca_crt" ] || [ -z "$tls_crt" ] || [ -z "$tls_key" ]; then
echo "ERROR: Failed to extract certificates!"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
244
to
248
| SERVICE_BRANCH: ${{ inputs.service_branch }} | ||
| INPUT_ARTIFACT_NAME: ${{ inputs.artifact_name }} | ||
| MATRIX_VERSION: ${{ matrix.version }} | ||
| MATRIX_VERSION: ${{ inputs.version }} | ||
| GITHUB_JOB: ${{ github.job }} | ||
| NAMESPACE: ${{ inputs.namespace }} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of change is this? (check all applicable)
Description
Fix several correctness and security issues across six shared actions and add missing
READMEs for
collect_diag_info,create_ingress, andget_certs. The fixes addressincorrectly declared
required: trueinputs that nostep actually uses, a duplicate
Initialize error flagstep inverify_installation,a monitoring diagnostic step that could be skipped on job failure, and a hard-coded
${{ matrix.version }}context read incollect_diag_infothat always resolved toempty outside a matrix job.
What is affected? (check all applicable)
.github/workflows/*workflow-config/*templates/*actions/*scripts/*docs/*resources/*orrestricted/*Services / scenarios impacted
repository_nameandpath_to_chartare now optional (callers with existingwith:blocks are unaffected)get_certs:service_nameis now optional — no caller behaviour changeverify_installation: duplicate env-var initialisation step removed (was a no-op)collect_diag_info: monitoring diagnostic step now runs underif: always()so itcollects data even when a preceding Helm deploy or verify step fails;
matrix.versioncontext access replaced with an explicit
versioninputRelated tickets and documents
actions/shared/*/README.mdVerification
All changes were verified by reading the action source and cross-checking against
callers in
.github/workflows/. No pipeline run was possible on this documentationbranch. The
required: falsechanges were confirmedsafe by grepping all callers — none would fail validation.
Compatibility / impact
inputs,outputs,secrets, job names)repository_nameandpath_to_chartincreate_restricted_resourcesandservice_namein
get_certschanged fromrequired: truetorequired: false. All existing callersalready pass these inputs explicitly, so no caller is broken. The change only relaxes
validation — previously callers that omitted these unused inputs would get an error.
Additional notes (optional)
collect_diag_infoaction previously read${{ matrix.version }}directly fromthe job matrix context, which is not accessible inside a composite action — it always
resolved to an empty string. The new
versioninput is an explicit pass-through;all current callers already supply
artifact_nameso the artifact name is unaffected.verify_installationaction still contains a duplicated monitoring check block(Phase 2 logic appears in both
Check service is readyandGet monitoring specific resourcessteps). This pre-existing duplication is documented in the README Notessection and is out of scope for this PR.