Skip to content

Fix #10380: Remove hardcoded sidecar template for istioctl kube-inject #10830

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

Merged
istio-testing merged 4 commits into from
Apr 11, 2019
Merged

Fix #10380: Remove hardcoded sidecar template for istioctl kube-inject #10830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a886f7b
Remove the hardcoded sidecar template for
mathspanda Mar 30, 2019
fbf11be
Remove deprecated flags in istioctl kube-inject
mathspanda Apr 1, 2019
7644bfa
update testdata after rebase
mathspanda Apr 10, 2019
aec8729
add rule for kubeinject.go in codecov.threshold
mathspanda Apr 11, 2019
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: 3 additions & 1 deletion Gopkg.lock
View file
Open in desktop

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

3 changes: 0 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,6 @@ export OUT_DIR=$(GO_TOP)/out
export ISTIO_OUT:=$(GO_TOP)/out/$(GOOS)_$(GOARCH)/$(BUILDTYPE_DIR)
export HELM=$(ISTIO_OUT)/helm

# istioctl kube-inject uses builtin config only if this env var is set.
Comment thread
mathspanda marked this conversation as resolved.
Outdated
export ISTIOCTL_USE_BUILTIN_DEFAULTS=1

# scratch dir: this shouldn't be simply 'docker' since that's used for docker.save to store tar.gz files
ISTIO_DOCKER:=${ISTIO_OUT}/docker_temp
# Config file used for building istio:proxy container.
Expand Down
1 change: 1 addition & 0 deletions codecov.threshold
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ istio.io/istio/pilot/pkg/serviceregistry/consul/monitor.go=20
istio.io/istio/pkg/mcp/creds/watcher.go=100
istio.io/istio/pkg/mcp/source/source.go=90
istio.io/istio/security/pkg/nodeagent=15
istio.io/istio/istioctl/cmd/istioctl/kubeinject.go=41

# Disable codecov check for testing and proto packages
istio.io/istio/security/proto=100
Expand Down
138 changes: 6 additions & 132 deletions istioctl/cmd/istioctl/kubeinject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
"istio.io/istio/pilot/cmd"
"istio.io/istio/pilot/pkg/kube/inject"
"istio.io/istio/pilot/pkg/model"
"istio.io/istio/pkg/env"
"istio.io/istio/pkg/kube"
"istio.io/istio/pkg/log"
"istio.io/istio/pkg/version"
Expand Down Expand Up @@ -118,33 +117,11 @@ func validateFlags() error {
if meshConfigFile == "" && meshConfigMapName == "" {
err = multierr.Append(err, errors.New("--meshConfigFile or --meshConfigMapName must be set"))
}

err = multierr.Append(err, inject.ValidateIncludeIPRanges(includeIPRanges))
err = multierr.Append(err, inject.ValidateExcludeIPRanges(excludeIPRanges))
err = multierr.Append(err, inject.ValidateIncludeInboundPorts(includeInboundPorts))
err = multierr.Append(err, inject.ValidateExcludeInboundPorts(excludeInboundPorts))
return err
}

