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-4518 pass custom manifests(MIWI) to hive cluster deployment as secret #3841

Merged
merged 2 commits into from
Sep 17, 2024
Merged

ARO-4518 pass custom manifests(MIWI) to hive cluster deployment as secret #3841

merged 2 commits into from
Sep 17, 2024

Conversation

rajdeepc2792
Copy link
Collaborator

@rajdeepc2792 rajdeepc2792 commented Sep 13, 2024 •
edited
Loading

Which issue this PR addresses:

Fixes (https://issues.redhat.com/browse/ARO-4518)

What this PR does / why we need it:

Call generatePlatformWorkloadIdentitySecrets() for generating the Platform Identity Secret for a MIWI cluster.

Pass all the Secrets as a hive cluster secret and pass it the name of hive secret to cluster deployment's Spec.Provisioning.ManifestsSecretRef

These secrets will be managed in ARO-Installer-Wrapper

Additional Handling of CredentialsSecretRef is required as it is only used for the Non-MIWI clusters, but since it is used to pass the cluster and subscription doc to wrapper it is still neded.

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

No

How do you know this will function as expected in production?

@rajdeepc2792 rajdeepc2792 added work-in-progress chainsaw Pull requests or issues owned by Team Chainsaw labels Sep 13, 2024
@rajdeepc2792 rajdeepc2792 changed the title ARO-4518 pass custom manifests to hive cluster deployment as secret ARO-4518 pass custom manifests(MIWI) to hive cluster deployment as secret Sep 13, 2024
@rajdeepc2792 rajdeepc2792 mentioned this pull request Sep 13, 2024
Open
1 task
cadenmarchese
cadenmarchese reviewed Sep 13, 2024
View reviewed changes
Copy link
Collaborator

@cadenmarchese cadenmarchese left a comment

Choose a reason for hiding this comment

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

just a quick pass on the draft PR. thank you!

pkg/hive/resources.go
return secret, nil
}

func clusterManifestsSecret(namespace string, customManifests map[string]kruntime.Object) (*corev1.Secret, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit - if this makes sense to you, I suggest a rename of this function to avoid confusion with the clusterManifestsSecret var:

Suggested change
func clusterManifestsSecret(namespace string, customManifests map[string]kruntime.Object) (*corev1.Secret, error) {
func generateClusterManifestsSecret(namespace string, customManifests map[string]kruntime.Object) (*corev1.Secret, error) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of changing the function name, changed the var name:- https://github.com/Azure/ARO-RP/pull/3841/files#diff-f83dd6c00cfc7b417ce96e6f7bfbe58ce118101ea90630f21d658abf3e96b129R60

generateClusterManifestsSecret name is similar to https://github.com/Azure/ARO-RP/pull/3841/files#diff-71a1dd610bf07a9e4ece9f782af2bad11e6f2d0e03df442019088f8c47265497R281 where the actual generation is taking place and keeping it similar to similar type of secret creations in the file.

pkg/hive/install.go Show resolved Hide resolved
pkg/hive/install.go Outdated Show resolved Hide resolved
@rajdeepc2792 rajdeepc2792 force-pushed the rajdeepc2792/ARO-4518-pass-the-miwi-secret-manifests-to-hive branch from d2406a2 to 43bbf18 Compare September 13, 2024 19:26
@rajdeepc2792 rajdeepc2792 force-pushed the rajdeepc2792/ARO-4518-pass-the-miwi-secret-manifests-to-hive branch from be2d1f6 to 6b916b5 Compare September 16, 2024 16:52
@rajdeepc2792 rajdeepc2792 marked this pull request as ready for review September 16, 2024 16:59
@rajdeepc2792
Copy link
Collaborator Author

rajdeepc2792 commented Sep 16, 2024 •
edited
Loading

/azp run ci,e2e

@azure-pipelines Azure Pipelines
Copy link

No pipelines are associated with this pull request.

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

bitoku
bitoku approved these changes Sep 17, 2024
View reviewed changes
pkg/hive/resources_test.go
r, _ := clusterAzureSecret("testNamespace", tt.oc, &subDoc)

_, ok := r.Data["osServicePrincipal.json"]
if tt.wantSP != ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: You can use assert pkg.

ARO-RP/pkg/cluster/networkprofile_test.go

Lines 294 to 295 in 5370de9

assert.Equal(t, tt.expectedErr, err, "Unexpected error exception")
assert.Equal(t, tt.expectedMTU, tt.m.doc.OpenShiftCluster.Properties.NetworkProfile.MTUSize, "MTUSize was not updated as expected exception")

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I'm personally in favor of using the assert pkg here, I don't think we have a consistent pattern for these assertions across our codebase's test cases, so I'm fine either way with changing this or letting it merge as-is.

tsatam
tsatam approved these changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

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

LGTM - excited to see this working!

@tsatam tsatam merged commit 1e3f475 into master Sep 17, 2024
24 checks passed
edisonLcardenas pushed a commit that referenced this pull request Sep 18, 2024
@rajdeepc2792 @edisonLcardenas
ARO-4518 pass custom manifests(MIWI) to hive cluster deployment as se…
58af156 
…cret (#3841)

* ARO-4518 pass custom manifests to hive cluster deployment as secret

* ARO-4518 add unit test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cadenmarchese cadenmarchese cadenmarchese left review comments

@bitoku bitoku bitoku approved these changes

@tsatam tsatam tsatam approved these changes

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

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

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl 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

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

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

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

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

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

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

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

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

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

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

Assignees

@rajdeepc2792 rajdeepc2792

@tsatam tsatam

Labels
chainsaw Pull requests or issues owned by Team Chainsaw ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rajdeepc2792 @bitoku @tsatam @cadenmarchese @kimorris27