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.

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

Common structs for Helm #3161

Merged
merged 1 commit into from
Aug 1, 2019
Merged

Common structs for Helm #3161

merged 1 commit into from
Aug 1, 2019

Conversation

alpeb
Copy link
Member

@alpeb alpeb commented Jul 30, 2019

This PR adds pkg/inject/template-values.go which contains the structs
needed to set the helm variables during injection. They can also be
leveraged during installation:

package inject

type (
        // Image contains the details to define a container image
        Image struct {
                Name       string
                PullPolicy string
                Version    string
        }

        // Port contains all the port-related setups
        Port struct {
                Admin               int32
                Control             int32
                Inbound             int32
                Outbound            int32
                IgnoreInboundPorts  string
                IgnoreOutboundPorts string
        }

        // Constraints wraps the Limit and Request settings for computational resources
        Constraints struct {
                Limit   string
                Request string
        }

        // Resources represents the computational resources setup for a given container
        Resources struct {
                CPU    Constraints
                Memory Constraints
        }

        // Identity contains the fields to set the identity variables in the proxy
        // sidecar container
        Identity struct {
                TrustDomain  string
                TrustAnchors string
        }

        // Proxy contains the fields to set the proxy sidecar container
        Proxy struct {
                Component             string
                ClusterDomain         string
                DisableIdentity       bool
                EnableExternalProfile bool
                HighAvailability      bool
                Identity              *Identity
                Image                 Image
                LogLevel              string
                ControlPlaneNamespace string
                Port                  Port
                UID                   int64
                ResourceRequirements  *Resources
        }

        // ProxyInit contains the fields to set the proxy-init container
        ProxyInit struct {
                Image                Image
                Port                 Port
                UID                  int64
                ResourceRequirements *Resources
        }
)

Signed-off-by: Alejandro Pedraza alejandro@buoyant.io

@l5d-bot
Copy link
Collaborator

l5d-bot commented Jul 30, 2019

Integration test results for 241c4d3: success 🎉
Log output: https://gist.github.com/68bd7539b2d6abb2474c097899ca951a

@ihcsim
Copy link
Contributor

ihcsim commented Jul 30, 2019

Thanks for putting this together. Per slack convo, I removed some of the redundant nested variables from values.yaml, which relied on YAML anchors to inherit their values from top-level variables. This approach is error-prone because e.g. helm install --set HighAvailability=true will only override the values of {{.HighAvailability}}, but not {{.Proxy.HighAvailability}}.

Here's the list of structs that correspond to the YAML in my values.yaml:

// top level variables 
type InjectValues struct {
	Namespace        string
	ClusterDomain    string
	HighAvailability bool

	Proxy     *Proxy
	ProxyInit *ProxyInit
}

type Proxy struct {
	Capabilities          *Capabilities
	Component             string
	DisableIdentity       bool
	DisableTap            bool
	EnableExternalProfile bool
	Image                 *Image
	Identity              *Identity
	LogLevel              string
	MountPaths            []*MountPath
	Ports                 *Ports
	Resources             *Resources
	UID                   string
}

type ProxyInit struct {
	Capabilities        *Capabilities
	IgnoreInboundPorts  string
	IgnoreOutboundPorts string
	Image               *Image
	MountPaths          []*MountPath
	Resources           *Resources
}

type Capabilities struct {
	Add  []string
	Drop []string
}

type Image struct {
	Name       string
	Version    string
	PullPolicy string
}

type Identity struct {
	Issuer struct {
		CrtPEM string
	}

	TrustDomain string
}

type MountPath struct {
	Name      string
	MountPath string
}

type Resources struct {
	CPU    Constraints
	Memory Constraints
}

type Constraints struct {
	Limit   string
	Request string
}

type Ports struct {
	Admin    int
	Control  int
	Inbound  int
	Outbound int
}

LMK if this works.

ihcsim pushed a commit that referenced this pull request Jul 30, 2019
Signed-off-by: Ivan Sim <ivan@buoyant.io>
ihcsim pushed a commit that referenced this pull request Jul 30, 2019
Signed-off-by: Ivan Sim <ivan@buoyant.io>
@alpeb
Copy link
Member Author

alpeb commented Jul 31, 2019

Looks great @ihcsim 👍
I've pushed a change with your modifications, plus some of my own:

  • I moved Identity from Proxy to InjectValues, according to values.yaml
  • In both Proxy and ProxyInit I replaced MountPaths []*MountPath with SAMountPath *SAMountPath given we're only adding a mount for the ServiceAccount so no need of a slice here
  • I created a separate Issuer struct to be able to instantiate that piece

@l5d-bot
Copy link
Collaborator

l5d-bot commented Jul 31, 2019

Integration test results for 273eef1: fail 😕
Log output: https://gist.github.com/090f0df58068a8454fd1c2fc62be6484

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

lgtm just a couple questions 🚢 👍

pkg/inject/template-values.go Outdated Show resolved Hide resolved
// ProxyInit contains the fields to set the proxy-init container
ProxyInit struct {
Capabilities *Capabilities
IgnoreInboundPorts string
Copy link
Member

@siggy siggy Jul 31, 2019

Choose a reason for hiding this comment

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

can/should this be an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we might. WDYT @ihcsim ?

Copy link
Contributor

@ihcsim ihcsim Jul 31, 2019

Choose a reason for hiding this comment

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

The value of this variable eventually gets mapped into the args of the proxy-init container. From a rendering perspective, I think it should remain as string. Otherwise, won't we end up with something like:

  initContainers:
  - args:
    ...
    - --inbound-ports-to-ignore
    - [4190,4191]
    - --outbound-ports-to-ignore
    - ["443"] 

@ihcsim
Copy link
Contributor

ihcsim commented Aug 1, 2019

I made a mistake in the Identity struct. Instead of Identity.Issuer.CrtPEM, the data plane should be using Identity.TrustAnchorsPEM. Although they both have the same default values, the former is used on the identity service (via Secret), whereas the trust anchor is used in the proxy's LINKERD2_PROXY_IDENTITY_TRUST_ANCHORS env var, which I believe is the one needed here.

@alpeb
Copy link
Member Author

alpeb commented Aug 1, 2019

I've just pushed the changes according to the previous message. I've also changed the name of injectValues to Values, to avoid stuttering since we're already in the inject package.
@ihcsim please let me know when you're ready for me to merge this into isim/helm-charts

@alpeb alpeb changed the base branch from isim/helm-charts to master August 1, 2019 18:38
Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@alpeb alpeb merged commit 2f72ad6 into master Aug 1, 2019
@alpeb alpeb deleted the alpeb/helm-charts-common branch August 1, 2019 19:00
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 1, 2019

Integration test results for 241bb7f: success 🎉
Log output: https://gist.github.com/faa8b9386df6519592d08603ec48650d

ihcsim pushed a commit that referenced this pull request Aug 2, 2019
Signed-off-by: Ivan Sim <ivan@buoyant.io>
ihcsim added a commit that referenced this pull request Aug 6, 2019
* Updated controller template with proxy partials
* Declare dependency in requirements.yaml
* Add partial template for proxy's metadata
* Add proxy-init partial template
* Script to lint Helm charts and update their dependencies
* Update partials chart Chart.yaml
* Add proxy-init and resource partial templates
* Replace hard coded namespace variable in proxy env var
* Ignore chart dependencies .tgz files
* Add missing fields and re-order YAML elements to match CLI output
* Reuse control plane's resource partial template in 'partials' chart
* Set the proxy's destination service address env var
* Add Grafana's template
* Update api version of controller RBAC
* Add Heartbeat template
* Remove duplicated resources partial template
* Add remainder control plane components templates
* Add template for the 'linkerd-config' config map
* Add debug container template
* Update proxy partial with 'disable-identity' and 'disable-tap' variables

Note that these are inject-only variables.
Also added the LINKERD2_PROXY_TAP_SVC_NAME env var.

* Add validation conditions to ensure identity and tap aren't disabled for
control plane components
* Add partials for service account token mount path and security context capabilities
* Change proxy and proxy-init templates to use global scope

Some of the nested variables are removed from values.yaml to ensure changes
made to root-level variables are propagated directly into the partial
templates. The previous approach of using YAML anchors in the
values.yaml to share common values can get out-of-sync when values are
changed via the Helm's `--set` option.

* Update templates and values file to match #3161
* Perform a dry run installation if there is a local Tiller
* Reorder JSON elements in linkerd-config
* Re-adjust nested partials indentation to work with inject 'patch' chart

Previously, the partials will render their content as an element in the list.
While it works for installation, the toJson function in the 'inject' patch code
ends up converting it into a JSON list, instead of the expected JSON
object.

* Trap the last fail command in the Helm shell script
* Add the identity trust anchor
* Address Thomas' feedback on handling HA

All the HA-related variables are moved to values-ha.yaml

* Convert ignore ports string to JSON list in linkerd-config

Also fixed some indentation issues.

* Add values-ha.yaml
* Include the service account token mount path only if identity is enabled
* Fixed malformed JSON in linkerd-config config map
* Rename chart to 'linkerd2'
* Add NOTES.txt
* Fix incorrect variable path in proxy template
* Remove fake TLS assets
* Add 'required' constraint to identity trust anchors variable
* Update tap templates per #3167
* Bump default version to edge-19.8.1 due to dependency on RSA support

Signed-off-by: Ivan Sim <ivan@buoyant.io>
cpretzer pushed a commit that referenced this pull request Aug 6, 2019
* Updated controller template with proxy partials
* Declare dependency in requirements.yaml
* Add partial template for proxy's metadata
* Add proxy-init partial template
* Script to lint Helm charts and update their dependencies
* Update partials chart Chart.yaml
* Add proxy-init and resource partial templates
* Replace hard coded namespace variable in proxy env var
* Ignore chart dependencies .tgz files
* Add missing fields and re-order YAML elements to match CLI output
* Reuse control plane's resource partial template in 'partials' chart
* Set the proxy's destination service address env var
* Add Grafana's template
* Update api version of controller RBAC
* Add Heartbeat template
* Remove duplicated resources partial template
* Add remainder control plane components templates
* Add template for the 'linkerd-config' config map
* Add debug container template
* Update proxy partial with 'disable-identity' and 'disable-tap' variables

Note that these are inject-only variables.
Also added the LINKERD2_PROXY_TAP_SVC_NAME env var.

* Add validation conditions to ensure identity and tap aren't disabled for
control plane components
* Add partials for service account token mount path and security context capabilities
* Change proxy and proxy-init templates to use global scope

Some of the nested variables are removed from values.yaml to ensure changes
made to root-level variables are propagated directly into the partial
templates. The previous approach of using YAML anchors in the
values.yaml to share common values can get out-of-sync when values are
changed via the Helm's `--set` option.

* Update templates and values file to match #3161
* Perform a dry run installation if there is a local Tiller
* Reorder JSON elements in linkerd-config
* Re-adjust nested partials indentation to work with inject 'patch' chart

Previously, the partials will render their content as an element in the list.
While it works for installation, the toJson function in the 'inject' patch code
ends up converting it into a JSON list, instead of the expected JSON
object.

* Trap the last fail command in the Helm shell script
* Add the identity trust anchor
* Address Thomas' feedback on handling HA

All the HA-related variables are moved to values-ha.yaml

* Convert ignore ports string to JSON list in linkerd-config

Also fixed some indentation issues.

* Add values-ha.yaml
* Include the service account token mount path only if identity is enabled
* Fixed malformed JSON in linkerd-config config map
* Rename chart to 'linkerd2'
* Add NOTES.txt
* Fix incorrect variable path in proxy template
* Remove fake TLS assets
* Add 'required' constraint to identity trust anchors variable
* Update tap templates per #3167
* Bump default version to edge-19.8.1 due to dependency on RSA support

Signed-off-by: Ivan Sim <ivan@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants