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

Bugfixes: Enterprise-in-GKE test not deploying test apps #452

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Bugfixes: Enterprise-in-GKE test not deploying test apps #452

merged 2 commits into from
Feb 10, 2022

Conversation

john-odonnell
Copy link
Contributor

@john-odonnell john-odonnell commented Feb 7, 2022 •
edited
Loading

Desired Outcome

In recent CI runs, the stage running end-to-end tests against Conjur Enterprise in GKE succeed without installing the Namespace Prep Helm chart or deploying the test apps.

From a passing Jenkins run:

++++++++++++++++++++++++++++++++++++++

Installing cluster prep chart

++++++++++++++++++++++++++++++++++++++
. . .
The Conjur/Authenticator Namespace preparation is complete.
 The following have been deployed:
- Golden ConfigMap
- Authenticator ClusterRole
++++++++++++++++++++++++++++++++++++++

Removing test environment

++++++++++++++++++++++++++++++++++++++

Updating this led to another bug:
Jenkins logs:

++++++++++++++++++++++++++++++++++++++

Installing namespace prep chart

++++++++++++++++++++++++++++++++++++++
Release "namespace-prep-7bc09deb-9" does not exist. Installing it now.
Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: ValidationError(RoleBinding.subjects[0]): missing required field "name" in io.k8s.api.rbac.v1.Subject

The authnK8sServiceAccount field in the Golden ConfigMap template is wrapped in a conditional, and is only created if the Cluster Prep Helm chart was told to create a serviceAccount. The chart's values.yml file states:

# If 'authnK8s.serviceAccount.create` is set to `true`, then this defaults
# to "conjur-serviceaccount". If 'authnK8s.serviceAccount.create` is set
# to `false`, then this is a required value.
# name: conjur-serviceaccount
# name:

In cases where authnK8s.serviceAccount.create is set to false, the provided name was not being used for the authnK8sServiceAccount field in the Golden ConfigMap.

Implemented Changes

  • End-to-end test stage for Enterprise-in-GKE now installs Namespace prep Helm chart and deploys test apps
  • When authnK8s.serviceAccount.create is set to false, the Golden ConfigMap template uses authnK8s.serviceAccount.name to fill its authnK8sServiceAccount field.

Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue link: insert issue ID

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: insert issue ID
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@john-odonnell john-odonnell marked this pull request as ready for review February 7, 2022 20:27
@john-odonnell john-odonnell requested a review from a team as a code owner February 7, 2022 20:27
szh
szh previously approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@szh szh left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

diverdane
diverdane reviewed Feb 8, 2022
View reviewed changes
helm/conjur-config-cluster-prep/templates/golden_configmap.yaml
@@ -22,9 +22,7 @@ data:
authnK8sClusterRole: {{ .Values.authnK8s.clusterRole.name | default "conjur-clusterrole" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to remove the conditional for authnK8sClusterRole: field as well.

bin/test-workflow/start
@@ -227,6 +227,7 @@ elif [[ "$CONJUR_PLATFORM" == "gke" ]]; then
eval "$conjur_init"
run_command_with_platform "$conjur_prep"
run_command_with_platform "$cluster_prep"
run_command_with_platform "$test_app_workflow"
Copy link
Contributor

Choose a reason for hiding this comment

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

D'oh!!!

diverdane
diverdane suggested changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@diverdane diverdane left a comment

Choose a reason for hiding this comment

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

Nice catch!
I think we want to remove the conditional for authnK8sClusterRole field as well.

@john-odonnell
Make conditional fields mandatory in Golden ConfigMap
6276346 
These fields were only included if the Cluster Prep
Helm chart created the corresponding object, but they
are required in every case:
- authnK8sServiceAccount
- authnK8sClusterRole
@john-odonnell john-odonnell force-pushed the enterprise-in-gke-bugfix branch from e20aa5d to 6276346 Compare February 9, 2022 16:25
diverdane
diverdane approved these changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@diverdane diverdane left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@john-odonnell john-odonnell merged commit 27e5e02 into master Feb 10, 2022
@john-odonnell john-odonnell deleted the enterprise-in-gke-bugfix branch February 10, 2022 22:19
@john-odonnell john-odonnell mentioned this pull request Jul 8, 2022
Merged
@gl-johnson gl-johnson mentioned this pull request Aug 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diverdane diverdane diverdane approved these changes

@szh szh szh left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@john-odonnell @szh @diverdane