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

Make local registry and nginx optional #121

Closed

Conversation

ptrivedi
Copy link
Contributor

@ptrivedi ptrivedi commented May 3, 2022

This change makes registry certificate hosting URL and tink-worker image name:tag configurable.
It also adds a check for whether registry certificate is required and allow for absolute URIs
to be specified for action images in workflow template.

  • If you are using a registry with a self-signed certificate, you can specify
    a custom URL of where the certificate is hosted. On boots, use extra-kernel-args
    command-line parameter to specify a kernel argument with key 'registry_cert_url'
    and value set to the URL location for registry CA certificate. Boots will pass this
    as a kernel command-line parameter to Hook

  • In order to specify a custom absolute URI name:tag for tink-worker image instead of
    defaulting to the hardcoded default of 'tink-worker:latest', use the
    extra-kernel-args command-line parameter on Boots to specify a kernel argument with
    key 'tink_worker_image' and value set to the absolute URI of tink-worker image.
    Boots passes this to hook as a kernel command-line parameter

  • If you are using a public registry with trusted CA certificate, bootkit and
    tink-docker do not need to download and setup registry certificate. Use
    extra-kernel-args command-line parameter on Boots to set a kernel parameter with
    key 'registry_cert_required' and value set to 'true' or 'false' to indicate this.
    Since we are making local registry optional, we need to make the certificate
    download and setup optional

  • Currently the tink-worker code always prepends the specified docker_registry to
    the action images specified in the tinkerbell action template. The newly added
    use_absolute_action_image_uri allows specifying the full URI for the action image
    in the template and prevents the hardcoded prepending of the registry to these.
    This allows action images to potentially reside in different registries. Pass this
    kernel as the key=value kernel parameter from Boots to Hook as explained above. Value
    would be set to 'true' or 'false'

Description

Why is this needed

Fixes: #

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

This change makes registry certificate hosting URL and tink-worker image name:tag configurable.
It also adds a check for whether registry certificate is required and allow for absolute URIs
to be specified for action images in workflow template.

* If you are using a registry with a self-signed certificate, you can specify
  a custom URL of where the certificate is hosted. On boots, use extra-kernel-args
  command-line parameter to specify a kernel argument with key 'registry_cert_url'
  and value set to the URL location for registry CA certificate. Boots will pass this
  as a kernel command-line parameter to Hook

* In order to specify a custom absolute URI name:tag for tink-worker image instead of
  defaulting to the hardcoded default of 'tink-worker:latest', use the
  extra-kernel-args command-line parameter on Boots to specify a kernel argument with
  key 'tink_worker_image' and value set to the absolute URI of tink-worker image.
  Boots passes this to hook as a kernel command-line parameter

* If you are using a public registry with trusted CA certificate, bootkit and
  tink-docker do not need to download and setup registry certificate. Use
  extra-kernel-args command-line parameter on Boots to set a kernel parameter with
  key 'registry_cert_required' and value set to 'true' or 'false' to indicate this.
  Since we are making local registry optional, we need to make the certificate
  download and setup optional

* Currently the tink-worker code always prepends the specified docker_registry to
  the action images specified in the tinkerbell action template. The newly added
  use_absolute_action_image_uri allows specifying the full URI for the action image
  in the template and prevents the hardcoded prepending of the registry to these.
  This allows action images to potentially reside in different registries. Pass this
  kernel as the key=value kernel parameter from Boots to Hook as explained above. Value
  would be set to 'true' or 'false'

Signed-off-by: Pooja Trivedi <tripooja@amazon.com>
@ptrivedi ptrivedi force-pushed the local-registry-nginx-optional branch from 9b6dcd8 to 50a5fce Compare May 3, 2022 22:15
@@ -24,6 +24,9 @@ type tinkConfig struct {
registry string
username string
password string
certURL string
Copy link
Contributor

Choose a reason for hiding this comment

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

I think certURL makes sense here, we can probably infer registryCertRequired based on the presence of certURL

@@ -24,6 +24,9 @@ type tinkConfig struct {
registry string
username string
password string
certURL string
registryCertRequired string
useAbsoluteImageURI string
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop this parameter

@@ -9,7 +9,7 @@ import (
"io/ioutil"
"net/http"
"os"
"path"
"path"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you run gofmt on this?

@jacobweinstock
Copy link
Member

This has gone stale and i believe all of this functionality now exists. Please re-open if you feel otherwise or would like to revisit this. Thank you!

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.

None yet

3 participants