-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add SVID hints on workload api client #220
Add SVID hints on workload api client #220
Conversation
Signed-off-by: Daniel Feldman <dfeldman.mn@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @guilhermocc! I have concerns, mostly around changing the exported functions without a clear understanding of the benefit we are getting.
v2/svid/common/optional/optional.go
Outdated
package optional | ||
|
||
// SVIDOptionals contains optional fields for SVIDs. | ||
type SVIDOptionals struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is likely that we'll gain any more "optional" fields on the SVIDs. I think I'd probably be more comfortable with just including the Hint field in each of the SVID structs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I did a bit of over eng here
v2/svid/x509svid/svid.go
Outdated
@@ -47,7 +50,7 @@ func Load(certFile, keyFile string) (*SVID, error) { | |||
// Parse parses the X509-SVID from PEM blocks containing certificate and key | |||
// bytes. The certificate must be one or more PEM blocks with ASN.1 DER. The | |||
// key must be a PEM block with PKCS#8 ASN.1 DER. | |||
func Parse(certBytes, keyBytes []byte) (*SVID, error) { | |||
func Parse(certBytes, keyBytes []byte, opts ...optional.SVIDOption) (*SVID, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that 2.0 has shipped, I don't think we should be changing the signature of exported functions unless the benefit is very clear. In this case, I don't really have a good sense on how hints will be leveraged enough to understand if it's really a hardship for someone to call Parse and then set the hint on the returned SVID v.s. having a way to set the SVID hint while parsing. I'd probably lean towards not providing these until we fully understand how folks will want to interact with the hint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I will keep things simple here
v2/internal/test/ca.go
Outdated
return certificateOption(func(c *x509.Certificate) { | ||
c.SerialNumber = serial | ||
}) | ||
type SvidOption struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where possible (everywhere expect protos), we should capitalize the full acronym in the name:
type SvidOption struct { | |
type SVIDOption struct { |
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
706a3b4
to
926e51a
Compare
v2/svid/x509svid/svid.go
Outdated
@@ -116,6 +120,11 @@ func (s *SVID) GetX509SVID() (*SVID, error) { | |||
return s, nil | |||
} | |||
|
|||
// SetHint sets the hint for the SVID. | |||
func (s *SVID) SetHint(hint string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of the setter?
v2/svid/jwtsvid/svid.go
Outdated
@@ -85,6 +88,11 @@ func (svid *SVID) Marshal() string { | |||
return svid.token | |||
} | |||
|
|||
// SetHint sets the hint for the JWT-SVID. | |||
func (svid *SVID) SetHint(hint string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of the setter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, I like to use setters for encapsulating eventual validation logic for setting the fields in the struct; especially in this case, in which this struct will and can be used by external applications. For now we don't have any validation here, and I'm not sure if we will have it in the future, so I would also be ok with not having this. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, hint is an opaque string by spec, so there isn't really any validation and I don't anticipate needing it in the future.
Even so, we need to be very mindful of every exported type/function coming out of these packages since there is a compatibility guarantee that we make towards the stability of the interfaces. The bar should be very high towards adding new exported methods/types that we'd be obligated to maintain.
Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @guilhermocc !
Add hint field to SVIDs retrieved by workload API client