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

Bug 1978798: Update ovnKubernetesConfig.policyAuditConfig Default #993

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

astoycos
Copy link
Contributor

We need to initialize ovnKubernetesConfig.policyAuditConfig
so that the proper defaults are populated when a CNO config
object is created

Signed-off-by: astoycos [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 20, 2021

@astoycos: This pull request references Bugzilla bug 1978798, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @asood-rh

In response to this:

Bug 1978798: Update ovnKubernetesConfig.policyAuditConfig Default

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 openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 20, 2021
@astoycos
Copy link
Contributor Author

/assign @sttts

@@ -239,6 +239,7 @@ spec:
policyAuditConfig:
description: policyAuditConfig is the configuration for network policy audit events. If unset, reported defaults are used.
type: object
default: null
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks wrong. What is kubebuilder doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure Maybe I need to specify the object here? Let me give that a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two ways to solve this:

  1. fix kubebuilder's controller-gen upstream
  2. use a yaml patch as we do in similar cases where kubebuilder is broken. Look in this repo for yaml-patch files as blueprint. A yaml patch allows you to set {} here explicitly as value. It is applied after controller-gen has been run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 sounds good I'll give a stab at debugging kubebuilder first, thanks!

@openshift-merge-robot
Copy link
Contributor

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

@openshift-ci openshift-ci bot removed the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 6, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 6, 2021

@openshift-merge-robot: This pull request references Bugzilla bug 1978798, which is invalid:

  • expected the bug to target the "4.10.0" release, but it targets "---" instead

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

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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 openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 6, 2021
@astoycos astoycos force-pushed the fix-audit-logging branch 2 times, most recently from 889a5f7 to 78c2313 Compare September 21, 2021 22:03
@astoycos
Copy link
Contributor Author

@sttts I bashed my head against kubebuilder for a while trying to fix this, however based on the design of controller-tools it doesn't seem like a very trivial thing to fix. I opened kubernetes-sigs/controller-tools#622 and asked for help in upstream/ and downstream slack channels to no avail. :(

For now I'd like to get this bug fixed with the following patch and then fix properly upstream once I can get more help/time.

@sttts
Copy link
Contributor

sttts commented Sep 22, 2021

/lgtm
/approve

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 22, 2021
Rename the Cluster Network Operator's
CRD so that a post schema generation
patches can be used on the generated
CRD Shema

Signed-off-by: astoycos <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
operator/v1/types_network.go Outdated Show resolved Hide resolved
operator/v1/types_network.go Outdated Show resolved Hide resolved
@astoycos
Copy link
Contributor Author

astoycos commented Sep 22, 2021

Sounds good I'll fix these... Also when I changed the naming to .crd.yaml from _crd.yaml we also seemed to have picked up some additional formatting that provides a maximum line length which wasn't done on the _crd.yaml this results in reformatting of the entire file so I'll add another commit for that too

@sttts
Copy link
Contributor

sttts commented Sep 22, 2021

The patch tool uses another library to format the yaml. So that's expected, ugly, but expected.

We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <[email protected]>
@astoycos
Copy link
Contributor Author

ACK I though I did something wrong :)

Removed the comments, should be good to go

@astoycos
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 22, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

@astoycos: This pull request references Bugzilla bug 1978798, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @asood-rh

In response to this:

/bugzilla 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 kubernetes/test-infra repository.

@openshift-ci openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 22, 2021
@sttts
Copy link
Contributor

sttts commented Sep 22, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit ba55515 into openshift:master Sep 22, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

@astoycos: All pull requests linked via external trackers have merged:

Bugzilla bug 1978798 has been moved to the MODIFIED state.

In response to this:

Bug 1978798: Update ovnKubernetesConfig.policyAuditConfig Default

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
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, sttts

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

@astoycos
Copy link
Contributor Author

/cherry-pick release-4.9

@astoycos
Copy link
Contributor Author

/cherry-pick release-4.8

@openshift-cherrypick-robot

@astoycos: new pull request created: #1014

In response to this:

/cherry-pick release-4.9

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-cherrypick-robot

@astoycos: #993 failed to apply on top of branch "release-4.8":

Applying: Rename the CNO CRD
Applying: Update ovnKubernetesConfig.policyAuditConfig Default
Using index info to reconstruct a base tree...
M	operator/v1/0000_70_cluster-network-operator_01.crd.yaml
Falling back to patching base and 3-way merge...
Auto-merging operator/v1/0000_70_cluster-network-operator_01.crd.yaml
CONFLICT (content): Merge conflict in operator/v1/0000_70_cluster-network-operator_01.crd.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Update ovnKubernetesConfig.policyAuditConfig Default
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.8

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.

@astoycos
Copy link
Contributor Author

/cherry-pick release-4.9

@openshift-cherrypick-robot

@astoycos: new pull request could not be created: failed to create pull request against openshift/api#release-4.9 from head openshift-cherrypick-robot:cherry-pick-993-to-release-4.9: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for openshift-cherrypick-robot:cherry-pick-993-to-release-4.9."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

In response to this:

/cherry-pick release-4.9

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.

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. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants