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 jib-maven 0.3 with certficate support for registry #691

Merged
merged 2 commits into from
Apr 8, 2021

Conversation

piyush-garg
Copy link
Contributor

This will add jib-maven 0.3 version with support for providing cert file
to talk to insecure registry

Fix #686

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes docs (if user facing)
  • Includes tests (if functionality of task changed or new task added)
  • Commit messages follow commit message best practices
  • Complies with Catalog Organization TEP, see example. Note An issue has been filed to automate this validation
    • File path follows <kind>/<name>/<version>/name.yaml

    • Has README.md at <kind>/<name>/<version>/README.md

    • Has mandatory metadata.labels - app.kubernetes.io/version the same as the <version> of the resource

    • Has mandatory metadata.annotations tekton.dev/pipelines.minVersion

    • mandatory spec.description follows the convention

        ```
      
        spec:
          description: >-
            one line summary of the resource
      
            Paragraph(s) to describe the resource.
        ```
      

See the contribution guide
for more details.


@tekton-robot tekton-robot requested review from a user and vinamra28 April 7, 2021 13:14
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 7, 2021
@tekton-robot
Copy link

Catlin Output

FILE: task/jib-maven/0.3/jib-maven.yaml
WARN : Step "build-and-push" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation
WARN : Step "digest-to-results" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation

@vinamra28
Copy link
Member

/retest

Copy link
Member

@chmouel chmouel left a comment

Choose a reason for hiding this comment

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

script: |
#!/bin/sh

[[ -f /etc/ssl/certs/$(params.CACERTFILE) ]] && \
Copy link
Member

Choose a reason for hiding this comment

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

$(workspaces.sscertdir.path) ?

Copy link
Contributor Author

@piyush-garg piyush-garg Apr 7, 2021

Choose a reason for hiding this comment

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

In case of scenarios, like user did not specify workspace but the operator is mounting the cert in taskrun pod, then this is not getting applied, so made it by explicitly specifying the path

workingDir: $(workspaces.source.path)/$(params.DIRECTORY)
env:
- name: HOME
value: /workspace
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to do this ? This usually can get problematic to redefine HOME generally speaking...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This I just made the same as the previous version. If this is not required I can remove it. Task working fine without this also c @chanseokoh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR which added this #407

TMD=$(mktemp -d)

# Generate SSL Certificate
openssl req -newkey rsa:4096 -nodes -sha256 -keyout "${TMD}"/ca.key -x509 -days 365 \
Copy link
Member

Choose a reason for hiding this comment

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

we should probably make that as default for add_sidecar_registry or add it as a add_sidecar_secure_registry if that's problematics for other tasks...

Copy link
Contributor Author

@piyush-garg piyush-garg Apr 8, 2021

Choose a reason for hiding this comment

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

Sure, I can add a new func for secure registry and use that in all tasks, maybe will handle it in different PR with changes in all tasks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done the changes in the same PR now

@vinamra28
Copy link
Member

message: 'Pipeline jib-maven-0-3/jib-maven-test-pipeline can''t be Run; it contains
        Tasks that don''t exist: Couldn''t retrieve Task "git-clone": clustertasks.tekton.dev
        "git-clone" not found'

reason for failure

@tekton-robot
Copy link

Catlin Output

FILE: task/jib-maven/0.3/jib-maven.yaml
WARN : Step "build-and-push" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation
WARN : Step "digest-to-results" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation

This will add jib-maven 0.3 version with support for providing cert file
to talk to insecure registry

Fix tektoncd#686
@piyush-garg
Copy link
Contributor Author

/test pull-catalog-catlin-lint

Copy link
Member

@vinamra28 vinamra28 left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2021
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021
@chmouel
Copy link
Member

chmouel commented Apr 8, 2021

/lgtm

thanks

@chmouel
Copy link
Member

chmouel commented Apr 8, 2021

There is the documentation to update tho to add that new add_sidecar_secure_registry, any chances you can update it ?

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2021
@afrittoli
Copy link
Member

/test pull-catalog-catlin-lint

1 similar comment
@afrittoli
Copy link
Member

/test pull-catalog-catlin-lint

@tekton-robot
Copy link

Catlin Output

FILE: task/jib-maven/0.3/jib-maven.yaml
WARN : Step "build-and-push" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation
WARN : Step "digest-to-results" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation

Created a function to run secure sidecar registry with cert
and calling that function from all tests to remove duplication
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2021
@tekton-robot
Copy link

Catlin Output

FILE: task/jib-maven/0.3/jib-maven.yaml
WARN : Step "build-and-push" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation
WARN : Step "digest-to-results" uses image "$(params.MAVEN_IMAGE)" that contains variables; skipping validation

@chmouel
Copy link
Member

chmouel commented Apr 8, 2021

/hold cancel

/lgtm

thanks

@tekton-robot tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 8, 2021
@piyush-garg
Copy link
Contributor Author

/test pull-tekton-catalog-integration-tests

@tekton-robot
Copy link

catlin.txt

@piyush-garg
Copy link
Contributor Author

/test pull-tekton-catalog-integration-tests

@piyush-garg
Copy link
Contributor Author

Error

Unable to connect to the server: net/http: request canceled (Client.Timeout exceeded while awaiting headers)

@tekton-robot
Copy link

catlin.txt

@vinamra28
Copy link
Member

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Custom CA Certs in jib-maven task
6 participants