Add support for additional git configs #240

onesolpark · 2020-03-24T05:44:16Z

Fixes #235
Add support for adding additional git configs through
args --git-configs=k1:v1,k2:v2 or through env variable GIT_CONFIGS

All key/values are added as global values to gitconfig

k8s-ci-robot · 2020-03-24T05:44:24Z

Welcome @onesolpark!

It looks like this is your first PR to kubernetes/git-sync 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/git-sync has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-03-24T05:44:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign stp-ip
You can assign the PR to them by writing /assign @stp-ip in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

onesolpark · 2020-03-24T05:44:51Z

/assign @stp-ip

thockin

Thanks for this. Some comments here.

Is there any e2e test we can craft that proves that this works?

thockin · 2020-03-24T19:58:43Z

README.md

@@ -109,5 +109,6 @@ docker run -d \
 | GIT_SYNC_HTTP_BIND              | `--http-bind`              | the bind address (including port) for git-sync's HTTP endpoint                                                                         | ""                            |
 | GIT_SYNC_HTTP_METRICS           | `--http-metrics`           | enable metrics on git-sync's HTTP endpoint                                                                                             | true                          |
 | GIT_SYNC_HTTP_PPROF             | `--http-pprof`             | enable the pprof debug endpoints on git-sync's HTTP endpoint                                                                           | false                         |
+| GIT_CONFIGS                     | `--git-configs`            | add additional git config in 'key1:value1,key2:value2' format (ex - http.sslCAInfo:/path/to/cert/file,http.version:HTTP/2)             | ""                            |


This should be GIT_SYNC_GIT_CONFIGS

thockin · 2020-03-24T20:03:02Z

cmd/git-sync/main.go

@@ -114,6 +114,9 @@ var flHTTPMetrics = flag.Bool("http-metrics", envBool("GIT_SYNC_HTTP_METRICS", t
 var flHTTPprof = flag.Bool("http-pprof", envBool("GIT_SYNC_HTTP_PPROF", false),
 	"enable the pprof debug endpoints on git-sync's HTTP endpoint")

+var flGitConfigs = flag.String("git-configs", envString("GIT_CONFIGS", ""),
+	"add additional git configs in 'key1:value1,key2:value2' format (ex - http.sslCAInfo:/path/to/cert/file,http.version:HTTP/2)")


nit: change "ex - " to "e.g. "

thockin · 2020-03-24T20:06:49Z

cmd/git-sync/main.go

@@ -283,6 +286,13 @@ func main() {
 		}
 	}

+	if *flGitConfigs != "" {
+		if err := setupGitConfigs(ctx); err != nil {


please pass the flag in here so that only main() deals with flags (mostly)

thockin · 2020-03-24T20:20:28Z

cmd/git-sync/main.go

+func setupGitConfigs(ctx context.Context) error {
+	log.V(1).Info("adding git configs from flag")
+
+	for _, config := range strings.Split(*flGitConfigs, ",") {


Are there any git-config keys or values that embed commas? I know it's weird, but if a path had a comma in it, this would break.

This also applies to colons. Simple string split probably is not enough.

A more robust approach would be a function that walks through the input string, one rune at a time and produces a []struct{k, v string}, which you can then iterate over safely. As you walk the string, any time you find a literal backslash, replace it with whatever comes next.

"\a\b\c" => "abc"
"foo\n" => "foon"
"hello\there" => "hello\there"
"/weird/path/with/,comma" => "/weird/path/with/,comma"

Or are we SURE that we never have to deal with this?

Then we want a unit test :)

As you pointed out, I believe I can't be sure that there won't be any commas and semicolons in these variables.

I'll change the function to go over rune at a time and escape based on backslashes.

The thing that concerns me is that you can actually put backslashes in gitconfig with double backslash \\

And if users want to put double backslash it would mean that they have to put \\\\ with the function logic.

"\a\b\c" => "abc"
"foo\n" => "foon"
"hello\there" => "hellothere"
"/weird/path/with/\,comma" => "/weird/path/with/,comma"

"weird\\\\backslashid" => "weird\\backslashid"

Does this sound fine?

I'll definitely add a unit test to check this logic :)

This got long. I spent a lot of time playing with git-config, finding edge-case behaviors.
PLEASE double check me.

Looking at the git-config docs:

Section names are case-insensitive. Only alphanumeric characters, - and . are allowed in section names.

and

The variable names are case-insensitive, allow only alphanumeric characters and -, and must start with an alphabetic character.

So keys can not hold comma or colon. if you walk the string and parse the flag-string into a []string, delimited by "true" commas, you can take everything up to the first : as key (stripping leading/trailing whitespace) and everything after that (stripping leading/trailing whitespace) as value. No need to escape : at all.

Regarding whitespace on values:

Leading whitespaces after name =, ... and trailing whitespaces of the line are discarded unless they are enclosed in double quotes. Internal whitespaces within the value are retained verbatim.

To break the flag-string, though, you need to be smart.

Let's consider a sort of pathological case. A key with a value that is a list, and another key with a simple value.

key1 = "k1"
value = "a:b, b:c, c:d"
key2 = "k2"
value = "v2"

Naive answer: k1:a:b, b:c, c:d, k2:v2

Obviously that doesn't work - it's ambiguous.

Option 1, escape commas: k1:a:b\, b:c\, c:d, k2:v2

Option 2 embed quotes: k1:"a:b, b:c, c:d", k2:v2

So I THINK we can say something like:

The value is a series of comma-delimited key-value pairs.

Each key must be a valid git-config compatible "[section].[variable]" (see git-config docs)

Each value must be a either a quoted string or an unquoted string

Within all values, the following escapes are supported:

\n => [newline]

\t => [tab]

\" => "

\, => ,

\\ => \

Within unquoted values, commas MUST be escaped. Within quoted values, commas MAY be escaped, but are not required to be.

Any other escape sequence is an error

When we build the string to pass to git config we DO NOT reformat it (e.g. no "%q"). Git is weird here, and (empirically) it seems correct to pass literal newlines, which git will internally escape.

If there's demand for more escapes we can consider expanding it later (e.g. git-config supports \b, but I don't see why we should).

Now, keep in mind that to get these INTO git-sync, we need to pass them through a shell or YAML or something, so they may get escaped again. That seems unavoidable. In truth, I hope the vast vast majority of people just use simple tokens, buy we should be precise and code defensively. :)

We'll need a good test to cover the various ways one could try to break this.

I've gone through the documentation as well.
I'll go and work on the codes for this and a solid test code as well.
btw thanks for the thorough review :)

thockin · 2020-03-24T20:21:30Z

cmd/git-sync/main.go

@@ -283,6 +286,13 @@ func main() {
 		}
 	}

+	if *flGitConfigs != "" {


Comment here that this should be the very last config section, so it overrides all others.

thockin · 2020-04-24T20:30:39Z

Any updates on this?

fejta-bot · 2020-07-23T21:11:40Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

thockin · 2020-07-24T20:36:40Z

ping?

ntdef · 2020-08-11T19:28:12Z

I'd really like to see this feature added if possible. I've never written any Go, otherwise I'd give it a crack myself.

Is there much that needs to be done to finalize this PR? I might be open to try completing the work.

thockin · 2020-08-11T19:33:03Z

Go is easy. Getting all the edge-cases in parsing right is the only hard part here :)

thockin · 2021-03-11T03:10:32Z

Hi, I have proposed #341 as a way to pass arbitrary args. That subsumes this abandoned PR. Thanks for the idea.

k8s-ci-robot · 2021-03-11T13:14:43Z

@onesolpark: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Add support for additional git configs

234dde7

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 24, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 24, 2020

k8s-ci-robot requested review from stp-ip and thockin March 24, 2020 05:44

k8s-ci-robot assigned stp-ip Mar 24, 2020

thockin reviewed Mar 24, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 23, 2020

thockin removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 24, 2020

thockin added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Sep 4, 2020

wseaton mentioned this pull request Jan 31, 2021

add a config option for ignoring git SSL verification opendatahub-io-contrib/airflow-on-k8s-operator#10

Merged

2 tasks

thockin mentioned this pull request Mar 11, 2021

Add --git-config flag #341

Merged

thockin closed this Mar 11, 2021

thockin mentioned this pull request Mar 11, 2021

Add --git-config flag #342

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional git configs #240

Add support for additional git configs #240

onesolpark commented Mar 24, 2020

k8s-ci-robot commented Mar 24, 2020

k8s-ci-robot commented Mar 24, 2020

onesolpark commented Mar 24, 2020

thockin left a comment

thockin Mar 24, 2020

thockin Mar 24, 2020

thockin Mar 24, 2020

thockin Mar 24, 2020

thockin Mar 24, 2020

onesolpark Mar 25, 2020

onesolpark Mar 25, 2020

thockin Mar 25, 2020 •

edited

Loading

onesolpark Mar 27, 2020

thockin Mar 24, 2020

thockin commented Apr 24, 2020

fejta-bot commented Jul 23, 2020

thockin commented Jul 24, 2020

ntdef commented Aug 11, 2020 •

edited

Loading

thockin commented Aug 11, 2020

thockin commented Mar 11, 2021

k8s-ci-robot commented Mar 11, 2021

Add support for additional git configs #240

Add support for additional git configs #240

Conversation

onesolpark commented Mar 24, 2020

k8s-ci-robot commented Mar 24, 2020

k8s-ci-robot commented Mar 24, 2020

onesolpark commented Mar 24, 2020

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Apr 24, 2020

fejta-bot commented Jul 23, 2020

thockin commented Jul 24, 2020

ntdef commented Aug 11, 2020 • edited Loading

thockin commented Aug 11, 2020

thockin commented Mar 11, 2021

k8s-ci-robot commented Mar 11, 2021

thockin Mar 25, 2020 •

edited

Loading

ntdef commented Aug 11, 2020 •

edited

Loading