Enhancement issue#60

[MChorfa]

#60 (comment)

[msftclas]

All CLA requirements met.

[vdice]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vdice]

This looks great @MChorfa ! A few comments to tend to, but otherwise looking good.

[MChorfa]

Hi @vdice,
Thank you for the feedback. The remaining question is regarding

  cafile: "path/to/cafile"
  certfile: "path/to/certfile"
   keyfile: "path/to/keyfile"
   username: "username"
   password: "password"

I tried to define them via credentials block into the porter file, but, at build it doesn't seem to pick it up.

mixins:
  - helm:
      repositories:
        stable:
          url: "https://kubernetes-charts.storage.googleapis.com"
          username: "$${HELM_USERNAME}"
          password: "$${HELM_PASSWORD}"

name: helm-mysql
version: 0.1.0
tag: getporter/helm-mysql:v0.1.0

credentials:
  - name: kubeconfig
    path: /root/.kube/config
  - name: helm_username
    env: "HELM_USERNAME"
  - name: helm_password
    env: "HELM_PASSWORD"

I tried also with : {{ bundle.credentials.helm_username }} and {{ bundle.credentials.helm_password }}. Same issue the build breaks.
So, the only hacky way is to define those values directly into the Dockerfile by using the environment variables or only support the public repositories without credentials.
Any suggestions?

[vdice]

Tested this branch and works great! Had a few more comments...

[vdice]

Any suggestions?

Ah, right. Interpolation of creds (via e.g. {{ bundle.credentials.helm_username }}) at the mixin config level wouldn't quite work as that section is added at build time but actual interpolation is currently handled at run time when running an action... We may need to think about how to best support this.

[vdice]

@MChorfa brainstorming on the username/password issue. For a workaround, we could go the route of adding files for both, just like the potential files for CA.cert/key, etc.

Assuming the files of repo-username and repo-password exist in our Porter bundle directory, containing appropriate values, we can then have this in our mixin config:

mixins:
  - helm:
      repositories:
        brigadecore:
          url: "https://brigadecore.github.io/charts"
          username: '"$(cat /repo-username)"'
          password: '"$(cat /repo-password)"'

The Dockerfile then shows:

RUN helm repo add brigadecore https://brigadecore.github.io/charts --username "$(cat /repo-username)" --password "$(cat /repo-password)"

Do you have a private Helm chart repo you can test this approach with?

It's still not ideal, but could be a good way to document for the time being and still get your additions/support for these fields in now. Then, we can decide on follow-up work for better UX, etc.

[MChorfa]

I can't reply to your comment directly ?

[MChorfa]

No i don't have a private helm repo... will setup one like chartmuseum next.

[MChorfa]

Assuming the files of repo-username and repo-password exist in our Porter bundle directory.

Does this mean that we have to embed the creds file into the invocation image?

[vdice]

Does this mean that we have to embed the creds file into the invocation image?

Indeed, it would :( The invocation image would need to be private. Not the best solution.

I'll keep brainstorming...

[MChorfa]

Does this mean that we have to embed the creds file into the invocation image?

Indeed, it would :( The invocation image would need to be private. Not the best solution.

I'll keep brainstorming...

The ideal way is to have porter allowing additional flags like ENV-VARS and pass them along at build time. Or, for the moment we allow only the Public repositories at the mixin config level and implement the capabilities of the credentials at runtime.

[vdice]

The ideal way is to have porter allowing additional flags like ENV-VARS and pass them along at build time. Or, for the moment we allow only the Public repositories at mixin config level and implement the credentials capabilities at runtime

Agreed. So, it sounds like we have a few potential follow-ups:

1. (Buildtime Docker flags): Adding the ability for Porter to append build arg flags to the invocation image build command.

   • In cli terms, docker build --build-arg repo_user=foo --build-arg repo_pw=password .
   • Maybe the quickest way is to enable it in the corresponding porter command: porter build --build-arg repo_user=foo ...

2. Like you suggested, if users want to inject bundle credentials into the help repo add command today, it would need to be done via a install/uninstall/etc. step currently. That may be motivation for sharing the same logic you've added but also in the install.go, upgrade.go, uninstall.go actions, etc. I'd probably still be in favor of merging this PR with auth setup also possible in the mixin config, but users would need to be aware of its current limitations (cannot currently interpolate bundle credentials). WDYT?

[MChorfa]

The ideal way is to have porter allowing additional flags like ENV-VARS and pass them along at build time. Or, for the moment we allow only the Public repositories at mixin config level and implement the credentials capabilities at runtime

Agreed. So, it sounds like we have a few potential follow-ups:

1. (Buildtime Docker flags): Adding the ability for Porter to append build arg flags to the invocation image build command.

   • In cli terms, docker build --build-arg repo_user=foo --build-arg repo_pw=password .
   • Maybe the quickest way is to enable it in the corresponding porter command: porter build --build-arg repo_user=foo ...

2. Like you suggested, if users want to inject bundle credentials into the help repo add command today, it would need to be done via a install/uninstall/etc. step currently. That may be motivation for sharing the same logic you've added but also in the install.go, upgrade.go, uninstall.go actions, etc. I'd probably still be in favor of merging this PR with auth setup also possible in the mixin config, but users would need to be aware of its current limitations (cannot currently interpolate bundle credentials). WDYT?

Yes, I completely agree with your strategy.
The cannot currently interpolate bundle credentials would be stated as a warning or error?

[vdice]

The cannot currently interpolate bundle credentials would be stated as a warning or error?

Good idea. Perhaps we check if any of the fields have {{ }}? Undecided if it should be a warning or an error/failure. WDYT?

[vdice]

@MChorfa Actually, I think we'll move forward with creating issues for build-time credential injection/interpolation, so perhaps we can hold off on the user warning/error if we'll have support in the near future. I'll update with a link to the ticket(s) once created.

[MChorfa]

@MChorfa Actually, I think we'll move forward with creating issues for build-time credential injection/interpolation, so perhaps we can hold off on the user warning/error if we'll have support in the near future. I'll update with a link to the ticket(s) once created.

Hi @vdice, So this PR will be merged? In the meantime, I am planning to start the implementation to support the same logic in [ install.go, upgrade.go, uninstall.go ] actions, etc. WDYT?

[vdice]

Hi @vdice, So this PR will be merged? In the meantime, I am planning to start the implementation to support the same logic in [ install.go, upgrade.go, uninstall.go ] actions, etc. WDYT?

Yes, let's proceed with merging this PR... lemme re-run CI and take a final look.

As for the second question, Do we think adding support for the same logic in the standard actions will be useful after we have build-time support for credentials?

[vdice]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MChorfa]

Hi @vdice, So this PR will be merged? In the meantime, I am planning to start the implementation to support the same logic in [ install.go, upgrade.go, uninstall.go ] actions, etc. WDYT?

Yes, let's proceed with merging this PR... lemme re-run CI and take a final look.

As for the second question, Do we think adding support for the same logic in the standard actions will be useful after we have build-time support for credentials?

I was thinking about the transitive charts?

[vdice]

I was thinking about the transitive charts?

@MChorfa Oh, perhaps for a bundle that itself stood up a chart repository and then further along needed to log into it? Anyways, if you get a chance, can you file an issue for what you envision the follow-up looking like? Thank you!

[MChorfa]

I was thinking about the transitive charts?

@MChorfa Oh, perhaps for a bundle that itself stood up a chart repository and then further along needed to log into it? Anyways, if you get a chance, can you file an issue for what you envision the follow-up looking like? Thank you!

HI @vdice,
Afterthoughts the config at Buildtime is the way to go because even if we set up a chart registry within the bundle. We don't get much of the benefits of implementing the mixin configuration at Runtime. Probably it is much easier to keep the chart registry outside the bundle.
Thank you!