[dev] Augment application model with expose parameters #11963
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The new version includes the bits for exporting the expose-related parameters that will be introduced in the following commits.
achilleasa
force-pushed
the
dev-augment-application-model-with-expose-parameters
branch
2 times, most recently
from
40b271e
to
d86c94e
Compare
achilleasa
force-pushed
the
dev-augment-application-model-with-expose-parameters
branch
from
d86c94e
to
c3f08ba
Compare
|
|
1 similar comment
|
|
achilleasa
force-pushed
the
dev-augment-application-model-with-expose-parameters
branch
from
c3f08ba
to
eef923b
Compare
|
@manadart I just realized that I forgot the |
|
|
achilleasa
force-pushed
the
dev-augment-application-model-with-expose-parameters
branch
from
eef923b
to
ea254f7
Compare
achilleasa
commented
Sep 3, 2020
| // application that should be reachable once the application is exposed. An | ||
| // empty returned list means that all endpoints should be reachable. | ||
| func (a *Application) ExposedEndpoints() []string { | ||
| if len(a.doc.ExposedEndpoints) == 0 { |
There was a problem hiding this comment.
@manadart I have also added these guard blocks to avoid breaking errors that check for a nil []string and not for a []string(nil). As an alternative, I could patch setExposed to check for nil args and generate $unset commands but I think it's better if we see the fields in the application docs even if they are empty.
|
|
|
|
achilleasa
force-pushed
the
dev-augment-application-model-with-expose-parameters
branch
from
ea254f7
to
930bce0
Compare
achilleasa
commented
Sep 3, 2020
Comment on lines
+695
to
+697
| // The default space entry has an empty string as its | ||
| // key but is not a valid endpoint. | ||
| if _, found := bindings[endpoint]; !found || endpoint == "" { |
There was a problem hiding this comment.
Yet another thing that I missed and fixed via force-pushing. The "" endpoint represents the default space binding but it is not a valid endpoint name ;-)
|
|
achilleasa
deleted the
dev-augment-application-model-with-expose-parameters
branch
jujubot
added a commit
that referenced
this pull request
Sep 7, 2020
…pose-parameters #11973 # Description of change Due to a change in the approach for modeling expose parameters, this PR revises the changes from #11963 so that we can specify a list of spaces and/or CIDRs that can access exposed application ports on a **per endpoint** basis. ## QA steps None needed. The `exposed` flag is still being set as usual and that's the only thing that the firewall worker considers when generating ingress rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
This PR is part of the work for enhancing the
juju exposecommand to target endpoints and/or allow operators to specify the spaces/CIDRs that should be able to access the application once exposed.The PR augments the application model with the following new fields:
As part of this set of changes, the
SetExposedmethod of the application model has been modified to accept (and validate) the above new set of fields.The CLI updates and related API changes for actually populating/querying the new fields will be tacked via a separate PR.
QA steps
Just check that
juju expose Xworks. The new fields are not used for now so there are no visible functional changes introduced by this PR.