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

⚠️Remove hard code manifest version and hash #3707

Closed
wants to merge 1 commit into from
Closed

⚠️Remove hard code manifest version and hash #3707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions cmd/clusterctl/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ func (f fakeClusterClient) Proxy() cluster.Proxy {
return f.fakeProxy
}

func (f *fakeClusterClient) CertManager() cluster.CertManagerClient {
return f.certManager
func (f *fakeClusterClient) CertManager() (cluster.CertManagerClient, error) {
return f.certManager, nil
}

func (f fakeClusterClient) ProviderComponents() cluster.ComponentsClient {
Expand Down
94 changes: 67 additions & 27 deletions cmd/clusterctl/client/cluster/cert_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package cluster

import (
"context"
"crypto/sha256"
"fmt"
"strings"
"time"

"github.com/pkg/errors"
Expand Down Expand Up @@ -49,17 +51,7 @@ const (
certmanagerVersionAnnotation = "certmanager.clusterctl.cluster.x-k8s.io/version"
certmanagerHashAnnotation = "certmanager.clusterctl.cluster.x-k8s.io/hash"

// NOTE: // If the cert-manager.yaml asset is modified, this line **MUST** be updated
// accordingly else future upgrades of the cert-manager component will not
// be possible, as there'll be no record of the version installed.
embeddedCertManagerManifestVersion = "v0.16.1"

// NOTE: The hash is used to ensure that when the cert-manager.yaml file is updated,
// the version number marker here is _also_ updated.
// You can either generate the SHA256 hash of the file, or alternatively
// run `go test` against this package. THe Test_VersionMarkerUpToDate will output
// the expected hash if it does not match the hash here.
embeddedCertManagerManifestHash = "af8c08a8eb65d102ba98889a89f4ad1d3db5d302edb5b8f8f3e69bb992faa211"
certmanagerVersionLabel = "helm.sh/chart"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to point out that after version v0.16.1 the cert-manager CRDs do not have this label.
So that would mean that checking just CustomResourceDefinitions won't work. But maybe that's fine if we don't plan to support cert-manager upgrades to v1.x.x
The label is available on Deployment types (if installed via helm charts).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the sharing, do you think we should move this check to deployment then?
or let it be ? because lack of further info on 0.16.x -> 1.x.x impact and implication of that..

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...I'm not fully sure. Maybe @fabriziopandini can chime in.
My thoughts for now is to keep it as is because currently we do not seem to have a defined plan for upgrading to cert-manager v1.x.x.
If we do want to do that upgrade, we can fix this as part of that story/issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I will keep it for now and hopefully someone else can provide additional comments

)

// CertManagerUpgradePlan defines the upgrade plan if cert-manager needs to be
Expand Down Expand Up @@ -89,21 +81,69 @@ type CertManagerClient interface {

// certManagerClient implements CertManagerClient .
type certManagerClient struct {
configClient config.Client
proxy Proxy
pollImmediateWaiter PollImmediateWaiter
configClient config.Client
proxy Proxy
pollImmediateWaiter PollImmediateWaiter
embeddedCertManagerManifestVersion string
embeddedCertManagerManifestHash string
}

// Ensure certManagerClient implements the CertManagerClient interface.
var _ CertManagerClient = &certManagerClient{}

// newCertMangerClient returns a certManagerClient.
func newCertMangerClient(configClient config.Client, proxy Proxy, pollImmediateWaiter PollImmediateWaiter) *certManagerClient {
return &certManagerClient{
func (cm *certManagerClient) setManifestHash() error {
yamlData, err := manifests.Asset(embeddedCertManagerManifestPath)
if err != nil {
return err
}
cm.embeddedCertManagerManifestHash = fmt.Sprintf("%x", sha256.Sum256(yamlData))
return nil
}

func (cm *certManagerClient) setManifestVersion() error {
// Gets the cert-manager objects from the embedded assets.
objs, err := cm.getManifestObjs()
if err != nil {
return err
}
found := false

for i := range objs {
o := objs[i]
if o.GetKind() == "CustomResourceDefinition" {
labels := o.GetLabels()
version, ok := labels[certmanagerVersionLabel]
if ok {
s := strings.Split(version, "-")
cm.embeddedCertManagerManifestVersion = s[2]
found = true
break
}
}
}
if !found {
return errors.Errorf("Failed to detect cert-manager version by searching for label %s in all CRDs", certmanagerVersionLabel)
}
return nil
}

// newCertManagerClient returns a certManagerClient.
func newCertManagerClient(configClient config.Client, proxy Proxy, pollImmediateWaiter PollImmediateWaiter) (*certManagerClient, error) {
cm := &certManagerClient{
configClient: configClient,
proxy: proxy,
pollImmediateWaiter: pollImmediateWaiter,
}
err := cm.setManifestVersion()
if err != nil {
return nil, err
}

err = cm.setManifestHash()
if err != nil {
return nil, err
}
return cm, nil
}

// Images return the list of images required for installing the cert-manager.
Expand Down Expand Up @@ -134,7 +174,7 @@ func (cm *certManagerClient) EnsureInstalled() error {
return nil
}

log.Info("Installing cert-manager", "Version", embeddedCertManagerManifestVersion)
log.Info("Installing cert-manager", "Version", cm.embeddedCertManagerManifestVersion)
return cm.install()
}

Expand Down Expand Up @@ -178,14 +218,14 @@ func (cm *certManagerClient) PlanUpgrade() (CertManagerUpgradePlan, error) {
return CertManagerUpgradePlan{}, errors.Wrap(err, "failed get cert manager components")
}

currentVersion, shouldUpgrade, err := shouldUpgrade(objs)
currentVersion, shouldUpgrade, err := cm.shouldUpgrade(objs)
if err != nil {
return CertManagerUpgradePlan{}, err
}

return CertManagerUpgradePlan{
From: currentVersion,
To: embeddedCertManagerManifestVersion,
To: cm.embeddedCertManagerManifestVersion,
ShouldUpgrade: shouldUpgrade,
}, nil
}
Expand All @@ -201,7 +241,7 @@ func (cm *certManagerClient) EnsureLatestVersion() error {
return errors.Wrap(err, "failed get cert manager components")
}

currentVersion, shouldUpgrade, err := shouldUpgrade(objs)
currentVersion, shouldUpgrade, err := cm.shouldUpgrade(objs)
if err != nil {
return err
}
Expand All @@ -220,7 +260,7 @@ func (cm *certManagerClient) EnsureLatestVersion() error {
}

// install the cert-manager version embedded in clusterctl
log.Info("Installing cert-manager", "Version", embeddedCertManagerManifestVersion)
log.Info("Installing cert-manager", "Version", cm.embeddedCertManagerManifestVersion)
return cm.install()
}

Expand Down Expand Up @@ -254,7 +294,7 @@ func (cm *certManagerClient) deleteObjs(objs []unstructured.Unstructured) error
return nil
}

func shouldUpgrade(objs []unstructured.Unstructured) (string, bool, error) {
func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (string, bool, error) {
needUpgrade := false
currentVersion := ""
for i := range objs {
Expand All @@ -279,7 +319,7 @@ func shouldUpgrade(objs []unstructured.Unstructured) (string, bool, error) {
return "", false, errors.Wrapf(err, "failed to parse version for cert-manager component %s/%s", obj.GetKind(), obj.GetName())
}

c, err := objSemVersion.Compare(embeddedCertManagerManifestVersion)
c, err := objSemVersion.Compare(cm.embeddedCertManagerManifestVersion)
if err != nil {
return "", false, errors.Wrapf(err, "failed to compare version for cert-manager component %s/%s", obj.GetKind(), obj.GetName())
}
Expand All @@ -292,7 +332,7 @@ func shouldUpgrade(objs []unstructured.Unstructured) (string, bool, error) {
case c == 0:
// if version == current, check the manifest hash; if it does not exists or if it is different, then upgrade
objHash, ok := obj.GetAnnotations()[certmanagerHashAnnotation]
if !ok || objHash != embeddedCertManagerManifestHash {
if !ok || objHash != cm.embeddedCertManagerManifestHash {
currentVersion = fmt.Sprintf("%s (%s)", objVersion, objHash)
needUpgrade = true
break
Expand Down Expand Up @@ -383,8 +423,8 @@ func (cm *certManagerClient) createObj(obj unstructured.Unstructured) error {
}
// persist the version number of stored resources to make a
// future enhancement to add upgrade support possible.
annotations[certmanagerVersionAnnotation] = embeddedCertManagerManifestVersion
annotations[certmanagerHashAnnotation] = embeddedCertManagerManifestHash
annotations[certmanagerVersionAnnotation] = cm.embeddedCertManagerManifestVersion
annotations[certmanagerHashAnnotation] = cm.embeddedCertManagerManifestHash
obj.SetAnnotations(annotations)

c, err := cm.proxy.NewClient()
Expand Down
Loading