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

Installer images use sha256 #2967

Merged
merged 4 commits into from
Jul 3, 2023
Merged

Installer images use sha256 #2967

merged 4 commits into from
Jul 3, 2023

Conversation

bennerv
Copy link
Collaborator

@bennerv bennerv commented Jun 15, 2023

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-3276

What this PR does / why we need it:

Allows us to push installer images without having them affect the entire fleet at once and in an inconsistent manner.

Right now here's how it happens:

  1. Push a new installer image
  2. imagePullPolicy on hive is IfNotPresent
  3. If the node has the image cached, it will use the older installer image
  4. if the node does not have the image cached, it will use the new installer image

our e2e will be inconsistent after an installer image push, and there's no guarantee around which image is used and when only the newer images are used.

Test plan for issue:

  1. Roll out this change via update_ocp_versions
  2. Run an e2e
  3. Check the installer image being used is being pulled with image digest

Is there any documentation that needs to be updated for this

Yes. TBD

High level:

  1. Push new image to all ACRs
  2. Update the SHAs used here
  3. Build the update_ocp_versions binary
  4. Run the ev2 release for update_ocp_versions in SDP manner
  5. Profit

There's some docs needed on how to pull the imagedigest for a new image.

hawkowl
hawkowl reviewed Jun 16, 2023
View reviewed changes
cmd/aro/update_ocp_versions.go
@@ -31,11 +31,19 @@ func getLatestOCPVersions(ctx context.Context, log *logrus.Entry) ([]api.OpenShi
ocpVersions := []api.OpenShiftVersion{}

for _, vers := range version.HiveInstallStreams {
installerPullSpec := fmt.Sprintf("%s/aro-installer:%s", dstRepo, vers.Version.MinorVersion())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Initially, I was planning that this would come from RP-Config (so that we can update the versions w/o a new code artifact). Should we do that instead, or after this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I think it makes sense to do this after to get the installer AAD migration changes in first.

@mbarnes mbarnes force-pushed the installer-images-use-sha256 branch from e52a714 to 3817007 Compare June 20, 2023 15:58
@mbarnes mbarnes force-pushed the installer-images-use-sha256 branch from 3817007 to 9e5b413 Compare June 29, 2023 16:03
@bennerv bennerv added the next-release To be included in the next RP release rollout label Jun 29, 2023
@bennerv bennerv force-pushed the installer-images-use-sha256 branch from b0a6ecd to 1d66f54 Compare June 29, 2023 17:30
bennerv and others added 2 commits June 29, 2023 14:32
@bennerv
Leverage image digests when using the aro-installer image
acc8a96
@mbarnes @bennerv
frontend: Discontinue adding default to enabled OCP versions
95d260f 
The CosmosDB "OpenShiftVersions" container in all environments is
now primed with supported OCP versions. RP no longer needs to hard
code a default in enabledOcpVersions.
@bennerv bennerv force-pushed the installer-images-use-sha256 branch from 3402a4d to 099ecc5 Compare June 29, 2023 18:32
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 29, 2023
View reviewed changes
pkg/util/cluster/cluster.go
cli := &http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
@bennerv bennerv force-pushed the installer-images-use-sha256 branch from 099ecc5 to 6580b43 Compare June 30, 2023 14:53
@mbarnes
Copy link
Contributor

mbarnes commented Jul 3, 2023 •
edited
Loading

Verified Hive is using a SHA-based installer image for the default image after deploying this PR to our INT environment.

public-int-installer-image

@bennerv bennerv removed the hold Hold label Jul 3, 2023
s-amann
s-amann reviewed Jul 3, 2023
View reviewed changes
pkg/cluster/version.go
// add the default OCP version as we require it as an install target
// if this is removed, we need to also update the logic in
// pkg/frontend/frontend.go, pkg/frontend/validate.go
defaultVersion := &api.OpenShiftVersion{
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to double check we have versions in all regions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's my only concern with this change. Essentially if we don't have any version within a region no clusters will successfully install with this change.

If we don't have a version though, both the frontend and backend should return that the version doesn't exist, which I think is okay because it should pop up on our cluster install failure dashboard and we can catch it.

Additionally, e2e will fail to run.

s-amann
s-amann reviewed Jul 3, 2023
View reviewed changes
pkg/frontend/validate.go
@@ -205,13 +205,12 @@ func validateAdminMasterVMSize(vmSize string) error {
func (f *frontend) validateInstallVersion(ctx context.Context, doc *api.OpenShiftCluster) error {
// If this request is from an older API or the user never specified
// the version to install we default to the DefaultInstallStream.Version
// TODO: We should set default version in cosmosdb instead of hardcoding it in golang code
Copy link
Contributor

Choose a reason for hiding this comment

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

is this TODO already in JIRA?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no i need to create it.

@bennerv bennerv merged commit 0378d0e into Azure:master Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkowl hawkowl hawkowl left review comments

@cadenmarchese cadenmarchese cadenmarchese left review comments

@jhoreman jhoreman jhoreman left review comments

@s-amann s-amann s-amann approved these changes

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@facchettos facchettos Awaiting requested review from facchettos

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@SudoBrendan SudoBrendan Awaiting requested review from SudoBrendan SudoBrendan is a code owner

@ellis-johnson ellis-johnson Awaiting requested review from ellis-johnson

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

Assignees
No one assigned
Labels
next-release To be included in the next RP release rollout ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bennerv @mbarnes @hawkowl @cadenmarchese @jhoreman @s-amann