Apply overrides to chart values on install #15025

[raykroeker]

Running linkerd upgrade fails after running install with an external ca. The issue presents at upgrade but the cause is due to install incorrectly initializing the chart values for identity, specifically it initializes the issuer versus querying for one that is already set.

linkerd install \
  --ha \
  --set proxy.metrics.hostnameLabels=true \
  --set identity.externalCA=true \
  --set identity.issuer.scheme=kubernetes.io/tls \
  --set policyValidator.externalSecret=true \
  --set policyValidator.injectCaFrom=cert-manager/webhook-ca \
  --set proxyInjector.externalSecret=true \
  --set proxyInjector.injectCaFrom=cert-manager/webhook-ca \
  --set profileValidator.externalSecret=true \
  --set profileValidator.injectCaFrom=cert-manager/webhook-ca \
  --ignore-cluster

The install process did not apply the '--set' command line options onto internal values state before attempting to initialize the issuer credentials.

• The change applies the overrides to the values before initialize.
• Add test code to cover initializeIssuerCredentials.
• Track unit test run history in 'go-test.json' in the root directory when running in the justfile.
• Ignore 'go-test.json.'
• Add testify assertions as a go module.

Added unit test and ran through the integration test suite.

Fixes #15025

[adleong]

This relies on the "partial unmarshal" behavior where existing fields in values will be retained with their value if the field is not present in data. This is a bit subtle and in a casual reading of this code, one might expect values to be completely replaced by data. I think this could use a comment.

[adleong]

I hesitate to introduce a new testing library that is used here but not throughout the project. I'd prefer to evaluate testify independently of this PR. Can the test here be written without it?

[alpeb]

Thanks @raykroeker , I think this correctly addresses the issue for the linkerd install command, but if I'm not mistaking the problem remains for linkerd upgrade, so the function upgradeControlPlane needs a similar treatment.

[alpeb]

No need to account for the CRDs here. Shorter version:

Suggested change

	buf := &bytes.Buffer{}
	err := renderCRDs(context.Background(), nil, buf, opts, "yaml")
	if err != nil {
	t.Fatalf("cannot render-crds for new-k8s-api opts=%+v err=%v",
	opts, err)
	}
	api, err := k8s.NewFakeAPIFromManifests([]io.Reader{buf})
	api, err := k8s.NewFakeAPI()

[alpeb]

Ok I think the issue might actually be a bit different. Say you install cert-manager+trust-manager configured for linkerd as described here. So you end up with the linkerd-identity-trust-roots ConfigMap deployed in the linkerd namespace. Then you install linkerd with

linkerd install \
  --set identity.externalCA=true \
  --set identity.issuer.scheme=kubernetes.io/tls

That will retrieve the trust-root and put it in linkerd's config:

$ k -n linkerd get secret linkerd-config-overrides -oyaml|yq .data.\"linkerd-config-overrides\"|base64 -d
identity:
  externalCA: true
  issuer:
    scheme: kubernetes.io/tls
    tls:
      crtPEM: |
        -----BEGIN CERTIFICATE-----
        MIIBiTCCAS6gAwIBAgIBATAKBggqhkjOPQQDAjAcMRowGAYDVQQDExFpZGVudGl0
        eS5saW5rZXJkLjAeFw0yNjA0MTcyMzU1MzRaFw0yNzA0MTcyMzU1NTRaMBwxGjAY
        BgNVBAMTEWlkZW50aXR5LmxpbmtlcmQuMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcD
        QgAEPfckMf1lPoSD8ZtyUpkGAqnojauHrYs4piTMDbC8JMu6nKu0Q5VmKmAbwooW
        ym5zSGNfmpgw3cb4jfl6GK6f+aNhMF8wDgYDVR0PAQH/BAQDAgEGMB0GA1UdJQQW
        MBQGCCsGAQUFBwMBBggrBgEFBQcDAjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQW
        BBT+llc7aLuvZVEWjtddAVCtMPnlOjAKBggqhkjOPQQDAgNJADBGAiEA4Fc86Nf7
        N1IsrOWpK2ca8lRsdUgap5ZgL/ozkiaZLYUCIQCCB9KDxjvbrKEnGMWRTHQvwvz1
        XtngXkk0F9AhlMPsdw==
        -----END CERTIFICATE-----
      keyPEM: |
        -----BEGIN EC PRIVATE KEY-----
        <snip>
        -----END EC PRIVATE KEY-----
identityTrustAnchorsPEM: |
  -----BEGIN CERTIFICATE-----
  MIIBiTCCAS6gAwIBAgIBATAKBggqhkjOPQQDAjAcMRowGAYDVQQDExFpZGVudGl0
  eS5saW5rZXJkLjAeFw0yNjA0MTcyMzU1MzRaFw0yNzA0MTcyMzU1NTRaMBwxGjAY
  BgNVBAMTEWlkZW50aXR5LmxpbmtlcmQuMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcD
  QgAEPfckMf1lPoSD8ZtyUpkGAqnojauHrYs4piTMDbC8JMu6nKu0Q5VmKmAbwooW
  ym5zSGNfmpgw3cb4jfl6GK6f+aNhMF8wDgYDVR0PAQH/BAQDAgEGMB0GA1UdJQQW
  MBQGCCsGAQUFBwMBBggrBgEFBQcDAjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQW
  BBT+llc7aLuvZVEWjtddAVCtMPnlOjAKBggqhkjOPQQDAgNJADBGAiEA4Fc86Nf7
  N1IsrOWpK2ca8lRsdUgap5ZgL/ozkiaZLYUCIQCCB9KDxjvbrKEnGMWRTHQvwvz1
  XtngXkk0F9AhlMPsdw==
  -----END CERTIFICATE-----

So when running linkerd upgrade, this check in options.go won't let us through:

	if values.Identity.Issuer.Scheme == string(corev1.SecretTypeTLS) {
		if values.Identity.Issuer.TLS.CrtPEM != "" {
			return errors.New("--identity-issuer-certificate-file must not be specified if --identity-external-issuer=true")
		}

I think that check needs to be relaxed for the upgrade case.

[alpeb]

... and avoiding linkerd install to actually pull that cert and put it into the config is what your change actually fixes. Makes sense to me now, sorry for the noise 😊