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

Allow additional CRs to be managed by the chart #78

Closed
wants to merge 59 commits into from
Closed

Allow additional CRs to be managed by the chart #78

wants to merge 59 commits into from

Conversation

kfox1111
Copy link
Collaborator

@kfox1111 kfox1111 commented Nov 4, 2023

Sometimes additional ClusterSPIFFEIDs and the other CRs are needed. Add support for the end user to manage those extra CRs via the chart.

kfox1111 and others added 30 commits October 25, 2023 12:47
@kfox1111
Add support for the new spire-controller-manager class feature
9fe0bd9 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix docs. Swich nested deployment to use controller manager
ef589a9 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Incorperate feedback
91c49f5 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Merge branch 'main' into controller-manager-class
eff2e04 
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@kfox1111
Test with nightly
7f195fb 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix global object naming clash
cdf6e14 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix missing dot
690ff7e 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix naming conflict with cluster ids
fb01de4 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix scoping issue
d683f5f 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix typo
1e30d9e 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix webhook name collision
0ddc76b 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix webhook reference and add note to user about className
85a12a9 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Upgrade has to work on the old version of the object before rename
93ff943 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Fix formatting
b8d276b 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Remove extra junk from job
f7466a4 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Easier local runs and wait for crds
3db9a0c 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Add missing crd upgrade
6ce33bc 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Merge branch 'main' into controller-manager-class
04a02cb 
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@kfox1111
Update upgrade notes
17283dd 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 @marcofranssen
Update charts/spire/charts/spire-server/templates/controller-manager-…
51599c3 
…cluster-ids.yaml

Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@kfox1111
Incorperate feedback
7aac57c 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Merge branch 'main' into controller-manager-class
74388e3 
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@kfox1111
Bump version to the released 0.4.0
d4f6d45 
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@kfox1111
Merge branch 'main' into controller-manager-class
d344095
@kfox1111
Fix docs
80036b6 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Merge in crd changes from upstream
1fff86d 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Add auto populate dns
8bbd20a 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Merge branch 'main' into controller-manager-class
48b7644
@kfox1111
Merge branch 'main' into controller-manager-class
3e9c053
@kfox1111 @faisal-memon
Update charts/spire/README.md
a009101 
Co-authored-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@kfox1111
Fix docs
cfe0ad9 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@faisal-memon faisal-memon added this to the 0.16.0 milestone Nov 8, 2023
Base automatically changed from controller-manager-class to main November 8, 2023 10:43
@kfox1111 kfox1111 marked this pull request as ready for review November 15, 2023 01:19
kfox1111
kfox1111 commented Nov 16, 2023
View reviewed changes
charts/spire/README.md Outdated Show resolved Hide resolved
faisal-memon
faisal-memon reviewed Nov 27, 2023
View reviewed changes
charts/spire/charts/spire-server/values.yaml Outdated
Comment on lines 490 to 507
clusterStaticEntries: {}
# foo:
# labels:
# foo: bar
# parentID: spiffe://example.com/bar
# spiffeID: spiffe://example.com/foo
# selectors:
# - k8s:pod-label:app.kubernetes.io/name:server
## @param controllerManager.identities.clusterFederatedTrustDomains Specify additional ClusterFederatedTrustDomain objects.
clusterFederatedTrustDomains: {}
# foo:
# labels:
# foo: bar
# bundleEndpointProfile:
# endpointSPIFFEID: spiffe://example.com/foo
# type: https_spiffe
# bundleEndpointURL: https://rootserver.example.com:1234
# trustDomain: example.com
Copy link
Collaborator

Choose a reason for hiding this comment

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

The options won't get documented in the readme with everything commented out. Would it be better to have something but disabled for documentation purposes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure. Would you want something like default: with a simplified example, with enabled: false?

faisal-memon
faisal-memon reviewed Nov 27, 2023
View reviewed changes
charts/spire/charts/spire-server/values.yaml Outdated
downstream: false
## @param controllerManager.identities.autoPopulateDNSNames Auto populate DNS names from services attached to pods
autoPopulateDNSNames: false
clusterSPIFFEIDs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of putting a comment above to mention that you can specify multiple?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

K. Took a first stab it it.

faisal-memon
faisal-memon reviewed Nov 27, 2023
View reviewed changes
charts/spire/charts/spire-server/values.yaml Outdated
autoPopulateDNSNames: false
clusterSPIFFEIDs:
default:
## @param controllerManager.identities.clusterSPIFFEIDs.default.enabled Flag to enable default identities for controller manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## @param controllerManager.identities.clusterSPIFFEIDs.default.enabled Flag to enable default identities for controller manager
## @param controllerManager.identities.clusterSPIFFEIDs.default.enabled Enable this identity for controller manager

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Manually merged suggestion due to conflict.

faisal-memon
faisal-memon reviewed Nov 27, 2023
View reviewed changes
charts/spire/charts/spire-server/values.yaml Outdated
downstream: false
## @param controllerManager.identities.clusterSPIFFEIDs.default.autoPopulateDNSNames Auto populate DNS names from services attached to pods
autoPopulateDNSNames: false
# foo:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# foo:
# You can specific additional ClusterSPIFFEIDs following this example.
# foo:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Manually merged suggestion due to conflict.

faisal-memon
faisal-memon reviewed Nov 27, 2023
View reviewed changes
charts/spire/charts/spire-server/values.yaml Outdated

## @param controllerManager.identities.clusterSPIFFEIDs.default.spiffeIDTemplate Spiffe ID template for identities
spiffeIDTemplate: spiffe://{{ .TrustDomain }}/ns/{{ .PodMeta.Namespace }}/sa/{{ .PodSpec.ServiceAccountName }}
## @param controllerManager.identities.clusterSPIFFEIDs.default.podSelector [object] Selector for pods to issue identity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anyway to have the comments not have default in them; instead have something more generic like [name].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. will have a look at the doc tools documentation...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dug through the docs and code a bit. I don't see a way to allow anything more generic then specifying the key to something existing.

@kfox1111
Incorperate feedback
c01e512 
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111
Copy link
Collaborator Author

Github's having some issues with this PR after the revert. Will close and make a new PR from the same branch.

@kfox1111 kfox1111 closed this Nov 27, 2023
@kfox1111
Copy link
Collaborator Author

Reopened as #117

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

@faisal-memon faisal-memon faisal-memon left review comments

@marcofranssen marcofranssen Awaiting requested review from marcofranssen marcofranssen is a code owner

@dfeldman dfeldman Awaiting requested review from dfeldman

@mrsabath mrsabath Awaiting requested review from mrsabath mrsabath is a code owner

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

Successfully merging this pull request may close these issues.
2 participants
@kfox1111 @faisal-memon