Repository: oauth2-proxy/oauth2-proxy
Pull request #916 (2020-11-12 19:22:28)

Add AlphaOptions struct to prepare for alpha config loading
<!--- Provide a general summary of your changes in the Title above -->

## Description

<!--- Describe your changes in detail -->
This PR adds a new struct to the options package called `AlphaOptions`.
This is a prerequisite to adding an option for users to load alpha configuration via a separate yaml file.

To enable YAML loading, we have to make sure that everything marshals from JSON as expected.
To enable this all children of the AlphaOptions struct have had json tags added, and the duration within the upstreams has been converted to an internal type to ensure that we can marshal and unmarshal duration strings as a user might expect.

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
This will allow us to add an alpha config loader that loads the `AlphaOptions` structure from a YAML file.

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
Unit tests only. There's no functional changes.

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 72d570f3..c9b7911b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -43,6 +43,7 @@
 
 ## Changes since v6.1.1
 
+- [#916](https://github.com/oauth2-proxy/oauth2-rpoxy/pull/916) Add AlphaOptions struct to prepare for alpha config loading (@JoelSpeed)
 - [#923](https://github.com/oauth2-proxy/oauth2-proxy/pull/923) Support TLS 1.3 (@aajisaka)
 - [#918](https://github.com/oauth2-proxy/oauth2-proxy/pull/918) Fix log header output (@JoelSpeed)
 - [#911](https://github.com/oauth2-proxy/oauth2-proxy/pull/911) Validate provider type on startup.
diff --git a/pkg/apis/options/alpha_options.go b/pkg/apis/options/alpha_options.go
new file mode 100644
index 00000000..1086ee2a
--- /dev/null
+++ b/pkg/apis/options/alpha_options.go
@@ -0,0 +1,31 @@
+package options
+
+// AlphaOptions contains alpha structured configuration options.
+// Usage of these options allows users to access alpha features that are not
+// available as part of the primary configuration structure for OAuth2 Proxy.
+//
+// :::warning
+// The options within this structure are considered alpha.
+// They may change between releases without notice.
+// :::
+type AlphaOptions struct {
+	// Upstreams is used to configure upstream servers.
+	// Once a user is authenticated, requests to the server will be proxied to
+	// these upstream servers based on the path mappings defined in this list.
+	Upstreams Upstreams `json:"upstreams,omitempty"`
+
+	// InjectRequestHeaders is used to configure headers that should be added
+	// to requests to upstream servers.
+	// Headers may source values from either the authenticated user's session
+	// or from a static secret value.
+	InjectRequestHeaders []Header `json:"injectRequestHeaders,omitempty"`
+
+	// InjectResponseHeaders is used to configure headers that should be added
+	// to responses from the proxy.
+	// This is typically used when using the proxy as an external authentication
+	// provider in conjunction with another proxy such as NGINX and its
+	// auth_request module.
+	// Headers may source values from either the authenticated user's session
+	// or from a static secret value.
+	InjectResponseHeaders []Header `json:"injectResponseHeaders,omitempty"`
+}
diff --git a/pkg/apis/options/common.go b/pkg/apis/options/common.go
index 60d352a5..b08bfa6d 100644
--- a/pkg/apis/options/common.go
+++ b/pkg/apis/options/common.go
@@ -1,14 +1,62 @@
 package options
 
+import (
+	"fmt"
+	"strconv"
+	"time"
+)
+
 // SecretSource references an individual secret value.
 // Only one source within the struct should be defined at any time.
 type SecretSource struct {
 	// Value expects a base64 encoded string value.
-	Value []byte
+	Value []byte `json:"value,omitempty"`
 
 	// FromEnv expects the name of an environment variable.
-	FromEnv string
+	FromEnv string `json:"fromEnv,omitempty"`
 
 	// FromFile expects a path to a file containing the secret value.
-	FromFile string
+	FromFile string `json:"fromFile,omitempty"`
+}
+
+// Duration is an alias for time.Duration so that we can ensure the marshalling
+// and unmarshalling of string durations is done as users expect.
+// Intentional blank line below to keep this first part of the comment out of
+// any generated references.
+
+// Duration is as string representation of a period of time.
+// A duration string is a is a possibly signed sequence of decimal numbers,
+// each with optional fraction and a unit suffix, such as "300ms", "-1.5h" or "2h45m".
+// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
+type Duration time.Duration
+
+// UnmarshalJSON parses the duration string and sets the value of duration
+// to the value of the duration string.
+func (d *Duration) UnmarshalJSON(data []byte) error {
+	input := string(data)
+	if unquoted, err := strconv.Unquote(input); err == nil {
+		input = unquoted
+	}
+
+	du, err := time.ParseDuration(input)
+	if err != nil {
+		return err
+	}
+	*d = Duration(du)
+	return nil
+}
+
+// MarshalJSON ensures that when the string is marshalled to JSON as a human
+// readable string.
+func (d *Duration) MarshalJSON() ([]byte, error) {
+	dStr := fmt.Sprintf("%q", d.Duration().String())
+	return []byte(dStr), nil
+}
+
+// Duration returns the time.Duration version of this Duration
+func (d *Duration) Duration() time.Duration {
+	if d == nil {
+		return time.Duration(0)
+	}
+	return time.Duration(*d)
 }
diff --git a/pkg/apis/options/header.go b/pkg/apis/options/header.go
index 0b2e1b69..4b41eff9 100644
--- a/pkg/apis/options/header.go
+++ b/pkg/apis/options/header.go
@@ -5,26 +5,26 @@ package options
 type Header struct {
 	// Name is the header name to be used for this set of values.
 	// Names should be unique within a list of Headers.
-	Name string `json:"name"`
+	Name string `json:"name,omitempty"`
 
 	// PreserveRequestValue determines whether any values for this header
 	// should be preserved for the request to the upstream server.
 	// This option only takes effet on injected request headers.
 	// Defaults to false (headers that match this header will be stripped).
-	PreserveRequestValue bool `json:"preserveRequestValue"`
+	PreserveRequestValue bool `json:"preserveRequestValue,omitempty"`
 
 	// Values contains the desired values for this header
-	Values []HeaderValue `json:"values"`
+	Values []HeaderValue `json:"values,omitempty"`
 }
 
 // HeaderValue represents a single header value and the sources that can
 // make up the header value
 type HeaderValue struct {
 	// Allow users to load the value from a secret source
-	*SecretSource
+	*SecretSource `json:",omitempty"`
 
 	// Allow users to load the value from a session claim
-	*ClaimSource
+	*ClaimSource `json:",omitempty"`
 }
 
 // ClaimSource allows loading a header value from a claim within the session
@@ -40,5 +40,5 @@ type ClaimSource struct {
 	// BasicAuthPassword converts this claim into a basic auth header.
 	// Note the value of claim will become the basic auth username and the
 	// basicAuthPassword will be used as the password value.
-	BasicAuthPassword *SecretSource
+	BasicAuthPassword *SecretSource `json:"basicAuthPassword,omitempty"`
 }
diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go
index 88784e9b..6fb2596b 100644
--- a/pkg/apis/options/legacy_options.go
+++ b/pkg/apis/options/legacy_options.go
@@ -27,7 +27,7 @@ func NewLegacyOptions() *LegacyOptions {
 		LegacyUpstreams: LegacyUpstreams{
 			PassHostHeader:  true,
 			ProxyWebSockets: true,
-			FlushInterval:   time.Duration(1) * time.Second,
+			FlushInterval:   DefaultUpstreamFlushInterval,
 		},
 
 		LegacyHeaders: LegacyHeaders{
@@ -62,7 +62,7 @@ type LegacyUpstreams struct {
 func legacyUpstreamsFlagSet() *pflag.FlagSet {
 	flagSet := pflag.NewFlagSet("upstreams", pflag.ExitOnError)
 
-	flagSet.Duration("flush-interval", time.Duration(1)*time.Second, "period between response flushing when streaming responses")
+	flagSet.Duration("flush-interval", DefaultUpstreamFlushInterval, "period between response flushing when streaming responses")
 	flagSet.Bool("pass-host-header", true, "pass the request Host Header to upstream")
 	flagSet.Bool("proxy-websockets", true, "enables WebSocket proxying")
 	flagSet.Bool("ssl-upstream-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS upstreams")
@@ -84,6 +84,7 @@ func (l *LegacyUpstreams) convert() (Upstreams, error) {
 			u.Path = "/"
 		}
 
+		flushInterval := Duration(l.FlushInterval)
 		upstream := Upstream{
 			ID:                    u.Path,
 			Path:                  u.Path,
@@ -91,7 +92,7 @@ func (l *LegacyUpstreams) convert() (Upstreams, error) {
 			InsecureSkipTLSVerify: l.SSLUpstreamInsecureSkipVerify,
 			PassHostHeader:        &l.PassHostHeader,
 			ProxyWebSockets:       &l.ProxyWebSockets,
-			FlushInterval:         &l.FlushInterval,
+			FlushInterval:         &flushInterval,
 		}
 
 		switch u.Scheme {
diff --git a/pkg/apis/options/upstreams.go b/pkg/apis/options/upstreams.go
index e879a107..6ae87487 100644
--- a/pkg/apis/options/upstreams.go
+++ b/pkg/apis/options/upstreams.go
@@ -2,6 +2,11 @@ package options
 
 import "time"
 
+const (
+	// DefaultUpstreamFlushInterval is the default value for the Upstream FlushInterval.
+	DefaultUpstreamFlushInterval = 1 * time.Second
+)
+
 // Upstreams is a collection of definitions for upstream servers.
 type Upstreams []Upstream
 
@@ -10,11 +15,11 @@ type Upstreams []Upstream
 type Upstream struct {
 	// ID should be a unique identifier for the upstream.
 	// This value is required for all upstreams.
-	ID string `json:"id"`
+	ID string `json:"id,omitempty"`
 
 	// Path is used to map requests to the upstream server.
 	// The closest match will take precedence and all Paths must be unique.
-	Path string `json:"path"`
+	Path string `json:"path,omitempty"`
 
 	// The URI of the upstream server. This may be an HTTP(S) server of a File
 	// based URL. It may include a path, in which case all requests will be served
@@ -26,19 +31,19 @@ type Upstream struct {
 	// - file://host/path
 	// If the URI's path is "/base" and the incoming request was for "/dir",
 	// the upstream request will be for "/base/dir".
-	URI string `json:"uri"`
+	URI string `json:"uri,omitempty"`
 
 	// InsecureSkipTLSVerify will skip TLS verification of upstream HTTPS hosts.
 	// This option is insecure and will allow potential Man-In-The-Middle attacks
 	// betweem OAuth2 Proxy and the usptream server.
 	// Defaults to false.
-	InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify"`
+	InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty"`
 
 	// Static will make all requests to this upstream have a static response.
 	// The response will have a body of "Authenticated" and a response code
 	// matching StaticCode.
 	// If StaticCode is not set, the response will return a 200 response.
-	Static bool `json:"static"`
+	Static bool `json:"static,omitempty"`
 
 	// StaticCode determines the response code for the Static response.
 	// This option can only be used with Static enabled.
@@ -47,14 +52,14 @@ type Upstream struct {
 	// FlushInterval is the period between flushing the response buffer when
 	// streaming response from the upstream.
 	// Defaults to 1 second.
-	FlushInterval *time.Duration `json:"flushInterval,omitempty"`
+	FlushInterval *Duration `json:"flushInterval,omitempty"`
 
 	// PassHostHeader determines whether the request host header should be proxied
 	// to the upstream server.
 	// Defaults to true.
-	PassHostHeader *bool `json:"passHostHeader"`
+	PassHostHeader *bool `json:"passHostHeader,omitempty"`
 
 	// ProxyWebSockets enables proxying of websockets to upstream servers
 	// Defaults to true.
-	ProxyWebSockets *bool `json:"proxyWebSockets"`
+	ProxyWebSockets *bool `json:"proxyWebSockets,omitempty"`
 }
diff --git a/pkg/upstream/http.go b/pkg/upstream/http.go
index 88c0afcd..a6e948c3 100644
--- a/pkg/upstream/http.go
+++ b/pkg/upstream/http.go
@@ -6,7 +6,6 @@ import (
 	"net/http/httputil"
 	"net/url"
 	"strings"
-	"time"
 
 	"github.com/mbland/hmacauth"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
@@ -98,9 +97,9 @@ func newReverseProxy(target *url.URL, upstream options.Upstream, errorHandler Pr
 
 	// Configure options on the SingleHostReverseProxy
 	if upstream.FlushInterval != nil {
-		proxy.FlushInterval = *upstream.FlushInterval
+		proxy.FlushInterval = upstream.FlushInterval.Duration()
 	} else {
-		proxy.FlushInterval = 1 * time.Second
+		proxy.FlushInterval = options.DefaultUpstreamFlushInterval
 	}
 
 	// InsecureSkipVerify is a configurable option we allow
diff --git a/pkg/validation/upstreams.go b/pkg/validation/upstreams.go
index 5cfe0b1e..7bd6b2d5 100644
--- a/pkg/validation/upstreams.go
+++ b/pkg/validation/upstreams.go
@@ -3,7 +3,6 @@ package validation
 import (
 	"fmt"
 	"net/url"
-	"time"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
 )
@@ -70,7 +69,7 @@ func validateStaticUpstream(upstream options.Upstream) []string {
 	if upstream.InsecureSkipTLSVerify {
 		msgs = append(msgs, fmt.Sprintf("upstream %q has insecureSkipTLSVerify, but is a static upstream, this will have no effect.", upstream.ID))
 	}
-	if upstream.FlushInterval != nil && *upstream.FlushInterval != time.Second {
+	if upstream.FlushInterval != nil && upstream.FlushInterval.Duration() != options.DefaultUpstreamFlushInterval {
 		msgs = append(msgs, fmt.Sprintf("upstream %q has flushInterval, but is a static upstream, this will have no effect.", upstream.ID))
 	}
 	if upstream.PassHostHeader != nil {


Test patch:
diff --git a/pkg/apis/options/common_test.go b/pkg/apis/options/common_test.go
new file mode 100644
index 00000000..8fc4176b
--- /dev/null
+++ b/pkg/apis/options/common_test.go
@@ -0,0 +1,88 @@
+package options
+
+import (
+	"encoding/json"
+	"errors"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Common", func() {
+	Context("Duration", func() {
+		type marshalJSONTableInput struct {
+			duration     Duration
+			expectedJSON string
+		}
+
+		DescribeTable("MarshalJSON",
+			func(in marshalJSONTableInput) {
+				data, err := in.duration.MarshalJSON()
+				Expect(err).ToNot(HaveOccurred())
+				Expect(string(data)).To(Equal(in.expectedJSON))
+
+				var d Duration
+				Expect(json.Unmarshal(data, &d)).To(Succeed())
+				Expect(d).To(Equal(in.duration))
+			},
+			Entry("30 seconds", marshalJSONTableInput{
+				duration:     Duration(30 * time.Second),
+				expectedJSON: "\"30s\"",
+			}),
+			Entry("1 minute", marshalJSONTableInput{
+				duration:     Duration(1 * time.Minute),
+				expectedJSON: "\"1m0s\"",
+			}),
+			Entry("1 hour 15 minutes", marshalJSONTableInput{
+				duration:     Duration(75 * time.Minute),
+				expectedJSON: "\"1h15m0s\"",
+			}),
+			Entry("A zero Duration", marshalJSONTableInput{
+				duration:     Duration(0),
+				expectedJSON: "\"0s\"",
+			}),
+		)
+
+		type unmarshalJSONTableInput struct {
+			json             string
+			expectedErr      error
+			expectedDuration Duration
+		}
+
+		DescribeTable("UnmarshalJSON",
+			func(in unmarshalJSONTableInput) {
+				// A duration must be initialised pointer before UnmarshalJSON will work.
+				zero := Duration(0)
+				d := &zero
+
+				err := d.UnmarshalJSON([]byte(in.json))
+				if in.expectedErr != nil {
+					Expect(err).To(MatchError(in.expectedErr.Error()))
+				} else {
+					Expect(err).ToNot(HaveOccurred())
+				}
+				Expect(d).ToNot(BeNil())
+				Expect(*d).To(Equal(in.expectedDuration))
+			},
+			Entry("1m", unmarshalJSONTableInput{
+				json:             "\"1m\"",
+				expectedDuration: Duration(1 * time.Minute),
+			}),
+			Entry("30s", unmarshalJSONTableInput{
+				json:             "\"30s\"",
+				expectedDuration: Duration(30 * time.Second),
+			}),
+			Entry("1h15m", unmarshalJSONTableInput{
+				json:             "\"1h15m\"",
+				expectedDuration: Duration(75 * time.Minute),
+			}),
+			Entry("am", unmarshalJSONTableInput{
+				json:             "\"am\"",
+				expectedErr:      errors.New("time: invalid duration \"am\""),
+				expectedDuration: Duration(0),
+			}),
+		)
+	})
+})
diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go
index 2e50edcc..44c8c728 100644
--- a/pkg/apis/options/legacy_options_test.go
+++ b/pkg/apis/options/legacy_options_test.go
@@ -17,8 +17,8 @@ var _ = Describe("Legacy Options", func() {
 			legacyOpts := NewLegacyOptions()
 
 			// Set upstreams and related options to test their conversion
-			flushInterval := 5 * time.Second
-			legacyOpts.LegacyUpstreams.FlushInterval = flushInterval
+			flushInterval := Duration(5 * time.Second)
+			legacyOpts.LegacyUpstreams.FlushInterval = time.Duration(flushInterval)
 			legacyOpts.LegacyUpstreams.PassHostHeader = true
 			legacyOpts.LegacyUpstreams.ProxyWebSockets = true
 			legacyOpts.LegacyUpstreams.SSLUpstreamInsecureSkipVerify = true
@@ -124,7 +124,7 @@ var _ = Describe("Legacy Options", func() {
 		skipVerify := true
 		passHostHeader := false
 		proxyWebSockets := true
-		flushInterval := 5 * time.Second
+		flushInterval := Duration(5 * time.Second)
 
 		// Test cases and expected outcomes
 		validHTTP := "http://foo.bar/baz"
@@ -199,7 +199,7 @@ var _ = Describe("Legacy Options", func() {
 					SSLUpstreamInsecureSkipVerify: skipVerify,
 					PassHostHeader:                passHostHeader,
 					ProxyWebSockets:               proxyWebSockets,
-					FlushInterval:                 flushInterval,
+					FlushInterval:                 time.Duration(flushInterval),
 				}
 
 				upstreams, err := legacyUpstreams.convert()
diff --git a/pkg/upstream/http_test.go b/pkg/upstream/http_test.go
index 8bfe9087..3ce5bd19 100644
--- a/pkg/upstream/http_test.go
+++ b/pkg/upstream/http_test.go
@@ -22,8 +22,8 @@ import (
 
 var _ = Describe("HTTP Upstream Suite", func() {
 
-	const flushInterval5s = 5 * time.Second
-	const flushInterval1s = 1 * time.Second
+	const flushInterval5s = options.Duration(5 * time.Second)
+	const flushInterval1s = options.Duration(1 * time.Second)
 	truth := true
 	falsum := false
 
@@ -52,7 +52,7 @@ var _ = Describe("HTTP Upstream Suite", func() {
 
 			rw := httptest.NewRecorder()
 
-			flush := 1 * time.Second
+			flush := options.Duration(1 * time.Second)
 
 			upstream := options.Upstream{
 				ID:                    in.id,
@@ -258,7 +258,7 @@ var _ = Describe("HTTP Upstream Suite", func() {
 		req := httptest.NewRequest("", "http://example.localhost/foo", nil)
 		rw := httptest.NewRecorder()
 
-		flush := 1 * time.Second
+		flush := options.Duration(1 * time.Second)
 		upstream := options.Upstream{
 			ID:                    "noPassHost",
 			PassHostHeader:        &falsum,
@@ -290,7 +290,7 @@ var _ = Describe("HTTP Upstream Suite", func() {
 
 	type newUpstreamTableInput struct {
 		proxyWebSockets bool
-		flushInterval   time.Duration
+		flushInterval   options.Duration
 		skipVerify      bool
 		sigData         *options.SignatureData
 		errorHandler    func(http.ResponseWriter, *http.Request, error)
@@ -319,7 +319,7 @@ var _ = Describe("HTTP Upstream Suite", func() {
 
 			proxy, ok := upstreamProxy.handler.(*httputil.ReverseProxy)
 			Expect(ok).To(BeTrue())
-			Expect(proxy.FlushInterval).To(Equal(in.flushInterval))
+			Expect(proxy.FlushInterval).To(Equal(in.flushInterval.Duration()))
 			Expect(proxy.ErrorHandler != nil).To(Equal(in.errorHandler != nil))
 			if in.skipVerify {
 				Expect(proxy.Transport).To(Equal(&http.Transport{
@@ -370,7 +370,7 @@ var _ = Describe("HTTP Upstream Suite", func() {
 		var proxyServer *httptest.Server
 
 		BeforeEach(func() {
-			flush := 1 * time.Second
+			flush := options.Duration(1 * time.Second)
 			upstream := options.Upstream{
 				ID:                    "websocketProxy",
 				PassHostHeader:        &truth,
diff --git a/pkg/validation/upstreams_test.go b/pkg/validation/upstreams_test.go
index 6b8f9829..122286ad 100644
--- a/pkg/validation/upstreams_test.go
+++ b/pkg/validation/upstreams_test.go
@@ -15,7 +15,7 @@ var _ = Describe("Upstreams", func() {
 		errStrings []string
 	}
 
-	flushInterval := 5 * time.Second
+	flushInterval := options.Duration(5 * time.Second)
 	staticCode200 := 200
 	truth := true