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

Jump to bottom

Move default openshift version #3094

Merged
merged 10 commits into from
Jan 18, 2024
Merged

Move default openshift version #3094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d058b35
api: Avoid referencing DefaultInstallStream in tests
Aug 11, 2023
9a94353
frontend: Avoid referencing DefaultInstallStream
Aug 10, 2023
cc1c8ea
monitor: Remove latestGaMinorVersion metric
Aug 11, 2023
b9e928e
update_ocp_versions: Refactoring
Aug 9, 2023
7e24288
update_ocp_versions: Read versions from an environment variable
Aug 9, 2023
38c6b31
cache_fallback_discovery_client_test.go: Hard-code version
Aug 17, 2023
ad5abb9
version: Remove DefaultInstallStreams
Aug 17, 2023
837f5fb
cmd/aro: Prefix environment variable constants with "env"
Aug 23, 2023
b021c32
cmd/aro: Add more environment variable constants
Aug 23, 2023
d89b290
e2e: Use internal default OpenShift version for CI
Oct 4, 2023
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
10 changes: 6 additions & 4 deletions cmd/aro/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ package main
// Licensed under the Apache License 2.0.

const (
DatabaseName = "DATABASE_NAME"
DatabaseAccountName = "DATABASE_ACCOUNT_NAME"
KeyVaultPrefix = "KEYVAULT_PREFIX"
DBTokenUrl = "DBTOKEN_URL"
envDatabaseName = "DATABASE_NAME"
envDatabaseAccountName = "DATABASE_ACCOUNT_NAME"
envKeyVaultPrefix = "KEYVAULT_PREFIX"
envDBTokenUrl = "DBTOKEN_URL"
envOpenShiftVersions = "OPENSHIFT_VERSIONS"
envInstallerImageDigests = "INSTALLER_IMAGE_DIGESTS"
)
8 changes: 4 additions & 4 deletions cmd/aro/dbtoken.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ func dbtoken(ctx context.Context, log *logrus.Entry) error {

go g.Run()

if err := env.ValidateVars(DatabaseAccountName); err != nil {
if err := env.ValidateVars(envDatabaseAccountName); err != nil {
return err
}

dbAccountName := os.Getenv(DatabaseAccountName)
dbAccountName := os.Getenv(envDatabaseAccountName)

clientOptions := &policy.ClientOptions{
ClientOptions: _env.Environment().ManagedIdentityCredentialOptions().ClientOptions,
Expand All @@ -87,10 +87,10 @@ func dbtoken(ctx context.Context, log *logrus.Entry) error {
return err
}

if err := env.ValidateVars(KeyVaultPrefix); err != nil {
if err := env.ValidateVars(envKeyVaultPrefix); err != nil {
return err
}
keyVaultPrefix := os.Getenv(KeyVaultPrefix)
keyVaultPrefix := os.Getenv(envKeyVaultPrefix)
dbtokenKeyvaultURI := keyvault.URI(_env, env.DBTokenKeyvaultSuffix, keyVaultPrefix)
dbtokenKeyvault := keyvault.NewManager(msiKVAuthorizer, dbtokenKeyvaultURI)

Expand Down
8 changes: 4 additions & 4 deletions cmd/aro/gateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ func gateway(ctx context.Context, log *logrus.Entry) error {

go g.Run()

if err := env.ValidateVars(DatabaseAccountName); err != nil {
if err := env.ValidateVars(envDatabaseAccountName); err != nil {
return err
}
dbc, err := database.NewDatabaseClient(log.WithField("component", "database"), _env, nil, m, nil, os.Getenv(DatabaseAccountName))
dbc, err := database.NewDatabaseClient(log.WithField("component", "database"), _env, nil, m, nil, os.Getenv(envDatabaseAccountName))
if err != nil {
return err
}
Expand Down Expand Up @@ -136,9 +136,9 @@ func getURL(isLocalDevelopmentMode bool) (string, error) {
return "https://localhost:8445", nil
}

if err := env.ValidateVars(DBTokenUrl); err != nil {
if err := env.ValidateVars(envDBTokenUrl); err != nil {
return "", err
}

return os.Getenv(DBTokenUrl), nil
return os.Getenv(envDBTokenUrl), nil
}
4 changes: 2 additions & 2 deletions cmd/aro/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ func DBName(isLocalDevelopmentMode bool) (string, error) {
return "ARO", nil
}

if err := env.ValidateVars(DatabaseName); err != nil {
if err := env.ValidateVars(envDatabaseName); err != nil {
return "", fmt.Errorf("%v (development mode)", err.Error())
}

return os.Getenv(DatabaseName), nil
return os.Getenv(envDatabaseName), nil
}
8 changes: 4 additions & 4 deletions cmd/aro/monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ func monitor(ctx context.Context, log *logrus.Entry) error {
return err
}

if err := env.ValidateVars(KeyVaultPrefix); err != nil {
if err := env.ValidateVars(envKeyVaultPrefix); err != nil {
return err
}
keyVaultPrefix := os.Getenv(KeyVaultPrefix)
keyVaultPrefix := os.Getenv(envKeyVaultPrefix)
// TODO: should not be using the service keyvault here
serviceKeyvaultURI := keyvault.URI(_env, env.ServiceKeyvaultSuffix, keyVaultPrefix)
serviceKeyvault := keyvault.NewManager(msiKVAuthorizer, serviceKeyvaultURI)
Expand All @@ -83,11 +83,11 @@ func monitor(ctx context.Context, log *logrus.Entry) error {
return err
}

if err := env.ValidateVars(DatabaseAccountName); err != nil {
if err := env.ValidateVars(envDatabaseAccountName); err != nil {
return err
}

dbAccountName := os.Getenv(DatabaseAccountName)
dbAccountName := os.Getenv(envDatabaseAccountName)
clientOptions := &policy.ClientOptions{
ClientOptions: _env.Environment().ManagedIdentityCredentialOptions().ClientOptions,
}
Expand Down
8 changes: 4 additions & 4 deletions cmd/aro/portal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,10 @@ func portal(ctx context.Context, log *logrus.Entry, audit *logrus.Entry) error {

go g.Run()

if err := env.ValidateVars(KeyVaultPrefix); err != nil {
if err := env.ValidateVars(envKeyVaultPrefix); err != nil {
return err
}
keyVaultPrefix := os.Getenv(KeyVaultPrefix)
keyVaultPrefix := os.Getenv(envKeyVaultPrefix)
// TODO: should not be using the service keyvault here
serviceKeyvaultURI := keyvault.URI(_env, env.ServiceKeyvaultSuffix, keyVaultPrefix)
serviceKeyvault := keyvault.NewManager(msiKVAuthorizer, serviceKeyvaultURI)
Expand All @@ -93,11 +93,11 @@ func portal(ctx context.Context, log *logrus.Entry, audit *logrus.Entry) error {
return err
}

if err := env.ValidateVars(DatabaseAccountName); err != nil {
if err := env.ValidateVars(envDatabaseAccountName); err != nil {
return err
}

dbAccountName := os.Getenv(DatabaseAccountName)
dbAccountName := os.Getenv(envDatabaseAccountName)
clientOptions := &policy.ClientOptions{
ClientOptions: _env.Environment().ManagedIdentityCredentialOptions().ClientOptions,
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/aro/rp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,11 @@ func rp(ctx context.Context, log, audit *logrus.Entry) error {
return err
}

if err := env.ValidateVars(DatabaseAccountName); err != nil {
if err := env.ValidateVars(envDatabaseAccountName); err != nil {
return err
}

dbAccountName := os.Getenv(DatabaseAccountName)
dbAccountName := os.Getenv(envDatabaseAccountName)
clientOptions := &policy.ClientOptions{
ClientOptions: _env.Environment().ManagedIdentityCredentialOptions().ClientOptions,
}
Expand Down
111 changes: 85 additions & 26 deletions cmd/aro/update_ocp_versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,13 @@ import (
"github.com/Azure/ARO-RP/pkg/util/version"
)

func getInstallerImageDigests(envKey string) (map[string]string, error) {
var installerImageDigests map[string]string
// Corresponds to configuration.openShiftVersions in RP-Config
type OpenShiftVersions struct {
DefaultStream map[string]string
InstallStreams map[string]string
}
Comment on lines +25 to +29
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems odd to me (someone who is new to the team and GO) that we don't have some large struct to validate the contents of the RP-config already. Do we always just query part of the config piecemeal like this? Wouldn't it be more typical to load the config at start time and then access some object as needed? (Assuming that we don't have any dynamic components to the config). What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

The main goal of this work was to move the hardcoded configuration of the versioning into the database. Historically this configuration has been hardcoded so there was not a need to call it other than using pkg/util/version/

What you suggest is a good idea but we haven't gotten to a something like a general pkg/util/config at a team. In time this will probably make sense to consolidate the configuration of the RP in one package. But it was out of scope for this effort. Mainly when this was talked about there was some worry about not knowing what all the configuration was, so we decided to move one by one instead. With the idea that we would consolidate later. This is what I remember anyways.

While I wasn't the main person working on this. Matt has been moved to other priorities at the moment so the rest of Loki team is trying to push this one over the finish line for him.

+1 for a follow up discussion and effort on this for sure.

Copy link
Contributor Author

@mbarnes mbarnes Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, ARO.Pipelines has the full struct and parser you're looking for. What's happening in this context is the parser is extracting the OpenShift version bits we need from RP-Config and passing it to the RP as a JSON blob by way of an environment variable. All we're parsing here is the environment variable, not RP-Config directly.


func getEnvironmentData(envKey string, envData any) error {
var err error

jsonData := []byte(os.Getenv(envKey))
Expand All @@ -34,57 +39,111 @@ func getInstallerImageDigests(envKey string) (map[string]string, error) {
if !env.IsLocalDevelopmentMode() {
jsonData, err = base64.StdEncoding.DecodeString(string(jsonData))
if err != nil {
return nil, fmt.Errorf("%s: Failed to decode base64: %v", envKey, err)
return fmt.Errorf("%s: Failed to decode base64: %w", envKey, err)
}
}

if err = json.Unmarshal(jsonData, &installerImageDigests); err != nil {
return nil, fmt.Errorf("%s: Failed to parse JSON: %v", envKey, err)
if err = json.Unmarshal(jsonData, envData); err != nil {
SudoBrendan marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("%s: Failed to parse JSON: %w", envKey, err)
}

return installerImageDigests, nil
return nil
}

func getLatestOCPVersions(ctx context.Context, log *logrus.Entry) ([]api.OpenShiftVersion, error) {
env, err := env.NewCoreForCI(ctx, log)
if err != nil {
func getOpenShiftVersions() (*OpenShiftVersions, error) {
const envKey = envOpenShiftVersions
var openShiftVersions OpenShiftVersions

if err := getEnvironmentData(envKey, &openShiftVersions); err != nil {
return nil, err
}
dstAcr := os.Getenv("DST_ACR_NAME")
acrDomainSuffix := "." + env.Environment().ContainerRegistryDNSSuffix

dstRepo := dstAcr + acrDomainSuffix
ocpVersions := []api.OpenShiftVersion{}
// The DefaultStream map must have exactly one entry.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small question to help with my understanding of how we do things:

This comment tells me the "What" but not the "Why" and in the past this has been the rule I've used to determine if we need a comment. So:

  • Do we need to update this comment to the "Why"?
  • Can we just drop the comment overall?
  • Do I have this all wrong? (Probably some amount of this ;) )

Copy link
Contributor

Choose a reason for hiding this comment

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

Always good to get feedback on this. Basically we have only one default version for install - the latest Y version. Instead of having a default version per Y stream.

numDefaultStreams := len(openShiftVersions.DefaultStream)
if numDefaultStreams != 1 {
return nil, fmt.Errorf("%s: DefaultStream must have exactly 1 entry, found %d", envKey, numDefaultStreams)
}

return &openShiftVersions, nil
}

func getInstallerImageDigests() (map[string]string, error) {
// INSTALLER_IMAGE_DIGESTS is the mapping of a minor version to
// the aro-installer wrapper digest. This allows us to utilize
// Azure Safe Deployment Practices (SDP) instead of pushing the
// version tag and deploying to all regions at once.
installerImageDigests, err := getInstallerImageDigests("INSTALLER_IMAGE_DIGESTS")
if err != nil {
const envKey = envInstallerImageDigests
var installerImageDigests map[string]string

if err := getEnvironmentData(envKey, &installerImageDigests); err != nil {
return nil, err
}

for _, vers := range version.AvailableInstallStreams {
installerPullSpec := fmt.Sprintf("%s/aro-installer:%s", dstRepo, vers.Version.MinorVersion())
digest, ok := installerImageDigests[vers.Version.MinorVersion()]
return installerImageDigests, nil
}

func appendOpenShiftVersions(ocpVersions []api.OpenShiftVersion, installStreams map[string]string, installerImageName string, installerImageDigests map[string]string, isDefault bool) ([]api.OpenShiftVersion, error) {
for fullVersion, openShiftPullspec := range installStreams {
openShiftVersion, err := version.ParseVersion(fullVersion)
if err != nil {
return nil, err
}
fullVersion = openShiftVersion.String() // trimmed of whitespace
minorVersion := openShiftVersion.MinorVersion()
installerDigest, ok := installerImageDigests[minorVersion]
if !ok {
return nil, fmt.Errorf("no digest found for version %s", vers.Version.String())
return nil, fmt.Errorf("no installer digest for version %s", minorVersion)
}
installerPullspec := fmt.Sprintf("%s:%s@%s", installerImageName, minorVersion, installerDigest)

ocpVersions = append(ocpVersions, api.OpenShiftVersion{
Properties: api.OpenShiftVersionProperties{
Version: vers.Version.String(),
OpenShiftPullspec: vers.PullSpec,
InstallerPullspec: installerPullSpec + "@" + digest,
Version: fullVersion,
OpenShiftPullspec: openShiftPullspec,
InstallerPullspec: installerPullspec,
Enabled: true,
Default: isDefault,
},
})
}

return ocpVersions, nil
}

func getLatestOCPVersions(ctx context.Context, log *logrus.Entry) ([]api.OpenShiftVersion, error) {
env, err := env.NewCoreForCI(ctx, log)
if err != nil {
return nil, err
}
dstAcr := os.Getenv("DST_ACR_NAME")
acrDomainSuffix := "." + env.Environment().ContainerRegistryDNSSuffix
installerImageName := dstAcr + acrDomainSuffix + "/aro-installer"

openShiftVersions, err := getOpenShiftVersions()
if err != nil {
return nil, err
}

installerImageDigests, err := getInstallerImageDigests()
if err != nil {
return nil, err
}

ocpVersions := make([]api.OpenShiftVersion, 0, len(openShiftVersions.DefaultStream)+len(openShiftVersions.InstallStreams))

ocpVersions, err = appendOpenShiftVersions(ocpVersions, openShiftVersions.DefaultStream, installerImageName, installerImageDigests, true)
if err != nil {
return nil, err
}

ocpVersions, err = appendOpenShiftVersions(ocpVersions, openShiftVersions.InstallStreams, installerImageName, installerImageDigests, false)
if err != nil {
return nil, err
}

return ocpVersions, nil
}

func getVersionsDatabase(ctx context.Context, log *logrus.Entry) (database.OpenShiftVersions, error) {
_env, err := env.NewCore(ctx, log)
if err != nil {
Expand Down Expand Up @@ -113,10 +172,10 @@ func getVersionsDatabase(ctx context.Context, log *logrus.Entry) (database.OpenS

m := statsd.New(ctx, log.WithField("component", "update-ocp-versions"), _env, os.Getenv("MDM_ACCOUNT"), os.Getenv("MDM_NAMESPACE"), os.Getenv("MDM_STATSD_SOCKET"))

if err := env.ValidateVars(KeyVaultPrefix); err != nil {
if err := env.ValidateVars(envKeyVaultPrefix); err != nil {
return nil, err
}
keyVaultPrefix := os.Getenv(KeyVaultPrefix)
keyVaultPrefix := os.Getenv(envKeyVaultPrefix)
serviceKeyvaultURI := keyvault.URI(_env, env.ServiceKeyvaultSuffix, keyVaultPrefix)
serviceKeyvault := keyvault.NewManager(msiKVAuthorizer, serviceKeyvaultURI)

Expand All @@ -125,11 +184,11 @@ func getVersionsDatabase(ctx context.Context, log *logrus.Entry) (database.OpenS
return nil, err
}

if err := env.ValidateVars(DatabaseAccountName); err != nil {
if err := env.ValidateVars(envDatabaseAccountName); err != nil {
return nil, err
}

dbAccountName := os.Getenv(DatabaseAccountName)
dbAccountName := os.Getenv(envDatabaseAccountName)
clientOptions := &policy.ClientOptions{
ClientOptions: _env.Environment().ManagedIdentityCredentialOptions().ClientOptions,
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/api/v20191231preview/openshiftcluster_validatestatic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/util/uuid"
"github.com/Azure/ARO-RP/pkg/util/version"
"github.com/Azure/ARO-RP/test/validate"
)

Expand Down Expand Up @@ -51,7 +50,7 @@ func validOpenShiftCluster() *OpenShiftCluster {
ClusterProfile: ClusterProfile{
PullSecret: `{"auths":{"registry.connect.redhat.com":{"auth":""},"registry.redhat.io":{"auth":""}}}`,
Domain: "cluster.location.aroapp.io",
Version: version.DefaultInstallStream.Version.String(),
Version: "4.10.0",
mbarnes marked this conversation as resolved.
Show resolved Hide resolved
ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/test-cluster", subscriptionID),
},
ConsoleProfile: ConsoleProfile{
Expand Down
3 changes: 1 addition & 2 deletions pkg/api/v20200430/openshiftcluster_validatestatic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/util/uuid"
"github.com/Azure/ARO-RP/pkg/util/version"
"github.com/Azure/ARO-RP/test/validate"
)

Expand Down Expand Up @@ -51,7 +50,7 @@ func validOpenShiftCluster() *OpenShiftCluster {
ClusterProfile: ClusterProfile{
PullSecret: `{"auths":{"registry.connect.redhat.com":{"auth":""},"registry.redhat.io":{"auth":""}}}`,
Domain: "cluster.location.aroapp.io",
Version: version.DefaultInstallStream.Version.String(),
Version: "4.10.0",
ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/test-cluster", subscriptionID),
},
ConsoleProfile: ConsoleProfile{
Expand Down
3 changes: 1 addition & 2 deletions pkg/api/v20210901preview/openshiftcluster_validatestatic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/util/uuid"
"github.com/Azure/ARO-RP/pkg/util/version"
"github.com/Azure/ARO-RP/test/validate"
)

Expand Down Expand Up @@ -65,7 +64,7 @@ func validOpenShiftCluster() *OpenShiftCluster {
ClusterProfile: ClusterProfile{
PullSecret: `{"auths":{"registry.connect.redhat.com":{"auth":""},"registry.redhat.io":{"auth":""}}}`,
Domain: "cluster.location.aroapp.io",
Version: version.DefaultInstallStream.Version.String(),
Version: "4.10.0",
ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/test-cluster", subscriptionID),
},
ConsoleProfile: ConsoleProfile{
Expand Down
3 changes: 1 addition & 2 deletions pkg/api/v20230904/openshiftcluster_validatestatic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/util/uuid"
"github.com/Azure/ARO-RP/pkg/util/version"
"github.com/Azure/ARO-RP/test/validate"
)

Expand Down Expand Up @@ -71,7 +70,7 @@ func validOpenShiftCluster(name, location string) *OpenShiftCluster {
ClusterProfile: ClusterProfile{
PullSecret: `{"auths":{"registry.connect.redhat.com":{"auth":""},"registry.redhat.io":{"auth":""}}}`,
Domain: "cluster.location.aroapp.io",
Version: version.DefaultInstallStream.Version.String(),
Version: "4.10.0",
ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/test-cluster", subscriptionID),
FipsValidatedModules: FipsValidatedModulesDisabled,
},
Expand Down
Loading
Loading