Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Fix bundle role/rolebinding naming conflict #333

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Jun 5, 2023

When you install a primary and a secondary deployment in the same Kubernetes cluster, use the same release name (spire), and configure the primary to give the bundle to the secondary, both releases want to own the same role/rolebinding name. This patch uses the same name as the configmap to ensure uniqueness.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 5, 2023

Alternate approach to #309

Copy link
Contributor

@edwbuck edwbuck left a comment

Choose a reason for hiding this comment

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

Better than having it refer to a different name, adding a hard-coded suffix.

Thanks for the work.

(Can you close the alternative PR?)

@edwbuck
Copy link
Contributor

edwbuck commented Jun 7, 2023

@faisal-memon @marcofranssen @mrsabath or anyone else. Can we get a second review on this item, so it can be merged or reworked?

@faisal-memon
Copy link
Contributor

What does the rendered name look like?

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 7, 2023

Its set the same as the configmap object its granting permissions to.
so, by default 'spire-bundle' or whatever the user overrides it to, such as 'spire-upstream-bundle'

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

I have been trying to render the chart to see the endresult for myself.

$ git checkout role-conflict-fix-alt
$ helm template -n spire-system --values examples/production/values.yaml spire ./charts/spire | grep 'bundle' 
Error: template: spire/charts/spire-server/templates/pre-upgrade-hook.yaml:66:27: executing "spire/charts/spire-server/templates/pre-upgrade-hook.yaml" at <{{template "spire-lib.kubectl-image" (dict "appVersion" $.Chart.AppVersion "image" .Values.controllerManager.validatingWebhookConfiguration.upgradeHook.image "global" .Values.global "KubeVersion" .Capabilities.KubeVersion.Version)}}>: template "spire-lib.kubectl-image" not defined

Use --debug flag to render out invalid YAML

Seems like something is broken when defining values.yaml

Without it works, but that doesn't show me how it works for multiple.

$ helm template -n spire-system  spire ./charts/spire | grep 'bundle' 
        "trust_bundle_path": "/run/spire/bundle/bundle.crt",