var (
hub string
tag string
sidecarProxyUID uint64
verbosity int
versionStr string // override build version
enableCoreDump bool
rewriteAppHTTPProbe bool
imagePullPolicy string
statusPort int
readinessInitialDelaySeconds uint32
readinessPeriodSeconds uint32
readinessFailureThreshold uint32
includeIPRanges string
excludeIPRanges string
includeInboundPorts string
excludeInboundPorts string
debugMode bool
emitTemplate bool
emitTemplate bool

inFilename string
outFilename string
Expand All @@ -155,8 +132,6 @@ var (
)

var (
useBuiltinDefaultsVar = env.RegisterBoolVar("ISTIOCTL_USE_BUILTIN_DEFAULTS", false, "")

injectCmd = &cobra.Command{
Use: "kube-inject",
Short: "Inject Envoy sidecar into Kubernetes pod resources",
Expand All @@ -177,10 +152,9 @@ The Istio project is continually evolving so the Istio sidecar
configuration may change unannounced. When in doubt re-run istioctl
kube-inject on deployments to get the most up-to-date changes.

To override the sidecar injection template built into istioctl, the
parameters --injectConfigFile or --injectConfigMapName can be used.
Both options override any other template configuration parameters, eg.
--hub and --tag. These options would typically be used with the
To override the sidecar injection template from kubernetes configmap
'istio-inject', the parameters --injectConfigFile or --injectConfigMapName
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Mar 28, 2019

Choose a reason for hiding this comment

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

istio-inject what's this?

can be used. Either of options would typically be used with the
file/configmap created with a new Istio release.
`,
Example: `
Expand Down Expand Up @@ -247,10 +221,6 @@ istioctl kube-inject -f deployment.yaml -o deployment-injected.yaml --injectConf
}()
}

if versionStr == "" {
versionStr = version.Info.String()
}

var meshConfig *meshconfig.MeshConfig
if meshConfigFile != "" {
if meshConfig, err = cmd.ReadMeshConfig(meshConfigFile); err != nil {
Expand All @@ -263,47 +233,7 @@ istioctl kube-inject -f deployment.yaml -o deployment-injected.yaml --injectConf
}

var sidecarTemplate string

// hub and tag params only work with ISTIOCTL_USE_BUILTIN_DEFAULTS
// so must be specified together. hub and tag no longer have defaults.
if hub != "" || tag != "" {

// ISTIOCTL_USE_BUILTIN_DEFAULTS is used to have legacy behavior.
if !useBuiltinDefaultsVar.Get() {
return errors.New("one of injectConfigFile or injectConfigMapName is required\n" +
"use the following command to get the current injector file\n" +
"kubectl -n istio-system get configmap istio-sidecar-injector " +
"-o=jsonpath='{.data.config}' > /tmp/injectConfigFile.yaml")
}

if hub == "" || tag == "" {
return fmt.Errorf("hub and tag are both required. got hub: '%v', tag: '%v'", hub, tag)
}

if sidecarTemplate, err = inject.GenerateTemplateFromParams(&inject.Params{
InitImage: inject.InitImageName(hub, tag, debugMode),
ProxyImage: inject.ProxyImageName(hub, tag, debugMode),
RewriteAppHTTPProbe: rewriteAppHTTPProbe,
Verbosity: verbosity,
SidecarProxyUID: sidecarProxyUID,
Version: versionStr,
EnableCoreDump: enableCoreDump,
Mesh: meshConfig,
ImagePullPolicy: imagePullPolicy,
StatusPort: statusPort,
ReadinessInitialDelaySeconds: readinessInitialDelaySeconds,
ReadinessPeriodSeconds: readinessPeriodSeconds,
ReadinessFailureThreshold: readinessFailureThreshold,
IncludeIPRanges: includeIPRanges,
ExcludeIPRanges: excludeIPRanges,
IncludeInboundPorts: includeInboundPorts,
ExcludeInboundPorts: excludeInboundPorts,
DebugMode: debugMode,
}); err != nil {
return err
}

} else if injectConfigFile != "" {
if injectConfigFile != "" {
Copy link
Copy Markdown
Contributor

@ayj ayj Mar 13, 2019

Choose a reason for hiding this comment

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

You should remove or throw an error with this change. Otherwise, someone could specify a flag and it would be ignored.

Copy link
Copy Markdown
Contributor Author

@mathspanda mathspanda Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

At present, these flags are already be ignored, unless set ISTIOCTL_USE_BUILTIN_DEFAULTS=1 and --hub and --tag at the same time .

Copy link
Copy Markdown
Contributor

@ayj ayj Mar 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the current code, there exists many inject related overrides that aren't hooked up to anything. Some of this will print a deprecation warning which is no longer accurate. If support for the build in template is removed, we should also remove all of those flags. This will produce an error (e.g. unknown flag) if users were previously depending on them which is exactly the behavior we want.

injectionConfig, err := ioutil.ReadFile(injectConfigFile) // nolint: vetshadow
if err != nil {
return err
Expand Down Expand Up @@ -345,9 +275,6 @@ const (
func init() {
rootCmd.AddCommand(injectCmd)

injectCmd.PersistentFlags().StringVar(&hub, "hub", "", "Docker hub")
injectCmd.PersistentFlags().StringVar(&tag, "tag", "", "Docker tag")

injectCmd.PersistentFlags().StringVar(&meshConfigFile, "meshConfigFile", "",
"mesh configuration filename. Takes precedence over --meshConfigMapName if set")
injectCmd.PersistentFlags().StringVar(&injectConfigFile, "injectConfigFile", "",
Expand All @@ -361,62 +288,9 @@ func init() {
"", "Input Kubernetes resource filename")
injectCmd.PersistentFlags().StringVarP(&outFilename, "output", "o",
"", "Modified output Kubernetes resource filename")
injectCmd.PersistentFlags().IntVar(&verbosity, "verbosity",
inject.DefaultVerbosity, "Runtime verbosity")
injectCmd.PersistentFlags().Uint64Var(&sidecarProxyUID, "sidecarProxyUID",
inject.DefaultSidecarProxyUID, "Envoy sidecar UID")
injectCmd.PersistentFlags().StringVar(&versionStr, "setVersionString",
"", "Override version info injected into resource")
// Default --coreDump=true for pre-alpha development. Core dump
// settings (i.e. sysctl kernel.*) affect all pods in a node and
// require privileges. This option should only be used by the cluster
// admin (see https://kubernetes.io/docs/concepts/cluster-administration/sysctl-cluster/)
// injector specific params are deprecated
injectCmd.PersistentFlags().BoolVar(&enableCoreDump, "coreDump",
true, "Enable/Disable core dumps in injected Envoy sidecar (--coreDump=true affects "+
"all pods in a node and should only be used the cluster admin)")
// TODO(incfly): deprecate this flag once hardcoded injection template is gone. By then, everything
// comes from configmap injector, whose template already contains rewriteAppHTTPProbe control switch.
injectCmd.PersistentFlags().BoolVar(&rewriteAppHTTPProbe, "rewriteAppProbe", false, "Whether injector "+
"rewrites the liveness health check to let kubelet health check the app when mtls is on.")
injectCmd.PersistentFlags().StringVar(&imagePullPolicy, "imagePullPolicy", inject.DefaultImagePullPolicy,
"Sets the container image pull policy. Valid options are Always,IfNotPresent,Never."+
"The default policy is IfNotPresent.")
injectCmd.PersistentFlags().IntVar(&statusPort, inject.StatusPortCmdFlagName, inject.DefaultStatusPort,
"HTTP Port on which to serve pilot agent status. The path /healthz/ can be used for health checking. "+
"If zero, agent status will not be provided.")
injectCmd.PersistentFlags().Uint32Var(&readinessInitialDelaySeconds, "readinessInitialDelaySeconds", inject.DefaultReadinessInitialDelaySeconds,
"The initial delay (in seconds) for the readiness probe.")
injectCmd.PersistentFlags().Uint32Var(&readinessPeriodSeconds, "readinessPeriodSeconds", inject.DefaultReadinessPeriodSeconds,
"The period between readiness probes (in seconds).")
injectCmd.PersistentFlags().Uint32Var(&readinessFailureThreshold, "readinessFailureThreshold", inject.DefaultReadinessFailureThreshold,
"The threshold for successive failed readiness probes.")
injectCmd.PersistentFlags().StringVar(&includeIPRanges, "includeIPRanges", inject.DefaultIncludeIPRanges,
"Comma separated list of IP ranges in CIDR form. If set, only redirect outbound traffic to Envoy for "+
"these IP ranges. All outbound traffic can be redirected with the wildcard character '*'.")
injectCmd.PersistentFlags().StringVar(&excludeIPRanges, "excludeIPRanges", "",
"Comma separated list of IP ranges in CIDR form. If set, outbound traffic will not be redirected for "+
"these IP ranges. Exclusions are only applied if configured to redirect all outbound traffic. By "+
"default, no IP ranges are excluded.")
injectCmd.PersistentFlags().StringVar(&includeInboundPorts, "includeInboundPorts", inject.DefaultIncludeInboundPorts,
"Comma separated list of inbound ports for which traffic is to be redirected to Envoy. All ports can "+
"be redirected with the wildcard character '*'.")
injectCmd.PersistentFlags().StringVar(&excludeInboundPorts, "excludeInboundPorts", "",
"Comma separated list of inbound ports. If set, inbound traffic will not be redirected for those "+
"ports. Exclusions are only applied if configured to redirect all inbound traffic. By default, no ports "+
"are excluded.")
injectCmd.PersistentFlags().BoolVar(&debugMode, "debug", false, "Use debug images and settings for the sidecar")

deprecatedFlags := []string{"coreDump", "imagePullPolicy", "includeIPRanges", "excludeIPRanges", "hub", "tag",
"includeInboundPorts", "excludeInboundPorts", "debug", "verbosity", "sidecarProxyUID", "setVersionString"}
for _, opt := range deprecatedFlags {
_ = injectCmd.PersistentFlags().MarkDeprecated(opt, "Use --injectConfigMapName or --injectConfigFile instead")
}

injectCmd.PersistentFlags().StringVar(&meshConfigMapName, "meshConfigMapName", defaultMeshConfigMapName,
fmt.Sprintf("ConfigMap name for Istio mesh configuration, key should be %q", configMapKey))
injectCmd.PersistentFlags().StringVar(&injectConfigMapName, "injectConfigMapName", defaultInjectConfigMapName,
fmt.Sprintf("ConfigMap name for Istio sidecar injection, key should be %q."+
"This option overrides any other sidecar injection config options, eg. --hub",
injectConfigMapKey))
fmt.Sprintf("ConfigMap name for Istio sidecar injection, key should be %q.", injectConfigMapKey))
}
78 changes: 48 additions & 30 deletions pilot/pkg/kube/inject/inject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ const (
ProxyContainerName = "istio-proxy"
)

const (
sidecarTemplateDelimBegin = "[["
sidecarTemplateDelimEnd = "]]"
)

// SidecarInjectionSpec collects all container types and volumes for
// sidecar mesh injection
type SidecarInjectionSpec struct {
Expand Down Expand Up @@ -177,14 +182,11 @@ func ProxyImageName(hub string, tag string, debug bool) string {
// Params describes configurable parameters for injecting istio proxy
// into a kubernetes resource.
type Params struct {
InitImage string `json:"initImage"`
ProxyImage string `json:"proxyImage"`
Verbosity int `json:"verbosity"`
SidecarProxyUID uint64 `json:"sidecarProxyUID"`
Version string `json:"version"`
Mesh *meshconfig.MeshConfig `json:"-"`
ImagePullPolicy string `json:"imagePullPolicy"`
StatusPort int `json:"statusPort"`
InitImage string `json:"initImage"`
ProxyImage string `json:"proxyImage"`
Version string `json:"version"`
ImagePullPolicy string `json:"imagePullPolicy"`
Tracer string `json:"tracer"`
// Comma separated list of IP ranges in CIDR form. If set, only redirect outbound traffic to Envoy for these IP
// ranges. All outbound traffic can be redirected with the wildcard character "*". Defaults to "*".
IncludeIPRanges string `json:"includeIPRanges"`
Expand All @@ -200,16 +202,20 @@ type Params struct {
ExcludeInboundPorts string `json:"excludeInboundPorts"`
// Comma separated list of virtual interfaces whose inbound traffic (from VM) will be treated as outbound
// By default, no interfaces are configured.
KubevirtInterfaces string `json:"kubevirtInterfaces"`
ReadinessInitialDelaySeconds uint32 `json:"readinessInitialDelaySeconds"`
ReadinessPeriodSeconds uint32 `json:"readinessPeriodSeconds"`
ReadinessFailureThreshold uint32 `json:"readinessFailureThreshold"`
RewriteAppHTTPProbe bool `json:"rewriteAppHTTPProbe"`
EnableCoreDump bool `json:"enableCoreDump"`
DebugMode bool `json:"debugMode"`
Privileged bool `json:"privileged"`
SDSEnabled bool `json:"sdsEnabled"`
EnableSdsTokenMount bool `json:"enableSdsTokenMount"`
KubevirtInterfaces string `json:"kubevirtInterfaces"`
Verbosity int `json:"verbosity"`
SidecarProxyUID uint64 `json:"sidecarProxyUID"`
Mesh *meshconfig.MeshConfig `json:"-"`
StatusPort int `json:"statusPort"`
ReadinessInitialDelaySeconds uint32 `json:"readinessInitialDelaySeconds"`
ReadinessPeriodSeconds uint32 `json:"readinessPeriodSeconds"`
ReadinessFailureThreshold uint32 `json:"readinessFailureThreshold"`
RewriteAppHTTPProbe bool `json:"rewriteAppHTTPProbe"`
EnableCoreDump bool `json:"enableCoreDump"`
DebugMode bool `json:"debugMode"`
Privileged bool `json:"privileged"`
SDSEnabled bool `json:"sdsEnabled"`
EnableSdsTokenMount bool `json:"enableSdsTokenMount"`
}

// Validate validates the parameters and returns an error if there is configuration issue.
Expand All @@ -226,6 +232,30 @@ func (p *Params) Validate() error {
return ValidateExcludeInboundPorts(p.ExcludeInboundPorts)
}

// intoHelmValues returns a map of the traversed path in helm values YAML to the param value.
func (p *Params) intoHelmValues() map[string]string {
vals := map[string]string{
"global.proxy_init.image": p.InitImage,
"global.proxy.image": p.ProxyImage,
"global.proxy.enableCoreDump": strconv.FormatBool(p.EnableCoreDump),
"global.proxy.privileged": strconv.FormatBool(p.Privileged),
"global.imagePullPolicy": p.ImagePullPolicy,
"global.proxy.statusPort": strconv.Itoa(p.StatusPort),
"global.proxy.tracer": p.Tracer,
"global.proxy.readinessInitialDelaySeconds": strconv.Itoa(int(p.ReadinessInitialDelaySeconds)),
"global.proxy.readinessPeriodSeconds": strconv.Itoa(int(p.ReadinessPeriodSeconds)),
"global.proxy.readinessFailureThreshold": strconv.Itoa(int(p.ReadinessFailureThreshold)),
"global.sds.enabled": strconv.FormatBool(p.SDSEnabled),
"global.sds.useTrustworthyJwt": strconv.FormatBool(p.EnableSdsTokenMount),
"global.proxy.includeIPRanges": p.IncludeIPRanges,
"global.proxy.excludeIPRanges": p.ExcludeIPRanges,
"global.proxy.includeInboundPorts": p.IncludeInboundPorts,
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Mar 28, 2019

Choose a reason for hiding this comment

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

Why you removed the p.IncludeInboundPorts value set in https://github.com/istio/istio/pull/10830/files#diff-660cae0de46bea17fa1f553d7c030f31L296, but still use it here?

Copy link
Copy Markdown
Contributor Author

@mathspanda mathspanda Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't want to break unit tests at present.

Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu Mar 30, 2019

Choose a reason for hiding this comment

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

iiuc, this is useless, can u clean up and fix the tests?

Copy link
Copy Markdown
Contributor Author

@mathspanda mathspanda Apr 1, 2019

Choose a reason for hiding this comment

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

After I removed hardcoded template, the tests read input from install/kubernetes/helm instead of original template. This map aims to create mappings between variables in install/kubernetes/helm and struct Params.

"global.proxy.excludeInboundPorts": p.ExcludeInboundPorts,
"sidecarInjectorWebhook.rewriteAppHTTPProbe": strconv.FormatBool(p.RewriteAppHTTPProbe),
}
return vals
}

// Config specifies the sidecar injection configuration This includes
// the sidecar template and cluster-side injection policy. It is used
// by kube-inject, sidecar injector, and http endpoint.
Expand Down Expand Up @@ -727,18 +757,6 @@ func intoObject(sidecarTemplate string, meshconfig *meshconfig.MeshConfig, in ru
return out, nil
}

// GenerateTemplateFromParams generates a sidecar template from the legacy injection parameters
func GenerateTemplateFromParams(params *Params) (string, error) {
// Validate the parameters before we go any farther.
if err := params.Validate(); err != nil {
return "", err
}

var tmp bytes.Buffer
err := template.Must(template.New("inject").Parse(parameterizedTemplate)).Execute(&tmp, params)
return tmp.String(), err
}

func getPortsForContainer(container corev1.Container) []string {
parts := make([]string, 0)
for _, p := range container.Ports {
Expand Down
Loading