Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Add feature status to environment vars #266

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: Add feature status to environment vars #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27045ca
Add feature status to environment vars
therealmitchconnors Nov 4, 2020
96c4326
fmt
therealmitchconnors Nov 5, 2020
df1880d
newline
therealmitchconnors Nov 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ctrlz/topics/assets/assets.gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions ctrlz/topics/assets/templates/env.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
<tr>
<th>Name</th>
<th>Value</th>
<th>Default</th>
<th>Status</th>
</tr>
</thead>

Expand All @@ -17,6 +19,8 @@
<tr>
<td>{{.Name}}</td>
<td>{{.Value}}</td>
<td>{{.DefaultValue}}</td>
<td>{{.FeatureStatus}}</td>
</tr>
{{ end }}
</tbody>
Expand Down
27 changes: 19 additions & 8 deletions ctrlz/topics/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2018 Istio Authors
/// Copyright 2018 Istio Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -23,6 +23,7 @@ import (

"istio.io/pkg/ctrlz/fw"
"istio.io/pkg/ctrlz/topics/assets"
"istio.io/pkg/env"
)

type envTopic struct {
Expand All @@ -42,20 +43,30 @@ func (envTopic) Prefix() string {
}

type envVar struct {
Name string `json:"name"`
Value string `json:"value"`
Name string `json:"name"`
Value string `json:"value"`
DefaultValue string `json:"defaultvalue"`
FeatureStatus string `json:"featurestatus"`
}

func getVars() []envVar {
env := os.Environ()
sort.Strings(env)
regEnv := env.VarDescriptions()
otherEnv := os.Environ()
sort.Strings(otherEnv)

result := []envVar{}
for _, v := range env {
visited := make(map[string]bool, len(regEnv))
for _, v := range regEnv {
visited[v.Name] = true
result = append(result, envVar{Name: v.Name, Value: v.GetGeneric(), DefaultValue: v.DefaultValue, FeatureStatus: v.FeatureStatus.String()})
}
for _, v := range otherEnv {
var eq = strings.Index(v, "=")
var name = v[:eq]
var value = v[eq+1:]
result = append(result, envVar{Name: name, Value: value})
if _, ok := visited[name]; !ok {
var value = v[eq+1:]
result = append(result, envVar{Name: name, Value: value, DefaultValue: "UNKNOWN", FeatureStatus: "UNKNOWN"})
}
}

return result
Expand Down
71 changes: 66 additions & 5 deletions env/var.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package env

import (
"fmt"
"os"
"sort"
"strconv"
Expand All @@ -42,6 +43,27 @@ const (
DURATION
)

type FeatureStatus byte

const (
Alpha FeatureStatus = iota
Copy link
Contributor

@louiscryan louiscryan Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In a world where we canonicalize these Unknown's should become forbidden. Can we add a 'Dev' status

I think there's also a class of 'industry std' environment variables that we depend on which would not belong to our canonicalization. (PATH, USER, ...) How would we handle those? Maybe add a "Platform" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Platform env vars are not registered with the env package, and the ctrlz page lists their default values as UNKNOWN. I don't think we have any concept of 'Dev' status elsewhere in the project. Would 'experimental' make more sense?

Beta
Stable
Unknown
)

func (s FeatureStatus) String() string {
switch s {
case Alpha:
return "Alpha"
case Beta:
return "Beta"
case Stable:
return "Stable"
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean for an env var to be stable? Don't we currently document that all env vars are alpha?

Put another way, why would we promote an env var to stable vs moving it to an API/mesh config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am all for the idea of migrating all stable environment variables to an API, but that does not represent the current state of the project. For instance, I believe we set the PILOT_TRACE_SAMPLING variable on every istio deployment, and it should probably be considered stable.

Copy link
Member

Choose a reason for hiding this comment

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

PILOT_TRACE_SAMPLING will be deprecated in 1.9 by the tracing api. We shouldn't mark something "stable" just because its been around for a while, we should mark it stable if we know its the right long term API and its properly tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The overall idea here is to give users a way to visualize the maturity of the features they are leveraging. We've already got that for Annotations with Nate's PR, and this would provide visibility into env vars, which could be combined in a single istioctl command to show users what features they are using, and what support they can expect. Given the prominence of many env vars, I doubt we would feed comfortable telling our users that all of them have only alpha level support.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we track the same state we're now tracking for annotation to cover the transition intent

deprecated, deprecatedBy

etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It might be helpful to track the removal of deprecated features too, so we can highlight that when checking upgrade readiness.

It's also useful to note that users may not be setting these directly, but via operator or helm values.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add experimental to here as well?

}
return "Unknown"
}

// Var describes a single environment variable
type Var struct {
// The name of the environment variable.
Expand All @@ -61,6 +83,9 @@ type Var struct {

// The type of the variable's value
Type VarType

// The support level of the feature controlled by this env var
FeatureStatus FeatureStatus
}

// StringVar represents a single string environment variable.
Expand Down Expand Up @@ -109,39 +134,49 @@ func VarDescriptions() []Var {

// RegisterStringVar registers a new string environment variable.
func RegisterStringVar(name string, defaultValue string, description string) StringVar {
v := Var{Name: name, DefaultValue: defaultValue, Description: description, Type: STRING}
v := Var{Name: name, DefaultValue: defaultValue, Description: description, Type: STRING, FeatureStatus: Unknown}
RegisterVar(v)
return StringVar{getVar(name)}
}

// RegisterBoolVar registers a new boolean environment variable.
func RegisterBoolVar(name string, defaultValue bool, description string) BoolVar {
v := Var{Name: name, DefaultValue: strconv.FormatBool(defaultValue), Description: description, Type: BOOL}
v := Var{Name: name, DefaultValue: strconv.FormatBool(defaultValue), Description: description, Type: BOOL, FeatureStatus: Unknown}
RegisterVar(v)
return BoolVar{getVar(name)}
}

// RegisterIntVar registers a new integer environment variable.
func RegisterIntVar(name string, defaultValue int, description string) IntVar {
v := Var{Name: name, DefaultValue: strconv.FormatInt(int64(defaultValue), 10), Description: description, Type: INT}
v := Var{Name: name, DefaultValue: strconv.FormatInt(int64(defaultValue), 10), Description: description, Type: INT, FeatureStatus: Unknown}
RegisterVar(v)
return IntVar{getVar(name)}
}

// RegisterFloatVar registers a new floating-point environment variable.
func RegisterFloatVar(name string, defaultValue float64, description string) FloatVar {
v := Var{Name: name, DefaultValue: strconv.FormatFloat(defaultValue, 'G', -1, 64), Description: description, Type: FLOAT}
v := Var{Name: name, DefaultValue: strconv.FormatFloat(defaultValue, 'G', -1, 64), Description: description, Type: FLOAT, FeatureStatus: Unknown}
RegisterVar(v)
return FloatVar{v}
}

// RegisterDurationVar registers a new duration environment variable.
func RegisterDurationVar(name string, defaultValue time.Duration, description string) DurationVar {
v := Var{Name: name, DefaultValue: defaultValue.String(), Description: description, Type: DURATION}
v := Var{Name: name, DefaultValue: defaultValue.String(), Description: description, Type: DURATION, FeatureStatus: Unknown}
RegisterVar(v)
return DurationVar{getVar(name)}
}

func SetVarStatus(v Var, f FeatureStatus) {
if val, ok := allVars[v.Name]; ok {
val.FeatureStatus = f
allVars[v.Name] = val
} else {
v.FeatureStatus = f
RegisterVar(v)
}
}

// RegisterVar registers a generic environment variable.
func RegisterVar(v Var) {
mutex.Lock()
Expand Down Expand Up @@ -169,6 +204,32 @@ func getVar(name string) Var {
return result
}

func (v Var) LookupGeneric() (string, bool) {
switch v.Type {
case STRING:
return StringVar{Var: v}.Lookup()
case INT:
val, found := IntVar{Var: v}.Lookup()
return strconv.Itoa(val), found
case BOOL:
val, found := BoolVar{Var: v}.Lookup()
return strconv.FormatBool(val), found
case FLOAT:
val, found := FloatVar{Var: v}.Lookup()
return strconv.FormatFloat(val, 'f', -1, 64), found
case DURATION:
val, found := DurationVar{Var: v}.Lookup()
return val.String(), found
default:
return fmt.Sprintf("type %T is not recognized", v), false
}
}

func (v Var) GetGeneric() string {
val, _ := v.LookupGeneric()
return val
}

// Get retrieves the value of the environment variable.
// It returns the value, which will be the default if the variable is not present.
// To distinguish between an empty value and an unset value, use Lookup.
Expand Down
29 changes: 29 additions & 0 deletions env/var_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,32 @@ func TestDupes(t *testing.T) {
t.Errorf("Expected 'XYZ', got '%s'", v.Description)
}
}

func TestVar_GetGeneric(t *testing.T) {
type fields struct {
Var Var
Value string
}
tests := []struct {
name string
fields fields
want string
}{
{
name: "first",
fields: fields{
Var: RegisterStringVar("test", "defaultval", "description").Var,
Value: "non-default",
},
want: "non-default",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
os.Setenv(tt.fields.Var.Name, tt.fields.Value)
if got := tt.fields.Var.GetGeneric(); got != tt.want {
t.Errorf("GetGeneric() = %v, want %v", got, tt.want)
}
})
}
}
3 changes: 1 addition & 2 deletions ledger/ledger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ import (
"testing"
"time"

"golang.org/x/sync/errgroup"

"github.com/google/uuid"
"github.com/spaolacci/murmur3"
"golang.org/x/sync/errgroup"
"gotest.tools/assert"
)

Expand Down
3 changes: 1 addition & 2 deletions ledger/trie_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@
package ledger

import (
"sync"
"time"

"istio.io/pkg/cache"

"sync"
)

type cacheDB struct {
Expand Down
3 changes: 1 addition & 2 deletions viperconfig/viperconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@ import (
"fmt"
"os"

"github.com/spf13/viper"

"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/spf13/viper"
)

// AddConfigFlag appends a persistent flag for retrieving Viper config, as well as an initializer
Expand Down