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.

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

OSPRH-5904: Integration of the OpenStack CRs into the insights-operator #951

Merged
merged 10 commits into from
Jul 30, 2024

Conversation

slawqo
Copy link
Contributor

@slawqo slawqo commented Jun 21, 2024

Control Plan of the Red Hat OpenStack starting from the version 18 (RHOSO 18) will be deployed using OpenShift operators as a set of OpenShift applications. Compute nodes will be deployed still as RHEL nodes and will be outside of the OpenShift cluster but all will be mangaged by the Dataplane-operator and only interface for OpenStack operators will be though the CRs:
- OpenStackControlPlane CR for the control plane,
- OpenStackDataplaneNodeSet and OpenStackDataplaneDeployment CRs for the
compute nodes.
This PR adds those OpenStack CRs to the insights-operator so that whole configuration of the OpenStack control plane may be gathered by the insights-operator and send to analysis to the Insights server.

Categories

  • Bugfix
  • Data Enhancement
  • Feature
  • Backporting
  • Others (CI, Infrastructure, Documentation)

Sample Archive

  • docs/insights-archive-sample/customresources/core.openstack.org/openstackcontrolplanes/openstack/openstack-galera-network-isolation.json
  • docs/insights-archive-sample/customresources/dataplane.openstack.org/openstackdataplanedeployments/openstack/edpm-deployment.json
  • docs/insights-archive-sample/customresources/dataplane.openstack.org/openstackdataplanenodesets/openstack/openstack-edpm.json

Unit Tests

  • pkg/gatherers/clusterconfig/gather_openstack_controlplanes_test.go
  • pkg/gatherers/clusterconfig/gather_openstack_dataplane_deployments_test.go
  • pkg/gatherers/clusterconfig/gather_openstack_dataplane_node_sets_test.go

Privacy

Anonymization functions are added and used while collecting OpenStack CRs. Anonymized data:

  • OpenstackControlplanes
    • Spec.dns.template.options
  • OpenstackDataplaneNodeSets
    • ansibleUser
    • edpm_sshd_allowed_ranges
    • dnsClusterAddresses
    • Status.allHostnames

Changelog

No

Breaking Changes

No

References

https://issues.redhat.com/browse/OSPRH-5904

Additional Info

To test what data are collected by the insights-operator with those changes, OpenStack has to be deployed on top of the OpenShift cluster. The easiest way to do this is to use CRC OpenShift and install_yamls tool. Please follow the guide to deploy OpenStack Control Plane and OpenStackDataPlane CRs.

@slawqo slawqo marked this pull request as draft June 21, 2024 09:13
@openshift-ci 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 Jun 21, 2024
@openshift-ci openshift-ci bot requested review from ncaak and tremes June 21, 2024 09:16
data.Object = anonymizeIpAddresses(data.Object)
data.Object = anonymizeFields(data.Object, fieldsToAnonymize)
data.Object = anonymizeCustomPathFields(data.Object, customFieldsToAnonymize)
data.Object = anonymizeStatusHostNames(data.Object)

Choose a reason for hiding this comment

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

each node object in the node set can have multiple hostnames assocrrated with it too.

there is a hostname subsection
https://github.com/openstack-k8s-operators/openstack-operator/blob/main/apis/core/v1beta1/openstackcontrolplane_types.go#L361-L370

the AnsibleOpts also contained sensitive info in general but that is a mapping filed with arbiary keys.

mouch of the value comes form processing those value but its harder to Anonymize in general.

at a minium we should be Anonymize ansibleHost

https://github.com/openstack-k8s-operators/openstack-operator/blob/main/apis/core/v1beta1/openstackcontrolplane_types.go#L361-L370

and any fixed ipes that exixits in the networks struct
https://github.com/openstack-k8s-operators/openstack-operator/blob/main/apis/core/v1beta1/openstackcontrolplane_types.go#L361-L370

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ansibleHost field added to be anonymized, fixed ips should be anonymized by the anonymizeIpAddresses() function. If not, please give me example of the CR where it is not anonymized, so I can fix it

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2024
@slawqo slawqo marked this pull request as ready for review July 10, 2024 09:51
@openshift-ci 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 Jul 10, 2024
@openshift-ci openshift-ci bot requested a review from tremes July 10, 2024 09:51
@slawqo
Copy link
Contributor Author

