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.

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

Adds hard coded commonName: istiod.istio-system.svc to istiod Certificate #91

Merged

Conversation

JoshVanL
Copy link
Contributor

This PR adds commonName: istiod.istio-system.svc as a hard coded value to the istiod Certificate. This is to allow for easier integration with Issuers that have default constraints on X509 Subjects being present when requesting certificates with DNS names. This is true for the AWSPCA Issuer.

This value is hardcoded and is not configurable through options such as app.istio.revisions. This is done under the assumption that the commonName doesn't actually need to match one of the DNS names in the cases where revisions are used, but not the default revision. If this is not the case, a workaround for users is to include the default revision, even if their istio installation doesn't include a default revision.

Once merged, we should release a new helm chart version to include this and #86 changes in the public repo.

fixes #90
/assign @irbekrm

NONE

Certificate

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Aug 26, 2021
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 26, 2021
@irbekrm
Copy link
Contributor

irbekrm commented Aug 26, 2021

Looks good to me, assuming it fixes their use case.

This is done under the assumption that the commonName doesn't actually need to match one of the DNS names in the cases where revisions are used, but not the default revision. If this is not the case, a workaround for users is to include the default revision, even if their istio installation doesn't include a default revision.

Should this be added as a comment above the helm chart option? Adding a hold in case you want to add that, else feel free to unhold.

/lgtm
/hold

@jetstack-bot jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 26, 2021
@JoshVanL
Copy link
Contributor Author

Looks good to me, assuming it fixes their use case.

This is done under the assumption that the commonName doesn't actually need to match one of the DNS names in the cases where revisions are used, but not the default revision. If this is not the case, a workaround for users is to include the default revision, even if their istio installation doesn't include a default revision.

Should this be added as a comment above the helm chart option? Adding a hold in case you want to add that, else feel free to unhold.

/lgtm
/hold

Good idea. MIDD (minimal issues driven development).

issuers with common name constraints

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2021
@irbekrm
Copy link
Contributor

irbekrm commented Aug 26, 2021

MIDD (minimal issues driven development)

fixes the problem 🤷🏼

Thanks for adding the comment.

/lgtm

/hold cancel

@jetstack-bot jetstack-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 26, 2021
@jetstack-bot jetstack-bot merged commit e42b714 into cert-manager:master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing integrate AWSPCA - CSR must mark the SAN extension critical when it has an empty subject
3 participants