Skip to content

Fix small CRD issue with toGroups#12440

Merged
aanm merged 1 commit intocilium:masterfrom
DataDog:lbernail/togroups-crd
Jul 9, 2020
Merged

Fix small CRD issue with toGroups#12440
aanm merged 1 commit intocilium:masterfrom
DataDog:lbernail/togroups-crd

Conversation

@lbernail
Copy link
Copy Markdown
Contributor

@lbernail lbernail commented Jul 7, 2020 •
edited
Loading

Fixes: f0049da ("pkg/k8s: fix all structural issues with CNP validation")

In 1.8.1, the CNP CRD uses the following for egress toGroups rules:

			"toGroups": {
				Type: "object",
				Description: `ToGroups is a list of constraints that will
				gather data from third-party providers and create a new
				derived policy.`,
				Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{
					"aws": AWSGroup,
				},
			}

However the rule expects toGroups to be an array. From the doc example:

  egress:
  - toGroups:
    - aws:
        securityGroupsIds:
        - 'sg-0f2146100a88d03c3'

This commit modifies the CRD to be consistent with the rule spec to avoid validation errors from Kubernetes.

Fix toGroups CRD to address validation errors

@lbernail lbernail requested a review from a team as a code owner July 7, 2020 07:35
@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 7, 2020
aanm
aanm approved these changes Jul 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

LGTM, can you add a Fixes: f0049da61f4f ("pkg/k8s: fix all structural issues with CNP validation") to your commit?

@aanm aanm added needs-backport/1.7 release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jul 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 7, 2020
@aanm aanm added needs-backport/1.6 kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. labels Jul 7, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 7, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.02%) to 36.918% when pulling d23446c on DataDog:lbernail/togroups-crd into 7afc2a9 on cilium:master.

@lbernail
Fix small CRD issue with toGroups
d23446c 
Fixes: f0049da ("pkg/k8s: fix all structural issues with CNP validation")

Signed-off-by: Laurent Bernaille <laurent.bernaille@datadoghq.com>
@lbernail lbernail force-pushed the lbernail/togroups-crd branch from f9b33fd to d23446c Compare July 7, 2020 08:57
@aanm
Copy link
Copy Markdown
Member

aanm commented Jul 8, 2020

test-me-please

@aanm
Copy link
Copy Markdown
Member

aanm commented Jul 9, 2020

test flake was fixed in upstream merging

@aanm aanm merged commit 678e7f0 into cilium:master Jul 9, 2020
@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jul 9, 2020

@lbernail @aanm are there any upgrade implications to this change?

@joestringer joestringer mentioned this pull request Jul 9, 2020
Merged
@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jul 9, 2020

Possible upgrade implication with schema version, please take a look at the v1.6 backports. We will need to come to a shared understanding of that version and consistently backport to other branches based on it.

@lbernail
Copy link
Copy Markdown
Contributor Author

lbernail commented Jul 10, 2020 •
edited by joestringer
Loading

are there any upgrade implications to this change?

@joestringer it worked fine from 1.8.1 to master but I haven't tested with earlier versions. Is validation enabled before 1.8? Because if it is it means toGroups has been broken for a while

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jul 10, 2020 •
edited
Loading

The related code for the schema has been around since v1.6, Not sure which versions that the schema validation is enabled for, but I seem to recall some upgrade notes early in v1.7 cycle around this.

Thinking this through, my take is that this PR tightens the CRD validation and should only affect users who currently run policies that would not pass the new validation. If they follow the upgrade instructions (specifically the preflight check) to validate their policies prior to upgrade, then they will not hit any issues.

@brb brb mentioned this pull request Jul 15, 2020
Merged
@jrfastab jrfastab mentioned this pull request Jul 21, 2020
Merged
@jrfastab jrfastab mentioned this pull request Jul 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aanm aanm aanm approved these changes

Assignees

No one assigned

Labels

kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lbernail @coveralls @aanm @joestringer @brb @pchaigno @christarazi