slawqo commented Jul 19, 2024

/retest

1 similar comment
@slawqo
Copy link
Contributor Author

slawqo commented Jul 22, 2024

/retest

// so that its string representation is empty, it is easier to just
// put 'xxx' in that place
if len(fieldValueStr) == 0 {
data[fieldName] = "xxx"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a blocker, but I don't understand this logic much. Why overwriting an empty string? It's not necessary, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it seems if fieldValue is e.g. list of items or is of type map[string]interface{} then in L59 mapping fieldValueStr will be empty string and in such case, I want to simply put 'xxx' instead of try to go deeper and look into that other nested type and iterate over it's fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK you're right https://play.golang.com/p/5RrmRPcrTnT. I thought it would panic, but it's not the case. Putting the xxx still seems a bit unnecessary but OK. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

It panics "only" when one return value is used for the "type casting", which make sense....and I didn't realize that.

fieldAnonymized = false
for _, fieldToAnonymize := range fieldsToAnonymize {
if fieldName == fieldToAnonymize {
fieldValueStr, _ := fieldValue.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar her. This can panic AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that in this case it will be ok, because if fieldValue will be of type like list or map[string]interface{} then fieldValueStr will be empty string and that will be handled in the if condition in L63

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Discussed in the thread below. Thanks.

@tremes
Copy link
Contributor

tremes commented Jul 26, 2024

I was able to create only openstackcontrolplanes and openstackversions CRs in my cluster. It works good! Thank you! I added few comments which would be good to address.

/approve

Copy link

openshift-ci bot commented Jul 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slawqo, tremes

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2024
@tremes tremes changed the title Integration of the OpenStack CRs into the insights-operator OSPRH-5904: Integration of the OpenStack CRs into the insights-operator Jul 26, 2024
For now it will drop only
'metadata/annotations/kubectl.kubernetes.io/last-applied-configuration'
but other fields can be added to that list too.
Control Plan of the Red Hat OpenStack starting from the version 18
(RHOSO 18) will be deployed using OpenShift operators as a set of
OpenShift applications.
Compute nodes will be deployed still as RHEL nodes and will be outside
of the OpenShift cluster but all will be mangaged by the
Dataplane-operator and only interface for OpenStack operators will be
though the CRs:

- OpenStackControlPlane CR for the control plane,
- OpenStackDataplaneNodeSet and OpenStackDataplaneDeployment CRs for the
  compute nodes.

This patch adds OpenStackDataplaneNodeSet CR to the insights-operator so that
whole configuration of the OpenStack control plane may be gathered by the
insights-operator and send to analysis to the Insights server.

Co-Authored-By: Yatin Karel <ykarel@redhat.com>

Related: #OSPRH-5904
Control Plan of the Red Hat OpenStack starting from the version 18
(RHOSO 18) will be deployed using OpenShift operators as a set of
OpenShift applications.
Compute nodes will be deployed still as RHEL nodes and will be outside
of the OpenShift cluster but all will be mangaged by the
Dataplane-operator and only interface for OpenStack operators will be
though the CRs:

- OpenStackControlPlane CR for the control plane,
- OpenStackDataplaneNodeSet and OpenStackDataplaneDeployment CRs for the
  compute nodes.

This patch adds OpenStackDataplaneDeployment CR to the insights-operator so that
whole configuration of the OpenStack control plane may be gathered by the
insights-operator and send to analysis to the Insights server.

Co-Authored-By: Yatin Karel <ykarel@redhat.com>

Related: #OSPRH-5904
Red Hat's openstack-operator in addition to the OpenStackControlPlane
and OpenStackDataPlane CRs uses also additonal CR 'openstackversions'
which reports version of the openstack(s) deployed in the OCP cluster.
This patch adds support for gathering also this CRs from the cluster if
they exists.

Related: #OSPRH-5904
@slawqo
Copy link
Contributor Author

slawqo commented Jul 29, 2024

Today I run locally insights-operator with those changes so if you want to see them, you can search for clusterID "f043c1f4-f148-4030-9f16-9d5c242d2c8c"

@tremes
Copy link
Contributor

tremes commented Jul 29, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2024
@tremes
Copy link
Contributor

tremes commented Jul 29, 2024

insights-operator-e2e-tests are failing, but this is a known failure.

@slawqo
Copy link
Contributor Author

slawqo commented Jul 29, 2024

/retest

@BaiyangZhou
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 30, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 30, 2024

@slawqo: This pull request references OSPRH-5904 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

Control Plan of the Red Hat OpenStack starting from the version 18 (RHOSO 18) will be deployed using OpenShift operators as a set of OpenShift applications. Compute nodes will be deployed still as RHEL nodes and will be outside of the OpenShift cluster but all will be mangaged by the Dataplane-operator and only interface for OpenStack operators will be though the CRs:

  • OpenStackControlPlane CR for the control plane,
  • OpenStackDataplaneNodeSet and OpenStackDataplaneDeployment CRs for the
    compute nodes.
    This PR adds those OpenStack CRs to the insights-operator so that whole configuration of the OpenStack control plane may be gathered by the insights-operator and send to analysis to the Insights server.

Categories

  • Bugfix
  • Data Enhancement
  • Feature
  • Backporting
  • Others (CI, Infrastructure, Documentation)

Sample Archive

  • docs/insights-archive-sample/customresources/core.openstack.org/openstackcontrolplanes/openstack/openstack-galera-network-isolation.json
  • docs/insights-archive-sample/customresources/dataplane.openstack.org/openstackdataplanedeployments/openstack/edpm-deployment.json
  • docs/insights-archive-sample/customresources/dataplane.openstack.org/openstackdataplanenodesets/openstack/openstack-edpm.json

Unit Tests

  • pkg/gatherers/clusterconfig/gather_openstack_controlplanes_test.go
  • pkg/gatherers/clusterconfig/gather_openstack_dataplane_deployments_test.go
  • pkg/gatherers/clusterconfig/gather_openstack_dataplane_node_sets_test.go

Privacy

Anonymization functions are added and used while collecting OpenStack CRs. Anonymized data:

  • OpenstackControlplanes
  • Spec.dns.template.options
  • OpenstackDataplaneNodeSets
  • ansibleUser
  • edpm_sshd_allowed_ranges
  • dnsClusterAddresses
  • Status.allHostnames

Changelog

No

Breaking Changes

No

References

https://issues.redhat.com/browse/OSPRH-5904

Additional Info

To test what data are collected by the insights-operator with those changes, OpenStack has to be deployed on top of the OpenShift cluster. The easiest way to do this is to use CRC OpenShift and install_yamls tool. Please follow the guide to deploy OpenStack Control Plane and OpenStackDataPlane CRs.

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.

@karelyatin
Copy link

/lgtm

@karelyatin
Copy link

/test insights-operator-e2e-tests

@tremes
Copy link
Contributor

tremes commented Jul 30, 2024

@karelyatin @slawqo the test failure is persistent. We need to override the test for now.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD b03bdbd and 2 for PR HEAD d0d5cc7 in total

@tremes
Copy link
Contributor

tremes commented Jul 30, 2024

The test failure is a known problem. We just need a new JIRA ticket to track it. cc @BaiyangZhou
/override ci/prow/insights-operator-e2e-tests

Copy link

openshift-ci bot commented Jul 30, 2024

@tremes: Overrode contexts on behalf of tremes: ci/prow/insights-operator-e2e-tests

In response to this:

The test failure is a known problem. We just need a new JIRA ticket to track it. cc @BaiyangZhou
/override ci/prow/insights-operator-e2e-tests

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 8024649 into openshift:master Jul 30, 2024
9 checks passed
Copy link

openshift-ci bot commented Jul 30, 2024

@slawqo: all tests passed!

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.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-insights-operator
This PR has been included in build ose-insights-operator-container-v4.18.0-202407301115.p0.g8024649.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-insights-operator
This PR has been included in build ose-insights-operator-container-v4.17.0-202407301116.p0.g8024649.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants