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

Adjust Gatekeeper Policy Installation For Improved CLI Installations #414

Closed
wants to merge 20 commits into from
Closed

Adjust Gatekeeper Policy Installation For Improved CLI Installations #414

wants to merge 20 commits into from

Conversation

cartyc
Copy link
Contributor

@cartyc cartyc commented Jun 26, 2023

This PR moves the gatekeeper constraints from the gatekeeper-policies package to the core-landing-zone package. This will help resolve issue #409 and allow for easier non-gitops based installation approaches as this allows for the ConstraintTemplate to be installed before the gatekeeper constraint file and prevents the issue where one would need to comment out of the constraint file first before apply the gatekeeper-policies package.

The overall goal of this PR is to improve the installation process for users of non-gitops based installs, kpt in particular. This would also allow us to install more constraintTemplates via the gatekeeper-policies package into the cluster without needing to enable them right away.

@cartyc cartyc requested a review from fmichaelobrien as a code owner June 26, 2023 12:39
fmichaelobrien
fmichaelobrien previously approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

  • nice, thanks for the constraints move - Chris

@cartyc
Copy link
Contributor Author

cartyc commented Jul 6, 2023

@alaincormier-ssc @davelanglois-ssc @fmichaelobrien I made the changes discussed yesterday and wanted to get your input before I start fixing the README and Kptfiles.

Created a new solution directory called policy-constraint-bundles with 2 bundles, guardrails-v1 and naming-policies.

@alaincormier-ssc
Copy link
Collaborator

alaincormier-ssc commented Jul 10, 2023 •
edited
Loading

@alaincormier-ssc @davelanglois-ssc @fmichaelobrien I made the changes discussed yesterday and wanted to get your input before I start fixing the README and Kptfiles.

Created a new solution directory called policy-constraint-bundles with 2 bundles, guardrails-v1 and naming-policies.

that should work
will each bundle be tagged by release-please?

@cartyc
Copy link
Contributor Author

cartyc commented Jul 10, 2023

That's what I was thinking, might be overkill but I believe having specific task based bundles makes sense. I was using the Anthos Gatekeeper Bundles as by point of reference.

@davelanglois-ssc
Copy link
Collaborator

Hey @cartyc, I really like the concept. If I understand correctly, the constrainttemplate would remain in the existing gatekeeper-policies package. Do you think that name still make sense ?

@cartyc
Copy link
Contributor Author

cartyc commented Jul 12, 2023

Hey @cartyc, I really like the concept. If I understand correctly, the constrainttemplate would remain in the existing gatekeeper-policies package. Do you think that name still make sense ?

That's correct, the package would just act as a means to load the constrainttemplates into the Config Controller or GKE instances, similar to what happens when you you enable the installation of the default template library on policy controller I think the name gatekeeper-policies still works but maybe the bundle names might need some additional tuning to help keep things more distinct. gatekeeper-policy-templates might be a good option though if we want to rename the package.

@fmichaelobrien
Copy link
Contributor

Thanks Chris. I'll review the template changes post last month's discussion - later today after my last deployment meet. Deploying a new lz anyway.

fmichaelobrien
fmichaelobrien previously approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

  • nice
  • dryrun is appreciated - in particular for the cost-center/centre ones
  • will reapprove if required after lint commit

@cartyc
Copy link
Contributor Author

cartyc commented Aug 9, 2023

Thanks Mike, I was just cracking open an editor to start hacking away at the linting issues.

fmichaelobrien
fmichaelobrien previously approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

fmichaelobrien
fmichaelobrien previously approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM
reviewed
c62264b#diff-0e4828475a445061adf098b63da0946c315796e05bb5354d1b142e2ec8b6d427

fmichaelobrien
fmichaelobrien previously approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

fmichaelobrien
fmichaelobrien previously approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

  • yes lint is soooo particular on CR/LF spaces...
  • thanks for taking the heat

@cartyc
Copy link
Contributor Author

cartyc commented Aug 31, 2023

@davelanglois-ssc should be good for a final review.

fmichaelobrien
fmichaelobrien previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM
the dryrun is appreciated

fmichaelobrien
fmichaelobrien previously approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

  • last change good

fmichaelobrien
fmichaelobrien approved these changes Sep 17, 2023
View reviewed changes
Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

LGTM

@cartyc cartyc mentioned this pull request Dec 12, 2023
Open
@obriensystems
Copy link
Collaborator

closing - keep reference code only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmichaelobrien fmichaelobrien fmichaelobrien approved these changes

@davelanglois-ssc davelanglois-ssc davelanglois-ssc approved these changes

@alaincormier-ssc alaincormier-ssc Awaiting requested review from alaincormier-ssc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cartyc @alaincormier-ssc @davelanglois-ssc @fmichaelobrien @obriensystems