# Source: spire/charts/spire-server/templates/bundle-configmap.yaml
  name: spire-bundle
            "k8sbundle": {
                "config_map": "spire-bundle",
# Role to be able to push certificate bundles to a configmap
  name: spire-bundle
    resourceNames: [spire-bundle]
  name: spire-bundle
  name: spire-bundle
            - name: spire-bundle
              mountPath: /run/spire/bundle
        - name: spire-bundle
            name: spire-bundle

Not sure why this change breaks it consistently, but on main branch it also randomly fails. Sometimes it succeeds, some times it fails. I guess the template rendering is kind of random from the perspective which resources are rendered first.

Update

Running it couple of times on this PR also gave me a successful run, but don't see the second bundle when grepping it.

$ helm template -n spire-system --values examples/production/values.yaml spire ./charts/spire | grep 'bundle' 
        "trust_bundle_path": "/run/spire/bundle/bundle.crt",
# Source: spire/charts/spire-server/templates/bundle-configmap.yaml
  name: spire-bundle
            "k8sbundle": {
                "config_map": "spire-bundle",
# Role to be able to push certificate bundles to a configmap
  name: spire-bundle
    resourceNames: [spire-bundle]
  name: spire-bundle
  name: spire-bundle
            - name: spire-bundle
              mountPath: /run/spire/bundle
        - name: spire-bundle
            name: spire-bundle

How do I test and check the impact of the resource naming using this change?

When you install a primary and a secondary deployment in the same Kubernetes
cluster, use the same release name (spire), and configure the primary to
give the bundle to the secondary, both releases want to own the same
role/rolebinding name. This patch uses the same name as the configmap
to ensure uniqueness.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 8, 2023

Its configured like: https://github.com/spiffe/helm-charts/pull/117/files#diff-40743f3e274c7e467663a938cef8fdd93afe9bfe23cf82d47bbd70dc756d92a3R4-R11

Its actually tested in that PR.

This pr doesn't make a second configmap. just makes sure the naming is unique when you have two instances of the chart.

@kfox1111
Copy link
Contributor Author

Can we unblock this? The issue with the macro seems unrelated.

@faisal-memon
Copy link
Contributor

What is the output of 'k get roles' and 'k get rolebindings' look like?

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

For reference, the problem I faced was due to running unittests locally,

That resulted into tgz files in the charts/spire/charts folder for all the subcharts. Those where generated from another branch. Therefore it didn't take the sourcecode for the dependencies and caused the conflict.

@marcofranssen marcofranssen merged commit c97a788 into main Jun 16, 2023
@marcofranssen marcofranssen deleted the role-conflict-fix-alt branch June 16, 2023 06:55
@kfox1111
Copy link
Contributor Author

Ah. that makes sense. When we breakout the charts, we may want to add a .helmignore entry in the parent chart to to try and avoid that happening?

marcofranssen added a commit that referenced this pull request Jun 19, 2023
* 57a9320 Add SPIRE 1.7.0 to main readme (#357)
* af36f7c Align the bash image version with other instances for spire-agent (#356)
* c11a8c0 Implement pre-delete hook for graceful delete of spiffe-oidc-discovery-provider (#353)
* a6dcf26 Allow for SPIRE Agent to run as non root user (#209)
* 9cf6049 Allow contributors to run linting easily on local
* e88f7f6 Add configmap annotation to spire-bundle configmap (#351)
* 020bde8 Add support to create a issuer and CA via cert-manager (#342)
* 9d504de Ignore .DS_Store files
* e6b608c Bump spire images to 1.7.0 (#348)
* c97a788 Fix bundle role/rolebinding naming conflict (#333)
* b66077e Bump peter-evans/create-pull-request from 5.0.1 to 5.0.2 (#349)
* d0da864 Add missing metadata to subcharts (#347)
* 4c0a1d5 Allow overriding test images (#186)
* 250fd5d Add missing global values to charts (#311)
* 5d8c907 Dropping k8s versions in CI older than 3, as per readme (#344)
* 8748933 Update upstream-ca-secret.yaml (#341)
* 4e07450 Fix ingress annotations for federation (#337)
* ea09199 Bump actions/checkout from 3.5.0 to 3.5.3
* 87fe198 Merge pull request #331 from edwbuck/key_conventions
* ddc0166 Fix line wrapping.
* 0cae9ce Update project/conventions.md
* cb18255 Update project/conventions.md
* 52e5c24 Upgrade Tornjak to image v1.2.2 (#328)
* 28e2abf Choose a different example for dotted Acronyms.
* d60d68c Added accidentally clipped explicit name guidelines.
* abe9fde Merge branch 'main' into key_conventions
* f6a7b62 Update project/conventions.md
* c4d19db Update project/conventions.md
* cfa9f78 Bump test chart dependencies (#332)
* c3213ab Initial submission of Helm Chart key naming conventions.
* 28c0824 Bump test chart dependencies (#322)
* d333154 Add Makefile for local testing (#327)
* 9fa1ec2 Improve Tornjak backend test (#321)
* 5b779dc Improve Tornjak frontend test (#320)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Jun 19, 2023
* 57a9320 Add SPIRE 1.7.0 to main readme (#357)
* af36f7c Align the bash image version with other instances for spire-agent (#356)
* c11a8c0 Implement pre-delete hook for graceful delete of spiffe-oidc-discovery-provider (#353)
* a6dcf26 Allow for SPIRE Agent to run as non root user (#209)
* 9cf6049 Allow contributors to run linting easily on local
* e88f7f6 Add configmap annotation to spire-bundle configmap (#351)
* 020bde8 Add support to create a issuer and CA via cert-manager (#342)
* 9d504de Ignore .DS_Store files
* e6b608c Bump spire images to 1.7.0 (#348)
* c97a788 Fix bundle role/rolebinding naming conflict (#333)
* b66077e Bump peter-evans/create-pull-request from 5.0.1 to 5.0.2 (#349)
* d0da864 Add missing metadata to subcharts (#347)
* 4c0a1d5 Allow overriding test images (#186)
* 250fd5d Add missing global values to charts (#311)
* 5d8c907 Dropping k8s versions in CI older than 3, as per readme (#344)
* 8748933 Update upstream-ca-secret.yaml (#341)
* 4e07450 Fix ingress annotations for federation (#337)
* ea09199 Bump actions/checkout from 3.5.0 to 3.5.3
* 87fe198 Merge pull request #331 from edwbuck/key_conventions
* ddc0166 Fix line wrapping.
* 0cae9ce Update project/conventions.md
* cb18255 Update project/conventions.md
* 52e5c24 Upgrade Tornjak to image v1.2.2 (#328)
* 28e2abf Choose a different example for dotted Acronyms.
* d60d68c Added accidentally clipped explicit name guidelines.
* abe9fde Merge branch 'main' into key_conventions
* f6a7b62 Update project/conventions.md
* c4d19db Update project/conventions.md
* cfa9f78 Bump test chart dependencies (#332)
* c3213ab Initial submission of Helm Chart key naming conventions.
* 28c0824 Bump test chart dependencies (#322)
* d333154 Add Makefile for local testing (#327)
* 9fa1ec2 Improve Tornjak backend test (#321)
* 5b779dc Improve Tornjak frontend test (#320)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants