[OCPCLOUD-1436] Introduce workflow with IMDSv2 enforced for AWS machines during installation by lobziik · Pull Request #28236 · openshift/release

lobziik · 2022-04-29T15:46:32Z

This pr introduces separate workflow which installs openshift with
enforced auth requirement on metadata service for control-plane and
compute machines. Adds a respective job for installer repo.

Enhancements document with more details:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-api/aws-imds-v2-support.md

lobziik · 2022-05-02T12:36:41Z

Rebased due to conflicts.

Previous rehearse job finished successfully. Think it should be ready to go.
https://prow.ci.openshift.org/job-history/gs/origin-ci-test/pr-logs/directory/rehearse-28236-pull-ci-openshift-installer-master-e2e-aws-metadata-service-auth-required

/cc @JoelSpeed

lobziik · 2022-05-02T12:45:13Z

The only question which i have is about owners... Who might be there?

patrickdillon

This LGTM, but I would prefer the job name be shorter. Typically for IPI these follow a pattern of:
e2e-- so in this case I suggested e2e-aws-imdsv2 although there might be a better name

lobziik · 2022-05-02T14:48:30Z

I did shorten the name of the installer job. Updated OWNERS, hope it is fine at this point :)

patrickdillon · 2022-05-02T18:53:32Z

/approve

JoelSpeed · 2022-05-03T10:16:25Z

I don't have an objection to adding this right now, but I would like to know if there are any future plans to remove it?

I would expect that once we have proved this is stable, we could turn this on by default for our CI. This would then catch any other components that aren't compliant before they enter payload (because this test doesn't run everywhere).

We may not make this the default in the installer for some time, but that doesn't stop it being the default for our testing IMO

patrickdillon · 2022-05-03T14:37:47Z

I would expect that once we have proved this is stable, we could turn this on by default for our CI. This would then catch any other components that aren't compliant before they enter payload (because this test doesn't run everywhere).

Another way of accomplishing this in the short term could be to create a periodic. That would test the release payload on a regular basis, rather than just indirectly through installer presubmits.

We may not make this the default in the installer for some time, but that doesn't stop it being the default for our testing IMO

This makes sense. Perhaps create a card to check in after a month or x amount of data? If this is going to happen regularly perhaps it is worth defining a process.

JoelSpeed · 2022-05-04T09:34:03Z

That would test the release payload on a regular basis, rather than just indirectly through installer presubmits.

That's a good idea, @lobziik what do you think to adding a release informing job?

Perhaps create a card to check in after a month or x amount of data?

Will do

lobziik · 2022-05-04T11:26:47Z

That's a good idea, @lobziik what do you think to adding a release informing job?

Good idea. ae44ac7 - i used TP periodic jobs as reference, not sure that that is all what needed for setup a periodic. PTAL

JoelSpeed · 2022-05-04T15:22:31Z

Dunno why, but all steps all around there have 4.5. Just made it the same, since i don't understand the reasons why it's 4.5 all around and for keep stuff consistent.

Ok, good enough reasoning for me

The proxy setup step pinned to 4.5 back in #11453 because it was writing Ignition config. If you don't have a reason to pin a specific version for your step, I'd strongly recommend not pinning.

JoelSpeed · 2022-05-04T15:23:02Z

Why disabled?

Who knows. Purposes of this file and directory are a bit cryptic for me. This entry was generated by make release-controllers.

All entries here have "disabled": true

lobziik · 2022-05-05T12:19:01Z

/retest

JoelSpeed · 2022-05-05T12:57:54Z

/lgtm

lobziik · 2022-05-09T15:06:14Z

/retest

lobziik · 2022-05-10T14:45:44Z

@deads2k @smarterclayton @wking Hello! Can you please take a look? :) This pr touches quite a lot around, due to new workflow and informing jobs addition...

wking · 2022-05-10T17:30:38Z

I'm not TRT, so not really my business, but... are they really interested in being approvers for this specific step?

wking · 2022-05-10T17:31:20Z

nit: inconsistent indentation for these two lines.

wking · 2022-05-10T17:36:21Z

Neither the cluster profile nor input files come into this step, do they? It's just AWS_METADATA_SERVICE_AUTH?

wking · 2022-05-10T17:40:28Z

The existing ipi-aws-pre chain already contains a bunch of environment knobs. It's unclear to me why you want a new workflow for this knob, vs. adding the knob to the existing chain and then setting the env you want in your job configuration. Using a workload to centralize config between multiple jobs makes sense for flavors that have lots of consuming jobs, but you only have the two consuming jobs...

Hm, I can not clearly explain the reasoning, why i did exactly that there. Thought that it should be the right thing. :D Maybe i tried to minimize rehearse load and do not touch other jobs, I don't remember exactly.

Would it be better to extend ipi-conf-aws instead and expose extra knob there?

Touching the main workload will indeed create a bunch of rehearsals on this one release PR. But how often do we expect to have to touch this logic in the release repo? I expect it will be rare, and the maintenance overhead of having devs keeping track of more decoupled pieces isn't free either 😉

I'm not all that concerned either way though, so 🤷 whatever makes the most sense to the folks who will be maintaining this code.

https://github.com/lobziik/release/pull/1 - i prepared patch which adds env knob to ipi-conf-aws step. Would like to get a review before i will update this pr... Still a bit afraid of massive rehearsals. Can you please check if it looks sane? :)

lobziik · 2022-05-23T12:57:26Z

I extended ipi-conf-aws step with new AWS_METADATA_SERVICE_AUTH env knob, this need review again. Now it should be possible to enable auth on metadata for each job which uses ipi-aws-pre chain.

JoelSpeed

/lgtm

lobziik · 2022-05-31T10:10:18Z

@wking can you please take a look? I really want to merge it, rebased it for 4th time already...

lobziik · 2022-06-01T10:36:02Z

/retest

vrutkovs · 2022-06-06T11:57:44Z

/approve

Rehearsals didn't pass just yet, but looks good so far

Introduces AWS_METADATA_SERVICE_AUTH env variable to enable IMDSv2 on AWS during installation time. Adds job which uses that to the installer presubmits.

lobziik · 2022-06-06T16:05:43Z

I excluded infroming jobs from this pr since it violates the process described https://docs.ci.openshift.org/docs/architecture/release-gating/#add-a-periodic-informative-job here.
I will add it in a separate follow up pr after this one will be merged.

cc @patrickdillon @JoelSpeed

JoelSpeed

/lgtm

openshift-ci · 2022-06-07T09:42:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, lobziik, patrickdillon, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~ci-operator/config/openshift/installer/OWNERS~~ [patrickdillon]
~~ci-operator/jobs/openshift/installer/OWNERS~~ [patrickdillon]
~~ci-operator/step-registry/ipi/OWNERS~~ [vrutkovs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-06-07T09:52:08Z

@lobziik: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/rehearse/periodic-ci-openshift-release-master-ci-4.12-e2e-aws-network-stress	1f095d392488a512979b459eaae023dd7d0ed44c	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/cluster-etcd-operator/master/e2e-aws-disruptive-ovn	`a834834`	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-openshift-tests-private-release-4.11-nightly-e2e-aws-ipi-byo-route53	`a834834`	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-openshift-tests-private-release-4.11-nightly-e2e-aws-arm64-ipi	`a834834`	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/cluster-image-registry-operator/master/e2e-aws-image-registry	`a834834`	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/cluster-cloud-controller-manager-operator/master/e2e-aws-ccm	52a56fec43f6de244f089850881687e40282aa4f	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/metallb-operator/main/operator-e2e-upgrade	52a56fec43f6de244f089850881687e40282aa4f	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/cluster-cloud-controller-manager-operator/master/e2e-aws-ccm-install	52a56fec43f6de244f089850881687e40282aa4f	link	unknown	`/test pj-rehearse`
ci/rehearse/redhat-openshift-ecosystem/community-operators-pipeline/main/4.9-deploy-operator-on-openshift	52a56fec43f6de244f089850881687e40282aa4f	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-multiarch-master-nightly-4.11-ocp-e2e-aws-upi-arm64	52a56fec43f6de244f089850881687e40282aa4f	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/csi-external-snapshotter/release-4.5/e2e-aws-csi	153cab6ade9cbb6e2cdb0c04dc5e8dbbb42dd4ce	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/installer/master/e2e-aws-upi-proxy	`124097c`	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/cluster-logging-operator/release-4.8/e2e-operator	b85a5a1344533d147fd771dfb7eb82b9473cbc8a	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-openshift-tests-private-release-4.11-amd64-nightly-e2e-aws-ipi-byo-route53	`124097c`	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-openshift-tests-private-release-4.11-amd64-nightly-e2e-aws-ipi	`124097c`	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/windows-machine-config-operator/master/aws-e2e-upgrade	`124097c`	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-release-master-ci-4.12-e2e-aws-imdsv2	153cab6ade9cbb6e2cdb0c04dc5e8dbbb42dd4ce	link	unknown	`/test pj-rehearse`
ci/rehearse/periodic-ci-openshift-hypershift-main-periodics-e2e-conformance-proxy	`124097c`	link	unknown	`/test pj-rehearse`
ci/prow/pj-rehearse	`124097c`	link	false	`/test pj-rehearse`
ci/rehearse/openshift/openshift-tests-private/master/debug-disasterrecovery-aws-ipi	153cab6ade9cbb6e2cdb0c04dc5e8dbbb42dd4ce	link	unknown	`/test pj-rehearse`
ci/rehearse/openshift/installer/master/e2e-aws-imdsv2	`124097c`	link	unknown	`/test pj-rehearse`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci · 2022-06-07T09:53:34Z

@lobziik: Updated the following 3 configmaps:

ci-operator-master-configs configmap in namespace ci at cluster app.ci using the following files:
- key openshift-installer-master.yaml using file ci-operator/config/openshift/installer/openshift-installer-master.yaml
job-config-master-presubmits configmap in namespace ci at cluster app.ci using the following files:
- key openshift-installer-master-presubmits.yaml using file ci-operator/jobs/openshift/installer/openshift-installer-master-presubmits.yaml
step-registry configmap in namespace ci at cluster app.ci using the following files:
- key ipi-conf-aws-commands.sh using file ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-commands.sh
- key ipi-conf-aws-ref.yaml using file ci-operator/step-registry/ipi/conf/aws/ipi-conf-aws-ref.yaml

Details

In response to this:

This pr introduces separate workflow which installs openshift with
enforced auth requirement on metadata service for control-plane and
compute machines. Adds a respective job for installer repo.

Enhancements document with more details:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-api/aws-imds-v2-support.md

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2022

openshift-ci Bot requested review from jhixson74 and patrickdillon April 29, 2022 15:47

lobziik changed the title ~~[WIP] [OCPCLOUD-1436] Introduce workflow with IMDSv2 enforced for AWS machines during installation~~ [OCPCLOUD-1436] Introduce workflow with IMDSv2 enforced for AWS machines during installation May 2, 2022

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 2, 2022

openshift-ci Bot requested a review from JoelSpeed May 2, 2022 12:36

patrickdillon reviewed May 2, 2022

View reviewed changes

Comment thread ci-operator/config/openshift/installer/openshift-installer-master.yaml Outdated

JoelSpeed reviewed May 4, 2022

View reviewed changes

openshift-ci Bot assigned JoelSpeed May 5, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2022

wking reviewed May 10, 2022

View reviewed changes

openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 18, 2022

lobziik requested review from JoelSpeed, patrickdillon and wking May 23, 2022 12:57

JoelSpeed reviewed May 23, 2022

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2022

openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 31, 2022

Introduce imdsv2 enablement knob in ipi-conf-aws step

124097c

Introduces AWS_METADATA_SERVICE_AUTH env variable to enable IMDSv2 on AWS during installation time. Adds job which uses that to the installer presubmits.

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2022

JoelSpeed reviewed Jun 7, 2022

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2022

openshift-merge-robot merged commit ca47eba into openshift:master Jun 7, 2022

lobziik mentioned this pull request Jun 13, 2022

Add periodic jobs for aws clusters with enabled IMDSv2 #29414

Merged

Conversation

lobziik commented Apr 29, 2022

Uh oh!

lobziik commented May 2, 2022

Uh oh!

lobziik commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lobziik commented May 2, 2022

Uh oh!

patrickdillon commented May 2, 2022

Uh oh!

JoelSpeed commented May 3, 2022

Uh oh!

patrickdillon commented May 3, 2022

Uh oh!

JoelSpeed commented May 4, 2022

Uh oh!

lobziik commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobziik May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobziik May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobziik commented May 5, 2022

Uh oh!

JoelSpeed commented May 5, 2022

Uh oh!

lobziik commented May 9, 2022

Uh oh!

lobziik commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobziik May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobziik May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobziik commented May 23, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

lobziik commented May 31, 2022

Uh oh!

lobziik commented Jun 1, 2022

Uh oh!

vrutkovs commented Jun 6, 2022

lobziik commented May 2, 2022 •

edited

Loading

lobziik commented May 4, 2022 •

edited

Loading

lobziik May 5, 2022 •

edited

Loading

lobziik May 5, 2022 •

edited

Loading

lobziik commented May 10, 2022 •

edited

Loading

lobziik May 12, 2022 •

edited

Loading

lobziik May 16, 2022 •

edited

Loading