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

Add linkerd-config-overrides secret #4911

Merged
merged 6 commits into from
Sep 29, 2020
Merged

Conversation

adleong
Copy link
Member

@adleong adleong commented Aug 26, 2020

This PR adds a new secret to the output of linkerd install called linkerd-config-overrides. This is the first step towards simplifying the configuration of the linkerd install and upgrade flow through the CLI. This secret contains the subset of the values.yaml which have been overridden. In other words, the subset of values which differ from their default values. The idea is that this will give us a simpler way to produce the linkerd upgrade output while still persisting options set during install. This will eventually replace the linkerd-config configmap entirely.

This PR only adds and populates the new secret. The secret is not yet read or used anywhere. Subsequent PRs will update individual control plane components to accept their configuration through flags and will update the linkerd upgrade flow to use this secret instead of the linkerd-config configmap.

This secret is only generated by the CLI and is not present or required when installing or upgrading with Helm.

Here are sample contents of the secret, base64 decoded. Note that identity tls context is saved as an override so that it can be persisted across updates. Since these fields contain private key material, this object must be a secret. This secret is only used for upgrades and thus only the CLI needs to be able to read it. We will not create any RBAC bindings to grant service accounts access to this secret.

global:
  identityTrustAnchorsPEM: |
    -----BEGIN CERTIFICATE-----
    MIIBhDCCASmgAwIBAgIBATAKBggqhkjOPQQDAjApMScwJQYDVQQDEx5pZGVudGl0
    eS5saW5rZXJkLmNsdXN0ZXIubG9jYWwwHhcNMjAwODI1MjMzMTU3WhcNMjEwODI1
    MjMzMjE3WjApMScwJQYDVQQDEx5pZGVudGl0eS5saW5rZXJkLmNsdXN0ZXIubG9j
    YWwwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQ0e7IPBlVZ03TL8UVlODllbh8b
    2pcM5mbtSGgpX9z0l3n5M70oHn715xu2szh63oBjPl2ZfOA5Bd43cJIksONQo0Iw
    QDAOBgNVHQ8BAf8EBAMCAQYwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMC
    MA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDSQAwRgIhAI7Sy8P+3TYCJBlK
    pIJSZD4lGTUyXPD4Chl/FwWdFfvyAiEA6AgCPbNCx1dOZ8RpjsN2icMRA8vwPtTx
    oSfEG/rBb68=
    -----END CERTIFICATE-----
heartbeatSchedule: '42 23 * * * '
identity:
  issuer:
    crtExpiry: "2021-08-25T23:32:17Z"
    tls:
      crtPEM: |
        -----BEGIN CERTIFICATE-----
        MIIBhDCCASmgAwIBAgIBATAKBggqhkjOPQQDAjApMScwJQYDVQQDEx5pZGVudGl0
        eS5saW5rZXJkLmNsdXN0ZXIubG9jYWwwHhcNMjAwODI1MjMzMTU3WhcNMjEwODI1
        MjMzMjE3WjApMScwJQYDVQQDEx5pZGVudGl0eS5saW5rZXJkLmNsdXN0ZXIubG9j
        YWwwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQ0e7IPBlVZ03TL8UVlODllbh8b
        2pcM5mbtSGgpX9z0l3n5M70oHn715xu2szh63oBjPl2ZfOA5Bd43cJIksONQo0Iw
        QDAOBgNVHQ8BAf8EBAMCAQYwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMC
        MA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDSQAwRgIhAI7Sy8P+3TYCJBlK
        pIJSZD4lGTUyXPD4Chl/FwWdFfvyAiEA6AgCPbNCx1dOZ8RpjsN2icMRA8vwPtTx
        oSfEG/rBb68=
        -----END CERTIFICATE-----
      keyPEM: |
        -----BEGIN EC PRIVATE KEY-----
        MHcCAQEEIJaqjoDnqkKSsTqJMGeo3/1VMfJTBsMEuMWYzdJVxIhToAoGCCqGSM49
        AwEHoUQDQgAENHuyDwZVWdN0y/FFZTg5ZW4fG9qXDOZm7UhoKV/c9Jd5+TO9KB5+
        9ecbtrM4et6AYz5dmXzgOQXeN3CSJLDjUA==
        -----END EC PRIVATE KEY-----

@adleong adleong requested a review from a team as a code owner August 26, 2020 00:16
@@ -141,7 +141,7 @@ identity:
# control plane annotation - do not edit
crtExpiryAnnotation: linkerd.io/identity-issuer-expiry

issuanceLifetime: 86400s
issuanceLifetime: 24h0m0s
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how go serializes this duration. By formatting it this way in the chart values.yaml, we avoid spuriously detecting it as overridden.

@@ -13,7 +13,7 @@ import (
"k8s.io/helm/pkg/timeconv"
)

const versionPlaceholder = "{version}"
const versionPlaceholder = "linkerdVersionValue"
Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect that this was missed in #4373

Copy link
Member

Choose a reason for hiding this comment

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

good catch 👍

@@ -91,6 +91,7 @@ type (
IdentityTrustDomain string `json:"identityTrustDomain"`
PrometheusURL string `json:"prometheusUrl"`
GrafanaURL string `json:"grafanaUrl"`
LinkerdVersion string `json:"linkerdVersion"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this field was missing to begin with.


// Tree is a structured representation of a string keyed tree document such as
// yaml or json.
type Tree map[string]interface{}
Copy link
Member Author

Choose a reason for hiding this comment

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

You down with ADT? Yeah you know me!
You down with ADT? Yeah you know me!
You down with ADT? Yeah you know me!
Who's down with ADT? NOT GOLANG

@alpeb
Copy link
Member

alpeb commented Aug 26, 2020

Ref #4914

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

This looks fine to me 👍

Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM!

Tested this out with various different overridden configurations i.e ha, addons-config, etc and works very well! 👍

pkg/tree/tree.go Outdated
return string(bytes), nil
}

// String returns a yaml represetation of the Tree or an error string if
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: representation

pkg/tree/tree.go Outdated
return diff, nil
}

// Prune removes all empty subtress. A subtree is considered empty if it does
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: subtrees

pkg/tree/tree.go Show resolved Hide resolved
@adleong
Copy link
Member Author

adleong commented Sep 25, 2020

This PR is a subset of #5005

I need to back-port a few changes from that PR into this one before it is ready to merge. Once that is done, I would like to merge this PR since it is a safe incremental step toward #5005 and will ultimately make that PR's diff smaller.

@adleong adleong marked this pull request as draft September 25, 2020 00:11
@adleong adleong force-pushed the alex/override-of-the-valkyries branch from 87da5e3 to c776584 Compare September 25, 2020 23:24
@adleong adleong marked this pull request as ready for review September 25, 2020 23:27
@adleong adleong merged commit 1784f06 into main Sep 29, 2020
@adleong adleong deleted the alex/override-of-the-valkyries branch September 29, 2020 15:01
@adleong adleong mentioned this pull request Sep 29, 2020
olix0r pushed a commit that referenced this pull request Sep 29, 2020
A conflict between #4911 and #4737 caused unit test to be broken.

#4737 added a new test to `upgrade_test.go` and the changes in
#4911 updated all of these test to ignore differences in the config
overrides secret.  Since these two PRs merged in parallel, the new
test was missing this update.

Update the new test to also ignore differences in the config overrides
secret as the other ones do.

Signed-off-by: Alex Leong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants