Look into providing container credentials #30

migmartri · 2021-07-28T16:54:36Z

Currently we load them through the environment, we want to look into supporting explicit providing of credentials maybe via config files.

josvazg · 2021-09-20T17:21:05Z

So at the API level I think the ide would be to go from:

func NewChartMover(chartPath string, imageHintsFile string, rules *RewriteRules, opts ...Option) (*ChartMover, error) {

To

func NewChartMover(ChartMoveRequest req, imageHintsFile string,  opts ...Option) (*ChartMover, error) {

Where ChartMoveRequest (or whatever we end up calling it) has all we need to define a chart move other the image hints file and the options.

I will play a bit on a merge request.

josvazg · 2021-09-20T17:50:26Z

WIP here: #70

migmartri · 2021-09-21T08:41:27Z

func NewChartMover(ChartMoveRequest req, imageHintsFile string, opts ...Option) (*ChartMover, error) {

I am not convinced by this change. To me, opts are optional modifiers. If the rewriteRules (and soon the creds/repo defs) are required, that should be a required parameter. Could we evolve the signature to instead accept a struct that encapsulates both the source and targets?

I'd keep in mind that whatever we model should leave room to source/targets that are potentially something different than OCI registries, i.e for Airgap envs. To me, chart syncer does a good job defining these inputs https://github.com/bitnami-labs/charts-syncer/blob/master/api/config.proto

josvazg · 2021-09-23T17:24:05Z

For testing maybe we can spin up a local registry with some injected creds:
https://docs.docker.com/registry/deploying/

I will be testing that next.

josvazg · 2021-09-24T17:32:20Z

I think it can be done.

I can run a docker-compose with the registry and relok8s using it. We can create a password beforehand and then docker-compose up -d could run some test passing creds on the CLI... bu that requires to implement the CLI arg passing as well.

I will first get it working "manually" in my laptop.

josvazg · 2021-09-27T18:12:50Z

I am trying to automate a test that spins up a local registry, with a random password generated on the fly, but I hit a more fundamental issue...

Our Dockerfile fails to build a docker image for the project because it assumes build/relok8s_linux exists, which it might not, as the Dockefile does not include building.
If you try to add a building stage, make test && make build fails with:

go: finding module for package github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/pkg/mover
go: finding module for package github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/internal/internalfakes
github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes imports
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/internal/yamlops: no matching versions for query "latest"
github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes imports
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/pkg/mover: no matching versions for query "latest"
github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/internalfakes imports
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/internal: no matching versions for query "latest"
github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes tested by
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes.test imports
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/test: no matching versions for query "latest"
github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/mover tested by
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/mover.test imports
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/internal/internalfakes: no matching versions for query "latest"
github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/mover tested by
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/mover.test imports
	github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/pkg/mover/moverfakes: no matching versions for query "latest"
make: *** [Makefile:57: vendor/modules.txt] Error 1

I have to stop the yak shaving here for today.

If any of you have a hint on why I am hitting that error, it will be most welcome.
cc @migmartri @petewall

petewall · 2021-09-27T18:49:58Z

Fair enough. The docker build expects that you've already built the binary. Do you have a branch with your build stage? I could try and iterate on it.

josvazg · 2021-09-27T18:52:04Z

Never mind, it was me not using a proper folder name for the build files.

josvazg · 2021-09-27T18:58:10Z

I was trying

# Copyright 2021 VMware, Inc.
# SPDX-License-Identifier: BSD-2-Clause
FROM golang:1.16

COPY Makefile go.* build cmd pkg internal test vendor /asset-relocation-tool-for-kubernetes/ 
ENV GOPATH=
WORKDIR /asset-relocation-tool-for-kubernetes/
RUN make test && make build

...

After setting the folder it is still failing the same way, maybe I typed it wrong?

I need to leave now.

josvazg · 2021-09-27T20:33:40Z

https://newbedev.com/how-to-copy-multiple-files-in-one-layer-using-a-dockerfile
^that was the issue, COPY was not doing what I meant it to do.

migmartri · 2021-09-28T08:07:55Z

Sorry I am missing some context. What is it of the new change that require having a local docker env?

Or is it that you want to add additional testing for different combinations of credentials for both source and target?

josvazg · 2021-09-28T09:51:59Z

Sorry, let me back up in the yak shaving sequence a bit...

So this change in draft for providing credentials as inputs needs some testing....

Unit testing will be a bit silly, as Go is strongly types I know the implementation of the authn.Keychain should be mostly correct, or the compiler will complain. What I need is a real use test against an actual registry to verify the passed in creds do actually work as expected.
You can run a local registry from a container with certain ease, including with adhoc creds. See https://docs.docker.com/registry/deploying/
So the idea is to setup a test that ideally runs with a docker-compose up and not much else. Spins up a registry and relok8s and does a tets move with credentials, or runs a program that runs the API move with credentials (as we do not have the CLI interface for custom creds now)
But for that I needed to get the relok8s container, preferably self built, but it does assume the binary is already there...
So I was trying to make the Dockerfile self build the container for Linux.
I managed to get it working this morning, now I need to try to complete the test with the registry.

petewall · 2021-09-28T14:59:08Z

Just to make sure, your build step should probably accept an ARG for the CLi version, so the version number is properly built into the relok8s binary. make build alone will embed dev.

josvazg · 2021-09-28T17:54:54Z

Just to make sure, your build step should probably accept an ARG for the CLi version, so the version number is properly built into the relok8s binary. make build alone will embed dev.

#72

ah yes I will have to fix that there ^

I was more focused on the registry test, that got more complex again, I cannot avoid TLS config apparently, so I need to create "proper" cert for the test...

I will fix that PR tomorrow as well

josvazg · 2021-09-29T07:44:04Z

https://github.com/vmware-tanzu/asset-relocation-tool-for-kubernetes/tree/registry-tests
^is a branch in were you can see what I have tested so far

I am close now to get the regular "docker based creds tests" working. In fact I am getting 401 for not good reason, as the docker login is said to be "successful?!" Not sure why is still failing.
The idea will be to test the regular docker based creds and also the passed in ones.

migmartri · 2021-10-04T09:49:14Z

Hi,

Thanks for the proposal #70 (comment) , I like it and I think it is on the right direction, I went ahead and came up with a modified version of it that, which changes can be summarized as

Makes sure that the Chart source and targets are objects and can contain different chart providers (similar to chart syncer). it's true that I make bundle part of the chart source object even though it will contain images too, but I think that's ok, makes sense in my mental model and it's better in my opinion than polluting the root object.
I choose a more protobuffers friendly syntax i.e oneof source.local|Helmrepository, instead of a selector based on a kind property.
I put the rewrites inside the target, I wan't sure about it but in some way it makes sense. In the same way you indicate the hints file in the source object, rewrites is a property that defines the target.

Let me know your thoughts,

Thanks!!
Miguel

Examples:

Example 1

Local Chart
No container registry credentials required or loaded from system
target chart name generated automatically

NOTE: This represents the current default behavior of relok8s

source: 
  hintsFile: images.yaml
  chart:
    local:
      path: "chart.tgz"
target:
  containerRewrites:
    registry: "final-dest.harbor.vmware.com"
    prefix: /custom

Example 2

Local Chart
No credentials required or loaded from system
Custom target chart path chart.relocated.tgz

source: 
  hintsFile: images.yaml
  chart:
    local:
      path: "chart.tgz"
target:
  containerRewrites:
    registry: "final-dest.harbor.vmware.com"
    prefix: /custom 
  chart:
    local:
      path: "my-path/*.relocated.tgz"

Example 3

Local Chart
Explicit credentials for the container images for both the source and the target

source: 
  hintsFile: images.yaml
  chart:
    local:
      path: "./chart.tgz"
  containers:
    repositories:
      - server: internal.harbor.vmware.com
        username: foobar
        password: deadbeef
target:
  containerRewrites:
    registry: "final-dest.harbor.vmware.com"
    prefix: /custom 
  containers:
    repositories:
      - server: final-dest.harbor.vmware.com
        username: foobar
        password: deadbeef

Example 4: Two steps relocation

Relocation happens in two steps
- Pull images from a private repository and store it in a remote bundle in also a private OCI registry (path will be also supported)
- Take bundle as source and finalize the rewrite.

# First leg
source: 
  hintsFile: ./images.yaml
  chart:
    local:
      path: "./tarball.tgz"
  containers:
    repositories:
      - server: internal.harbor.vmware.com
        username: foobar
        password: deadbeef
target:
  chart:
    # The bundle contains the Helm Chart, it's container images as well as the hints file
    partialRelocationBundle: 
      uri: internal.harbor.vmware.com/chart-bundle:1.0
      username: foobar
      password: deadbeef

# Second leg
source: 
  chart:
    partialRelocationBundle: 
      uri: internal.harbor.vmware.com/chart-bundle:1.0
      username: foobar
      password: deadbeef
target:
  containerRewrites:
    registry: "final-dest.harbor.vmware.com"
    prefix: /custom 
  chart:
    local:
      path: "./tarball-v1.0.relocated.tgz"
  containers:
    repositories:
      - server: final-dest.harbor.vmware.com
        username: foobar
        password: deadbeef

All the options

source: 
  hintsFile: ./images.yaml
  chart:
   # one of these is required
    local:
      path: "./tarball.tgz"
    HelmRepository:
      name: "mychart"
      repository:
        url: https://charts.bitnami.com/bitnami
        username: optional
        password: deadbeef
    HelmOCI: 
      uri: internal.harbor.vmware.com/tests/mychart
      username: foo
      password: bar
    partialRelocationBundle: # Used for intermediate
      uri: internal.harbor.vmware.com/chart-bundle
      username: foobar
      password: deadbeef
  containers:
    repositories:
      - server: internal.harbor.vmware.com
        username: foobar
        password: deadbeef
target:
  containerRewrites:
    registry: "final-dest.harbor.vmware.com"
    prefix: /custom 
  chart:
    local:
      path: "my-path/*.relocated.tgz" 
    # + OCI + Chart Museum
  containers:
    repositories:
      - server: final-dest.harbor.vmware.com
        username: foobar
        password: deadbeef

josvazg · 2021-10-05T15:35:34Z

Thanks I now understand what you were after and why.

Let me write down the minimum structure we are to support now:

source: 
  hintsFile: images.yaml # Optional: can be in the original chart itself
  chart:
    local: # we will also support `helmRepo` and `helmOCI` 
      path: "./tarball.tgz"
  containers:
    repository: # we mentioned going for a single authenticated source repo initially
      server: internal.harbor.vmware.com
      username: foobar # auth can be of other types, but for now we just support user/password
      password: deadbeef
target:
  containerRewrites:
    registry: "final-dest.harbor.vmware.com"
    prefix: /custom 
  chart:
    local:
      path: "my-path/*.relocated.tgz" 
  containers:
    repository:
      server: final-dest.harbor.vmware.com
      username: foobar
      password: deadbeef

Notes:

Moving within the same repository with different credentials, for different users or accounts is the main reason to separate the source and destination credentials, as we cannot resolve the credentials to us by the repository and path, but just the host.
The use case of moving within the same repository with the same credentials is so rare that forcing the user to repeat justifies this setup without any extra fallback.

josvazg · 2021-10-14T17:29:52Z

With #72 & #73 landed we now need to finish and merge #70.

Then we will need:

A PR to make registry tests use custom credentials instead of just docker ones.
Another PR to use the registry tests in the CI or github actions.

josvazg · 2021-11-11T10:13:59Z

At this point API credentials are supported at the API level.

We are not going to add support at the CLI level for now, as we are prioritizing the Airgap support case instead for now.

migmartri · 2022-02-04T13:44:04Z

Closing this issue in favor of a CLI specific one #141

Thanks for the implementation @josvazg!

josvazg self-assigned this Sep 15, 2021

josvazg mentioned this issue Oct 5, 2021

Passing creds to the API #70

Merged

migmartri closed this as completed Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Look into providing container credentials #30

Look into providing container credentials #30

migmartri commented Jul 28, 2021

josvazg commented Sep 20, 2021

josvazg commented Sep 20, 2021

migmartri commented Sep 21, 2021 •

edited

Loading

josvazg commented Sep 23, 2021

josvazg commented Sep 24, 2021

josvazg commented Sep 27, 2021 •

edited

Loading

petewall commented Sep 27, 2021

josvazg commented Sep 27, 2021

josvazg commented Sep 27, 2021 •

edited

Loading

josvazg commented Sep 27, 2021 •

edited

Loading

migmartri commented Sep 28, 2021 •

edited

Loading

josvazg commented Sep 28, 2021 •

edited

Loading

petewall commented Sep 28, 2021

josvazg commented Sep 28, 2021

josvazg commented Sep 29, 2021

migmartri commented Oct 4, 2021 •

edited

Loading

josvazg commented Oct 5, 2021 •

edited

Loading

josvazg commented Oct 14, 2021

josvazg commented Nov 11, 2021

migmartri commented Feb 4, 2022

Look into providing container credentials #30

Look into providing container credentials #30

Comments

migmartri commented Jul 28, 2021

josvazg commented Sep 20, 2021

josvazg commented Sep 20, 2021

migmartri commented Sep 21, 2021 • edited Loading

josvazg commented Sep 23, 2021

josvazg commented Sep 24, 2021

josvazg commented Sep 27, 2021 • edited Loading

petewall commented Sep 27, 2021

josvazg commented Sep 27, 2021

josvazg commented Sep 27, 2021 • edited Loading

josvazg commented Sep 27, 2021 • edited Loading

migmartri commented Sep 28, 2021 • edited Loading

josvazg commented Sep 28, 2021 • edited Loading

petewall commented Sep 28, 2021

josvazg commented Sep 28, 2021

josvazg commented Sep 29, 2021

migmartri commented Oct 4, 2021 • edited Loading

Examples:

Example 1

Example 2

Example 3

Example 4: Two steps relocation

All the options

josvazg commented Oct 5, 2021 • edited Loading

josvazg commented Oct 14, 2021

josvazg commented Nov 11, 2021

migmartri commented Feb 4, 2022

migmartri commented Sep 21, 2021 •

edited

Loading

josvazg commented Sep 27, 2021 •

edited

Loading

josvazg commented Sep 27, 2021 •

edited

Loading

josvazg commented Sep 27, 2021 •

edited

Loading

migmartri commented Sep 28, 2021 •

edited

Loading

josvazg commented Sep 28, 2021 •

edited

Loading

migmartri commented Oct 4, 2021 •

edited

Loading

josvazg commented Oct 5, 2021 •

edited

Loading