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

ARO-10633 | Add ARM Helper Indentity supporting properties #695

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ARO-10633 | Add ARM Helper Indentity supporting properties #695

wants to merge 1 commit into from

Conversation

cgiradkar
Copy link
Collaborator

What this PR does

Jira:
Link to demo recording:

Special notes for your reviewer

ahitacat
ahitacat reviewed Oct 9, 2024
View reviewed changes
cluster-service/Makefile Outdated Show resolved Hide resolved
@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from 9ae10ca to 8f001cb Compare October 9, 2024 12:59
ahitacat
ahitacat requested changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

I saw some things that can be changed

cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch 2 times, most recently from 2754c66 to 4b8728b Compare October 9, 2024 15:18
ahitacat
ahitacat requested changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

The arm cert aro-dev-arm-helper doesn't exist in the keyvault.

cluster-service/Makefile Outdated Show resolved Hide resolved
@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from 4b8728b to 51fe1c8 Compare October 10, 2024 08:04
@cgiradkar cgiradkar marked this pull request as ready for review October 10, 2024 08:04
@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from 51fe1c8 to c9eacba Compare October 10, 2024 10:22
ahitacat
ahitacat requested changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

In the deploy-integ of the Makefile target it is missing the ARM_HELPER_IDENTITY_CERT_NAME.

cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
cluster-service/Makefile Outdated Show resolved Hide resolved
cluster-service/Makefile Outdated Show resolved Hide resolved
cluster-service/deploy/openshift-templates/arohcp-service-template.yml Outdated Show resolved Hide resolved
@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from c9eacba to d414db5 Compare October 11, 2024 08:21
ahitacat
ahitacat requested changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

This looks good now 😄
Have you deployed it? It will require the changes in CS https://gitlab.cee.redhat.com/service/uhc-clusters-service/-/merge_requests/8715#c1d3677ee87cc3a195920969d2b562701064aed4

cluster-service/Makefile Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Please rebase pull request.

@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from d414db5 to 3f8669c Compare October 16, 2024 12:19
ahitacat
ahitacat previously approved these changes Oct 16, 2024
View reviewed changes
Copy link
Collaborator

@ahitacat ahitacat left a comment

Choose a reason for hiding this comment

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

LGTM!
Chetan and I tested that the arm-helper-cert was stored in /secrets/arm-indentity.

@cgiradkar
Copy link
Collaborator Author

After merging the CS MR (910676a), tested again with the latest CS tag and CS could be deployed on svc cluster with pods running as expected.
clusters-service-7f5d87bf86-gg2bx 2/2 Running 0 3m47s

@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from 3f8669c to d14c43f Compare October 17, 2024 11:50
@cgiradkar cgiradkar force-pushed the ARO-10633-ARM-Helper-Identity branch from d14c43f to e7d370d Compare October 17, 2024 11:56
SudoBrendan
SudoBrendan requested changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

Assuming this relates to https://issues.redhat.com/browse/ARO-10633 ? The work seems good. Can we replace hard-coded CLIENT_ID values with lookups?

cluster-service/Makefile
Comment on lines 10 to +11
AZURE_FIRST_PARTY_APPLICATION_CLIENT_ID ?= "57e54810-3138-4f38-bd3b-29cb33f4c358"
AZURE_ARM_HELPER_IDENTITY_CLIENT_ID ?= "2c6ca254-36bd-43c8-a7a8-fe880bc2c489"
Copy link
Collaborator

Choose a reason for hiding this comment

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

While not the end of the world, we should probably replace both of these hard-coded values with lookups instead similar to other places in our Makefiles, it seems the convention is:

SOME_CLIENT_ID ?= $(shell az identity show \
			-g ${RESOURCEGROUP} \
			-n SOME_CLIENT_NAME \
			--query clientId)

This will improve our security posture and make our automation resilient to turnover of these IDs in the event they are rotated for whatever reason.

@geoberle
Copy link
Collaborator

Assuming this relates to https://issues.redhat.com/browse/ARO-10633 ? The work seems good. Can we replace hard-coded CLIENT_ID values with lookups?

Variable lookup support will land soon for ARO HCP infra and service deployments.

@github-actions GitHub Actions
Copy link

Please rebase pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SudoBrendan SudoBrendan SudoBrendan requested changes

@ahitacat ahitacat ahitacat left review comments

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

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

@mjlshen mjlshen Awaiting requested review from mjlshen

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

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

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cgiradkar @geoberle @SudoBrendan @ahitacat