[4.21] OCPBUGS-59514: redacted install config for baremetal #9972

iurygregory · 2025-09-29T01:43:40Z

No description provided.

openshift-ci-robot · 2025-09-29T01:43:46Z

@iurygregory: This pull request references Jira Issue OCPBUGS-59514, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

iurygregory · 2025-09-29T01:48:29Z

/jira refresh

openshift-ci-robot · 2025-09-29T01:48:38Z

@iurygregory: This pull request references Jira Issue OCPBUGS-59514, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jadhaj

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-09-29T01:50:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rwsu for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

patrickdillon

/label backport-risk-assessed

Hm perhaps I'm missing something obvious, but could you simply do this:

	case newConfig.BareMetal != nil:
		for _, host := range newConfig.BareMetal.Hosts {
			host.BMC.Password = "REDACTED"
		}

This way you would not need to update this code for future fields. Not sure why other platforms don't do this, so perhaps I am missing something!

iurygregory · 2025-09-29T16:19:25Z

@patrickdillon thanks for the feedback, let me push a change with your idea and test via cluster-bot!

zaneb · 2025-09-29T23:08:24Z

It looks like another instance of this problem was already fixed by OCPBUGS-61353, but there is still one more that this doesn't solve.

Clearly this is going to keep happening though. I don't feel like this is fixed until we have code that uses reflect to go through the whole structure and redact any fields with a json tag that contains user or password.

This way you would not need to update this code for future fields. Not sure why other platforms don't do this, so perhaps I am missing something!

I think the problem is that this changes the original install-config, meaning the redacted version will get written to the asset store. The InstallConfig struct is passed by value, so it is safe to overwrite the pullSecret at the top level, but it contains references to nested structs (Platform struct is embedded by value, but it contains pointers to the actual platform implementation), and only the references get copied.

We need to either go through (a hopefully better version of) the same rigmarole as the other platforms or generate DeepCopy() functions for the InstallConfig. The latter would be vastly preferable, since the current code is extremely brittle if not already broken (e.g. Port field is missing from VCenters).

barbacbd

This seems ok. Is there any way you can add some commit notes maybe why this is needed? I also like Patrick's comment to add Redacted instead of an empty string; I think it gives an indication for why it was set the way that it was.

patrickdillon · 2025-09-30T18:02:24Z

Platform struct is embedded by value, but it contains pointers to the actual platform implementation

+1. I knew I was missing something and this point was exactly it. Ignore my suggestion!

patrickdillon · 2025-09-30T18:03:46Z

& apologies for misleading you @iurygregory

Assisted-By: Claude Code Sonnet 4 Signed-off-by: Iury Gregory Melo Ferreira <[email protected]>

openshift-ci · 2025-10-24T20:24:18Z

@iurygregory: 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/prow/e2e-vsphere-ovn-techpreview	`18cc555`	link	false	`/test e2e-vsphere-ovn-techpreview`
ci/prow/e2e-aws-ovn-fips	`18cc555`	link	false	`/test e2e-aws-ovn-fips`
ci/prow/aws-private	`18cc555`	link	false	`/test aws-private`
ci/prow/e2e-aws-default-config	`18cc555`	link	false	`/test e2e-aws-default-config`
ci/prow/e2e-aws-ovn-shared-vpc-custom-security-groups	`18cc555`	link	false	`/test e2e-aws-ovn-shared-vpc-custom-security-groups`
ci/prow/e2e-aws-ovn-heterogeneous	`18cc555`	link	false	`/test e2e-aws-ovn-heterogeneous`
ci/prow/e2e-ibmcloud-ovn	`18cc555`	link	false	`/test e2e-ibmcloud-ovn`
ci/prow/e2e-openstack-ovn	`18cc555`	link	true	`/test e2e-openstack-ovn`
ci/prow/e2e-vsphere-ovn	`18cc555`	link	true	`/test e2e-vsphere-ovn`
ci/prow/azure-ovn-marketplace-images	`18cc555`	link	false	`/test azure-ovn-marketplace-images`
ci/prow/e2e-azure-ovn	`18cc555`	link	true	`/test e2e-azure-ovn`
ci/prow/e2e-aws-ovn-shared-vpc-edge-zones	`18cc555`	link	false	`/test e2e-aws-ovn-shared-vpc-edge-zones`
ci/prow/e2e-vsphere-ovn-disk-setup-techpreview	`18cc555`	link	false	`/test e2e-vsphere-ovn-disk-setup-techpreview`
ci/prow/azure-private	`18cc555`	link	false	`/test azure-private`
ci/prow/e2e-metal-single-node-live-iso	`18cc555`	link	false	`/test e2e-metal-single-node-live-iso`
ci/prow/e2e-vsphere-multi-vcenter-ovn	`18cc555`	link	false	`/test e2e-vsphere-multi-vcenter-ovn`
ci/prow/e2e-azurestack	`18cc555`	link	false	`/test e2e-azurestack`
ci/prow/e2e-aws-byo-subnet-role-security-groups	`18cc555`	link	false	`/test e2e-aws-byo-subnet-role-security-groups`
ci/prow/e2e-azure-ovn-shared-vpc	`18cc555`	link	false	`/test e2e-azure-ovn-shared-vpc`
ci/prow/e2e-aws-ovn-single-node	`18cc555`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-aws-ovn-edge-zones-manifest-validation	`18cc555`	link	true	`/test e2e-aws-ovn-edge-zones-manifest-validation`
ci/prow/e2e-aws-ovn-edge-zones	`18cc555`	link	false	`/test e2e-aws-ovn-edge-zones`
ci/prow/okd-scos-e2e-aws-ovn	`5e4737a`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-vsphere-ovn-hybrid-env	`18cc555`	link	false	`/test e2e-vsphere-ovn-hybrid-env`
ci/prow/openstack-manifests	`18cc555`	link	true	`/test openstack-manifests`
ci/prow/e2e-openstack-proxy	`18cc555`	link	false	`/test e2e-openstack-proxy`
ci/prow/e2e-vsphere-ovn-zones	`18cc555`	link	false	`/test e2e-vsphere-ovn-zones`
ci/prow/e2e-azure-default-config	`18cc555`	link	false	`/test e2e-azure-default-config`
ci/prow/e2e-openstack-nfv-intel	`18cc555`	link	false	`/test e2e-openstack-nfv-intel`
ci/prow/e2e-aws-ovn-imdsv2	`18cc555`	link	false	`/test e2e-aws-ovn-imdsv2`
ci/prow/e2e-nutanix-ovn	`18cc555`	link	false	`/test e2e-nutanix-ovn`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

tthvo · 2025-11-05T01:40:36Z

We need to either go through (a hopefully better version of) the same rigmarole as the other platforms or generate DeepCopy() functions for the InstallConfig. The latter would be vastly preferable, since the current code is extremely brittle if not already broken (e.g. Port field is missing from VCenters).

Looks like you predicted #10025, Zane. This PR should make the whole copying much easier and individual overrides can be done just like Patrick's previous suggestion.

tthvo · 2025-11-05T01:48:12Z

Clearly this is going to keep happening though. I don't feel like this is fixed until we have code that uses reflect to go through the whole structure and redact any fields with a json tag that contains user or password.

Looks like a good scenarios to use Claude 😁

Current approach looks OK to me though resolve the issue today unless you want to wait for #10025, @iurygregory

iurygregory · 2025-11-06T14:00:02Z

Clearly this is going to keep happening though. I don't feel like this is fixed until we have code that uses reflect to go through the whole structure and redact any fields with a json tag that contains user or password.

Looks like a good scenarios to use Claude 😁

Current approach looks OK to me though resolve the issue today unless you want to wait for #10025, @iurygregory

Hey @tthvo , I'll be waiting for #10025 to land so I can update things!

patrickdillon · 2025-11-12T19:42:13Z

I'm not sure how much #10025 will help. I mean that I honestly don't know.

I did have a thought though that JSON serialization would be a just fine way to copy one go struct to another, right?

edit: Ok clearly #10025 will help, and should merge soon.

iurygregory · 2025-11-14T11:42:32Z

I'm not sure how much #10025 will help. I mean that I honestly don't know.

I did have a thought though that JSON serialization would be a just fine way to copy one go struct to another, right?

edit: Ok clearly #10025 will help, and should merge soon.

Thanks @patrickdillon I will wait for #10025 to merge so I can update this one.

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 29, 2025

openshift-ci bot requested review from barbacbd, bfournie and jadhaj September 29, 2025 01:48

iurygregory force-pushed the OCPBUGS-59514 branch from 2d85a68 to 2f70ace Compare September 29, 2025 01:53

patrickdillon reviewed Sep 29, 2025

View reviewed changes

openshift-ci bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Sep 29, 2025

iurygregory force-pushed the OCPBUGS-59514 branch from 2f70ace to 749c837 Compare September 29, 2025 16:34

barbacbd reviewed Sep 30, 2025

View reviewed changes

iurygregory force-pushed the OCPBUGS-59514 branch 2 times, most recently from 18cc555 to 1cb7810 Compare October 24, 2025 14:50

[4.21] OCPBUGS-59514: redacted install config

5e4737a

Assisted-By: Claude Code Sonnet 4 Signed-off-by: Iury Gregory Melo Ferreira <[email protected]>

iurygregory force-pushed the OCPBUGS-59514 branch from 1cb7810 to 5e4737a Compare October 24, 2025 16:09

[4.21] OCPBUGS-59514: redacted install config for baremetal #9972

Are you sure you want to change the base?

[4.21] OCPBUGS-59514: redacted install config for baremetal #9972

Conversation

iurygregory commented Sep 29, 2025

Uh oh!

openshift-ci-robot commented Sep 29, 2025

Uh oh!

iurygregory commented Sep 29, 2025

Uh oh!

openshift-ci-robot commented Sep 29, 2025

Uh oh!

openshift-ci bot commented Sep 29, 2025

Uh oh!

patrickdillon left a comment

Choose a reason for hiding this comment

Uh oh!

iurygregory commented Sep 29, 2025

Uh oh!

zaneb commented Sep 29, 2025

Uh oh!

barbacbd left a comment • 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!

patrickdillon commented Sep 30, 2025

Uh oh!

patrickdillon commented Sep 30, 2025

Uh oh!

openshift-ci bot commented Oct 24, 2025

Uh oh!

tthvo commented Nov 5, 2025

Uh oh!

tthvo commented Nov 5, 2025

Uh oh!

iurygregory commented Nov 6, 2025

Uh oh!

patrickdillon commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iurygregory commented Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

barbacbd left a comment •

edited

Loading

patrickdillon commented Nov 12, 2025 •

edited

Loading