Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

chartsync: support Git HTTPS credentials using secret #172

Merged
merged 6 commits into from
Jan 21, 2020

Conversation

richardcase
Copy link
Contributor

@richardcase richardcase commented Jan 3, 2020

Started to add https basic authentication to the git chart source. the
username and password can be set as a static value or from a secret.
For example:

spec:
  releaseName: my-app
  chart:
    git: https://github.com/someonesomerepo.git
    ref: master
    path: charts/my-app
    secretRef:
      name: git-credentials

Still a work in progress

Relates to: #94

@richardcase richardcase changed the title [WIP] adding https basic auth to git source Added https basic auth to git source Jan 8, 2020
@stefanprodan
Copy link
Member

@richardcase I think this should be paused until we decide how to handle git/helm repos outside the HelmRelease spec. See #142

@richardcase
Copy link
Contributor Author

richardcase commented Jan 8, 2020

@richardcase I think this should be paused until we decide how to handle git/helm repos outside the HelmRelease spec. See #142

@stefanprodan - Sure. Although i really need this functionality for a specific scenario i'm working on. I'll work from a fork. If you need any help with #142 let me know

@stefanprodan
Copy link
Member

If you need this then please make it more compact like in my proposal:

spec:
  releaseName: my-app
  chart:
    git: https://github.com/someonesomerepo.git
    ref: master
    path: charts/my-app
    secretRef: #optional
      name: git-basic-auth #required
apiVersion: v1
kind: Secret
metadata:
  name: git-basic-auth
type: Opaque
data:
  username: <BASE64> #required
  password: <BASE64> #required

@richardcase
Copy link
Contributor Author

If you need this then please make it more compact like in my proposal

Sure i can do that. I will make that change now

@hiddeco
Copy link
Member

hiddeco commented Jan 8, 2020

In addition to all of the above, instead of merging in master it is better for reviewers and Git history clutter to rebase on top of master.

@richardcase
Copy link
Contributor Author

In addition to all of the above, instead of merging in master it is better for reviewers and Git history clutter to rebase on top of master.

Understood, that was a mess up my side....a case of PEBKAC

@richardcase richardcase changed the title Added https basic auth to git source [WIP] Added https basic auth to git source Jan 15, 2020
@richardcase richardcase changed the title [WIP] Added https basic auth to git source Added https basic auth to git source Jan 15, 2020
@richardcase
Copy link
Contributor Author

@stefanprodan @hiddeco - i've made the change to simplify the specifiying of the creds as suggested. So as suggested you have:

spec:
  releaseName: my-release
  chart:
    git: https://github.com/someorg/somerepo.git
    ref: master
    path: charts/my-chart
    secretRef:
      name: git-pwd
apiVersion: v1
kind: Secret
metadata:
  name: git-pwd
type: Opaque
data:
  password: supersecret
  username: joeblogs

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Besides the in-code comments, I think the mirrorName function needs to be made smarter as the credentials from other HelmRelease resources (and namespaces) will now leak by configuring a source.GitURL (without credentials) that is already in use by another HelmRelease (with credentials).

You may get away with just adding the namespace, if I am not mistaken this is the same approach Flux uses for image pull secrets.

cmd/helm-operator/main.go Outdated Show resolved Hide resolved
pkg/apis/helm.fluxcd.io/v1/types.go Outdated Show resolved Hide resolved
pkg/chartsync/git.go Outdated Show resolved Hide resolved
pkg/chartsync/git.go Show resolved Hide resolved
pkg/chartsync/git.go Outdated Show resolved Hide resolved
@richardcase
Copy link
Contributor Author

You may get away with just adding the namespace, if I am not mistaken this is the same approach Flux uses for image pull secrets.

@hiddeco - i checked and flux does use the namespace. I changed the mirrorName to use a composite of namespace, auth secret name, git url. Do you think this is ok?

pkg/chartsync/git.go Outdated Show resolved Hide resolved
@hiddeco
Copy link
Member

hiddeco commented Jan 21, 2020

@richardcase thanks a lot 🥇

@stefanprodan can you take a look and see if this matches your expectations for the issue you mentioned? Feature wise it looks fine and mergeable to me (after I get rid of the persistent alias caused by the linter).

@hiddeco hiddeco changed the title Added https basic auth to git source chartsync: support Git HTTPS credentials using secret Jan 21, 2020
@stefanprodan
Copy link
Member

stefanprodan commented Jan 21, 2020

@hiddeco it looks Ok to me, it needs a rebase with master are you going to do that when fixing the aliases?

@hiddeco
Copy link
Member

hiddeco commented Jan 21, 2020

@stefanprodan yep, will do the rebase and some minor fixes that triggered my OCD 👍

Started to add https basic authentication to the git chart source. the
username and password can be set as a static value or from a secret.
For example:

```yaml
spec:
  releaseName: my-app
  chart:
    git: https://github.com/someonesomerepo.git
    ref: master
    path: charts/my-app
    auth:
      username:
        value: joebloggs
      password:
        valueFrom:
          name: git-pwd
          key: authpwd

```

Relates to: fluxcd#94
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Rebased and tidied up, awesome contribution @richardcase 🙌 🍰

pkg/chartsync/git.go Show resolved Hide resolved
pkg/chartsync/git.go Outdated Show resolved Hide resolved
@stefanprodan
Copy link
Member

In the future we should consider adding support for cross-namespace secrets like we have done in #219

Added error handling/logging if there is an error reported when
adding the auth details. Also added comments to new types.
Based on review feedback the way to specify the http auth credentials
has been simplified so that you just provide a secret name. The
referenced secret is assumed to have 2 keys: username, password.
richardcase and others added 3 commits January 21, 2020 17:36
Incorporated changes from review. Changed the mirror name to be and
composite key comprising namespace, auth secret name and git url.
Based on review feedback the mirrot name logic has been changed to
reduce duplicate mirrors if there are no creds
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants