Introduce a config file model for Galley by ozevren · Pull Request #10834 · istio/istio

ozevren · 2019-01-09T17:54:57Z

Galley is starting to accummulate too many command-line flags. It is going to get worse with future improvements, such as reverse-MCP. This adds a settings file model, and plumbs it through.

Add a single file-format with sub-sections, that closely map to the existing command-line flags.
Add a mostly-empty settings file, that is contains examples and documentation of various setting values.
Plumb it through the command-line.

galley/pkg/settings/parse.go

galley/pkg/settings/default.go

galley/pkg/settings/doc.go

galley/pkg/settings/parse_test.go

googlebot · 2019-01-09T17:58:03Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2019-01-09T18:17:59Z

CLAs look good, thanks!

ayj · 2019-01-09T20:51:17Z

galley/pkg/settings/settings.proto

nit: s/metadata/annotations :)

Do we want gRPC stream metadata and resource annotations?

ayj · 2019-01-09T20:54:30Z

galley/pkg/settings/settings.proto

TLS is a form of authentication. Should it be configured as part of the auth plugin?

This needs to be strongly typed, at least for the validation case. For MCP case, we can model it as an "auth provider".

Just pushed another change that incorporates an "Auth Provider" model. I am not going to check this in though. I'd like to get the config model using current settings in first, then change both the settings & the code to be Auth Provider based.

istio-testing · 2019-01-13T01:38:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ozevren
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: hklai

If they are not already assigned, you can assign the PR to them by writing /assign @hklai in a comment when ready.

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

The pull request process is described here

Details

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

ozevren · 2019-01-13T02:24:49Z

/test istio_auth_sds_e2e

istio-testing · 2019-01-13T02:25:53Z

@ozevren: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-simpleTests-minProfile.sh	`c51478d`	link	`/test e2e-simpleTestsMinProfile`
prow/istio-integ-k8s-tests.sh	`c51478d`	link	`/test istio-integ-k8s-tests`
prow/istio-pilot-multicluster-e2e.sh	`c51478d`	link	`/test istio-pilot-multicluster-e2e`
prow/e2e_pilotv2_auth_sds.sh	`c51478d`	link	`/test istio_auth_sds_e2e`

Details

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. I understand the commands that are listed here.

geeknoid · 2019-01-15T17:55:22Z

galley/cmd/galley/cmd/root.go

-	serverArgs.IntrospectionOptions.AttachCobraFlags(rootCmd)
-
-	// validation config
-	rootCmd.PersistentFlags().StringVar(&validationArgs.WebhookConfigFile,


For compat, I think we need to keep the flags that existed in 1.0. You can mark them as deprecated, but they should still work in the same way as before until 1.3

geeknoid · 2019-01-15T18:04:41Z

galley/pkg/settings/settings.proto

+}
+
+// Introspection (CtrlZ) related settings
+message Introspection {


WOuld it make sense to leave this on the command-line, like the log flags?

ayj · 2019-01-17T20:35:35Z

galley/pkg/settings/conversion.go

+
+	k := s.Processing.Source.GetKubernetes()
+	if k != nil && k.ResyncPeriod != nil {
+		if resyncPeriod, err = ptypes.Duration(k.ResyncPeriod); err != nil {


Shouldn't this use the types.DurationFromProto from github.com/gogo/protobuf/types if settings.proto is compiled with gogo proto?

ayj · 2019-01-17T20:48:24Z

install/kubernetes/helm/subcharts/galley/templates/settings.yaml.tpl

@@ -0,0 +1,254 @@
+{{ define "settings.yaml.tpl" }}


Is this autogenerated from the default settings, or written by hand? Are we relying on PR reviews to maintain consistency between the two?

istio-testing · 2019-01-24T19:26:09Z

@ozevren: PR needs rebase.

Details

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.

stale · 2019-02-07T19:32:06Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-03-13T23:32:07Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

ozevren · 2019-03-20T16:06:39Z

Will move this to master.

ozevren requested a review from ayj January 9, 2019 17:54

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 9, 2019

istio-testing requested review from JimmyCYJ, costinm, geeknoid, liamawhite and linsun January 9, 2019 17:55

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 9, 2019

istio-testing requested review from mandarjog, ostromart and ymesika January 9, 2019 17:55

ozevren requested review from jeffmendoza and removed request for JimmyCYJ, costinm, liamawhite, linsun, mandarjog, ostromart and ymesika January 9, 2019 17:55

ozevren changed the base branch from master to release-1.1 January 9, 2019 17:55

golangcibot reviewed Jan 9, 2019

View reviewed changes

galley/pkg/settings/parse.go Outdated Show resolved Hide resolved

galley/pkg/settings/default.go Outdated Show resolved Hide resolved

galley/pkg/settings/doc.go Outdated Show resolved Hide resolved

galley/pkg/settings/parse_test.go Outdated Show resolved Hide resolved

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Jan 9, 2019

istio-testing added the approved label Jan 9, 2019

googlebot added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Jan 9, 2019

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 9, 2019

ayj reviewed Jan 9, 2019

View reviewed changes

ozben added 9 commits January 12, 2019 17:26

Fix lint problems.

0a8f1f4

Add a sample settings file to the deployment.

3ec5158

Fix linter issues.

6077ab7

Remove the redundant volume mount.

e0cc9e5

linter fixes.

cc43484

Apply templating to the settings file.

434785d

One more lint cleanup.

cda6afd

Fix config => settings in deployment scripts.

286353e

Add Gopkg.lock change.

c51478d

istio-testing removed the approved label Jan 13, 2019

ozevren changed the title ~~WIP: Introduce a config file model for Galley~~ Introduce a config file model for Galley Jan 13, 2019

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 13, 2019

geeknoid reviewed Jan 15, 2019

View reviewed changes

ayj reviewed Jan 17, 2019

View reviewed changes

jeffmendoza mentioned this pull request Jan 18, 2019

Auth plugin to be used for Galley dial out #11068

Closed

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 24, 2019

stale bot added the stale label Feb 7, 2019

ozevren removed the stale label Feb 27, 2019

ozevren mentioned this pull request Mar 1, 2019

galley ingest mcp #11525

Closed

stale bot added the stale label Mar 13, 2019

ozevren closed this Mar 20, 2019

ozevren deleted the gl-6-configfile branch August 23, 2019 23:21

Conversation

ozevren commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented Jan 9, 2019

Uh oh!

googlebot commented Jan 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jan 13, 2019

Uh oh!

ozevren commented Jan 13, 2019

Uh oh!

istio-testing commented Jan 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jan 24, 2019

Uh oh!

stale bot commented Feb 7, 2019

Uh oh!

stale bot commented Mar 13, 2019

Uh oh!

ozevren commented Mar 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ozevren commented Jan 9, 2019 •

edited

Loading